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 <[email protected]>
> ---
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.
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.:
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.
> + }
> +
> 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
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.
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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
