When several Logical_Routers were configured with the same dynamic-routing-vrf-id, only one of them (or none) actually had its routes synced to the shared host routing table (VRF).
Route exchange reconciles each VRF routing table as a single authoritative set: re_nl_sync_routes() dumps the table and removes every OVN owned route that is not part of the supplied set. Only one datapath's routes were kept per table, so when a second Logical_Router used the same dynamic-routing-vrf-id the collision was detected, logged and synchronisation of that table was skipped entirely. Pass the routes of all datapaths sharing a table to re_nl_sync_routes() and reconcile their union. Equivalent routes advertised by several datapaths are installed exactly once: while building the union only the occurrence from the first table is kept, which also lets handle_route_msg() stop at the first match. Add a system test with two Logical_Routers exporting their connected routes and a shared static prefix to the same VRF, including removal of both routers. CC: Jacob Tanenbaum <[email protected]> Assisted-by: Claude Opus 4.8, Claude Code Signed-off-by: Jakob Mueller <[email protected]> --- v2: - Reconcile the union of all datapaths' routes and install equivalent routes only once, instead of just dropping the can_sync guard, so re_nl_add_route() cannot fail with EEXIST (Dumitru). - Extend the test with a static prefix advertised by both routers and with removal of the routers one after the other (Dumitru). - Move Signed-off-by to the end of the commit message (Dumitru). NEWS | 3 +- controller/route-exchange-netlink.c | 48 +++++++++---- controller/route-exchange-netlink.h | 2 +- controller/route-exchange.c | 86 ++++++++++------------- tests/system-ovn.at | 101 ++++++++++++++++++++++++++++ tests/test-ovn-netlink.c | 8 ++- 6 files changed, 182 insertions(+), 66 deletions(-) diff --git a/NEWS b/NEWS index 5782df27d..dada500d0 100644 --- a/NEWS +++ b/NEWS @@ -17,7 +17,8 @@ Post v26.03.0 "lb-add", "meter-add", "lr-policy-add", and "lr-policy-del", and fixed the "nfg-list" signature. - Dynamic Routing: - * Allow multiple routers to read the same VRF table. + * Allow multiple routers to read from and advertise their routes to + the same VRF table. * Add support for hub-and-spoke propagation via the "hub-spoke" option in dynamic-routing-redistribute settings. * Add ECMP/multi-homing support for EVPN FDB entries. FDB entries diff --git a/controller/route-exchange-netlink.c b/controller/route-exchange-netlink.c index a9fd1f58a..a9ea1f4c1 100644 --- a/controller/route-exchange-netlink.c +++ b/controller/route-exchange-netlink.c @@ -203,7 +203,9 @@ struct route_msg_handle_data { struct hmapx *routes_to_advertise; struct vector *learned_routes; struct vector *stale_routes; - const struct hmap *routes; + /* Vector of "const struct hmap *", each holding advertise_route_entry + * nodes for a datapath sharing this routing table. */ + const struct vector *route_tables; }; static void @@ -260,11 +262,14 @@ handle_route_msg(const struct route_table_msg *msg, const struct advertise_route_entry re = advertise_route_from_route_data(rd); if (handle_data->routes_to_advertise) { - ar = advertise_route_find(re.priority, &re.addr, re.plen, - &re.nexthop, handle_data->routes); - if (ar) { - hmapx_find_and_delete(handle_data->routes_to_advertise, ar); - return; + const struct hmap *routes; + VECTOR_FOR_EACH (handle_data->route_tables, routes) { + ar = advertise_route_find(re.priority, &re.addr, re.plen, + &re.nexthop, routes); + if (ar) { + hmapx_find_and_delete(handle_data->routes_to_advertise, ar); + return; + } } } @@ -319,7 +324,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, +re_nl_sync_routes(uint32_t table_id, const struct vector *route_tables, struct vector *learned_routes) { struct hmapx routes_to_advertise = HMAPX_INITIALIZER(&routes_to_advertise); @@ -327,15 +332,32 @@ re_nl_sync_routes(uint32_t table_id, const struct hmap *routes, VECTOR_EMPTY_INITIALIZER(struct advertise_route_entry); struct advertise_route_entry *ar; - HMAP_FOR_EACH (ar, node, routes) { - hmapx_add(&routes_to_advertise, ar); + /* Equivalent routes may be advertised by multiple datapaths sharing this + * routing table. Only keep the occurrence from the first table that has + * it, so each route is installed in the kernel exactly once and + * handle_route_msg() can stop at the first match as well. */ + size_t n_prev = 0; + const struct hmap *routes; + VECTOR_FOR_EACH (route_tables, routes) { + HMAP_FOR_EACH (ar, node, routes) { + bool duplicate = false; + for (size_t i = 0; i < n_prev && !duplicate; i++) { + duplicate = advertise_route_find( + ar->priority, &ar->addr, ar->plen, &ar->nexthop, + vector_get(route_tables, i, const struct hmap *)); + } + if (!duplicate) { + hmapx_add(&routes_to_advertise, ar); + } + } + n_prev++; } - /* 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. */ + /* Remove routes from the system that are not in any of the route tables + * and remove entries from routes_to_advertise that match routes already + * installed in the system. */ struct route_msg_handle_data data = { - .routes = routes, + .route_tables = route_tables, .routes_to_advertise = &routes_to_advertise, .learned_routes = learned_routes, .stale_routes = &stale_routes, diff --git a/controller/route-exchange-netlink.h b/controller/route-exchange-netlink.h index f79ae05db..5da980a0c 100644 --- a/controller/route-exchange-netlink.h +++ b/controller/route-exchange-netlink.h @@ -59,7 +59,7 @@ 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); -int re_nl_sync_routes(uint32_t table_id, const struct hmap *routes, +int re_nl_sync_routes(uint32_t table_id, const struct vector *route_tables, struct vector *learned_routes); int re_nl_cleanup_routes(uint32_t table_id); diff --git a/controller/route-exchange.c b/controller/route-exchange.c index b86eb43bf..e8e2bf4a5 100644 --- a/controller/route-exchange.c +++ b/controller/route-exchange.c @@ -247,10 +247,11 @@ static int route_exchange_nl_status; struct advertised_routes_entry { struct hmap_node node; + /* Contains "struct advertise_datapath_entry *" for all datapaths that + * advertise routes on this routing table. Multiple datapaths may share a + * single table when they use the same dynamic-routing-vrf-id. */ struct hmapx datapaths; - const struct hmap *routes; uint32_t table_id; - bool can_sync; }; void @@ -301,17 +302,6 @@ route_exchange_run(const struct route_exchange_ctx_in *r_ctx_in, 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; } } @@ -320,54 +310,50 @@ route_exchange_run(const struct route_exchange_ctx_in *r_ctx_in, 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)); + hmapx_add(&entry->datapaths, CONST_CAST(void *, ad)); } struct advertised_routes_entry *arte; HMAP_FOR_EACH_POP (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, - &received_routes); - 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); - } - vector_push(r_ctx_out->route_table_watches, &arte->table_id); - vector_destroy(&received_routes); + + struct hmapx_node *dp_node; + + /* Collect the route tables of all datapaths sharing this routing + * table so they are synced together as a single authoritative set. */ + struct vector route_tables = + VECTOR_EMPTY_INITIALIZER(const struct hmap *); + HMAPX_FOR_EACH (dp_node, &arte->datapaths) { + const struct advertise_datapath_entry *adpe = dp_node->data; + const struct hmap *routes = &adpe->routes; + vector_push(&route_tables, &routes); + } + + struct vector received_routes = + VECTOR_EMPTY_INITIALIZER(struct re_nl_received_route_node); + error = re_nl_sync_routes(arte->table_id, &route_tables, + &received_routes); + SET_ROUTE_EXCHANGE_NL_STATUS(error); + vector_destroy(&route_tables); + + struct ovsdb_idl_index *sbrec_learned_route_by_datapath = + r_ctx_in->sbrec_learned_route_by_datapath; + HMAPX_FOR_EACH (dp_node, &arte->datapaths) { + const struct advertise_datapath_entry *adpe = dp_node->data; + sb_sync_learned_routes(&received_routes, adpe->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); } + vector_push(r_ctx_out->route_table_watches, &arte->table_id); + vector_destroy(&received_routes); hmapx_destroy(&arte->datapaths); free(arte); diff --git a/tests/system-ovn.at b/tests/system-ovn.at index ed5d63fd3..521adff49 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -17340,6 +17340,107 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD([ +AT_SETUP([dynamic-routing - multiple LRs sharing a VRF]) +AT_KEYWORDS([dynamic-routing]) + +VRF_RESERVE([1337]) + +ovn_start +OVS_TRAFFIC_VSWITCHD_START() + +ADD_BR([br-int]) +check ovs-vsctl \ + -- set Open_vSwitch . external-ids:system-id=hv1 \ + -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ + -- set bridge br-int fail-mode=secure other-config:disable-in-band=true + +start_daemon ovn-controller + +dnl Two independent logical routers configured with the same +dnl dynamic-routing-vrf-id. Each redistributes a distinct connected subnet +dnl plus a static route for the same shared prefix. Only lr1 maintains the +dnl VRF device, but both must export their routes to it. +check ovn-nbctl \ + -- lr-add lr1 \ + -- set logical_router lr1 options:dynamic-routing=true \ + options:dynamic-routing-vrf-id=1337 \ + -- lrp-add lr1 lr1-gw 00:00:00:01:00:10 42.10.10.12/24 \ + -- lrp-set-gateway-chassis lr1-gw hv1 10 \ + -- lrp-set-options lr1-gw dynamic-routing-maintain-vrf=true \ + -- lrp-add lr1 lr1-int 00:00:00:00:01:02 30.0.1.1/24 \ + -- lrp-set-options lr1-int \ + dynamic-routing-redistribute=connected,static \ + -- lr-add lr2 \ + -- set logical_router lr2 options:dynamic-routing=true \ + options:dynamic-routing-vrf-id=1337 \ + -- lrp-add lr2 lr2-gw 00:00:00:02:00:10 42.20.10.22/24 \ + -- lrp-set-gateway-chassis lr2-gw hv1 10 \ + -- lrp-add lr2 lr2-int 00:00:00:00:02:02 30.0.2.1/24 \ + -- lrp-set-options lr2-int \ + dynamic-routing-redistribute=connected,static \ + -- ls-add ls1 \ + -- lsp-add-router-port ls1 ls1-lr1-gw lr1-gw \ + -- ls-add ls2 \ + -- lsp-add-router-port ls2 ls2-lr2-gw lr2-gw \ + -- ls-add ls-int1 \ + -- lsp-add-router-port ls-int1 ls-int1-lr lr1-int \ + -- lsp-add ls-int1 w1 \ + -- lsp-set-addresses w1 "00:00:00:00:00:01 30.0.1.11" \ + -- ls-add ls-int2 \ + -- lsp-add-router-port ls-int2 ls-int2-lr lr2-int \ + -- lsp-add ls-int2 w2 \ + -- lsp-set-addresses w2 "00:00:00:00:00:02 30.0.2.11" + +dnl Both routers advertise a static route for the same prefix. +check ovn-nbctl lr-route-add lr1 203.0.113.0/24 30.0.1.11 +check ovn-nbctl lr-route-add lr2 203.0.113.0/24 30.0.2.11 + +check ovs-vsctl add-port br-int w1 \ + -- set interface w1 type=internal external_ids:iface-id=w1 +check ovs-vsctl add-port br-int w2 \ + -- set interface w2 type=internal external_ids:iface-id=w2 +check ovn-nbctl --wait=hv sync +wait_for_ports_up w1 w2 + +AT_CHECK([ip vrf show ovnvrf1337], [0], [dnl +ovnvrf1337 1337 +]) + +dnl Both routers must have their connected routes installed in the shared VRF. +dnl Previously a second router exporting to an already-used VRF was silently +dnl dropped, so only one (or none) of these routes would appear. The shared +dnl static prefix is advertised by both routers but must be installed only +dnl once. +OVN_ROUTE_EQUAL([ovnvrf1337], [dnl +blackhole 30.0.1.0/24 proto ovn metric 1000 +blackhole 30.0.2.0/24 proto ovn metric 1000 +blackhole 203.0.113.0/24 proto ovn metric 1000]) + +dnl Removing one router must not disturb the remaining router's routes, +dnl including the shared prefix it still advertises. +check ovn-nbctl --wait=hv lr-del lr2 +OVN_ROUTE_EQUAL([ovnvrf1337], [dnl +blackhole 30.0.1.0/24 proto ovn metric 1000 +blackhole 203.0.113.0/24 proto ovn metric 1000]) + +dnl Removing the last router must clean up the routes and the VRF device. +check ovn-nbctl --wait=hv lr-del lr1 +OVS_WAIT_WHILE([ip link show dev ovnvrf1337]) + +OVN_CLEANUP_CONTROLLER([hv1]) + +OVN_CLEANUP_NORTHD + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d +/connection dropped.*/d"]) + +AT_CLEANUP +]) + OVN_FOR_EACH_NORTHD([ AT_SETUP([dynamic-routing - multiple DGP with same priority]) diff --git a/tests/test-ovn-netlink.c b/tests/test-ovn-netlink.c index 84de2bd63..416d5f9d0 100644 --- a/tests/test-ovn-netlink.c +++ b/tests/test-ovn-netlink.c @@ -231,8 +231,14 @@ test_route_sync(struct ovs_cmdl_context *ctx) advertise_route_hash(&ar->addr, &ar->nexthop, ar->plen)); } - ovs_assert(re_nl_sync_routes(table_id, &routes_to_advertise, + struct vector route_tables = + VECTOR_EMPTY_INITIALIZER(const struct hmap *); + const struct hmap *routes = &routes_to_advertise; + vector_push(&route_tables, &routes); + + ovs_assert(re_nl_sync_routes(table_id, &route_tables, &received_routes) == 0); + vector_destroy(&route_tables); struct ds msg = DS_EMPTY_INITIALIZER; -- 2.54.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
