Hi Jakob,
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.
>
> Add a system test exercising two Logical_Routers that export their
> connected routes to the same dynamic-routing-vrf-id.
>
Thanks again for the patch!
> Fixes: ce663bbc5d99 ("ovn-controller: Allow two datapaths to monitor the same
> vrf.")
> CC: Jacob Tanenbaum <[email protected]>
> Signed-off-by: Jakob Mueller <[email protected]>
> Assisted-by: Claude Opus 4.8, Claude Code
Nit: The "Signed-off-by" is normally the last tag of the commit message.
> ---
> 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;
We're not correctly handling the "union" of the multiple sets of
advertised routes here, or am I reading this wrong?
I.e., if the same route needs to be advertised by two datapaths, here we
only remove it from the first datapath's set of "routes that need to be
added to the vrf table". But the same route content would be present
(as a different pointer) in the routes_to_advertise hmapx.
Later, re_nl_sync_routes() will try to add these equivalent routes and
call re_nl_add_route() which will likely fail with -EEXIST, causing
continuos recomputations.
Do we need to "continue" here instead of "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);
route_table_watches can now contain duplicates. Is that an issue?
> + 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])
Maybe we should enhance the test here to see what happens when both
routers try to advertise the same route, e.g., a static route for the
same prefix.
Should we also check what happens when we remove one router? And then
the second one?
> +
> +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;
>
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
