On 6/28/26 4:16 PM, Jakob Mueller via dev 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 any
> OVN owned route that is not part of the supplied set. The advertised
> routes were grouped per table, but only a single datapath's route hmap
> was retained per table. When a second datapath mapped to the same
> table, the code detected the collision, logged a warning and set
> can_sync to false, which skipped synchronisation of that table
> entirely.
>
> Make re_nl_sync_routes() accept a vector of route tables and reconcile
> their union, so the routes of all datapaths sharing a VRF are synced to
> the host routing table together. route_exchange_run() now collects
> every datapath advertising on a given table, syncs them in a single
> netlink call and distributes the learned routes back to each of them.
>
Hi Jakob,
Thanks a lot for the contribution!
> Add a system test exercising two Logical_Routers that export their
> connected routes to the same dynamic-routing-vrf-id.
>
> Fixes: ce663bbc5d99 ("ovn-controller: Allow two datapaths to monitor the same
> vrf.")
I'm not sure I completely agree with the "Fixes" tag here. It was never
really a goal to be able to "write" to the same vrf routes from two
different routers. So by design we didn't really allow that.
However, I agree, it might be a nice thing to support.
Jacob, as you worked on this recently, would you happen to have some
time to review Jakob's patch below?
Thanks,
Dumitru
> CC: Jacob Tanenbaum <[email protected]>
> Signed-off-by: Jakob Mueller <[email protected]>
> Assisted-by: Claude Opus 4.8, Claude Code
> ---> NEWS | 3 +-
> controller/route-exchange-netlink.c | 34 +++++++-----
> controller/route-exchange-netlink.h | 2 +-
> controller/route-exchange.c | 86 ++++++++++++-----------------
> tests/system-ovn.at | 80 +++++++++++++++++++++++++++
> tests/test-ovn-netlink.c | 8 ++-
> 6 files changed, 147 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..a50e4a0e3 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,18 @@ 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);
> + const struct hmap *routes;
> + VECTOR_FOR_EACH (route_tables, routes) {
> + HMAP_FOR_EACH (ar, node, routes) {
> + hmapx_add(&routes_to_advertise, ar);
> + }
> }
>
> - /* 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..ad6e3f120 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -17340,6 +17340,86 @@ 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 Only lr1 maintains the 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
> \
> + -- 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
> \
> + -- 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"
> +
> +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.
> +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])
> +
> +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;
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
