On Tue, Oct 13, 2026 at 3:09 PM Mark Michelson <[email protected]> wrote:
> On Sun, Oct 11, 2026 at 8:48 PM Jacob Tanenbaum <[email protected]> > wrote: > > > > > > > > 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? > > OK, I get it now. I didn't realize that update_sb_monitors() was > dynamically updating what we were monitoring. The idea is that on > engine_run 1, a SB datapath may get classified as "local". However, on > this first run, we are not monitoring the Advertised_Route table for > routes pertaining to this datapath. After the engine run completes, > update_sb_monitors() adds a monitor for Advertised_Routes for that > particular datapath so that the next IDL run can potentially pull in > the Advertised_Routes for that datapath. This way, on engine_run 2, we > know that the Advertised_Routes are being monitored for this datapath. > The sb_ar_condition_scoped variable gets set true when we have local > datapaths, and therefore we are now known to be monitoring the > Advertised_Route table in some capacity. The idea is that if any rows > of Advertised_Route are being monitored at all, then it is safe for > en_route_exchange_run() to continue. > > This makes a lot more sense now, and it also now has made me think of > a different potential issue we could see. What happens if we restart > and there are no local datapaths for quite a long time? We could be > put in a position where we are keeping dead routes around that should > be deleted. Should there be some sort of failsafe added so that if > some period of time passes after startup with no local_datapaths, we > can assume that our routes are dead and we should delete them? > I hadn't considered that; let me investigate and get back to you. > > > > > > >> > >> > >> > + : 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
