On 9/17/25 6:27 PM, Frode Nordahl wrote: > 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! >
Cool, thanks a lot for trying it out! Applied to main and backported down to 25.03. Regards, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
