Hi Dumitru
Em qua., 28 de jan. de 2026 às 08:03, Dumitru Ceara <[email protected]> escreveu: > On 1/24/26 6:04 PM, Lucas Vargas Dias via dev wrote: > > If logical router has more than one LRP as gateway router port > > and dynamic routing configured, dynamic-routing-port-name could be > > used to specify the LRP that will be used to dynamic routing. > > However, if all LRPs learning routes, routes from LRP without > > dynamic-routing-port-name must be flushed. > > This happens when LRPs are scheduled in the same chassis. > > > > Signed-off-by: Lucas Vargas Dias <[email protected]> > > --- > > Hi Lucas, > > Thanks for the patch! > > > controller/route-exchange.c | 39 ++++++++++- > > tests/system-ovn.at | 126 ++++++++++++++++++++++++++++++++++++ > > 2 files changed, 164 insertions(+), 1 deletion(-) > > > > diff --git a/controller/route-exchange.c b/controller/route-exchange.c > > index ae44ffe69..62f9a729b 100644 > > --- a/controller/route-exchange.c > > +++ b/controller/route-exchange.c > > @@ -26,6 +26,7 @@ > > #include "openvswitch/list.h" > > > > #include "lib/ovn-sb-idl.h" > > +#include "lib/uuidset.h" > > > > #include "binding.h" > > #include "ha-chassis.h" > > @@ -149,12 +150,33 @@ sb_sync_learned_routes(const struct vector > *learned_routes, > > struct hmap sync_routes = HMAP_INITIALIZER(&sync_routes); > > const struct sbrec_learned_route *sb_route; > > struct route_entry *route_e; > > + struct uuidset uuid_set = UUIDSET_INITIALIZER(&uuid_set); > > Maybe use a more explicit variable name instead of "uuid_set"? > > Agree > > + bool has_dynamic_routing_port_name = false; > > Nit: I'd reorder the declarations to maintain the reverse xmas tree. > > We actually don't need the has_dynamic_routing_port_name variable. It's > equivalent to "!uuidset_is_empty(&uuid_set)". > > Agree > > > > struct sbrec_learned_route *filter = > > > sbrec_learned_route_index_init_row(sbrec_learned_route_by_datapath); > > sbrec_learned_route_index_set_datapath(filter, datapath); > > SBREC_LEARNED_ROUTE_FOR_EACH_EQUAL (sb_route, filter, > > > sbrec_learned_route_by_datapath) { > > + route_add_entry(&sync_routes, sb_route, false); > > + const char *dynamic_routing_port_name = > > + smap_get(&sb_route->logical_port->options, > > + "dynamic-routing-port-name"); > > Nit: indentation; this should be one space to the left. > Agree > > > + if (!dynamic_routing_port_name) { > > + const struct sbrec_port_binding *cr_pb = > > + lport_get_cr_port(sbrec_port_binding_by_name, > > + sb_route->logical_port, NULL); > > + if (cr_pb) { > > + dynamic_routing_port_name = > > + smap_get(&cr_pb->options, > "dynamic-routing-port-name"); > > + } > > + } > > + > > + if (dynamic_routing_port_name) { > > + uuidset_insert(&uuid_set, > &sb_route->logical_port->header_.uuid); > > + has_dynamic_routing_port_name = true; > > + } > > + > > /* If the port is not local we don't care about it. > > * Some other ovn-controller will handle it. > > * We may not use smap_get since the value might be validly > NULL. */ > > @@ -162,10 +184,25 @@ sb_sync_learned_routes(const struct vector > *learned_routes, > > sb_route->logical_port->logical_port)) { > > continue; > > } > > - route_add_entry(&sync_routes, sb_route, true); > > + route_e = route_lookup(&sync_routes, datapath, > > + sb_route->logical_port, > > + sb_route->ip_prefix, > > + sb_route->nexthop); > > 'route_e' is the one we just added in the beginning of the loop, right? > Why do we do this lookup here? Why not store a pointer to the one we > add? We'd have to change route_add_entry() then to return what it > inserted. > Agree > > > + if (route_e) { > > + route_e->stale = true; > > + } > > } > > sbrec_learned_route_index_destroy_row(filter); > > > > + if (has_dynamic_routing_port_name) { > > + HMAP_FOR_EACH_SAFE (route_e, hmap_node, &sync_routes) { > > + if (!uuidset_find(&uuid_set, > > + &route_e->sb_route->logical_port->header_.uuid)) { > > + route_e->stale = true; > > So, at this point we also mark all SB.Learned_Routes that were learned > on a logical port that's a chassis-redirect port bound to a different > chassis as stale. And lower in this function we'll delete it if we > don't have it in the VRF's routing table as an extern_learned route. > > But that other chassis might have the route in its VRF. ovn-controller > on that other chassis will wake up due to the SB update and recreate the > route. > > And this will go on forever. > > Or am I missing something? > Or was your intention actually to use the 'uuid_set' to store only > SB.Learned_Route records whose chassis-redirect logical_port is set > _and_ is local to the current chassis? > This scenario is impossible to test with a system-ovn.at test (we only > have one OVN chassis in those). Would it be possible to add a multinode > test for it? We have some BGP tests there. > > The idea would be to also ensure that in such cases ovn-controllers > don't delete resources maintained by others. > You're right, my intention was to store only the learned routes from the current chassis. I'll adjust to checking the chassis from the logical port or cr port. > > > + } > > + } > > + } > > + uuidset_destroy(&uuid_set); > > struct re_nl_received_route_node *learned_route; > > VECTOR_FOR_EACH_PTR (learned_routes, learned_route) { > > char *ip_prefix = normalize_v46_prefix(&learned_route->prefix, > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index 6eaf38a4c..8b82cc3a0 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -20531,3 +20531,129 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/.*error > receiving.*/d > > /.*terminating with signal 15.*/d"]) > > AT_CLEANUP > > ]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([dynamic-routing - BGP learned routes with router filter name > and multiple DGPs]) > > + > > +# This test validates that BGP learned routes work correctly: > > +# 1. Routes added to the VRF appear in Learned_Route table > > +# 2. Stopping ovn-controller remove learned routes > > +# > > +# Topology: > > +# +---------+ > > +# | public | > > +# +----+----+ > > +# | > > +# +----+---+ > > +# | lr-frr | (in VRF 10) > > +# +----+---+ > > +# | > > +# +------+-------+ > > +# |local-bgp-port| (in VRF 10) > > +# +--------------+ > > + > > +ovn_start > > +OVS_TRAFFIC_VSWITCHD_START() > > +ADD_BR([br-int]) > > +ADD_BR([br-ex]) > > + > > +check ovs-ofctl add-flow br-ex action=normal > > + > > +# Set external-ids in br-int needed for ovn-controller > > +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 > > + > > +# Configure bridge mappings for localnet > > +check ovs-vsctl set Open_vSwitch . > external-ids:ovn-bridge-mappings=phys:br-ex > > + > > +id=10 > > Nit: s/id/vrf. > > We should also: > > VRF_RESERVE([$vrf]) > > > + > > +# Start ovn-controller > > +start_daemon ovn-controller > > + > > +check ip link add vrf-$id type vrf table $id > > +on_exit "ip link del vrf-$id" > > +check ip link set vrf-$id up > > + > > +# Create public logical switch with localnet port > > +check ovn-nbctl ls-add public > > +check ovn-nbctl lsp-add-localnet-port public ln_port phys > > + > > +# Create lr-frr with dynamic routing in VRF $id > > +check ovn-nbctl lr-add lr-frr \ > > + -- set Logical_Router lr-frr \ > > + options:dynamic-routing=true \ > > + options:dynamic-routing-vrf-id=$id \ > > + options:dynamic-routing-redistribute=static > > + > > +check ovn-nbctl lrp-add lr-frr lrp-local-bgp-port 00:00:00:00:00:03 > 20.0.0.3/24 \ > > + -- set Logical_Router_Port lrp-local-bgp-port > options:dynamic-routing-maintain-vrf=true \ > > If maintain-vrf=true, why do we create the vrf ourselves above? > Actually, I forgot to change for maintain-vrf=false here > > > + -- set Logical_Router_Port lrp-local-bgp-port > options:routing-protocol-redirect=local-bgp-port > > + > > +check ovn-nbctl lrp-set-gateway-chassis lrp-local-bgp-port hv1 > > +check ovn-nbctl lsp-add-router-port public public-lr-frr > lrp-local-bgp-port > > + > > +check ovn-nbctl lrp-add lr-frr lrp-dgp-dummy 00:00:00:00:00:04 > 20.0.1.3/24 > > +check ovn-nbctl lrp-set-gateway-chassis lrp-dgp-dummy hv1 > > +check ovn-nbctl ls-add ls-dummy > > +check ovn-nbctl lsp-add-router-port ls-dummy lsp-dummy lrp-dgp-dummy > > + > > + > > +# Create local-bgp-port in VRF 10 > > +check ovs-vsctl add-port br-int local-bgp-port \ > > + -- set Interface local-bgp-port type=internal \ > > + -- set Interface local-bgp-port external_ids:iface-id=local-bgp-port > > + > > +check ovn-nbctl lsp-add public local-bgp-port \ > > + -- lsp-set-addresses local-bgp-port unknown > > + > > +# Configure local-bgp-port interface and add to VRF > > +check ip link set local-bgp-port master vrf-$id > > +check ip link set local-bgp-port address 00:00:00:00:00:03 > > +check ip addr add dev local-bgp-port 20.0.0.3/24 > > +check ip link set local-bgp-port up > > + > > +# Wait for everything to be ready > > +wait_for_ports_up > > +check ovn-nbctl --wait=hv sync > > + > > If we remove the explicit vrf creation in the beginning of this test, we > should also wait here until ovn-controller created the VRF > (maintain-vrf=true), i.e.: > > OVS_WAIT_WHILE([ip link | grep -q ovnvrf$vrf:.*UP]) > > > +# Check lrp-local-bgp-port has dynamic-routing option set. > > +check_row_count Port_Binding 1 logical_port=cr-lrp-local-bgp-port > 'options:dynamic-routing=true' > > + > > +# Add static routes > > +check ovn-nbctl lr-route-add lr-frr 10.10.2.1 20.0.0.42 > lrp-local-bgp-port > > + > > +# Verify advertised routes exist > > +AS_BOX([Advertised_Route]) > > +wait_row_count Advertised_Route 1 ip_prefix=10.10.2.1 > > + > > +# Add a route to the VRF (simulating BGP learning a route) > > +check ip route add 10.10.3.1 via 20.0.0.25 vrf vrf-$id proto zebra > > + > > + > > Nit: missing space. > > > +# Verify learned route appears in SB database > > +check_row_count Learned_Route 2 ip_prefix=10.10.3.1 > > + > > +check ovn-nbctl --wait=hv set Logical_Router_Port lrp-local-bgp-port > options:dynamic-routing-port-name=bgpvrf1002 > > + > > +check_row_count Learned_Route 1 ip_prefix=10.10.3.1 > > + > > +# Stop ovn-controller > > +OVN_CLEANUP_CONTROLLER([hv1], [], [], [lr-frr]) > > +check ovn-nbctl --wait=sb sync > > + > > +# Verify routes are removed in SB database. > > +wait_row_count Learned_Route 0 > > + > > +OVN_CLEANUP_NORTHD > > + > > +as > > +OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d > > +/failed to query port patch-.*/d > > +/.*terminating with signal 15.*/d"]) > > +AT_CLEANUP > > +]) > > Regards, > Dumitru > > Regards, Lucas -- _‘Esta mensagem é direcionada apenas para os endereços constantes no cabeçalho inicial. Se você não está listado nos endereços constantes no cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas estão imediatamente anuladas e proibidas’._ * **‘Apesar do Magazine Luiza tomar todas as precauções razoáveis para assegurar que nenhum vírus esteja presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por quaisquer perdas ou danos causados por esse e-mail ou por seus anexos’.* _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
