On Thu, Oct 8, 2026 at 3:42 PM Mark Michelson <[email protected]> wrote:
> Hi Jacob, I had a look at this, and I think I get the gist of what's > happening. I have a question below. > > On Fri, Oct 2, 2026 at 9:13 AM Jacob Tanenbaum via dev > <[email protected]> wrote: > > > > On restart, ovn-controller monitored the whole cluster's > > Advertised_Route table at startup, and only ran its kernel route > > sync once all of it had been read. The whole-cluster pull added > > measurable startup latency in large deployments, and the guard was > > coarser than it needed to be: the controller only ever deletes > > routes that it installed itself, and it only needs the advertised > > routes of the local route-exchange datapaths to know which ones > > those are. > > > > Monitor no Advertised_Route rows at startup. Once local datapaths > > are known the monitor condition is already scoped to them, so the > > dump the controller waits for is small. > > > > Defer the route-exchange kernel sync until the controller can be > > confident that the Advertised_Route view is complete: the > > condition seqno shows the dump was delivered, and the current > > condition either covers the local route-exchange datapaths, or no > > local route-exchange datapaths remain and the initial sync has > > already run. The kernel routes persist across the deferral, so > > none are lost. The datapath set changing or a southbound > > reconnect re-arms the gate. > > > > The condition covering the local datapaths is what keeps this > > safe. Waiting only on the condition seqno could arm the sync > > against the initial empty condition, and the first sync would > > then delete the routes installed before the restart. > > > > With no local route-exchange datapaths there is no scoped dump to > > wait for, so once the initial sync has run the view is complete > > and the sync may run to clean up the VRFs and routes of the > > removed datapaths. Deferring it instead would leak the VRF and > > routes of the last removed dynamic-routing router. > > > > The dynamic-routing system test now checks that the deferred sync > > is enabled after a restart and that the installed OVN routes > > survive a --restart of the controller. > > > > Reported-at: https://redhat.atlassian.net/browse/FDP-3992 > > Assisted-by: Qwen3.8-27B-FP8, opencode > > Signed-off-by: Jacob Tanenbaum <[email protected]> > > > > --- > > v2: > > * Fixed issues with the sb_monitor_all case > > * Fixed the update to ovn-inc-proc-graph-dump.at > > * Allow the route sync to run once the local route-exchange > > datapath set becomes empty, so the VRF and routes of the > > last removed dynamic-routing router are cleaned up. > > --- > > NEWS | 4 + > > TODO.rst | 5 -- > > controller/ovn-controller.c | 126 +++++++++++++++++++++++++++++-- > > tests/ovn-inc-proc-graph-dump.at | 3 +- > > tests/system-ovn.at | 11 ++- > > 5 files changed, 137 insertions(+), 12 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index 7f94d0b14..207c5a03c 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -19,6 +19,10 @@ Post v26.09.0 > > - Removed OVN's ovs-bugtool plugin and helper scripts. > > - Removed ovn-sim utility scripts. > > - Removed ovn-docker utility scripts. > > + - ovn-controller no longer monitors the whole cluster's > Advertised_Route > > + table at startup; it defers its kernel route updates until the > initial > > + dump of the advertised routes of the local route-exchange > datapaths is > > + complete. > > > > OVN v26.09.0 - xxx xx xxxx > > -------------------------- > > diff --git a/TODO.rst b/TODO.rst > > index 023eb27f6..dc55c8895 100644 > > --- a/TODO.rst > > +++ b/TODO.rst > > @@ -145,11 +145,6 @@ OVN To-do List > > * Add incremental processing of en_dynamic_routes for stateful > configuration > > changes. > > > > - * The ovn-controller currently loads all Advertised_Route entries on > startup. > > - This is to prevent deleting our routes on restart. If we defer > updating > > - routes until we are sure to have loaded all necessary > Advertised_Routes > > - this could be changed. > > - > > * Improve handling of the Learned_Route table in ovn-controller > conditional > > monitoring; once a new local datapath is added we need to wait for > > monitoring conditions to update before we actually try to learn > routes. > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index d57ff316d..f5ee8b75a 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -176,6 +176,16 @@ struct controller_engine_ctx { > > struct if_status_mgr *if_mgr; > > const unsigned int *ovnsb_expected_cond_seqno; > > const bool *sb_monitor_all; > > + /* True once every table that the current monitor conditions > request has > > + * been fully received (the SB condition seqno has caught up). The > > + * route-exchange node uses this to defer its destructive kernel > sync > > + * until the data it relies on is complete. */ > > + bool sb_all_data_loaded; > > + /* True once the Advertised_Route monitor condition covers the local > > + * route-exchange datapaths (or, in monitor-all mode, everything). > It is > > + * false for the initial empty condition that is used before any > local > > + * datapath is known. */ > > + bool sb_ar_condition_scoped; > > }; > > > > /* Pending packet to be injected into connected OVS. */ > > @@ -360,11 +370,13 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > > sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, > "l2gateway"); > > sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, > "l3gateway"); > > > > - /* Monitor all advertised routes during startup. > > - * Otherwise, once we claim a port on startup we do not yet > know the > > - * routes to advertise and might wrongly delete already > installed > > - * ones. */ > > - ovsdb_idl_condition_add_clause_true(&ar); > > + /* Do not monitor any Advertised_Route rows during startup. The > > + * route-exchange node defers its destructive kernel sync until > it has > > + * seen the initial dump of the advertised routes of the local > > + * route-exchange datapaths, so there is no risk of deleting > routes > > + * we installed previously. Once local datapaths are known the > > + * condition is scoped to them (below), which avoids pulling > the whole > > + * cluster's advertised routes on startup. */ > > } > > if (local_ifaces) { > > const char *name; > > @@ -773,6 +785,11 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct > ovsdb_idl *ovnsb_idl, > > if (sb_cond_seqno) { > > *sb_cond_seqno = next_cond_seqno; > > } > > + /* In monitor-all mode the Advertised_Route condition matches > > + * everything, so it already covers the local datapaths. */ > > + if (ctx) { > > + ctx->sb_ar_condition_scoped = true; > > + } > > } > > if (monitor_all_p) { > > *monitor_all_p = monitor_all; > > @@ -5739,6 +5756,22 @@ struct ed_type_route_exchange { > > bool sb_changes_pending; > > /* What the last run learned from the kernel routing tables. */ > > struct route_exchange_state *state; > > + /* True once the initial Advertised_Route dump for the local > > + * route-exchange datapaths is complete. Until then the destructive > > + * kernel sync is deferred so that we cannot delete routes that we > > + * installed previously but that are not yet present in our > (partial) > > + * view of the SB database. */ > > + bool initial_routes_loaded; > > + /* True once the first armed kernel sync has run. Unlike > > + * initial_routes_loaded, it is not cleared when the local > > + * route-exchange datapath set changes or on SB reconnect, so the > > + * sync can still run (to clean up VRFs and routes) once the set > > + * becomes empty. The start-up danger window in which an empty > > + * Advertised_Route view would delete routes installed before the > > + * restart is guarded by sb_all_data_loaded, which is false both > > + * during the start-up bootstrap and while the Advertised_Route > > + * table is reloaded after an SB reconnect. */ > > + bool initial_sync_completed; > > }; > > > > static void > > @@ -5789,6 +5822,51 @@ en_route_exchange_run(struct engine_node *node, > void *data) > > return EN_STALE; > > } > > > > + struct ed_type_route *route_data = > > + engine_get_input_data("route", node); > > + struct controller_engine_ctx *ctrl_ctx = > > + engine_get_context()->client_ctx; > > + > > + /* The set of local route-exchange datapaths selects the > Advertised_Route > > + * rows that are relevant to this chassis. Until the initial dump > of > > + * exactly those rows is complete, we must not run the destructive > kernel > > + * sync: it deletes every OVN route it does not see, so a partial > view > > + * would delete routes we installed before a restart that have not > > + * reached our view yet. With no local route-exchange datapaths > there is > > + * no scoped dump to wait for, so once the first armed sync has run > the > > + * view is complete and the sync may run to clean up the VRFs and > routes > > + * of the removed datapaths. */ > > + bool have_local_re_datapaths = > > + !hmap_is_empty(&route_data->announce_routes); > > + bool ready = ctrl_ctx->sb_all_data_loaded > > + && (have_local_re_datapaths > > + ? ctrl_ctx->sb_ar_condition_scoped > > I might be missing something, but I don't understand the need for the > sb_ar_condition_scoped boolean. > > Here, if route_data has announce_routes, then we determine readiness > by checking if sb_ar_condition_scoped is true. sb_ar_condition_scoped > will be true under two circumstances: > 1) monitor_all is enabled. > 2) There are local_datapaths in runtime_data. > > For (1), we can, for all intents and purposes, think of > sb_ar_condition_scoped as being always true. > For (2), in order for route_data to have announce_routes, it had to > set those based on local_datapaths from the runtime_data. > > I did notice that sb_ar_condition_scoped can be set to false if the > IDL disconnects from the OVSDB server. We set the var false, then on > the next engine run, we'll potentially set it back to true for one of > the two reasons listed above after the engine run is complete. This > means that if the IDL disconnects, then we can have one of two > situations occur during the first engine run after reconnecting: > > 1) The recompute results in no route_data->announce_routes. > 2) The recompute results in route_data->announce_routes being true. > > In case (1), we may determine ourselves ready since > re->initial_sync_completed may have been set true on a previous call > to en_route_exchange_run(). In case (2), we'll always see ourselves as > unready since sb_ar_condition_scoped was set false before the engine > run. > > To me this seems strange. If the main issue is about startup latency, > then why would we declare ourselves unready in the situation where we > disconnect from the OVSDB server, and the next engine run after > reconnecting results in announce_routes being present? Moreover, why > would we consider ourselves ready if there are no announce_routes? > > IMO, I think the ready boolean should be set as: > > bool ready = ctrl_ctx->sb_all_data_loaded && (have_local_re_datapaths > || re->initial_sync_completed); > > This way, we won't run if the OVSDB monitor conditions are out of > whack. On startup, we'll run as soon as announce_routes are detected > as present for the first time. Then on any IDL disconnects, since we > finished an initial sync, we'll declare ourselves ready every time > from then on. This would allow us to get rid of the > sb_ar_condition_scoped field altogether. > > What do you think? > I think we need sb_ar_condition_scoped. sb_all_data_loaded represents if the dump of the current sb condition is complete while sb_ar_condition_scoped is if the current condition is one that actually selects the local datapaths route exchange rows. At startup we initially add no monitor for advertised routes and build up the advertised route condition per local datapath on runs of update_sb_monitors(). So it is possible that all the data for the current condition is loaded but none of the advertised routes are because we have no condition for them. The issue is ordering, On the iteration where a local route_exchange datapath first appears in the engine's view, the engine already sees announce_routes non-empty, but the IDL's effective AR condition is still the last iterations, the empty one, so sb_ar_condition_scoped is still false. Then update_sb_monitors() runs after the engine_run() which updates sb_ar_condition_scoped and we can be sure we have all the needed data in the next iteration. Does that make sense? > > > + : re->initial_sync_completed); > > + > > + if (re->initial_routes_loaded && !ready) { > > + /* The set of local route-exchange datapaths changed or the SB > > + * connection reset; defer the destructive sync again until the > new > > + * scoped dump is complete. */ > > + re->initial_routes_loaded = false; > > + VLOG_INFO("Route-exchange datapath set changed; deferring route > " > > + "sync until the Advertised_Route dump completes."); > > + return EN_UNCHANGED; > > + } > > + if (!re->initial_routes_loaded) { > > + if (!ready) { > > + return EN_UNCHANGED; /* still loading; kernel untouched */ > > + } > > + re->initial_routes_loaded = true; > > + /* Past the start-up danger window from now on: the view has > been > > + * complete enough to run the destructive sync once, so a later > > + * transition to an empty local route-exchange datapath set (the > > + * last dynamic-routing router removed) may run it to clean up. > */ > > + re->initial_sync_completed = true; > > + VLOG_INFO("Advertised_Route dump complete for local > route-exchange " > > + "datapaths; enabling route sync."); > > + /* Fall through: run once now to reconcile the kernel. */ > > + } > > + > > vector_clear(&rt_notify->watches); > > > > struct route_exchange_ctx_in r_ctx_in; > > @@ -5811,6 +5889,13 @@ route_exchange_route_table_handler(struct > engine_node *node, void *data) > > struct ed_type_route_table_notify *rt_notify = > > engine_get_input_data("route_table_notify", node); > > > > + /* While the initial dump is still pending the full run has not > happened > > + * yet, so no kernel route watches are registered and nothing was > lost. > > + * The first armed run reconciles the kernel from scratch. */ > > + if (!re->initial_routes_loaded) { > > + return EN_HANDLED_UNCHANGED; > > + } > > + > > /* We were not told about every change, so the routes we know of > are not > > * necessarily the ones the kernel has. */ > > if (rt_notify->resync) { > > @@ -7394,6 +7479,10 @@ inc_proc_ovn_controller_init( > > engine_add_input(&en_route_exchange, &en_route_exchange_status, > NULL); > > engine_add_input(&en_route_exchange, &en_sb_ro, > > route_exchange_sb_ro_handler); > > + /* Re-evaluate the initial-dump readiness gate whenever the SB > monitor > > + * conditions change (including the final ack of the scoped > > + * Advertised_Route dump). */ > > + engine_add_input(&en_route_exchange, &en_sb_cond_seqno, NULL); > > > > engine_add_input(&en_addr_sets, &en_sb_address_set, > > addr_sets_sb_address_set_handler); > > @@ -8171,6 +8260,8 @@ main(int argc, char *argv[]) > > .if_mgr = if_status_mgr_create(), > > .ovnsb_expected_cond_seqno = &ovnsb_expected_cond_seqno, > > .sb_monitor_all = &sb_monitor_all, > > + .sb_all_data_loaded = false, > > + .sb_ar_condition_scoped = false, > > }; > > struct if_status_mgr *if_mgr = ctrl_engine_ctx.if_mgr; > > > > @@ -8240,6 +8331,10 @@ main(int argc, char *argv[]) > > if (!new_ovnsb_cond_seqno) { > > VLOG_INFO("OVNSB IDL reconnected, force recompute."); > > engine_set_force_recompute(); > > + /* The monitor conditions are re-applied on reconnect, > so the > > + * previously scoped Advertised_Route condition no > longer > > + * holds. */ > > + ctrl_engine_ctx.sb_ar_condition_scoped = false; > > } > > ovnsb_cond_seqno = new_ovnsb_cond_seqno; > > } > > @@ -8254,6 +8349,18 @@ main(int argc, char *argv[]) > > daemon_started_recently_ignore(); > > } > > > > + /* Whether the rows selected by the current monitor conditions > are all > > + * present in the IDL. This is recomputed every loop iteration > so > > + * that engine nodes can rely on it being up to date for the > > + * conditions that were applied so far. > > + * If we have sb_monitor_all that means we have all data that > we would > > + * ever need. */ > > + ctrl_engine_ctx.sb_all_data_loaded = > > + sb_monitor_all || > > + (ovnsb_cond_seqno == ovnsb_expected_cond_seqno > > + && ovnsb_expected_cond_seqno != UINT_MAX > > + && ovnsb_cond_seqno != 0); > > + > > struct engine_context eng_ctx = { > > .ovs_idl_txn = ovs_idl_txn, > > .ovnsb_idl_txn = ovnsb_idl_txn, > > @@ -8574,6 +8681,15 @@ main(int argc, char *argv[]) > > > &runtime_data->lbinding_data.bindings, > > &runtime_data->local_datapaths, > > sb_monitor_all); > > + /* The Advertised_Route condition now > covers the > > + * local datapaths (or everything, in > > + * monitor-all mode). If there are no local > > + * datapaths yet, there is nothing to cover > and > > + * the empty condition is sufficient. */ > > + ctrl_engine_ctx.sb_ar_condition_scoped = > > + sb_monitor_all || > > + !hmap_is_empty( > > + &runtime_data->local_datapaths); > > bool condition_changed = ovnsb_cond_seqno != > > > ovnsb_expected_cond_seqno; > > if (had_all_data && condition_changed) { > > diff --git a/tests/ovn-inc-proc-graph-dump.at b/tests/ > ovn-inc-proc-graph-dump.at > > index 8348c3467..a388f3d25 100644 > > --- a/tests/ovn-inc-proc-graph-dump.at > > +++ b/tests/ovn-inc-proc-graph-dump.at > > @@ -469,6 +469,7 @@ digraph "Incremental-Processing-Engine" { > > SB_learned_route [[style=filled, shape=box, fillcolor=white, > label="SB_learned_route"]]; > > route_table_notify [[style=filled, shape=box, fillcolor=white, > label="route_table_notify"]]; > > route_exchange_status [[style=filled, shape=box, > fillcolor=white, label="route_exchange_status"]]; > > + sb_cond_seqno [[style=filled, shape=box, fillcolor=white, > label="sb_cond_seqno"]]; > > route_exchange [[style=filled, shape=box, fillcolor=white, > label="route_exchange"]]; > > OVS_open_vswitch -> route_exchange [[label=""]]; > > SB_chassis -> route_exchange [[label=""]]; > > @@ -478,6 +479,7 @@ digraph "Incremental-Processing-Engine" { > > route_table_notify -> route_exchange > [[label="route_exchange_route_table_handler"]]; > > route_exchange_status -> route_exchange [[label=""]]; > > sb_ro -> route_exchange [[label="route_exchange_sb_ro_handler"]]; > > + sb_cond_seqno -> route_exchange [[label=""]]; > > garp_rarp [[style=filled, shape=box, fillcolor=white, > label="garp_rarp"]]; > > OVS_open_vswitch -> garp_rarp [[label=""]]; > > SB_chassis -> garp_rarp [[label=""]]; > > @@ -497,7 +499,6 @@ digraph "Incremental-Processing-Engine" { > > SB_acl_id [[style=filled, shape=box, fillcolor=white, > label="SB_acl_id"]]; > > acl_id [[style=filled, shape=box, fillcolor=white, > label="acl_id"]]; > > SB_acl_id -> acl_id [[label=""]]; > > - sb_cond_seqno [[style=filled, shape=box, fillcolor=white, > label="sb_cond_seqno"]]; > > datapaths_updated [[style=filled, shape=box, fillcolor=white, > label="datapaths_updated"]]; > > sb_cond_seqno -> datapaths_updated > [[label="datapaths_update_sb_cond_handler"]]; > > runtime_data -> datapaths_updated > [[label="datapaths_updated_runtime_data_handler"]]; > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index 13e62bf9b..e0f1c6f0d 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -15739,6 +15739,12 @@ blackhole 198.51.100.0/24 proto ovn metric 1000 > > 233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 30 > onlink > > 233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 40 > onlink]) > > > > +# Route exchange defers its kernel sync until the initial > Advertised_Route > > +# dump for the local route-exchange datapaths is complete. The routes > above > > +# can only be installed once that happens, so verify the deferred sync > was > > +# actually enabled. > > +OVS_WAIT_UNTIL([test "$(grep -c 'Advertised_Route dump complete for > local route-exchange datapaths' ovn-controller.log)" -ge 1]) > > + > > # Changing the vrf name will switch to the new one. > > # The old vrf will be removed. > > check ovn-nbctl --wait=hv set Logical_Router_Port internet-phys \ > > @@ -15757,7 +15763,10 @@ blackhole 198.51.100.0/24 proto ovn metric 1000 > > 233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 30 > onlink > > 233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 40 > onlink]) > > > > -# Stopping with --restart will not touch the routes. > > +# Stopping with --restart will not touch the routes. While the restarted > > +# controller loads its Advertised_Routes the kernel sync is deferred, > so a > > +# controller that synced from a partial view would have deleted the OVN > routes > > +# below; verify they all survive the restart. > > OVN_CONTROLLER_EXIT([],[--restart]) > > OVN_ROUTE_EQUAL([ovnvrf1338], [dnl > > blackhole 192.0.2.1 proto ovn metric 100 > > -- > > 2.55.0 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
