On 8/26/26 6:00 PM, Jakob Mueller wrote:
> 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]>
> ---
> v3:
> - Rebased onto current main; sb_sync_learned_routes() gained a chassis
> argument. No functional changes.
>
> 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).
>
Hi Jakob,
Thanks for the v3!
> NEWS | 3 +-
> controller/route-exchange-netlink.c | 52 ++++++++++----
> controller/route-exchange-netlink.h | 2 +-
> controller/route-exchange.c | 88 ++++++++++--------------
> tests/system-ovn.at | 101 ++++++++++++++++++++++++++++
> tests/test-ovn-netlink.c | 8 ++-
> 6 files changed, 187 insertions(+), 67 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 40a1b9867..3ce26dd1a 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -49,7 +49,8 @@ OVN v26.09.0 - xxx xx xxxx
> "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 8f1615c4e..2d066858a 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,36 @@ 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; i++) {
> + const struct hmap *prev = vector_get(route_tables, i,
> + const struct hmap *);
> + if (advertise_route_find(ar->priority, &ar->addr, ar->plen,
> + &ar->nexthop, prev)) {
> + duplicate = true;
> + break;
> + }
> + }
> + 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 c137b5119..bb72cc0e5 100644
> --- a/controller/route-exchange-netlink.h
> +++ b/controller/route-exchange-netlink.h
> @@ -57,7 +57,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 027375071..d816a6686 100644
> --- a/controller/route-exchange.c
> +++ b/controller/route-exchange.c
> @@ -302,10 +302,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
> @@ -356,17 +357,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;
> }
> }
> @@ -375,55 +365,51 @@ 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,
> - r_ctx_in->chassis);
> - }
> - 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,
> + r_ctx_in->chassis);
> }
> + 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 973c46728..836a2e635 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -17355,6 +17355,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
In general it's better to not mention old behavior, I'd just remove this
sentence.
> +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;
>
I took care of the minor comment I had on the test and applied the patch
to main.
I also added you to the AUTHORS.rst list.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
