Learning a route was one pass over the netlink dump of a routing table: the routes OVN cares about were turned into Learned_Route rows as they were read. That works only as long as reading the table is the only thing that can make the result change, which is not the case, since a route resolves through a kernel nexthop object that changes on its own.
Keep the routes of the table OVN may learn from as they were reported, and resolve them into Learned_Route rows in a second step. Reading the table and deciding what it means to OVN are now separate, so a later patch can redo just the second step. Along the way, give the two rules the learn path is built on one home each: whether a route is one OVN learns, which was spread over the dump handler, and which datapaths distribute routes into a table, which was inlined in route_exchange_run(). No functional change intended. Assisted-by: Claude Opus 5, Cursor Signed-off-by: Han Zhou <[email protected]> --- controller/route-exchange-netlink.c | 209 ++++++++++++++++++++-------- controller/route-exchange-netlink.h | 34 +++-- controller/route-exchange.c | 183 ++++++++++++++---------- tests/test-ovn-netlink.c | 36 ++++- 4 files changed, 321 insertions(+), 141 deletions(-) diff --git a/controller/route-exchange-netlink.c b/controller/route-exchange-netlink.c index 56c998d3b3b6..b652d1ea3f48 100644 --- a/controller/route-exchange-netlink.c +++ b/controller/route-exchange-netlink.c @@ -266,11 +266,103 @@ ovn_route_msg_format(struct ds *ds, const struct ovn_route_msg *msg) } } -/* Appends a learned route for the prefix in 'rd' reachable through the leaf +/* Returns true if 'msg' describes a route OVN may learn: one installed into + * its table by a dynamic routing protocol, rather than by OVN itself or by a + * user. Protocol values above RTPROT_STATIC are the ones used by the dynamic + * routing protocols. */ +static bool +route_is_learn_relevant(const struct ovn_route_msg *msg) +{ + return msg->protocol != RTPROT_OVN + && msg->protocol > RTPROT_STATIC + && !prefix_is_link_local(&msg->prefix, msg->plen); +} + +/* Two routes for one prefix may differ only by their metric, so it is part of + * a route's identity. */ +static uint32_t +cached_route_hash(const struct ovn_route_msg *msg) +{ + uint32_t hash = hash_bytes(&msg->prefix, sizeof msg->prefix, 0); + + hash = hash_int(msg->plen, hash); + return hash_int(msg->priority, hash); +} + +static struct re_nl_cached_route * +cached_route_find(const struct hmap *routes, const struct ovn_route_msg *msg) +{ + struct re_nl_cached_route *cr; + HMAP_FOR_EACH_WITH_HASH (cr, node, cached_route_hash(msg), routes) { + if (cr->msg->plen == msg->plen + && cr->msg->priority == msg->priority + && ipv6_addr_equals(&cr->msg->prefix, &msg->prefix)) { + return cr; + } + } + + return NULL; +} + +/* Applies the change 'msg' to the routes cached for its table: a route + * reported as added replaces the one it has the identity of, a route reported + * as removed drops it. Changes to routes OVN does not learn from are ignored. + * + * Returns true if 'routes' changed. */ +bool +re_nl_cached_routes_apply(struct hmap *routes, const struct ovn_route_msg *msg) +{ + if (!route_is_learn_relevant(msg)) { + return false; + } + + struct re_nl_cached_route *cr = cached_route_find(routes, msg); + + if (msg->nlmsg_type == RTM_DELROUTE) { + if (!cr) { + return false; + } + + hmap_remove(routes, &cr->node); + free(cr->msg); + free(cr); + return true; + } + + if (cr) { + size_t size = ovn_route_msg_size(msg); + if (size == ovn_route_msg_size(cr->msg) + && !memcmp(cr->msg, msg, size)) { + return false; + } + + free(cr->msg); + cr->msg = ovn_route_msg_clone(msg); + return true; + } + + cr = xmalloc(sizeof *cr); + cr->msg = ovn_route_msg_clone(msg); + hmap_insert(routes, &cr->node, cached_route_hash(msg)); + + return true; +} + +void +re_nl_cached_routes_clear(struct hmap *routes) +{ + struct re_nl_cached_route *cr; + HMAP_FOR_EACH_POP (cr, node, routes) { + free(cr->msg); + free(cr); + } +} + +/* Appends a learned route for the prefix in 'msg' reachable through the leaf * nexthop object 'nhe' to 'learned_routes'. */ static void learn_route_via_nexthop(const struct nexthop_entry *nhe, - const struct route_data *rd, + const struct ovn_route_msg *msg, struct vector *learned_routes) { if (ipv6_is_zero(&nhe->addr)) { @@ -280,8 +372,8 @@ learn_route_via_nexthop(const struct nexthop_entry *nhe, } struct re_nl_received_route_node rr = (struct re_nl_received_route_node) { - .prefix = rd->rta_dst, - .plen = rd->rtm_dst_len, + .prefix = msg->prefix, + .plen = msg->plen, .nexthop = nhe->addr, }; ovs_strlcpy(rr.ifname, nhe->ifname, sizeof rr.ifname); @@ -290,7 +382,7 @@ 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 'rd' to 'learned_routes' for + * 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. * @@ -299,7 +391,7 @@ learn_route_via_nexthop(const struct nexthop_entry *nhe, * 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 route_data *rd, + const struct ovn_route_msg *msg, struct vector *learned_routes, struct hmap *referenced_nhids) { @@ -314,7 +406,7 @@ learn_routes_via_nexthop_id(const struct hmap *nexthops, uint32_t id, } if (!nhe->n_grps) { - learn_route_via_nexthop(nhe, rd, learned_routes); + learn_route_via_nexthop(nhe, msg, learned_routes); return; } @@ -333,24 +425,53 @@ learn_routes_via_nexthop_id(const struct hmap *nexthops, uint32_t id, continue; } - learn_route_via_nexthop(grp->gateway, rd, learned_routes); + learn_route_via_nexthop(grp->gateway, msg, learned_routes); + } +} + +void +re_nl_resolve_route(const struct ovn_route_msg *msg, + const struct hmap *nexthops, + 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, + referenced_nhids); + return; + } + + for (size_t i = 0; i < msg->n_nexthops; i++) { + const struct ovn_route_nexthop *nh = &msg->nexthops[i]; + + if (ipv6_is_zero(&nh->addr)) { + /* This is most likely an address on the local link. As we just + * want to learn remote routes we do not need it. */ + continue; + } + + struct re_nl_received_route_node rr; + rr = (struct re_nl_received_route_node) { + .prefix = msg->prefix, + .plen = msg->plen, + .nexthop = nh->addr, + }; + ovs_strlcpy(rr.ifname, nh->ifname, sizeof rr.ifname); + + vector_push(learned_routes, &rr); } } struct route_msg_handle_data { struct hmapx *routes_to_advertise; - struct vector *learned_routes; struct vector *stale_routes; const struct hmap *routes; - /* Kernel nexthop objects (struct nexthop_entry), used to resolve routes - * that reference their next hop(s) through a nexthop id (RTA_NH_ID). - * Must be set whenever 'learned_routes' is. */ - const struct hmap *nexthops; - - /* Collects the nexthop ids (struct nexthop_id_node) the learned routes - * depend on. Must be set whenever 'learned_routes' is. */ - struct hmap *referenced_nhids; + /* Routes of the table OVN may learn from (struct re_nl_cached_route), + * rebuilt from the dump. NULL if the caller does not learn routes. */ + struct hmap *learned_routes; }; static void @@ -369,46 +490,16 @@ handle_route_msg(const struct route_table_msg *msg, return; } - /* This route is not from us, learn it only if it's > RTPROT_STATIC, - * those protocol values are used by dynamic routing protocols. - * This should prevent us from learning static routes installed - * by users in the VRF. */ + /* This route is not from us, so it is one we may learn. */ if (rd->rtm_protocol != RTPROT_OVN) { - if (rd->rtm_protocol <= RTPROT_STATIC) { - return; - } if (!handle_data->learned_routes) { return; } - if (prefix_is_link_local(&rd->rta_dst, rd->rtm_dst_len)) { - return; - } - if (rd->rta_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(handle_data->nexthops, rd->rta_nhid, - rd, handle_data->learned_routes, - handle_data->referenced_nhids); - return; - } - struct route_data_nexthop *nexthop; - LIST_FOR_EACH (nexthop, nexthop_node, &rd->nexthops) { - if (ipv6_is_zero(&nexthop->addr)) { - /* This is most likely an address on the local link. - * As we just want to learn remote routes we do not need it.*/ - continue; - } - struct re_nl_received_route_node rr; - rr = (struct re_nl_received_route_node) { - .prefix = rd->rta_dst, - .plen = rd->rtm_dst_len, - .nexthop = nexthop->addr, - }; - memcpy(rr.ifname, nexthop->ifname, IFNAMSIZ); - rr.ifname[IFNAMSIZ] = '\0'; - - vector_push(handle_data->learned_routes, &rr); - } + + struct ovn_route_msg *route_msg = + ovn_route_msg_from_route_data(RTM_NEWROUTE, rd); + re_nl_cached_routes_apply(handle_data->learned_routes, route_msg); + free(route_msg); return; } @@ -475,9 +566,7 @@ re_nl_encode_nexthop(struct ofpbuf *request, bool dst_is_ipv4, int re_nl_sync_routes(uint32_t table_id, const struct hmap *routes, - const struct hmap *nexthops, - struct vector *learned_routes, - struct hmap *referenced_nhids) + struct hmap *learned_routes) { struct hmapx routes_to_advertise = HMAPX_INITIALIZER(&routes_to_advertise); struct vector stale_routes = @@ -488,6 +577,12 @@ re_nl_sync_routes(uint32_t table_id, const struct hmap *routes, hmapx_add(&routes_to_advertise, ar); } + if (learned_routes) { + /* The dump below tells us about every route of the table, so whatever + * we knew about it is replaced. */ + re_nl_cached_routes_clear(learned_routes); + } + /* Remove routes from the system that are not in the routes hmap and * remove entries from routes hmap that match routes already installed * in the system. */ @@ -496,8 +591,6 @@ re_nl_sync_routes(uint32_t table_id, const struct hmap *routes, .routes_to_advertise = &routes_to_advertise, .learned_routes = learned_routes, .stale_routes = &stale_routes, - .nexthops = nexthops, - .referenced_nhids = referenced_nhids, }; route_table_dump_one_table(table_id, AF_INET, handle_route_msg, &data); route_table_dump_one_table(table_id, AF_INET6, handle_route_msg, &data); diff --git a/controller/route-exchange-netlink.h b/controller/route-exchange-netlink.h index 26aa8de9328e..65bc14eb386c 100644 --- a/controller/route-exchange-netlink.h +++ b/controller/route-exchange-netlink.h @@ -77,6 +77,18 @@ struct ovn_route_msg *ovn_route_msg_from_route_data( struct ovn_route_msg *ovn_route_msg_clone(const struct ovn_route_msg *); void ovn_route_msg_format(struct ds *, const struct ovn_route_msg *); +/* A route of a kernel routing table OVN may learn from, kept as the kernel + * reported it so that it can be resolved again without reading the table + * anew. */ +struct re_nl_cached_route { + struct hmap_node node; + struct ovn_route_msg *msg; +}; + +bool re_nl_cached_routes_apply(struct hmap *routes, + const struct ovn_route_msg *); +void re_nl_cached_routes_clear(struct hmap *routes); + struct re_nl_received_route_node { struct in6_addr prefix; unsigned int plen; @@ -85,6 +97,16 @@ struct re_nl_received_route_node { char ifname[IFNAMSIZ + 1]; }; +/* 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, 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 hmap *referenced_nhids); + int re_nl_create_vrf(const char *ifname, uint32_t table_id); int re_nl_delete_vrf(const char *ifname); @@ -95,15 +117,11 @@ void re_route_format(struct ds *, uint32_t table_id, const struct in6_addr *dst, unsigned int plen, const struct in6_addr *nexthop, int err); -/* Syncs the routes OVN advertises in 'table_id' and collects the routes - * learned from it into 'learned_routes'. 'nexthops' contains the kernel - * nexthop objects (struct nexthop_entry) used to resolve learned routes that - * reference their next hop through a nexthop id, the ids used in the process - * are collected into 'referenced_nhids' (struct nexthop_id_node). */ +/* Syncs the routes OVN advertises in 'table_id' with the kernel and, unless + * 'learned_routes' is NULL, rebuilds it from the routes of the table OVN may + * learn from (struct re_nl_cached_route). */ int re_nl_sync_routes(uint32_t table_id, const struct hmap *routes, - const struct hmap *nexthops, - struct vector *learned_routes, - struct hmap *referenced_nhids); + struct hmap *learned_routes); int re_nl_cleanup_routes(uint32_t table_id); diff --git a/controller/route-exchange.c b/controller/route-exchange.c index 3ce2b990ad05..db84aecb0979 100644 --- a/controller/route-exchange.c +++ b/controller/route-exchange.c @@ -308,6 +308,103 @@ struct advertised_routes_entry { bool can_sync; }; +/* Records that 'ad' distributes routes into the table 'table_id', creating the + * entry for the table in 'advertised_routes' if it is the first datapath to do + * so. Only one datapath may distribute routes into a table, any further one + * makes the table unusable. */ +static void +advertised_routes_add(struct hmap *advertised_routes, + const struct advertise_datapath_entry *ad, + uint32_t table_id) +{ + struct advertised_routes_entry *entry = NULL; + uint32_t hash = maintained_route_table_hash(table_id); + HMAP_FOR_EACH_WITH_HASH (entry, node, hash, advertised_routes) { + if (entry->table_id == table_id) { + if (!hmap_is_empty(&ad->routes)) { + if (entry->routes && !hmap_is_empty(entry->routes)) { + VLOG_WARN_RL(&rl, + "Multiple datapaths are distributing " + "routes on routing table %"PRIu32, + table_id); + entry->can_sync = false; + } else { + entry->routes = &ad->routes; + } + } + break; + } + } + + if (entry == NULL) { + entry = xmalloc(sizeof *entry); + *entry = (struct advertised_routes_entry) { + .datapaths = HMAPX_INITIALIZER(&entry->datapaths), + .routes = &ad->routes, + .table_id = table_id, + .can_sync = true, + }; + hmap_insert(advertised_routes, &entry->node, hash); + } + + if (!entry->can_sync) { + return; + } + + hmapx_add(&entry->datapaths, CONST_CAST(void *, ad->db)); +} + +static void +advertised_routes_destroy(struct hmap *advertised_routes) +{ + struct advertised_routes_entry *arte; + HMAP_FOR_EACH_POP (arte, node, advertised_routes) { + hmapx_destroy(&arte->datapaths); + free(arte); + } + 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'. */ +static void +resolve_and_sync_learned_routes( + const struct hmap *learned_routes, 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); + + const struct re_nl_cached_route *cr; + HMAP_FOR_EACH (cr, node, learned_routes) { + re_nl_resolve_route(cr->msg, r_ctx_in->nexthops, &received_routes, + r_ctx_out->referenced_nhids); + } + + struct hmapx_node *dp_node; + HMAPX_FOR_EACH (dp_node, datapaths) { + const struct sbrec_datapath_binding *db = dp_node->data; + struct advertise_datapath_entry *adpe = + advertise_datapath_find(r_ctx_in->announce_routes, db); + if (!adpe) { + VLOG_WARN_RL(&rl, "Cannot sync datapath binding "UUID_FMT", " + "bound ports not found", + UUID_ARGS(&db->header_.uuid)); + continue; + } + + sb_sync_learned_routes(&received_routes, db, &adpe->bound_ports, + r_ctx_in->ovnsb_idl_txn, + r_ctx_in->sbrec_port_binding_by_name, + r_ctx_in->sbrec_learned_route_by_datapath, + &r_ctx_out->sb_changes_pending, + r_ctx_in->chassis); + } + + vector_destroy(&received_routes); +} + void route_exchange_run(const struct route_exchange_ctx_in *r_ctx_in, struct route_exchange_ctx_out *r_ctx_out) @@ -352,83 +449,27 @@ route_exchange_run(const struct route_exchange_ctx_in *r_ctx_in, sset_find_and_delete(&old_maintained_vrfs, ad->vrf_name); } - struct advertised_routes_entry *entry = NULL; - uint32_t hash = maintained_route_table_hash(table_id); - HMAP_FOR_EACH_WITH_HASH (entry, node, hash, &advertised_routes) { - if (entry->table_id == table_id) { - if (!hmap_is_empty(&ad->routes)) { - if (entry->routes && !hmap_is_empty(entry->routes)) { - VLOG_WARN_RL(&rl, - "Multiple datapaths are distributing " - "routes on routing table %"PRIu32, - table_id); - entry->can_sync = false; - } else { - entry->routes = &ad->routes; - } - } - break; - } - } - - if (entry == NULL) { - entry = xmalloc(sizeof *entry); - *entry = (struct advertised_routes_entry) { - .datapaths = HMAPX_INITIALIZER(&entry->datapaths), - .routes = &ad->routes, - .table_id = table_id, - .can_sync = true, - }; - hmap_insert(&advertised_routes, &entry->node, hash); - } - - if (!entry->can_sync) { - continue; - } - - hmapx_add(&entry->datapaths, CONST_CAST(void *, ad->db)); + advertised_routes_add(&advertised_routes, ad, table_id); } + struct hmap learned_routes = HMAP_INITIALIZER(&learned_routes); struct advertised_routes_entry *arte; - HMAP_FOR_EACH_POP (arte, node, &advertised_routes) { + HMAP_FOR_EACH (arte, node, &advertised_routes) { maintained_route_table_add(arte->table_id); - if (arte->can_sync) { - struct vector received_routes = - VECTOR_EMPTY_INITIALIZER(struct re_nl_received_route_node); - error = re_nl_sync_routes(arte->table_id, arte->routes, - r_ctx_in->nexthops, &received_routes, - r_ctx_out->referenced_nhids); - SET_ROUTE_EXCHANGE_NL_STATUS(error); - - struct ovsdb_idl_index *sbrec_learned_route_by_datapath = - r_ctx_in->sbrec_learned_route_by_datapath; - struct hmapx_node *dp_node; - HMAPX_FOR_EACH (dp_node, &arte->datapaths) { - const struct sbrec_datapath_binding *db = dp_node->data; - struct advertise_datapath_entry *adpe = - advertise_datapath_find(r_ctx_in->announce_routes, - db); - if (!adpe) { - VLOG_WARN_RL(&rl, "Cannot sync datapath binding " - UUID_FMT", bound ports not found", - UUID_ARGS(&db->header_.uuid)); - continue; - } - sb_sync_learned_routes(&received_routes, db, - &adpe->bound_ports, - r_ctx_in->ovnsb_idl_txn, - r_ctx_in->sbrec_port_binding_by_name, - sbrec_learned_route_by_datapath, - &r_ctx_out->sb_changes_pending, - r_ctx_in->chassis); - } - vector_push(r_ctx_out->route_table_watches, &arte->table_id); - vector_destroy(&received_routes); + if (!arte->can_sync) { + continue; } - hmapx_destroy(&arte->datapaths); - free(arte); + error = re_nl_sync_routes(arte->table_id, arte->routes, + &learned_routes); + SET_ROUTE_EXCHANGE_NL_STATUS(error); + + resolve_and_sync_learned_routes(&learned_routes, &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); /* Remove routes in tables previously maintained by us. */ struct maintained_route_table_entry *mrt; @@ -459,7 +500,7 @@ route_exchange_run(const struct route_exchange_ctx_in *r_ctx_in, sset_delete(&old_maintained_vrfs, SSET_NODE_FROM_NAME(vrf_name)); } sset_destroy(&old_maintained_vrfs); - hmap_destroy(&advertised_routes); + advertised_routes_destroy(&advertised_routes); } void diff --git a/tests/test-ovn-netlink.c b/tests/test-ovn-netlink.c index ba779938028f..03a40284c0e6 100644 --- a/tests/test-ovn-netlink.c +++ b/tests/test-ovn-netlink.c @@ -190,6 +190,32 @@ test_host_if_monitor(struct ovs_cmdl_context *ctx) sset_destroy(&if_names); } +/* Syncs the routes OVN advertises in 'table_id' and resolves the routes it + * learns from it into 'received_routes', as route_exchange does. */ +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 hmap *referenced_nhids) +{ + struct hmap learned_routes = HMAP_INITIALIZER(&learned_routes); + + int err = re_nl_sync_routes(table_id, routes_to_advertise, + &learned_routes); + + const struct re_nl_cached_route *cr; + HMAP_FOR_EACH (cr, node, &learned_routes) { + re_nl_resolve_route(cr->msg, nexthops, received_routes, + referenced_nhids); + } + + re_nl_cached_routes_clear(&learned_routes); + hmap_destroy(&learned_routes); + + return err; +} + static void test_route_sync(struct ovs_cmdl_context *ctx) { @@ -237,8 +263,9 @@ test_route_sync(struct ovs_cmdl_context *ctx) * can only be resolved against the kernel nexthop table. */ nexthops_sync(&nexthops); - ovs_assert(re_nl_sync_routes(table_id, &routes_to_advertise, &nexthops, - &received_routes, &referenced_nhids) == 0); + ovs_assert(sync_and_resolve_routes(table_id, &routes_to_advertise, + &nexthops, &received_routes, + &referenced_nhids) == 0); struct ds msg = DS_EMPTY_INITIALIZER; @@ -370,8 +397,9 @@ test_route_sync_nhids(struct ovs_cmdl_context *ctx) VECTOR_EMPTY_INITIALIZER(struct re_nl_received_route_node); nexthops_sync(&nexthops); - ovs_assert(re_nl_sync_routes(table_id, &routes_to_advertise, &nexthops, - &received_routes, &referenced_nhids) == 0); + 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) { -- 2.38.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
