On Wed, Sep 16, 2026 at 5:31 AM Ales Musil <[email protected]> wrote: > > > > On Thu, Sep 10, 2026 at 9:52 AM Han Zhou <[email protected]> wrote: >> >> Every notification about a change to a kernel routing table made >> route_exchange recompute, which reads all watched tables, for both address >> families, and rebuilds the Learned_Route rows of every datapath that uses >> them. A routing daemon that reconverges sends a burst of such >> notifications, and none of them says anything about the tables OVN is not >> interested in. >> >> Keep the routes read from a table and let the notifications maintain them. >> A route change is applied to the routes of its table, and only the >> Learned_Route rows of that table are written again. A change to a route OVN >> does not learn from, e.g. one a user configured in the VRF, ends there. >> >> Reading the tables again is left to the cases that need it: the routes OVN >> advertises or the datapaths that distribute them changed, notifications were >> missed, or the southbound database cannot be written to right now. >> >> With 10000 routes learned from a single table, handling one route >> notification takes around 40ms, where reading that table again takes 94ms. >> What is saved is the netlink dump of both address families. Resolving the >> routes of the table and reconciling the Learned_Route rows of its datapaths >> remain, and both still scale with the size of the table rather than with the >> size of the change. >> >> Assisted-by: Claude Opus 5, Cursor >> Signed-off-by: Han Zhou <[email protected]> >> --- > > > Hi Han, > > thank you for the patch. I have two small comments down below. > >> >> .../topics/dynamic-routing/architecture.rst | 7 +- >> controller/ovn-controller.c | 112 ++++++--- >> controller/route-exchange.c | 219 ++++++++++++++++-- >> controller/route-exchange.h | 28 ++- >> tests/ovn-inc-proc-graph-dump.at | 2 +- >> tests/system-ovn-netlink.at | 74 ++++++ >> tests/system-ovn.at | 39 ++++ >> tests/test-ovn-netlink.c | 76 ++++++ >> 8 files changed, 508 insertions(+), 49 deletions(-) >> >> diff --git a/Documentation/topics/dynamic-routing/architecture.rst b/Documentation/topics/dynamic-routing/architecture.rst >> index e4be2ccf874d..cf7de2b79d28 100644 >> --- a/Documentation/topics/dynamic-routing/architecture.rst >> +++ b/Documentation/topics/dynamic-routing/architecture.rst >> @@ -346,8 +346,11 @@ routing daemons. This monitoring is performed via Netlink route >> notifications (``RTNLGRP_IPV4_ROUTE`` and ``RTNLGRP_IPV6_ROUTE``). >> >> When a route change is detected in a watched VRF table, >> -``ovn-controller`` dumps the table contents and processes each route. >> -The following filtering rules apply: >> +``ovn-controller`` applies it to the routes it already knows about in that >> +table and updates the ``Learned_Route`` records that follow from them. The >> +table itself is read in full only when ``ovn-controller`` starts watching it, >> +when the configuration of the logical routers using it changes, or when a >> +notification was missed. The following filtering rules apply: >> >> - Routes with protocol ``RTPROT_OVN`` are **skipped** because they were >> installed by ``ovn-controller`` itself (advertised routes). >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >> index 244489b18d50..eb7c1c58c040 100644 >> --- a/controller/ovn-controller.c >> +++ b/controller/ovn-controller.c >> @@ -5716,6 +5716,9 @@ struct ed_type_route_table_notify { >> /* Routes ('struct ovn_route_msg *', owned) the last run was told about, >> * limited to the tables in 'watches'. */ >> struct vector changed_routes; >> + /* Set when notifications were missed, in which case 'changed_routes' does >> + * not describe everything that happened to the watched tables. */ >> + bool resync; >> }; >> >> static void >> @@ -5734,34 +5737,17 @@ struct ed_type_route_exchange { >> /* Set to true when SB is readonly and we have routes that need >> * to be inserted into SB. */ >> bool sb_changes_pending; >> + /* What the last run learned from the kernel routing tables. */ >> + struct route_exchange_state *state; >> }; >> >> -static enum engine_node_state >> -en_route_exchange_run(struct engine_node *node, void *data) >> +static void >> +route_exchange_ctx_init(struct engine_node *node, >> + struct route_exchange_ctx_in *r_ctx_in, >> + struct route_exchange_ctx_out *r_ctx_out) >> { >> - struct ed_type_route_exchange *re = data; >> - struct ovsdb_idl_index *sbrec_learned_route_by_datapath = >> - engine_ovsdb_node_get_index( >> - engine_get_input("SB_learned_route", node), >> - "datapath"); >> - >> - struct ovsdb_idl_index *sbrec_port_binding_by_name = >> - engine_ovsdb_node_get_index( >> - engine_get_input("SB_port_binding", node), >> - "name"); >> - struct ed_type_route *route_data = >> - engine_get_input_data("route", node); >> - struct ed_type_route_table_notify *rt_notify = >> - engine_get_input_data("route_table_notify", node); >> - >> - /* There can not actually be any routes to advertise unless we also have >> - * the Learned_Route table, since they where introduced in the same >> - * release. */ >> - if (!sbrec_server_has_learned_route_table(re->sb_idl)) { >> - return EN_STALE; >> - } >> + struct ed_type_route *route_data = engine_get_input_data("route", node); >> >> - vector_clear(&rt_notify->watches); >> const struct ovsrec_open_vswitch_table *ovs_table = >> EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); >> const char *chassis_id = get_ovs_chassis_id(ovs_table); >> @@ -5775,19 +5761,42 @@ en_route_exchange_run(struct engine_node *node, void *data) >> = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); >> ovs_assert(chassis); >> >> - struct route_exchange_ctx_in r_ctx_in = { >> + *r_ctx_in = (struct route_exchange_ctx_in) { >> .ovnsb_idl_txn = engine_get_context()->ovnsb_idl_txn, >> - .sbrec_learned_route_by_datapath = sbrec_learned_route_by_datapath, >> - .sbrec_port_binding_by_name = sbrec_port_binding_by_name, >> + .sbrec_learned_route_by_datapath = engine_ovsdb_node_get_index( >> + engine_get_input("SB_learned_route", node), "datapath"), >> + .sbrec_port_binding_by_name = engine_ovsdb_node_get_index( >> + engine_get_input("SB_port_binding", node), "name"), >> .chassis = chassis, >> .announce_routes = &route_data->announce_routes, >> }; >> - struct route_exchange_ctx_out r_ctx_out = { >> + *r_ctx_out = (struct route_exchange_ctx_out) { >> .sb_changes_pending = false, >> - .route_table_watches = &rt_notify->watches, >> }; >> +} >> + >> +static enum engine_node_state >> +en_route_exchange_run(struct engine_node *node, void *data) >> +{ >> + struct ed_type_route_exchange *re = data; >> + struct ed_type_route_table_notify *rt_notify = >> + engine_get_input_data("route_table_notify", node); >> >> - route_exchange_run(&r_ctx_in, &r_ctx_out); >> + /* There can not actually be any routes to advertise unless we also have >> + * the Learned_Route table, since they where introduced in the same >> + * release. */ >> + if (!sbrec_server_has_learned_route_table(re->sb_idl)) { >> + return EN_STALE; >> + } >> + >> + vector_clear(&rt_notify->watches); >> + >> + struct route_exchange_ctx_in r_ctx_in; >> + struct route_exchange_ctx_out r_ctx_out; >> + route_exchange_ctx_init(node, &r_ctx_in, &r_ctx_out); >> + r_ctx_out.route_table_watches = &rt_notify->watches; >> + >> + route_exchange_run(re->state, &r_ctx_in, &r_ctx_out); >> route_table_notify_update(&rt_notify->watches); >> >> re->sb_changes_pending = r_ctx_out.sb_changes_pending; >> @@ -5795,6 +5804,39 @@ en_route_exchange_run(struct engine_node *node, void *data) >> return EN_UPDATED; >> } >> >> +static enum engine_input_handler_result >> +route_exchange_route_table_handler(struct engine_node *node, void *data) >> +{ >> + struct ed_type_route_exchange *re = data; >> + struct ed_type_route_table_notify *rt_notify = >> + engine_get_input_data("route_table_notify", node); >> + >> + /* 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) { >> + return EN_UNHANDLED; >> + } >> + >> + struct route_exchange_ctx_in r_ctx_in; >> + struct route_exchange_ctx_out r_ctx_out; >> + route_exchange_ctx_init(node, &r_ctx_in, &r_ctx_out); >> + >> + switch (route_exchange_handle_route_changes(re->state, &r_ctx_in, >> + &r_ctx_out, >> + &rt_notify->changed_routes)) { >> + case ROUTE_EXCHANGE_UNHANDLED: >> + return EN_UNHANDLED; >> + case ROUTE_EXCHANGE_UNCHANGED: >> + return EN_HANDLED_UNCHANGED; >> + case ROUTE_EXCHANGE_UPDATED: >> + break; >> + } >> + >> + re->sb_changes_pending |= r_ctx_out.sb_changes_pending; >> + >> + return EN_HANDLED_UPDATED; >> +} >> + >> static enum engine_input_handler_result >> route_exchange_sb_ro_handler(struct engine_node *node OVS_UNUSED, void *data) >> { >> @@ -5814,12 +5856,15 @@ en_route_exchange_init(struct engine_node *node OVS_UNUSED, >> struct ed_type_route_exchange *re = xzalloc(sizeof *re); >> >> re->sb_idl = arg->sb_idl; >> + re->state = route_exchange_state_create(); >> return re; >> } >> >> static void >> -en_route_exchange_cleanup(void *data OVS_UNUSED) >> +en_route_exchange_cleanup(void *data) >> { >> + struct ed_type_route_exchange *re = data; >> + route_exchange_state_destroy(re->state); >> } >> >> /* The route_table_notify node is an input node, but the watches are >> @@ -5837,11 +5882,13 @@ en_route_table_notify_run(struct engine_node *node OVS_UNUSED, void *data) >> struct ed_type_route_table_notify *rtn = data; >> >> route_table_notify_clear_changes(rtn); >> + rtn->resync = false; >> >> for (size_t i = 0; i < ARRAY_SIZE(route_notifiers); i++) { >> /* We cannot tell whether a table we watch was among the changes we >> * missed, so assume it was. */ >> if (ovn_netlink_notifier_lost(route_notifiers[i])) { >> + rtn->resync = true; >> state = EN_UPDATED; >> } >> >> @@ -7346,7 +7393,8 @@ inc_proc_ovn_controller_init( >> engine_noop_handler); >> engine_add_input(&en_route_exchange, &en_sb_port_binding, >> engine_noop_handler); >> - engine_add_input(&en_route_exchange, &en_route_table_notify, NULL); >> + engine_add_input(&en_route_exchange, &en_route_table_notify, >> + route_exchange_route_table_handler); >> 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); >> diff --git a/controller/route-exchange.c b/controller/route-exchange.c >> index 492697503381..20a1ac7cf564 100644 >> --- a/controller/route-exchange.c >> +++ b/controller/route-exchange.c >> @@ -31,6 +31,7 @@ >> #include "binding.h" >> #include "ha-chassis.h" >> #include "local_data.h" >> +#include "nexthop-exchange.h" >> #include "route.h" >> #include "route-exchange.h" >> #include "route-exchange-netlink.h" >> @@ -43,6 +44,19 @@ struct maintained_route_table_entry { >> uint32_t table_id; >> }; >> >> +/* What route_exchange knows about one kernel routing table it syncs. */ >> +struct route_table_state { >> + struct hmap_node node; >> + uint32_t table_id; >> + /* Routes of the table OVN learns from (struct re_nl_cached_route). */ >> + struct hmap learned_routes; >> +}; >> + >> +struct route_exchange_state { >> + /* Contains 'struct route_table_state', by table id. */ >> + struct hmap tables; >> +}; >> + >> static struct hmap _maintained_route_tables = >> HMAP_INITIALIZER(&_maintained_route_tables); >> static struct sset _maintained_vrfs = SSET_INITIALIZER(&_maintained_vrfs); >> @@ -86,6 +100,69 @@ maintained_route_table_add(uint32_t table_id) >> hmap_insert(&_maintained_route_tables, &mrt->node, hash); >> } >> >> +static struct route_table_state * >> +route_table_state_find(const struct route_exchange_state *state, >> + uint32_t table_id) >> +{ >> + struct route_table_state *rt; >> + HMAP_FOR_EACH_WITH_HASH (rt, node, maintained_route_table_hash(table_id), >> + &state->tables) { >> + if (rt->table_id == table_id) { >> + return rt; >> + } >> + } >> + >> + return NULL; >> +} >> + >> +static struct route_table_state * >> +route_table_state_get(struct route_exchange_state *state, uint32_t table_id) >> +{ >> + struct route_table_state *rt = route_table_state_find(state, table_id); >> + if (rt) { >> + return rt; >> + } >> + >> + rt = xmalloc(sizeof *rt); >> + rt->table_id = table_id; >> + hmap_init(&rt->learned_routes); >> + hmap_insert(&state->tables, &rt->node, >> + maintained_route_table_hash(table_id)); >> + >> + return rt; >> +} >> + >> +static void >> +route_table_state_destroy(struct route_exchange_state *state, >> + struct route_table_state *rt) >> +{ >> + hmap_remove(&state->tables, &rt->node); >> + re_nl_cached_routes_clear(&rt->learned_routes); >> + hmap_destroy(&rt->learned_routes); >> + free(rt); >> +} >> + >> +struct route_exchange_state * >> +route_exchange_state_create(void) >> +{ >> + struct route_exchange_state *state = xmalloc(sizeof *state); >> + >> + hmap_init(&state->tables); >> + return state; >> +} >> + >> +void >> +route_exchange_state_destroy(struct route_exchange_state *state) >> +{ >> + struct route_table_state *rt; >> + HMAP_FOR_EACH_SAFE (rt, node, &state->tables) { >> + route_table_state_destroy(state, rt); >> + } >> + >> + hmap_destroy(&state->tables); >> + free(state); >> +} >> + >> static struct route_entry * >> route_add_entry(struct hmap *routes, >> const struct sbrec_learned_route *sb_route, >> @@ -353,6 +430,37 @@ advertised_routes_tables(const struct hmapx *datapaths, >> } >> } >> >> +static struct advertised_routes_entry * >> +advertised_routes_find(const struct hmap *advertised_routes, uint32_t table_id) >> +{ >> + struct advertised_routes_entry *arte; >> + HMAP_FOR_EACH_WITH_HASH (arte, node, >> + maintained_route_table_hash(table_id), >> + advertised_routes) { >> + if (arte->table_id == table_id) { >> + return arte; >> + } >> + } >> + >> + return NULL; >> +} >> + >> +/* Maps every routing table OVN distributes routes into to the datapaths that >> + * do so. */ >> +static void >> +advertised_routes_build(struct hmap *advertised_routes, >> + const struct hmap *announce_routes) >> +{ >> + const struct advertise_datapath_entry *ad; >> + HMAP_FOR_EACH (ad, node, announce_routes) { >> + uint32_t table_id = route_get_table_id(ad->db); >> + >> + if (TABLE_ID_VALID(table_id)) { >> + advertised_routes_add(advertised_routes, ad, table_id); >> + } >> + } >> +} >> + >> static void >> advertised_routes_destroy(struct hmap *advertised_routes) >> { >> @@ -364,12 +472,11 @@ advertised_routes_destroy(struct hmap *advertised_routes) >> hmap_destroy(advertised_routes); >> } >> >> -/* Turns 'learned_routes', the routes of a kernel routing table OVN may learn >> - * from, into Learned_Route rows of every datapath in 'datapaths' ('struct >> - * advertise_datapath_entry *'). */ >> +/* Turns the routes OVN learns from the table 'rt' into Learned_Route rows of >> + * every datapath in 'datapaths' ('struct advertise_datapath_entry *'). */ >> static void >> -resolve_and_sync_learned_routes( >> - const struct hmap *learned_routes, const struct hmapx *datapaths, >> +route_table_resolve_and_sync( >> + struct route_table_state *rt, const struct hmapx *datapaths, >> const struct route_exchange_ctx_in *r_ctx_in, >> struct route_exchange_ctx_out *r_ctx_out) >> { >> @@ -377,7 +484,7 @@ resolve_and_sync_learned_routes( >> VECTOR_EMPTY_INITIALIZER(struct re_nl_received_route_node); >> >> const struct re_nl_cached_route *cr; >> - HMAP_FOR_EACH (cr, node, learned_routes) { >> + HMAP_FOR_EACH (cr, node, &rt->learned_routes) { >> re_nl_resolve_route(cr->msg, &received_routes); >> } >> >> @@ -397,8 +504,87 @@ resolve_and_sync_learned_routes( >> vector_destroy(&received_routes); >> } >> >> +/* Redoes the Learned_Route rows of every table in 'changed_tables' ('struct >> + * route_table_state'). */ >> +static enum route_exchange_handled >> +resync_changed_tables(const struct hmapx *changed_tables, >> + const struct route_exchange_ctx_in *r_ctx_in, >> + struct route_exchange_ctx_out *r_ctx_out) >> +{ >> + if (hmapx_is_empty(changed_tables)) { >> + return ROUTE_EXCHANGE_UNCHANGED; >> + } >> + >> + if (!r_ctx_in->ovnsb_idl_txn) { >> + /* Without a transaction we could only record that there are changes >> + * left to write, which a full run does better. */ >> + return ROUTE_EXCHANGE_UNHANDLED; >> + } >> + >> + struct hmap advertised_routes = HMAP_INITIALIZER(&advertised_routes); >> + enum route_exchange_handled handled = ROUTE_EXCHANGE_UPDATED; >> + >> + advertised_routes_build(&advertised_routes, r_ctx_in->announce_routes); >> + >> + struct hmapx_node *hn; >> + HMAPX_FOR_EACH (hn, changed_tables) { >> + struct route_table_state *rt = hn->data; >> + const struct advertised_routes_entry *arte = >> + advertised_routes_find(&advertised_routes, rt->table_id); >> + >> + if (!arte) { >> + /* The datapaths distributing routes into the table are not the >> + * ones it was synced for. */ >> + handled = ROUTE_EXCHANGE_UNHANDLED; >> + break; >> + } >> + >> + route_table_resolve_and_sync(rt, &arte->datapaths, r_ctx_in, >> + r_ctx_out); >> + } >> + >> + advertised_routes_destroy(&advertised_routes); >> + >> + return handled; >> +} >> + >> +enum route_exchange_handled >> +route_exchange_handle_route_changes( >> + struct route_exchange_state *state, >> + const struct route_exchange_ctx_in *r_ctx_in, >> + struct route_exchange_ctx_out *r_ctx_out, >> + const struct vector *changed_routes) >> +{ >> + struct hmapx changed_tables = HMAPX_INITIALIZER(&changed_tables); >> + enum route_exchange_handled handled; >> + >> + const struct ovn_route_msg *msg; >> + VECTOR_FOR_EACH (changed_routes, msg) { >> + struct route_table_state *rt = >> + route_table_state_find(state, msg->table_id); >> + if (!rt) { >> + /* A table we have not read, so we do not know the rest of it >> + * either. */ >> + handled = ROUTE_EXCHANGE_UNHANDLED; >> + goto out; >> + } >> + >> + if (re_nl_cached_routes_apply(&rt->learned_routes, msg)) { >> + hmapx_add(&changed_tables, rt); >> + } >> + } >> + >> + handled = resync_changed_tables(&changed_tables, r_ctx_in, r_ctx_out); >> + >> +out: >> + hmapx_destroy(&changed_tables); >> + >> + return handled; >> +} >> + >> void >> -route_exchange_run(const struct route_exchange_ctx_in *r_ctx_in, >> +route_exchange_run(struct route_exchange_state *state, >> + const struct route_exchange_ctx_in *r_ctx_in, >> struct route_exchange_ctx_out *r_ctx_out) >> { >> struct hmap advertised_routes = HMAP_INITIALIZER(&advertised_routes); >> @@ -444,26 +630,33 @@ route_exchange_run(const struct route_exchange_ctx_in *r_ctx_in, >> advertised_routes_add(&advertised_routes, ad, table_id); >> } >> >> - struct hmap learned_routes = HMAP_INITIALIZER(&learned_routes); >> struct advertised_routes_entry *arte; >> HMAP_FOR_EACH (arte, node, &advertised_routes) { >> maintained_route_table_add(arte->table_id); >> >> + struct route_table_state *rt = route_table_state_get(state, >> + arte->table_id); >> struct vector route_tables = >> VECTOR_EMPTY_INITIALIZER(const struct hmap *); >> advertised_routes_tables(&arte->datapaths, &route_tables); >> >> error = re_nl_sync_routes(arte->table_id, &route_tables, >> - &learned_routes); >> + &rt->learned_routes); >> SET_ROUTE_EXCHANGE_NL_STATUS(error); >> vector_destroy(&route_tables); >> >> - resolve_and_sync_learned_routes(&learned_routes, &arte->datapaths, >> - r_ctx_in, r_ctx_out); >> + route_table_resolve_and_sync(rt, &arte->datapaths, r_ctx_in, >> + r_ctx_out); >> vector_push(r_ctx_out->route_table_watches, &arte->table_id); >> } >> - re_nl_cached_routes_clear(&learned_routes); >> - hmap_destroy(&learned_routes); >> + >> + /* Forget the tables we do not sync anymore. */ >> + struct route_table_state *rt; >> + HMAP_FOR_EACH_SAFE (rt, node, &state->tables) { >> + if (!advertised_routes_find(&advertised_routes, rt->table_id)) { >> + route_table_state_destroy(state, rt); >> + } >> + } >> >> /* Remove routes in tables previously maintained by us. */ >> struct maintained_route_table_entry *mrt; >> diff --git a/controller/route-exchange.h b/controller/route-exchange.h >> index a1ef4a359dfc..f52ea505585c 100644 >> --- a/controller/route-exchange.h >> +++ b/controller/route-exchange.h >> @@ -31,12 +31,38 @@ struct route_exchange_ctx_in { >> }; >> >> struct route_exchange_ctx_out { >> + /* Populated by route_exchange_run() only, NULL otherwise. */ > > > Nit: This comment seems unnecessary. > >> >> struct vector *route_table_watches; >> bool sb_changes_pending; >> }; >> >> -void route_exchange_run(const struct route_exchange_ctx_in *, >> +/* What route_exchange knows about the kernel routing tables it syncs, kept >> + * between runs so that a change to one of them can be applied without reading >> + * them all again. */ >> +struct route_exchange_state; >> + >> +struct route_exchange_state *route_exchange_state_create(void); >> +void route_exchange_state_destroy(struct route_exchange_state *); >> + >> +void route_exchange_run(struct route_exchange_state *, >> + const struct route_exchange_ctx_in *, >> struct route_exchange_ctx_out *); >> + >> +enum route_exchange_handled { >> + /* The change cannot be applied to what the last route_exchange_run() left >> + * behind, so it has to run again. */ >> + ROUTE_EXCHANGE_UNHANDLED, >> + /* The change does not affect the routes OVN learned. */ >> + ROUTE_EXCHANGE_UNCHANGED, >> + ROUTE_EXCHANGE_UPDATED, >> +}; >> + >> +/* Updates the routes OVN learned after the kernel reported the route changes >> + * in 'changed_routes' ('struct ovn_route_msg *'). */ >> +enum route_exchange_handled route_exchange_handle_route_changes( >> + struct route_exchange_state *, const struct route_exchange_ctx_in *, >> + struct route_exchange_ctx_out *, const struct vector *changed_routes); >> + > > > All of those updated and new function should have proper implementation in > route-exchange-stub.c. > >> >> void route_exchange_cleanup_vrfs(void); >> void route_exchange_destroy(void); >> >> diff --git a/tests/ovn-inc-proc-graph-dump.at b/tests/ ovn-inc-proc-graph-dump.at >> index 44bf5689e853..b0c29d1b5535 100644 >> --- a/tests/ovn-inc-proc-graph-dump.at >> +++ b/tests/ovn-inc-proc-graph-dump.at >> @@ -471,7 +471,7 @@ digraph "Incremental-Processing-Engine" { >> route -> route_exchange [[label=""]]; >> SB_learned_route -> route_exchange [[label="engine_noop_handler"]]; >> SB_port_binding -> route_exchange [[label="engine_noop_handler"]]; >> - route_table_notify -> route_exchange [[label=""]]; >> + 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"]]; >> garp_rarp [[style=filled, shape=box, fillcolor=white, label="garp_rarp"]]; >> diff --git a/tests/system-ovn-netlink.at b/tests/system-ovn-netlink.at >> index d534814a2ff5..69c25d6e9544 100644 >> --- a/tests/system-ovn-netlink.at >> +++ b/tests/system-ovn-netlink.at >> @@ -617,6 +617,80 @@ AT_CHECK_UNQUOTED([ovstest test-ovn-netlink route-table-notify \ >> >> AT_CLEANUP >> >> +AT_SETUP([sync netlink routes - table update]) >> +AT_KEYWORDS([netlink-routes]) >> +CHECK_VRF() >> + >> +table_id=100 >> + >> +check ip link add vrf-$table_id type vrf table $table_id >> +on_exit 'ip link del vrf-$table_id' >> +check ip link set dev vrf-$table_id up >> + >> +check ip link add lo-test type dummy >> +on_exit 'ip link del lo-test' >> +check ip link set lo-test master vrf-$table_id >> +check ip addr add 20.0.0.10/24 dev lo-test >> +check ip link set up lo-test >> + >> +dnl A route installed by a dynamic routing protocol is there from the start. >> +check ip route add 10.10.10.0/24 via 20.0.0.1 vrf vrf-$table_id proto zebra >> + >> +dnl A route added to the table joins the ones we know of. >> +AT_CHECK_UNQUOTED([ovstest test-ovn-netlink route-table-update $table_id \ >> + "ip route add 10.10.11.0/24 via 20.0.0.2 vrf vrf-$table_id proto zebra" \ >> + | sort], [0], [dnl >> +Applied route add >> +Route table_id=$table_id dst=10.10.10.0 plen=24 proto=11 priority=0 nexthop=20.0.0.1,dev=lo-test >> +Route table_id=$table_id dst=10.10.11.0 plen=24 proto=11 priority=0 nexthop=20.0.0.2,dev=lo-test >> +]) >> + >> +dnl A route removed from the table leaves them. >> +AT_CHECK_UNQUOTED([ovstest test-ovn-netlink route-table-update $table_id \ >> + "ip route del 10.10.11.0/24 vrf vrf-$table_id" | sort], [0], [dnl >> +Applied route delete >> +Route table_id=$table_id dst=10.10.10.0 plen=24 proto=11 priority=0 nexthop=20.0.0.1,dev=lo-test >> +]) >> + >> +dnl Sending a route somewhere else replaces the one we know of. >> +AT_CHECK_UNQUOTED([ovstest test-ovn-netlink route-table-update $table_id \ >> + "ip route replace 10.10.10.0/24 via 20.0.0.3 vrf vrf-$table_id proto zebra" \ >> + | sort], [0], [dnl >> +Applied route add >> +Route table_id=$table_id dst=10.10.10.0 plen=24 proto=11 priority=0 nexthop=20.0.0.3,dev=lo-test >> +]) >> + >> +dnl A route OVN does not learn, here one configured by a user, is left out. >> +AT_CHECK_UNQUOTED([ovstest test-ovn-netlink route-table-update $table_id \ >> + "ip route add 10.10.12.0/24 via 20.0.0.4 vrf vrf-$table_id proto static" \ >> + | sort], [0], [dnl >> +Ignored route add >> +Route table_id=$table_id dst=10.10.10.0 plen=24 proto=11 priority=0 nexthop=20.0.0.3,dev=lo-test >> +]) >> + >> +dnl The kernel keeps several routes to one prefix that differ only by their >> +dnl metric, so the metric is part of what tells them apart. >> +check ip route add 10.10.13.0/24 via 20.0.0.5 metric 100 vrf vrf-$table_id \ >> + proto zebra >> +AT_CHECK_UNQUOTED([ovstest test-ovn-netlink route-table-update $table_id \ >> + "ip route add 10.10.13.0/24 via 20.0.0.6 metric 200 vrf vrf-$table_id proto zebra" \ >> + | sort], [0], [dnl >> +Applied route add >> +Route table_id=$table_id dst=10.10.10.0 plen=24 proto=11 priority=0 nexthop=20.0.0.3,dev=lo-test >> +Route table_id=$table_id dst=10.10.13.0 plen=24 proto=11 priority=100 nexthop=20.0.0.5,dev=lo-test >> +Route table_id=$table_id dst=10.10.13.0 plen=24 proto=11 priority=200 nexthop=20.0.0.6,dev=lo-test >> +]) >> + >> +dnl Removing one of them keeps the other. >> +AT_CHECK_UNQUOTED([ovstest test-ovn-netlink route-table-update $table_id \ >> + "ip route del 10.10.13.0/24 metric 100 vrf vrf-$table_id" | sort], [0], [dnl >> +Applied route delete >> +Route table_id=$table_id dst=10.10.10.0 plen=24 proto=11 priority=0 nexthop=20.0.0.3,dev=lo-test >> +Route table_id=$table_id dst=10.10.13.0 plen=24 proto=11 priority=200 nexthop=20.0.0.6,dev=lo-test >> +]) >> + >> +AT_CLEANUP >> + >> AT_SETUP([sync netlink nexthops - learn nexthops]) >> AT_KEYWORDS([netlink-nexthops]) >> >> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >> index 799779276de6..766d838190c9 100644 >> --- a/tests/system-ovn.at >> +++ b/tests/system-ovn.at >> @@ -21045,6 +21045,45 @@ ip_prefix : "10.10.3.1" >> ip_prefix : "10.10.4.1" >> ]) >> >> +AS_BOX([$(date +%H:%M:%S.%03N) Learned routes follow the notifications]) >> + >> +# How many times ovn-controller rebuilt the routes it learns. A sync >> +# completes only after it ran the engine, and it reads the notifications right >> +# before running it, so a sync after a change to a routing table tells us what >> +# it decided to do about that change. >> +route_recomputes() { >> + ovn-appctl -t ovn-controller inc-engine/show-stats route_exchange recompute >> +} >> + >> +# The steps above leave work behind that would otherwise land in the middle of >> +# the checks below, wait for it to finish first. >> +check ovn-nbctl --wait=hv sync >> +OVS_WAIT_UNTIL([settled=$(route_recomputes); sleep 2; >> + test "$settled" = "$(route_recomputes)"]) >> + >> +# A route appearing in the table, or leaving it, is picked up from the >> +# notification about it rather than by reading the table again. >> +re_recompute=$(route_recomputes) >> +check ip route add 10.10.6.1 via 20.0.0.25 vrf vrf-$vni proto zebra >> +wait_row_count Learned_Route 1 ip_prefix=10.10.6.1 nexthop=20.0.0.25 >> +AT_CHECK_UNQUOTED([route_recomputes], [0], [$re_recompute >> +]) >> + >> +re_recompute=$(route_recomputes) >> +check ip route del 10.10.6.1 vrf vrf-$vni >> +wait_row_count Learned_Route 0 ip_prefix=10.10.6.1 >> +AT_CHECK_UNQUOTED([route_recomputes], [0], [$re_recompute >> +]) >> + >> +# A route OVN does not learn from does not even reach the tables it keeps. >> +re_recompute=$(route_recomputes) >> +check ip route add 10.10.7.1 via 20.0.0.25 vrf vrf-$vni >> +check ovn-nbctl --wait=hv sync >> +check_row_count Learned_Route 0 ip_prefix=10.10.7.1 >> +AT_CHECK_UNQUOTED([route_recomputes], [0], [$re_recompute >> +]) >> +check ip route del 10.10.7.1 vrf vrf-$vni >> + >> # Verify that we can have one router read and another write from the same >> # vrf table. >> # >> diff --git a/tests/test-ovn-netlink.c b/tests/test-ovn-netlink.c >> index 3c2c14dc5066..f75a6f295cab 100644 >> --- a/tests/test-ovn-netlink.c >> +++ b/tests/test-ovn-netlink.c >> @@ -369,6 +369,80 @@ test_nexthop_table_notify(struct ovs_cmdl_context *ctx) >> ovn_netlink_notifiers_destroy(); >> } >> >> +/* Reports the routes of 'table_id' OVN learns from after applying the changes >> + * caused by running 'shell_command' to them. Unlike "route-sync", which reads >> + * the whole table, this goes through the incremental update path. */ >> +static void >> +test_route_table_update(struct ovs_cmdl_context *ctx) >> +{ >> + static const enum ovn_netlink_notifier_type types[] = { >> + OVN_NL_NOTIFIER_ROUTE_V4, OVN_NL_NOTIFIER_ROUTE_V6, >> + }; >> + unsigned int shift = 1; >> + >> + unsigned int table_id; >> + if (!test_read_uint_value(ctx, shift++, "table id", &table_id)) { >> + return; >> + } >> + >> + const char *cmd = test_read_value(ctx, shift++, "shell_command"); >> + if (!cmd) { >> + return; >> + } >> + >> + struct hmap routes_to_advertise = HMAP_INITIALIZER(&routes_to_advertise); >> + struct hmap learned_routes = HMAP_INITIALIZER(&learned_routes); >> + struct vector route_tables = >> + VECTOR_EMPTY_INITIALIZER(const struct hmap *); >> + struct ds ds = DS_EMPTY_INITIALIZER; >> + >> + const struct hmap *routes = &routes_to_advertise; >> + vector_push(&route_tables, &routes); >> + >> + for (size_t i = 0; i < ARRAY_SIZE(types); i++) { >> + ovn_netlink_update_notifier(types[i], true); >> + } >> + ovs_assert(re_nl_sync_routes(table_id, &route_tables, >> + &learned_routes) == 0); >> + vector_destroy(&route_tables); >> + /* The routes are up to date, anything reported so far is among them. */ >> + for (size_t i = 0; i < ARRAY_SIZE(types); i++) { >> + ovn_netlink_notifier_flush(types[i]); >> + } >> + >> + run_command_under_notifier(cmd); >> + >> + for (size_t i = 0; i < ARRAY_SIZE(types); i++) { >> + struct vector *msgs = ovn_netlink_get_msgs(types[i]); >> + struct ovn_route_msg *msg; >> + >> + VECTOR_FOR_EACH (msgs, msg) { >> + if (msg->table_id != table_id) { >> + continue; >> + } >> + >> + printf("%s route %s\n", >> + re_nl_cached_routes_apply(&learned_routes, msg) >> + ? "Applied" : "Ignored", >> + msg->nlmsg_type == RTM_NEWROUTE ? "add" : "delete"); >> + } >> + ovn_netlink_notifier_flush(types[i]); >> + } >> + >> + const struct re_nl_cached_route *cr; >> + HMAP_FOR_EACH (cr, node, &learned_routes) { >> + ds_clear(&ds); >> + ovn_route_msg_format(&ds, cr->msg); >> + printf("Route %s\n", ds_cstr(&ds)); >> + } >> + >> + ds_destroy(&ds); >> + re_nl_cached_routes_clear(&learned_routes); >> + hmap_destroy(&learned_routes); >> + hmap_destroy(&routes_to_advertise); >> + ovn_netlink_notifiers_destroy(); >> +} >> + >> /* Dumps the nexthop table after applying the changes caused by running >> * 'shell_command' to it. Unlike "nexthop-sync", which builds the table from >> * scratch, this goes through the incremental update path. */ >> @@ -421,6 +495,8 @@ test_ovn_netlink(int argc, char *argv[]) >> {"route-sync", NULL, 1, INT_MAX, test_route_sync, OVS_RO}, >> {"route-table-notify", NULL, 1, 1, >> test_route_table_notify, OVS_RO}, >> + {"route-table-update", NULL, 2, 2, >> + test_route_table_update, OVS_RO}, >> {"nexthop-sync", NULL, 0, 0, test_nexthop_sync, OVS_RO}, >> {"nexthop-table-notify", NULL, 1, 1, >> test_nexthop_table_notify, OVS_RO}, >> -- >> 2.38.1 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > Other than that it looks good. > Acked-by: Ales Musil <[email protected]>
Thanks Ales for the review. I addressed all your comments and pushed to main. I will figure out backporting to release branches tomorrow. Best, Han > > Regards, > Ales > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
