Every notification about a kernel route or nexthop object made route_exchange recompute, which reads all watched routing 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 the ids of the nexthop objects they resolve through, 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. - A nexthop object change is looked for among the ids the routes of a table resolve through, and only the tables that use it are resolved again. The kernel nexthop table is shared with EVPN, whose changes now cost nothing here. 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. Measured with 10000 routes in one table. Before, a notification read the table again and cost 92 ms: 68 ms to dump it over netlink and parse the messages, 3 ms to resolve the routes and 21 ms to reconcile the Learned_Route rows. After, the same notification costs 25 ms because only the last two steps are left. That cost is still proportional to the size of the table the change is in and not to the change itself. Assisted-by: Claude Opus 5, Cursor Signed-off-by: Han Zhou <[email protected]> --- .../topics/dynamic-routing/architecture.rst | 7 +- controller/ovn-controller.c | 156 +++++++---- controller/route-exchange.c | 264 +++++++++++++++++- controller/route-exchange.h | 37 ++- tests/ovn-inc-proc-graph-dump.at | 2 +- tests/system-ovn-netlink.at | 74 +++++ tests/system-ovn.at | 25 +- tests/test-ovn-netlink.c | 70 +++++ 8 files changed, 553 insertions(+), 82 deletions(-) diff --git a/Documentation/topics/dynamic-routing/architecture.rst b/Documentation/topics/dynamic-routing/architecture.rst index 4a363b80a87c..df0059e74d1c 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 7fa8f2adb563..cea5b27c0a6a 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 @@ -5763,39 +5766,19 @@ 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; - /* Ids (struct nexthop_id_node) of the kernel nexthop objects the routes - * learned during the last run depend on. */ - struct hmap referenced_nhids; + /* 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); + struct ed_type_route *route_data = engine_get_input_data("route", node); struct ed_type_nexthop_exchange *nhe_data = engine_get_input_data("nexthop_exchange", 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; - } - - 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); @@ -5809,28 +5792,51 @@ en_route_exchange_run(struct engine_node *node, void *data) = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); ovs_assert(chassis); - nexthop_ids_clear(&re->referenced_nhids); - - /* Routes we learn may reference their next hop through a nexthop id, which - * we can only resolve while the kernel nexthop table is tracked. */ - nexthop_exchange_update(nhe_data, NEXTHOP_EXCHANGE_USER_ROUTE, - !hmap_is_empty(&route_data->announce_routes)); - - 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, .nexthops = &nhe_data->nexthops, }; - 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, - .referenced_nhids = &re->referenced_nhids, }; +} + +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); + struct ed_type_nexthop_exchange *nhe_data = + engine_get_input_data("nexthop_exchange", node); + struct ed_type_route *route_data = engine_get_input_data("route", 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); + + /* Routes we learn may reference their next hop through a nexthop id, which + * we can only resolve while the kernel nexthop table is tracked. */ + nexthop_exchange_update(nhe_data, NEXTHOP_EXCHANGE_USER_ROUTE, + !hmap_is_empty(&route_data->announce_routes)); + + 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; @@ -5838,6 +5844,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_nexthop_handler(struct engine_node *node, void *data) { @@ -5850,17 +5889,24 @@ route_exchange_nexthop_handler(struct engine_node *node, void *data) return EN_UNHANDLED; } - /* Only the nexthop objects the routes we learned depend on can affect - * them. Anything else, e.g. the FDB nexthops used by EVPN or the ones - * used by routes in tables we do not watch, is not worth a recompute. */ - uint32_t id; - VECTOR_FOR_EACH (&nhe_data->changed_ids, id) { - if (nexthop_ids_contains(&re->referenced_nhids, id)) { - 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_nexthop_changes(re->state, &r_ctx_in, + &r_ctx_out, + &nhe_data->changed_ids)) { + case ROUTE_EXCHANGE_UNHANDLED: + return EN_UNHANDLED; + case ROUTE_EXCHANGE_UNCHANGED: + return EN_HANDLED_UNCHANGED; + case ROUTE_EXCHANGE_UPDATED: + break; } - return EN_HANDLED_UNCHANGED; + re->sb_changes_pending |= r_ctx_out.sb_changes_pending; + + return EN_HANDLED_UPDATED; } static enum engine_input_handler_result @@ -5882,7 +5928,7 @@ 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; - hmap_init(&re->referenced_nhids); + re->state = route_exchange_state_create(); return re; } @@ -5890,8 +5936,7 @@ static void en_route_exchange_cleanup(void *data) { struct ed_type_route_exchange *re = data; - nexthop_ids_clear(&re->referenced_nhids); - hmap_destroy(&re->referenced_nhids); + route_exchange_state_destroy(re->state); } /* The route_table_notify node is an input node, but the watches are @@ -5909,11 +5954,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; } @@ -7364,7 +7411,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); /* Routes referencing a nexthop id have to be re-learned whenever the * nexthop object they point at changes, the route itself is not updated diff --git a/controller/route-exchange.c b/controller/route-exchange.c index db84aecb0979..5320ba04c136 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,23 @@ 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; + /* Ids (struct nexthop_id_node) of the kernel nexthop objects those routes + * resolve through. A change to any other nexthop object cannot affect + * what OVN learned from this table. */ + struct hmap referenced_nhids; +}; + +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 +104,72 @@ 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_init(&rt->referenced_nhids); + 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); + nexthop_ids_clear(&rt->referenced_nhids); + hmap_destroy(&rt->referenced_nhids); + 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, @@ -354,6 +438,37 @@ advertised_routes_add(struct hmap *advertised_routes, hmapx_add(&entry->datapaths, CONST_CAST(void *, ad->db)); } +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) { @@ -365,21 +480,24 @@ 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'. */ +/* Turns the routes OVN learns from the table 'rt' into Learned_Route rows of + * every datapath in 'datapaths'. */ 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) { struct vector received_routes = VECTOR_EMPTY_INITIALIZER(struct re_nl_received_route_node); + /* Which nexthop objects matter is decided by the routes we have now. */ + nexthop_ids_clear(&rt->referenced_nhids); + 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, r_ctx_in->nexthops, &received_routes, - r_ctx_out->referenced_nhids); + &rt->referenced_nhids); } struct hmapx_node *dp_node; @@ -405,8 +523,118 @@ 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 || !arte->can_sync) { + /* 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; +} + +enum route_exchange_handled +route_exchange_handle_nexthop_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_nhids) +{ + struct hmapx changed_tables = HMAPX_INITIALIZER(&changed_tables); + + /* Only a nexthop object one of the routes we learned resolves through can + * change what we learned. Anything else, e.g. the FDB nexthops used by + * EVPN or the ones used by routes in tables we do not sync, leaves the + * Learned_Route rows as they are. */ + struct route_table_state *rt; + HMAP_FOR_EACH (rt, node, &state->tables) { + uint32_t id; + VECTOR_FOR_EACH (changed_nhids, id) { + if (nexthop_ids_contains(&rt->referenced_nhids, id)) { + hmapx_add(&changed_tables, rt); + break; + } + } + } + + enum route_exchange_handled handled = + resync_changed_tables(&changed_tables, r_ctx_in, r_ctx_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); @@ -452,7 +680,6 @@ 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); @@ -460,16 +687,27 @@ route_exchange_run(const struct route_exchange_ctx_in *r_ctx_in, continue; } + struct route_table_state *rt = route_table_state_get(state, + arte->table_id); error = re_nl_sync_routes(arte->table_id, arte->routes, - &learned_routes); + &rt->learned_routes); SET_ROUTE_EXCHANGE_NL_STATUS(error); - 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) { + const struct advertised_routes_entry *e = + advertised_routes_find(&advertised_routes, rt->table_id); + + if (!e || !e->can_sync) { + 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 5565e4c635a2..99e7de81e7fa 100644 --- a/controller/route-exchange.h +++ b/controller/route-exchange.h @@ -36,17 +36,42 @@ struct route_exchange_ctx_in { }; struct route_exchange_ctx_out { + /* Populated by route_exchange_run() only, NULL otherwise. */ struct vector *route_table_watches; bool sb_changes_pending; - - /* Collects the ids (struct nexthop_id_node) of the kernel nexthop objects - * the learned routes depend on. A change to any other nexthop object - * cannot affect them. */ - struct hmap *referenced_nhids; }; -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 *'), or after the kernel nexthop + * objects with the ids in 'changed_nhids' (uint32_t) changed. */ +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); +enum route_exchange_handled route_exchange_handle_nexthop_changes( + struct route_exchange_state *, const struct route_exchange_ctx_in *, + struct route_exchange_ctx_out *, const struct vector *changed_nhids); + 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 49d7e61525d9..b8ee9d2b791f 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=""]]; nexthop_exchange -> route_exchange [[label="route_exchange_nexthop_handler"]]; sb_ro -> route_exchange [[label="route_exchange_sb_ro_handler"]]; diff --git a/tests/system-ovn-netlink.at b/tests/system-ovn-netlink.at index 5b6772da38ad..d47982b6e2dd 100644 --- a/tests/system-ovn-netlink.at +++ b/tests/system-ovn-netlink.at @@ -729,6 +729,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 18c06031a1a9..f8d18affa34b 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -21008,15 +21008,28 @@ AT_CHECK_UNQUOTED([route_recomputes], [0], [$re_recompute ]) # Replacing the object the route does name moves the route, even though the -# kernel reports nothing about the route itself. Relearning has to happen for -# this change, waiting for the route to move would also accept it happening -# later on for an unrelated reason. +# kernel reports nothing about the route itself, and it does so with what +# ovn-controller already knows about the table rather than by reading it +# again. re_recompute=$(route_recomputes) check ip nexthop replace id $nh_used via 20.0.0.27 dev local-bgp-port -check ovn-nbctl --wait=hv sync -AT_CHECK([test $(route_recomputes) -gt $re_recompute]) - wait_row_count Learned_Route 1 ip_prefix=10.10.5.1 nexthop=20.0.0.27 +AT_CHECK_UNQUOTED([route_recomputes], [0], [$re_recompute +]) + +# A route appearing in the table, or leaving it, is picked up from the +# notification alone as well. +re_recompute=$(route_recomputes) +check ip route add 10.10.6.1 nhid $nh_used vrf vrf-$vni proto zebra +wait_row_count Learned_Route 1 ip_prefix=10.10.6.1 nexthop=20.0.0.27 +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 +]) # Restore what the rest of the test expects to find. check ip route del 10.10.5.1 vrf vrf-$vni diff --git a/tests/test-ovn-netlink.c b/tests/test-ovn-netlink.c index 03a40284c0e6..e25332c87fa1 100644 --- a/tests/test-ovn-netlink.c +++ b/tests/test-ovn-netlink.c @@ -414,6 +414,74 @@ test_route_sync_nhids(struct ovs_cmdl_context *ctx) vector_destroy(&received_routes); } +/* 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 ds ds = DS_EMPTY_INITIALIZER; + + 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, &routes_to_advertise, + &learned_routes) == 0); + /* 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. */ @@ -467,6 +535,8 @@ test_ovn_netlink(int argc, char *argv[]) {"route-sync-nhids", NULL, 1, 1, test_route_sync_nhids, 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
