Any change to a nexthop object that is not an FDB one makes 'route_exchange' relearn the routes of every table it syncs and reconcile them against the southbound database. The kernel nexthop table is global though, so nexthop objects belonging to route tables we do not watch, e.g. those a routing daemon maintains for the main table, cause that work just as much as the ones we do use.
Track which nexthop ids the routes learned from a table were resolved through and relearn a table only when one of the objects reported as changed is among them. A route depends on the members of the group it refers to as well, since replacing a member moves the route without the group itself being announced. Ids that cannot be resolved are recorded too, so that a route starts being learned once the object it names shows up. The whole table is read again when it starts being tracked, and there is no way to tell what changed in it then, so keep recomputing in that case. This makes 'is_fdb' unused, the id comparison covers it: an FDB nexthop is only ever named by a bridge FDB entry, never by a route. Assisted-by: Claude Opus 5, Cursor Signed-off-by: Han Zhou <[email protected]> --- controller/nexthop-exchange.c | 5 +-- controller/nexthop-exchange.h | 52 +++++++++++++++++++++++++++-- controller/ovn-controller.c | 32 +++++++++--------- controller/route-exchange-netlink.c | 21 +++++++++--- controller/route-exchange-netlink.h | 6 ++-- controller/route-exchange.c | 31 +++++++++++++---- controller/route-exchange.h | 6 ++-- tests/system-ovn-netlink.at | 8 +++++ tests/system-ovn.at | 11 +++++- tests/test-ovn-netlink.c | 48 ++++++++++++++++++++++++-- 10 files changed, 179 insertions(+), 41 deletions(-) diff --git a/controller/nexthop-exchange.c b/controller/nexthop-exchange.c index ff2766e7bf40..c464b6913cb3 100644 --- a/controller/nexthop-exchange.c +++ b/controller/nexthop-exchange.c @@ -56,8 +56,7 @@ nexthop_grp_weight(const struct nexthop_grp *entry) /* Populates 'nexthops' with all nexthop entries (struct nexthop_entry) that * exist in the kernel nexthop table. Both the FDB nexthops used by EVPN and - * the nexthops referenced by routes through a nexthop id are included, use - * 'is_fdb' to tell them apart. */ + * the nexthops referenced by routes through a nexthop id are included. */ void nexthops_sync(struct hmap *nexthops) { @@ -210,7 +209,6 @@ nh_table_parse__(struct ofpbuf *buf, size_t ofs, const struct nlmsghdr *nlmsg, static const struct nl_policy policy[] = { [NHA_ID] = { .type = NL_A_U32 }, - [NHA_FDB] = { .type = NL_A_FLAG, .optional = true }, [NHA_BLACKHOLE] = { .type = NL_A_FLAG, .optional = true }, [NHA_OIF] = { .type = NL_A_U32, .optional = true }, [NHA_GROUP] = { .type = NL_A_UNSPEC, .optional = true, @@ -264,7 +262,6 @@ nh_table_parse__(struct ofpbuf *buf, size_t ofs, const struct nlmsghdr *nlmsg, .id = nl_attr_get_u32(attrs[NHA_ID]), .addr = addr, .is_blackhole = nl_attr_get_flag(attrs[NHA_BLACKHOLE]), - .is_fdb = nl_attr_get_flag(attrs[NHA_FDB]), .n_grps = n_grps, }; diff --git a/controller/nexthop-exchange.h b/controller/nexthop-exchange.h index ff6d39b0d729..aff4f205dc94 100644 --- a/controller/nexthop-exchange.h +++ b/controller/nexthop-exchange.h @@ -20,8 +20,11 @@ #include <netinet/in.h> #include <stdbool.h> #include <stdint.h> +#include <stdlib.h> +#include "hash.h" #include "openvswitch/hmap.h" +#include "util.h" struct ds; struct ofpbuf; @@ -47,9 +50,6 @@ struct nexthop_entry { char ifname[IFNAMSIZ + 1]; /* True if the nexthop discards the traffic sent to it. */ bool is_blackhole; - /* True if the nexthop belongs to a bridge FDB, i.e. it is used by EVPN - * rather than by the routing table. */ - bool is_fdb; /* Number of group entries, "0" in case of gateway entry. */ size_t n_grps; /* Array of group entries. */ @@ -65,6 +65,52 @@ struct nh_table_msg { struct nexthop_entry *nhe; }; +/* Member of a set of kernel nexthop object ids. */ +struct nexthop_id_node { + struct hmap_node hmap_node; + uint32_t id; +}; + +/* Adds 'id' to the set 'ids' if it is not there yet. */ +static inline void +nexthop_ids_add(struct hmap *ids, uint32_t id) +{ + uint32_t hash = hash_int(id, 0); + + struct nexthop_id_node *node; + HMAP_FOR_EACH_WITH_HASH (node, hmap_node, hash, ids) { + if (node->id == id) { + return; + } + } + + node = xmalloc(sizeof *node); + node->id = id; + hmap_insert(ids, &node->hmap_node, hash); +} + +static inline bool +nexthop_ids_contains(const struct hmap *ids, uint32_t id) +{ + const struct nexthop_id_node *node; + HMAP_FOR_EACH_WITH_HASH (node, hmap_node, hash_int(id, 0), ids) { + if (node->id == id) { + return true; + } + } + + return false; +} + +static inline void +nexthop_ids_clear(struct hmap *ids) +{ + struct nexthop_id_node *node; + HMAP_FOR_EACH_POP (node, hmap_node, ids) { + free(node); + } +} + void nexthops_sync(struct hmap *nexthops); void nexthop_entry_format(struct ds *ds, const struct nexthop_entry *nhe); struct nexthop_entry *nexthop_entry_find(const struct hmap *nexthops, diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 60c7f91b65a0..03dea537093c 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -5750,9 +5750,11 @@ struct ed_type_nexthop_exchange { bool users[NEXTHOP_EXCHANGE_USER_MAX]; bool enabled; bool recompute; - /* True if the last run changed any nexthop that is not an EVPN FDB one, - * i.e. any nexthop a route may be referencing. */ - bool routing_changed; + /* Ids (uint32_t) of the nexthops the last run changed. */ + struct vector changed_ids; + /* True if the last run rebuilt the whole table, in which case + * 'changed_ids' is not usable as any nexthop may have changed. */ + bool resynced; }; static void nexthop_exchange_update(struct ed_type_nexthop_exchange *, @@ -5882,11 +5884,9 @@ route_exchange_nexthop_handler(struct engine_node *node, void *data) struct ed_type_nexthop_exchange *nhe_data = engine_get_input_data("nexthop_exchange", node); - /* Only the nexthops a route can reference through a nexthop id matter - * here, changes limited to the FDB nexthops used by EVPN cannot affect - * the routes we learn. */ - if (!nhe_data->routing_changed) { - return EN_HANDLED_UNCHANGED; + /* The whole table was rebuilt, we cannot tell what changed in it. */ + if (nhe_data->resynced) { + return EN_UNHANDLED; } struct route_exchange_ctx_in r_ctx_in; @@ -5894,7 +5894,8 @@ route_exchange_nexthop_handler(struct engine_node *node, void *data) 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)) { + &r_ctx_out, + &nhe_data->changed_ids)) { case ROUTE_EXCHANGE_UNHANDLED: return EN_UNHANDLED; case ROUTE_EXCHANGE_UNCHANGED: @@ -6634,6 +6635,7 @@ en_nexthop_exchange_init(struct engine_node *node OVS_UNUSED, struct ed_type_nexthop_exchange *nhe_data = xmalloc(sizeof *nhe_data); *nhe_data = (struct ed_type_nexthop_exchange) { .nexthops = HMAP_INITIALIZER(&nhe_data->nexthops), + .changed_ids = VECTOR_EMPTY_INITIALIZER(uint32_t), .enabled = false, .recompute = true, }; @@ -6647,6 +6649,7 @@ en_nexthop_exchange_cleanup(void *data) struct ed_type_nexthop_exchange *nhe_data = data; nexthops_destroy(&nhe_data->nexthops); hmap_destroy(&nhe_data->nexthops); + vector_destroy(&nhe_data->changed_ids); } static enum engine_node_state @@ -6658,6 +6661,9 @@ en_nexthop_exchange_run(struct engine_node *node OVS_UNUSED, void *data) return EN_UNCHANGED; } + vector_clear(&nhe_data->changed_ids); + nhe_data->resynced = false; + /* The messages we did get do not describe every change, so the table has * to be read again to find out what it looks like now. */ if (ovn_netlink_notifier_lost(OVN_NL_NOTIFIER_NEXTHOP)) { @@ -6672,19 +6678,15 @@ en_nexthop_exchange_run(struct engine_node *node OVS_UNUSED, void *data) ovn_netlink_notifier_flush(OVN_NL_NOTIFIER_NEXTHOP); nhe_data->recompute = false; - nhe_data->routing_changed = true; + nhe_data->resynced = true; return EN_UPDATED; } struct vector *msgs = ovn_netlink_get_msgs(OVN_NL_NOTIFIER_NEXTHOP); - nhe_data->routing_changed = false; const struct nh_table_msg *msg; VECTOR_FOR_EACH_PTR (msgs, msg) { - if (!msg->nhe->is_fdb) { - nhe_data->routing_changed = true; - break; - } + vector_push(&nhe_data->changed_ids, &msg->nhe->id); } bool updated = nexthops_handle_changes(&nhe_data->nexthops, msgs); diff --git a/controller/route-exchange-netlink.c b/controller/route-exchange-netlink.c index 19883b3c3f67..6303e3fa10b8 100644 --- a/controller/route-exchange-netlink.c +++ b/controller/route-exchange-netlink.c @@ -384,14 +384,21 @@ learn_route_via_nexthop(const struct nexthop_entry *nhe, /* Resolves the kernel nexthop object identified by 'id' against 'nexthops' * and appends a learned route for the prefix in 'msg' to 'learned_routes' for * each usable next hop. A nexthop group yields one learned route per - * member. */ + * member. + * + * Every nexthop id the route depends on is added to 'referenced_nhids', even + * the ones that cannot be resolved right now, so that the caller knows which + * changes to the kernel nexthop table may affect this route. */ static void learn_routes_via_nexthop_id(const struct hmap *nexthops, uint32_t id, const struct ovn_route_msg *msg, - struct vector *learned_routes) + struct vector *learned_routes, + struct hmap *referenced_nhids) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); + nexthop_ids_add(referenced_nhids, id); + const struct nexthop_entry *nhe = nexthop_entry_find(nexthops, id); if (!nhe) { VLOG_DBG_RL(&rl, "could not resolve nexthop id %"PRIu32, id); @@ -406,6 +413,10 @@ learn_routes_via_nexthop_id(const struct hmap *nexthops, uint32_t id, for (size_t i = 0; i < nhe->n_grps; i++) { const struct nexthop_grp_entry *grp = &nhe->grps[i]; + /* Replacing a member does not change the group itself, so the route + * depends on the members just as much as on the group. */ + nexthop_ids_add(referenced_nhids, grp->id); + /* The kernel does not allow a nexthop group to contain other groups, * so a single level of indirection is all we have to follow. */ if (!grp->gateway) { @@ -421,12 +432,14 @@ learn_routes_via_nexthop_id(const struct hmap *nexthops, uint32_t id, void re_nl_resolve_route(const struct ovn_route_msg *msg, const struct hmap *nexthops, - struct vector *learned_routes) + struct vector *learned_routes, + struct hmap *referenced_nhids) { if (msg->nhid) { /* The next hop(s) are not encoded in the route itself, they are * described by a separate kernel nexthop object. */ - learn_routes_via_nexthop_id(nexthops, msg->nhid, msg, learned_routes); + learn_routes_via_nexthop_id(nexthops, msg->nhid, msg, learned_routes, + referenced_nhids); return; } diff --git a/controller/route-exchange-netlink.h b/controller/route-exchange-netlink.h index 1c8e675db44b..982d90751156 100644 --- a/controller/route-exchange-netlink.h +++ b/controller/route-exchange-netlink.h @@ -100,10 +100,12 @@ struct re_nl_received_route_node { /* Turns the route 'msg' into the routes OVN learns from it, appending them to * 'learned_routes'. 'nexthops' contains the kernel nexthop objects (struct * nexthop_entry) used to resolve a route that references its next hop through - * a nexthop id. */ + * a nexthop id, the ids used in the process are collected into + * 'referenced_nhids' (struct nexthop_id_node). */ void re_nl_resolve_route(const struct ovn_route_msg *, const struct hmap *nexthops, - struct vector *learned_routes); + struct vector *learned_routes, + struct hmap *referenced_nhids); int re_nl_create_vrf(const char *ifname, uint32_t table_id); int re_nl_delete_vrf(const char *ifname); diff --git a/controller/route-exchange.c b/controller/route-exchange.c index 20e85e83a94c..45f204bf2686 100644 --- a/controller/route-exchange.c +++ b/controller/route-exchange.c @@ -50,6 +50,10 @@ struct route_table_state { 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 { @@ -126,6 +130,7 @@ route_table_state_get(struct route_exchange_state *state, uint32_t table_id) 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)); @@ -139,6 +144,8 @@ route_table_state_destroy(struct route_exchange_state *state, 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); } @@ -483,9 +490,13 @@ route_table_resolve_and_sync( 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, &rt->learned_routes) { - re_nl_resolve_route(cr->msg, r_ctx_in->nexthops, &received_routes); + re_nl_resolve_route(cr->msg, r_ctx_in->nexthops, &received_routes, + &rt->referenced_nhids); } struct hmapx_node *dp_node; @@ -586,20 +597,28 @@ 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) + struct route_exchange_ctx_out *r_ctx_out, + const struct vector *changed_nhids) { struct hmapx changed_tables = HMAPX_INITIALIZER(&changed_tables); - /* A route names a nexthop object, the object does not name the routes - * using it, so every table we know of may resolve differently now. */ + /* 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) { - hmapx_add(&changed_tables, rt); + 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; diff --git a/controller/route-exchange.h b/controller/route-exchange.h index 1845782e0e2c..18436f87f5ad 100644 --- a/controller/route-exchange.h +++ b/controller/route-exchange.h @@ -68,11 +68,11 @@ 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); -/* Updates the routes OVN learned after the kernel nexthop objects they may - * resolve through changed. */ +/* Updates the routes OVN learned after the kernel nexthop objects with the + * ids in 'changed_nhids' (uint32_t) changed. */ enum route_exchange_handled route_exchange_handle_nexthop_changes( struct route_exchange_state *, const struct route_exchange_ctx_in *, - struct route_exchange_ctx_out *); + 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/system-ovn-netlink.at b/tests/system-ovn-netlink.at index ee7272394e58..d47982b6e2dd 100644 --- a/tests/system-ovn-netlink.at +++ b/tests/system-ovn-netlink.at @@ -637,6 +637,14 @@ Route table_id=$table_id dst=10.10.30.0 plen=24 nexthop=20.0.0.2 Route table_id=$table_id dst=fd20:100:: plen=64 nexthop=fd20::1 ]) +dnl The learned routes depend on the nexthop objects they name and, for a +dnl group, on its members as well: replacing a member moves the routes without +dnl the group itself being reported as changed. +AT_CHECK_UNQUOTED([ovstest test-ovn-netlink route-sync-nhids $table_id | sort], + [0], [$(printf 'Referenced nexthop id=%s\n' \ + $nh_v4 $nh_v4_b $nh_v6 $nh_grp | sort) +]) + dnl Replacing a nexthop object changes where the routes referencing it point, dnl without the kernel touching the routes themselves. check ip nexthop replace id $nh_v4 via 20.0.0.9 dev lo-test diff --git a/tests/system-ovn.at b/tests/system-ovn.at index fb1e3032882c..4f70d0a15699 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -21113,7 +21113,16 @@ check ovn-nbctl --wait=hv sync OVS_WAIT_UNTIL([settled=$(route_recomputes); sleep 2; test "$settled" = "$(route_recomputes)"]) -# Replacing the object the route names moves the route, even though the +# No route names this object, so there is nothing to relearn for it. +re_recompute=$(route_recomputes) +nh_unused=$(nexthop_alloc) +check ip nexthop add id $nh_unused via 20.0.0.26 dev local-bgp-port +on_exit "ip nexthop del id $nh_unused" +check ovn-nbctl --wait=hv sync +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. diff --git a/tests/test-ovn-netlink.c b/tests/test-ovn-netlink.c index c2440ba8ca11..27d4ffd5550f 100644 --- a/tests/test-ovn-netlink.c +++ b/tests/test-ovn-netlink.c @@ -196,7 +196,8 @@ static int sync_and_resolve_routes(uint32_t table_id, const struct hmap *routes_to_advertise, const struct hmap *nexthops, - struct vector *received_routes) + struct vector *received_routes, + struct hmap *referenced_nhids) { struct hmap learned_routes = HMAP_INITIALIZER(&learned_routes); struct vector route_tables = @@ -209,7 +210,8 @@ sync_and_resolve_routes(uint32_t table_id, const struct re_nl_cached_route *cr; HMAP_FOR_EACH (cr, node, &learned_routes) { - re_nl_resolve_route(cr->msg, nexthops, received_routes); + re_nl_resolve_route(cr->msg, nexthops, received_routes, + referenced_nhids); } re_nl_cached_routes_clear(&learned_routes); @@ -230,6 +232,7 @@ test_route_sync(struct ovs_cmdl_context *ctx) } struct hmap routes_to_advertise = HMAP_INITIALIZER(&routes_to_advertise); + struct hmap referenced_nhids = HMAP_INITIALIZER(&referenced_nhids); struct hmap nexthops = HMAP_INITIALIZER(&nexthops); struct vector received_routes = VECTOR_EMPTY_INITIALIZER(struct re_nl_received_route_node); @@ -265,7 +268,8 @@ test_route_sync(struct ovs_cmdl_context *ctx) nexthops_sync(&nexthops); ovs_assert(sync_and_resolve_routes(table_id, &routes_to_advertise, - &nexthops, &received_routes) == 0); + &nexthops, &received_routes, + &referenced_nhids) == 0); struct ds msg = DS_EMPTY_INITIALIZER; @@ -282,6 +286,8 @@ done: free(e); } hmap_destroy(&routes_to_advertise); + nexthop_ids_clear(&referenced_nhids); + hmap_destroy(&referenced_nhids); nexthops_destroy(&nexthops); hmap_destroy(&nexthops); vector_destroy(&received_routes); @@ -377,6 +383,41 @@ test_nexthop_table_notify(struct ovs_cmdl_context *ctx) ovn_netlink_notifiers_destroy(); } +/* Reports the kernel nexthop objects that the routes learned from 'table_id' + * depend on. */ +static void +test_route_sync_nhids(struct ovs_cmdl_context *ctx) +{ + unsigned int table_id; + + if (!test_read_uint_value(ctx, 1, "table id", &table_id)) { + return; + } + + struct hmap routes_to_advertise = HMAP_INITIALIZER(&routes_to_advertise); + struct hmap referenced_nhids = HMAP_INITIALIZER(&referenced_nhids); + struct hmap nexthops = HMAP_INITIALIZER(&nexthops); + struct vector received_routes = + VECTOR_EMPTY_INITIALIZER(struct re_nl_received_route_node); + + nexthops_sync(&nexthops); + ovs_assert(sync_and_resolve_routes(table_id, &routes_to_advertise, + &nexthops, &received_routes, + &referenced_nhids) == 0); + + const struct nexthop_id_node *node; + HMAP_FOR_EACH (node, hmap_node, &referenced_nhids) { + printf("Referenced nexthop id=%"PRIu32"\n", node->id); + } + + nexthop_ids_clear(&referenced_nhids); + hmap_destroy(&referenced_nhids); + nexthops_destroy(&nexthops); + hmap_destroy(&nexthops); + hmap_destroy(&routes_to_advertise); + 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. */ @@ -501,6 +542,7 @@ test_ovn_netlink(int argc, char *argv[]) test_neighbor_table_notify, OVS_RO}, {"host-if-monitor", NULL, 2, 3, test_host_if_monitor, OVS_RO}, {"route-sync", NULL, 1, INT_MAX, test_route_sync, OVS_RO}, + {"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, -- 2.38.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
