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.
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
---
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;
--
2.54.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
