On Wed, Oct 29, 2025 at 4:26 PM Felix Huettner via dev < [email protected]> wrote:
> On Wed, Oct 29, 2025 at 12:24:06PM +0100, Dumitru Ceara wrote: > > For routers with dynamic-routing enabled, if router ports are configured > > with 'dynamic-routing-port-name=<LSP>' external routes should only be > > learned if they're configured to use the Linux device that's configured > > for the underlying OVS interface that binds <LSP>. > > > > There were however a couple of races that made this feature unreliable: > > > > 1. the en_route I-P engine handler for port binding changes completely > > ignored changes for port bindings corresponding to LSPs that are > > configured as 'dynamic-routing-port-name=<LSP>'. If the LSP was > > bound _after_ the router port configuration was parsed the en_route > > data wouldn't be correctly recomputed and ovn-controller would ignore > > routes that used the underlying OVS device until the next time a > > recompute of the en_route node would happen (potentially indefinitely > > long). > > > > 2. the function to extract the OVS interface name corresponding to a > > port binding logical_port was using the local_binding_is_up() helper > > to determine of the logical switch port is bound locally. While that > > may seem correct (because it also takes into account whether OF rules > > have been fully installed in OVS for that port - ovn-installed=true) > > it introduces a race. If ovn-controller can't write the OVS > > external_id (e.g., the last OVS transaction is still executing) the > > function returns false. > > > > Once the OVS transaction succeeds there's no easy way to inform the > > en_route I-P engine node that it has to recompute. > > > > However, from the en_route node perspective it's enough to check > > that: > > - a local binding exists for that LSP > > - the local binding is claimed by the local chassis (it's "chassis > > resident") > > > > This commit takes this path as it has the lowest chance of > > introducing bugs due to missed I-P dependencies. > > > > A system test is also added to cover the scenarios above. > > Acked-By: Felix Huettner <[email protected]> > > > > > Fixes: d7d886eca553 ("controller: Support learning routes per iface.") > > Reported-at: https://issues.redhat.com/browse/FDP-1842 > > Signed-off-by: Dumitru Ceara <[email protected]> > > --- > > controller/binding.c | 44 +++++++++--------- > > controller/binding.h | 1 + > > controller/ovn-controller.c | 25 +++++++++++ > > controller/route.c | 11 +++-- > > controller/route.h | 5 +++ > > tests/system-ovn.at | 90 +++++++++++++++++++++++++++++++++++++ > > 6 files changed, 149 insertions(+), 27 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 74330b2560..b429323a9d 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -808,8 +808,6 @@ static struct binding_lport *local_binding_add_lport( > > struct local_binding *, > > const struct sbrec_port_binding *, > > enum en_lport_type); > > -static struct binding_lport *local_binding_get_primary_lport( > > - struct local_binding *); > > static struct binding_lport *local_binding_get_first_lport( > > struct local_binding *lbinding); > > static struct binding_lport > *local_binding_get_primary_or_localport_lport( > > @@ -898,6 +896,27 @@ local_binding_get_primary_pb(const struct shash > *local_bindings, > > return b_lport ? b_lport->pb : NULL; > > } > > > > +/* Returns the primary binding lport if present in lbinding's > > + * binding lports list. A binding lport is considered primary > > + * if binding lport's type is LP_VIF and the name matches > > + * with the 'lbinding'. > > + */ > > +struct binding_lport * > > +local_binding_get_primary_lport(struct local_binding *lbinding) > > +{ > > + if (!lbinding) { > > + return NULL; > > + } > > + > > + struct binding_lport *b_lport = > local_binding_get_first_lport(lbinding); > > + if (b_lport && b_lport->type == LP_VIF && > > + !strcmp(lbinding->name, b_lport->name)) { > > + return b_lport; > > + } > > + > > + return NULL; > > +} > > + > > ofp_port_t > > local_binding_get_lport_ofport(const struct shash *local_bindings, > > const char *pb_name) > > @@ -3506,27 +3525,6 @@ local_binding_get_first_lport(struct > local_binding *lbinding) > > return NULL; > > } > > > > -/* Returns the primary binding lport if present in lbinding's > > - * binding lports list. A binding lport is considered primary > > - * if binding lport's type is LP_VIF and the name matches > > - * with the 'lbinding'. > > - */ > > -static struct binding_lport * > > -local_binding_get_primary_lport(struct local_binding *lbinding) > > -{ > > - if (!lbinding) { > > - return NULL; > > - } > > - > > - struct binding_lport *b_lport = > local_binding_get_first_lport(lbinding); > > - if (b_lport && b_lport->type == LP_VIF && > > - !strcmp(lbinding->name, b_lport->name)) { > > - return b_lport; > > - } > > - > > - return NULL; > > -} > > - > > static struct binding_lport * > > local_binding_get_primary_or_localport_lport(struct local_binding > *lbinding) > > { > > diff --git a/controller/binding.h b/controller/binding.h > > index 8d978544f9..beca9cda97 100644 > > --- a/controller/binding.h > > +++ b/controller/binding.h > > @@ -163,6 +163,7 @@ void local_binding_data_destroy(struct > local_binding_data *); > > > > const struct sbrec_port_binding *local_binding_get_primary_pb( > > const struct shash *local_bindings, const char *pb_name); > > +struct binding_lport *local_binding_get_primary_lport(struct > local_binding *); > > ofp_port_t local_binding_get_lport_ofport(const struct shash > *local_bindings, > > const char *pb_name); > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index ea65d3a3e8..c2dab41c11 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -5135,6 +5135,11 @@ struct ed_type_route { > > * locally. */ > > struct sset tracked_ports_remote; > > > > + /* Contains all the currently configured dynamic-routing-port-name > values > > + * on all datapaths. > > + */ > > + struct sset filtered_ports; > > + > > /* Contains struct advertise_datapath_entry */ > > struct hmap announce_routes; > > > > @@ -5186,6 +5191,7 @@ en_route_run(struct engine_node *node, void *data) > > struct route_ctx_out r_ctx_out = { > > .tracked_re_datapaths = &re_data->tracked_route_datapaths, > > .tracked_ports_local = &re_data->tracked_ports_local, > > + .filtered_ports = &re_data->filtered_ports, > > .tracked_ports_remote = &re_data->tracked_ports_remote, > > .announce_routes = &re_data->announce_routes, > > }; > > @@ -5194,6 +5200,7 @@ en_route_run(struct engine_node *node, void *data) > > tracked_datapaths_clear(r_ctx_out.tracked_re_datapaths); > > sset_clear(r_ctx_out.tracked_ports_local); > > sset_clear(r_ctx_out.tracked_ports_remote); > > + sset_clear(r_ctx_out.filtered_ports); > > > > route_run(&r_ctx_in, &r_ctx_out); > > return EN_UPDATED; > > @@ -5209,6 +5216,7 @@ en_route_init(struct engine_node *node OVS_UNUSED, > > hmap_init(&data->tracked_route_datapaths); > > sset_init(&data->tracked_ports_local); > > sset_init(&data->tracked_ports_remote); > > + sset_init(&data->filtered_ports); > > hmap_init(&data->announce_routes); > > data->ovnsb_idl = arg->sb_idl; > > > > @@ -5223,6 +5231,7 @@ en_route_cleanup(void *data) > > tracked_datapaths_destroy(&re_data->tracked_route_datapaths); > > sset_destroy(&re_data->tracked_ports_local); > > sset_destroy(&re_data->tracked_ports_remote); > > + sset_destroy(&re_data->filtered_ports); > > route_cleanup(&re_data->announce_routes); > > hmap_destroy(&re_data->announce_routes); > > } > > @@ -5300,6 +5309,14 @@ route_runtime_data_handler(struct engine_node > *node, void *data) > > return EN_UNHANDLED; > > } > > > > + /* If this logical port name is used to filter on which > router > > + * ports learning should happen then process the changes. */ > > + if (sset_find(&re_data->filtered_ports, name)) { > > + /* XXX: Until we get I-P support for route exchange we > need to > > + * request recompute. */ > > + return EN_UNHANDLED; > > + } > > + > > const char *dp_name = smap_get(&lport->pb->options, > > "distributed-port"); > > if (dp_name && sset_contains(tracked_ports, dp_name)) { > > @@ -5365,6 +5382,14 @@ route_sb_port_binding_data_handler(struct > engine_node *node, void *data) > > * request recompute. */ > > return EN_UNHANDLED; > > } > > + > > + /* If this logical port name is used to filter on which router > > + * ports learning should happen then process the changes. */ > > + if (sset_find(&re_data->filtered_ports, > sbrec_pb->logical_port)) { > > + /* XXX: Until we get I-P support for route exchange we need > to > > + * request recompute. */ > > + return EN_UNHANDLED; > > + } > > } > > > > return EN_HANDLED_UNCHANGED; > > diff --git a/controller/route.c b/controller/route.c > > index db4aa4122b..adffc335bc 100644 > > --- a/controller/route.c > > +++ b/controller/route.c > > @@ -125,13 +125,15 @@ ifname_from_port_name(const struct smap > *port_mapping, > > return iface; > > } > > > > - if (!local_binding_is_up(local_bindings, port_name, chassis)) { > > + const struct binding_lport *b_lport = > > + > local_binding_get_primary_lport(local_binding_find(local_bindings, > > + port_name)); > > + > > + if (!b_lport || !lport_pb_is_chassis_resident(chassis, > b_lport->pb)) { > > return NULL; > > } > > > > - struct local_binding *binding = local_binding_find(local_bindings, > > - port_name); > > - return binding->iface->name; > > + return b_lport->lbinding->iface->name; > > } > > > > static void > > @@ -238,6 +240,7 @@ route_run(struct route_ctx_in *r_ctx_in, > > smap_add(&ad->bound_ports, local_peer->logical_port, > > ifname); > > } > > + sset_add(r_ctx_out->filtered_ports, port_name); > > } > > } > > > > diff --git a/controller/route.h b/controller/route.h > > index c2c92e70b6..438d66859a 100644 > > --- a/controller/route.h > > +++ b/controller/route.h > > @@ -49,6 +49,11 @@ struct route_ctx_out { > > * locally. */ > > struct sset *tracked_ports_remote; > > > > + /* Contains all the currently configured dynamic-routing-port-name > values > > + * on all datapaths. > > + */ > > + struct sset *filtered_ports; > > + > > /* Contains struct advertise_datapath_entry */ > > struct hmap *announce_routes; > > }; > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index 2b880ec378..78d12cbdc4 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -17396,6 +17396,96 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query > port patch-.*/d > > AT_CLEANUP > > ]) > > > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([dynamic-routing - route learning filter port name]) > > + > > +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 > > +check ovn-nbctl \ > > + -- lr-add lr \ > > + -- set logical_router lr options:chassis=hv1 \ > > + options:dynamic-routing=true \ > > + options:requested-tnl-key=1337 \ > > + -- lrp-add lr lrp1 00:00:00:00:00:01 1.1.1.1/24 \ > > + -- lrp-add lr lrp2 00:00:00:00:00:02 2.2.2.1/24 \ > > + -- lrp-set-options lrp2 dynamic-routing-port-name=vif2 \ > > + dynamic-routing-maintain-vrf=true \ > > + -- ls-add ls1 \ > > + -- lsp-add-router-port ls1 ls1-lr lrp1 \ > > + -- lsp-add ls1 vif1 \ > > + -- ls-add ls2 \ > > + -- lsp-add-router-port ls2 ls2-lr lrp2 \ > > + -- lsp-add ls2 vif2 > > + > > +check ovs-vsctl add-port br-int vif1 \ > > + -- set interface vif1 type=internal external_ids:iface-id=vif1 > > +check ovn-nbctl --wait=hv sync > > +wait_for_ports_up vif1 > > + > > +lrp1=$(fetch_column port_binding _uuid logical_port="lrp1") > > +lrp2=$(fetch_column port_binding _uuid logical_port="lrp2") > > + > > +AT_CHECK([ip vrf show ovnvrf1337], [0], [dnl > > +ovnvrf1337 1337 > > +]) > > + > > +AS_BOX([Unbound vif2: no routes learned]) > > +check ovs-vsctl add-port br-int vif2 \ > > + -- set interface vif2 type=internal > > +check ip link set vif2 up > > +check ip route add 3.3.3.0/24 via 2.2.2.2 dev vif2 onlink vrf > ovnvrf1337 > > +check ovn-nbctl --wait=hv sync > > +check_row_count Learned_Route 0 > > + > > +AS_BOX([Bound vif2: routes learned on lrp2]) > > +check ovs-vsctl set interface vif2 external-ids:iface-id=vif2 > > +wait_for_ports_up vif2 > > +check_row_count Learned_Route 1 ip_prefix=3.3.3.0/24 \ > > + nexthop=2.2.2.2 \ > > + logical_port=$lrp2 > > +check_row_count Learned_Route 1 > > + > > +AS_BOX([No dynamic-routing-port-name: routes learned on lrp1 and lrp2]) > > +check ovn-nbctl --wait=hv \ > > + remove logical_router_port lrp2 options dynamic-routing-port-name > > +check_row_count Learned_Route 1 ip_prefix=3.3.3.0/24 \ > > + nexthop=2.2.2.2 \ > > + logical_port=$lrp1 > > +check_row_count Learned_Route 1 ip_prefix=3.3.3.0/24 \ > > + nexthop=2.2.2.2 \ > > + logical_port=$lrp2 > > +check_row_count Learned_Route 2 > > + > > +OVN_CLEANUP_CONTROLLER([hv1]) > > + > > +as ovn-sb > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > + > > +as ovn-nb > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > + > > +as northd > > +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > > + > > +as > > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > > +/connection dropped.*/d"]) > > + > > +AT_CLEANUP > > +]) > > + > > OVN_FOR_EACH_NORTHD([ > > AT_SETUP([Mac binding aging - Probing]) > > AT_KEYWORDS([mac_binding_probing]) > > -- > > 2.51.0 > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thank you Dumitru and Felix, I went ahead, merged this into main and backported all the way down to 25.03. In order to make it work on 25.03 I had to also backport https://github.com/ovn-org/ovn/commit/aa088003. Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
