On 9/17/25 14:21, Dumitru Ceara wrote:
On 9/16/25 8:25 PM, Frode Nordahl wrote:
The commit in the fixes tag introduced options to map between the
system interface routes are learned on, and an LRP.
However, when an LR has LRPs with and without the option,
routes are attempted associated with LRPs without the option,
contrary to the documented behavior.
This was hidden in the original dynamic-routing - Gateway Router
test by populating all LRPs with the option, however this does
not represent a real world use case.
Ensure only LRPs with the option are considered when at least one
of LRPs in an LR has the option set.
Reported-at: https://launchpad.net/bugs/2123914
Fixes: d7d886eca553 ("controller: Support learning routes per iface.")
Signed-off-by: Frode Nordahl <fnord...@ubuntu.com>
---
Hi Frode,
Thanks for the fix!
controller/route.c | 14 ++++++++++++++
tests/system-ovn.at | 7 ++++---
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/controller/route.c b/controller/route.c
index 7afa2578d..56c1bd528 100644
--- a/controller/route.c
+++ b/controller/route.c
@@ -152,6 +152,7 @@ route_run(struct route_ctx_in *r_ctx_in,
{
struct advertise_datapath_entry *ad;
const struct local_datapath *ld;
+ bool lr_has_port_name_filter;
Nit: I'd move the declaration inside the loop to reduce its scope, it's
not needed anywhere else. While at it, I'd also move the declaration of
'ad' inside the two loops.
struct smap port_mapping = SMAP_INITIALIZER(&port_mapping);
build_port_mapping(&port_mapping, r_ctx_in->dynamic_routing_port_mapping);
@@ -161,6 +162,7 @@ route_run(struct route_ctx_in *r_ctx_in,
continue;
}
ad = NULL;
+ lr_has_port_name_filter = false;
I think we still have a slight bug because route_exchange_find_port()
can return NULL early if the chassis-redirect port associated to the
local_peer is not bound on the current chassis. That means
lr_has_port_name_filter would not be set to 'true' if all
chassis-redirect ports are bound to different chassis.
This case I did miss, thank you for pointing it out.
We could change northd to mark the datapath as "having at least one
port with dynamic-routing-port-name set" but I think we can also
just change route_exchange_find_port() to return the port name, e.g.:
Expanding some of our structs did cross my mind, but I opted like you
that it was a bit too much at this stage.
const struct sbrec_port_binding*
route_exchange_find_port(struct ovsdb_idl_index *sbrec_port_binding_by_name,
const struct sbrec_chassis *chassis,
const struct sbrec_port_binding *pb,
const char **dynamic_routing_port_name)
{
if (dynamic_routing_port_name) {
*dynamic_routing_port_name = NULL;
}
if (!pb) {
return NULL;
}
if (route_exchange_relevant_port(pb)) {
if (dynamic_routing_port_name) {
*dynamic_routing_port_name =
smap_get(&pb->options, "dynamic-routing-port-name");
}
return pb;
}
const struct sbrec_port_binding *cr_pb =
lport_get_cr_port(sbrec_port_binding_by_name, pb, NULL);
if (!cr_pb) {
return NULL;
}
if (dynamic_routing_port_name) {
*dynamic_routing_port_name =
smap_get(&cr_pb->options, "dynamic-routing-port-name");
}
if (!lport_pb_is_chassis_resident(chassis, cr_pb)) {
return NULL;
}
if (route_exchange_relevant_port(cr_pb)) {
return cr_pb;
}
return NULL;
}
/* This is a LR datapath, find LRPs with route exchange options
* that are bound locally. */
@@ -209,6 +211,8 @@ route_run(struct route_ctx_in *r_ctx_in,
This used to be:
const char *port_name = smap_get(&repb->options,
"dynamic-routing-port-name");
if (!port_name) {
But with the amended route_exchange_find_port() we could remove the lookup
from here.
/* No port-name set, so we learn routes from all ports. */
smap_add(&ad->bound_ports, local_peer->logical_port, "");
} else {
+ lr_has_port_name_filter = true;
With the amended route_exchange_find_port() we would have to move this
just after that call.
+
/* If a port_name is set the we filter for the name as set in
* the port-mapping or the interface name of the local
* binding. If the port is not in the port_mappings and not
@@ -224,6 +228,16 @@ route_run(struct route_ctx_in *r_ctx_in,
}
if (ad) {
Nit: I'd add a comment here, maybe something like:
/* If at least one bound port has dynamic-routing-port-name
* configured, ignore the ones that don't. */
+ if (lr_has_port_name_filter) {
+ struct smap_node *node;
+
+ SMAP_FOR_EACH_SAFE (node, &ad->bound_ports) {
+ if (node->value && *node->value == '\0') {
As we skipped patch 1/2 this should just be:
if (!node->value) {
[...]
}
+ smap_remove_node(&ad->bound_ports, node);
+ }
+ }
Alternatively, we could change 'struct advertise_datapath_entry' and
add the lr_has_port_name_filter bool as a field there. We could
propagate that all the way down to sb_sync_learned_routes(). But I'm
not sure I like that more so let's not do it for now.
I also did not find any obvious near struct to expand upon so I agree.
+ }
+
tracked_datapath_add(ld->datapath, TRACKED_RESOURCE_NEW,
r_ctx_out->tracked_re_datapaths);
hmap_insert(r_ctx_out->announce_routes, &ad->node,
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 8e356df6f..b2319b180 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -16374,8 +16374,7 @@ check ovn-nbctl lr-add internet \
check ovn-nbctl lrp-add internet internet-public \
00:00:02:01:02:03 192.0.2.1/24 \
-- set Logical_Router_Port internet-public \
- options:dynamic-routing-redistribute="connected,static" \
- options:dynamic-routing-port-name=wedontlearnstuffhere
+ options:dynamic-routing-redistribute="connected,static"
check ovn-nbctl lsp-add public public-internet \
-- set Logical_Switch_Port public-internet type=router \
options:router-port=internet-public \
@@ -16548,7 +16547,9 @@ check ovn-nbctl set Logical_Router_Port internet-phys \
check_row_count Learned_Route 0
check ip route add 233.252.0.0/24 via 192.168.10.10 dev lo onlink vrf
ovnvrf1337
check ovn-nbctl --wait=hv sync
-check_row_count Learned_Route 1
+# With a Gateway Router all LRPs are locally bound, and without explicit
+# mapping/filtering they will all learn the route.
+check_row_count Learned_Route 2
lp=$(fetch_column port_binding _uuid logical_port=internet-phys)
check_row_count Learned_Route 1 logical_port=$lp ip_prefix=233.252.0.0/24
nexthop=192.168.10.10
I'm not sure it's very easy to read my suggestions above as they
refer to code that's not in the diff so I pushed a commit to my
fork with the suggested changes:
https://github.com/dceara/ovn/commit/959c2e5
Thanks a lot, I made use of this to test this in our environment and I
can confirm it still works as expected.
Please let me know if this looks OK to you too. If so, I could
just squash it with your patch and avoid the need for a v2.
Thank you for your thorough review, this works for me!
--
Frode Nordahl
For reference here's the incremental diff inline as well:
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 330227933d..874fe0cfa1 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -5240,7 +5240,7 @@ route_runtime_data_handler(struct engine_node *node, void
*data)
struct tracked_lport *lport = shash_node->data;
if (route_exchange_find_port(sbrec_port_binding_by_name, chassis,
- lport->pb)) {
+ lport->pb, NULL)) {
/* XXX: Until we get I-P support for route exchange we need to
* request recompute. */
return EN_UNHANDLED;
@@ -5320,7 +5320,7 @@ route_sb_port_binding_data_handler(struct engine_node
*node, void *data)
}
if (route_exchange_find_port(sbrec_port_binding_by_name,
- chassis, sbrec_pb)) {
+ chassis, sbrec_pb, NULL)) {
/* XXX: Until we get I-P support for route exchange we need to
* request recompute. */
return EN_UNHANDLED;
diff --git a/controller/route.c b/controller/route.c
index ea7c3060a2..db4aa4122b 100644
--- a/controller/route.c
+++ b/controller/route.c
@@ -52,12 +52,21 @@ advertise_route_hash(const struct in6_addr *dst, unsigned
int plen)
const struct sbrec_port_binding*
route_exchange_find_port(struct ovsdb_idl_index *sbrec_port_binding_by_name,
const struct sbrec_chassis *chassis,
- const struct sbrec_port_binding *pb)
+ const struct sbrec_port_binding *pb,
+ const char **dynamic_routing_port_name)
{
+ if (dynamic_routing_port_name) {
+ *dynamic_routing_port_name = NULL;
+ }
+
if (!pb) {
return NULL;
}
if (route_exchange_relevant_port(pb)) {
+ if (dynamic_routing_port_name) {
+ *dynamic_routing_port_name =
+ smap_get(&pb->options, "dynamic-routing-port-name");
+ }
return pb;
}
@@ -68,6 +77,11 @@ route_exchange_find_port(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
return NULL;
}
+ if (dynamic_routing_port_name) {
+ *dynamic_routing_port_name =
+ smap_get(&cr_pb->options, "dynamic-routing-port-name");
+ }
+
if (!lport_pb_is_chassis_resident(chassis, cr_pb)) {
return NULL;
}
@@ -150,9 +164,7 @@ void
route_run(struct route_ctx_in *r_ctx_in,
struct route_ctx_out *r_ctx_out)
{
- struct advertise_datapath_entry *ad;
const struct local_datapath *ld;
- bool lr_has_port_name_filter;
struct smap port_mapping = SMAP_INITIALIZER(&port_mapping);
build_port_mapping(&port_mapping, r_ctx_in->dynamic_routing_port_mapping);
@@ -161,18 +173,23 @@ route_run(struct route_ctx_in *r_ctx_in,
if (vector_is_empty(&ld->peer_ports) || ld->is_switch) {
continue;
}
- ad = NULL;
- lr_has_port_name_filter = false;
+ struct advertise_datapath_entry *ad = NULL;
+ bool lr_has_port_name_filter = false;
/* This is a LR datapath, find LRPs with route exchange options
* that are bound locally. */
const struct peer_ports *peers;
VECTOR_FOR_EACH_PTR (&ld->peer_ports, peers) {
const struct sbrec_port_binding *local_peer = peers->local;
+ const char *port_name;
+
const struct sbrec_port_binding *repb =
route_exchange_find_port(r_ctx_in->sbrec_port_binding_by_name,
r_ctx_in->chassis,
- local_peer);
+ local_peer, &port_name);
+ if (port_name) {
+ lr_has_port_name_filter = true;
+ }
if (!repb) {
continue;
}
@@ -205,15 +222,11 @@ route_run(struct route_ctx_in *r_ctx_in,
ad->db->tunnel_key);
}
- const char *port_name = smap_get(&repb->options,
- "dynamic-routing-port-name");
if (!port_name) {
/* No port-name set, so we learn routes from all ports. */
smap_add_nocopy(&ad->bound_ports,
xstrdup(local_peer->logical_port), NULL);
} else {
- lr_has_port_name_filter = true;
-
/* If a port_name is set the we filter for the name as set in
* the port-mapping or the interface name of the local
* binding. If the port is not in the port_mappings and not
@@ -229,11 +242,13 @@ route_run(struct route_ctx_in *r_ctx_in,
}
if (ad) {
+ /* If at least one bound port has dynamic-routing-port-name
+ * configured, ignore the ones that don't. */
if (lr_has_port_name_filter) {
struct smap_node *node;
SMAP_FOR_EACH_SAFE (node, &ad->bound_ports) {
- if (node->value && *node->value == '\0') {
+ if (!node->value) {
smap_remove_node(&ad->bound_ports, node);
}
}
@@ -249,8 +264,9 @@ route_run(struct route_ctx_in *r_ctx_in,
const struct sbrec_advertised_route *route;
SBREC_ADVERTISED_ROUTE_TABLE_FOR_EACH (route,
r_ctx_in->advertised_route_table) {
- ad = advertise_datapath_find(r_ctx_out->announce_routes,
- route->datapath);
+ struct advertise_datapath_entry *ad =
+ advertise_datapath_find(r_ctx_out->announce_routes,
+ route->datapath);
if (!ad) {
continue;
}
diff --git a/controller/route.h b/controller/route.h
index 09aff89ffe..c2c92e70b6 100644
--- a/controller/route.h
+++ b/controller/route.h
@@ -80,7 +80,8 @@ struct advertise_route_entry {
const struct sbrec_port_binding *route_exchange_find_port(
struct ovsdb_idl_index *sbrec_port_binding_by_name,
const struct sbrec_chassis *chassis,
- const struct sbrec_port_binding *pb);
+ const struct sbrec_port_binding *pb,
+ const char **dynamic_routing_port_name);
uint32_t advertise_route_hash(const struct in6_addr *dst, unsigned int plen);
void route_run(struct route_ctx_in *, struct route_ctx_out *);
void route_cleanup(struct hmap *announce_routes);
---
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
