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
