Hi Dumitru Thanks for the review. Em sex., 13 de fev. de 2026 às 12:03, Dumitru Ceara <[email protected]> escreveu:
> On 1/30/26 12:55 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 new revision! > > > controller/ovn-controller.c | 16 ++++ > > controller/route-exchange.c | 47 +++++++++++- > > controller/route-exchange.h | 1 + > > tests/ovn-inc-proc-graph-dump.at | 2 + > > tests/system-ovn.at | 125 +++++++++++++++++++++++++++++++ > > On the v0 review I was asking if it's possible to add a multinode.at > test, should we do that? It was quite hard for me to reason about all > possible cases just with the system test you added. Checking with two > hypervisors would create more confidence in the behavior of this code. > > I see the problem related to order of configurations when DGPs are in the same chassis. But you're right, I'll add multinode tests to checking with two hypervisors and to be more easy understanding the code. > > 5 files changed, 187 insertions(+), 4 deletions(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 2d9b3e033..1c7b3b256 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -5494,10 +5494,24 @@ en_route_exchange_run(struct engine_node *node, > void *data) > > return EN_STALE; > > } > > > > + const struct ovsrec_open_vswitch_table *ovs_table = > > + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); > > + const char *chassis_id = get_ovs_chassis_id(ovs_table); > > + ovs_assert(chassis_id); > > + > > + struct ovsdb_idl_index *sbrec_chassis_by_name = > > + engine_ovsdb_node_get_index( > > + engine_get_input("SB_chassis", node), > > + "name"); > > + const struct sbrec_chassis *chassis > > + = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); > > + ovs_assert(chassis); > > + > > struct route_exchange_ctx_in r_ctx_in = { > > .ovnsb_idl_txn = engine_get_context()->ovnsb_idl_txn, > > .sbrec_learned_route_by_datapath = > sbrec_learned_route_by_datapath, > > .sbrec_port_binding_by_name = sbrec_port_binding_by_name, > > + .chassis = chassis, > > .announce_routes = &route_data->announce_routes, > > }; > > struct route_exchange_ctx_out r_ctx_out = { > > @@ -6679,6 +6693,8 @@ inc_proc_ovn_controller_init( > > engine_add_input(&en_route, &en_sb_datapath_binding, > > route_sb_datapath_binding_handler); > > > > + engine_add_input(&en_route_exchange, &en_ovs_open_vswitch, NULL); > > + engine_add_input(&en_route_exchange, &en_sb_chassis, NULL); > > engine_add_input(&en_route_exchange, &en_route, NULL); > > engine_add_input(&en_route_exchange, &en_sb_learned_route, > > engine_noop_handler); > > diff --git a/controller/route-exchange.c b/controller/route-exchange.c > > index ae44ffe69..f8a36f55d 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" > > @@ -86,7 +87,7 @@ maintained_route_table_add(uint32_t table_id) > > hmap_insert(&_maintained_route_tables, &mrt->node, hash); > > } > > > > -static void > > +static struct route_entry * > > route_add_entry(struct hmap *routes, > > const struct sbrec_learned_route *sb_route, > > bool stale) > > @@ -102,6 +103,7 @@ route_add_entry(struct hmap *routes, > > hash = hash_string(sb_route->ip_prefix, hash); > > > > hmap_insert(routes, &route_e->hmap_node, hash); > > + return route_e; > > } > > > > static struct route_entry * > > @@ -144,28 +146,64 @@ sb_sync_learned_routes(const struct vector > *learned_routes, > > struct ovsdb_idl_txn *ovnsb_idl_txn, > > struct ovsdb_idl_index > *sbrec_port_binding_by_name, > > struct ovsdb_idl_index > *sbrec_learned_route_by_datapath, > > - bool *sb_changes_pending) > > + bool *sb_changes_pending, > > + const struct sbrec_chassis *chassis) > > { > > struct hmap sync_routes = HMAP_INITIALIZER(&sync_routes); > > const struct sbrec_learned_route *sb_route; > > - struct route_entry *route_e; > > + struct route_entry *route_e = NULL; > > + struct uuidset uuid_set = UUIDSET_INITIALIZER(&uuid_set); > > On the v0 review I was asking if it's possible to use a more > descriptive name instead of 'uuid_set'. I guess that slipped > through. > Sorry, I forget this. > > > > > 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) { > > + const struct sbrec_port_binding *cr_pb = > > + lport_get_cr_port(sbrec_port_binding_by_name, > > + sb_route->logical_port, NULL); > > + const char *dynamic_routing_port_name = > > + smap_get(&sb_route->logical_port->options, > > + "dynamic-routing-port-name"); > > + if (!dynamic_routing_port_name && cr_pb) { > > + dynamic_routing_port_name = > > + smap_get(&cr_pb->options, "dynamic-routing-port-name"); > > + } > > + > > + if (sb_route->logical_port->chassis == chassis || > > + (cr_pb && cr_pb->chassis == chassis)) { > > + route_e = route_add_entry(&sync_routes, sb_route, false); > > + if (dynamic_routing_port_name) { > > + uuidset_insert(&uuid_set, > > + &sb_route->logical_port->header_.uuid); > > + } > > + } > > + > > /* 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. */ > > if (!smap_get_node(bound_ports, > > sb_route->logical_port->logical_port)) { > > + route_e = NULL; > > + continue; > > + } > > + if (route_e) { > > + route_e->stale = true; > > continue; > > } > > route_add_entry(&sync_routes, sb_route, true); > > } > > sbrec_learned_route_index_destroy_row(filter); > > > > + if (!uuidset_is_empty(&uuid_set)) { > > + 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; > > + } > > + } > > + } > > + uuidset_destroy(&uuid_set); > > I'm not sure the changes in sb_sync_learned_routes() are correct. Please > see below in the system test part, I commented about why I think the test > is wrong, which would mean that ovn-controller's behavior is also wrong. > > I tried to simplify the test for my case. > > 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, > > @@ -304,7 +342,8 @@ route_exchange_run(const struct > route_exchange_ctx_in *r_ctx_in, > > &ad->bound_ports, > r_ctx_in->ovnsb_idl_txn, > > r_ctx_in->sbrec_port_binding_by_name, > > > r_ctx_in->sbrec_learned_route_by_datapath, > > - &r_ctx_out->sb_changes_pending); > > + &r_ctx_out->sb_changes_pending, > > + r_ctx_in->chassis); > > > > route_table_add_watch_request(&r_ctx_out->route_table_watches, > > table_id); > > diff --git a/controller/route-exchange.h b/controller/route-exchange.h > > index e3791c331..53828a8b9 100644 > > --- a/controller/route-exchange.h > > +++ b/controller/route-exchange.h > > @@ -24,6 +24,7 @@ struct route_exchange_ctx_in { > > struct ovsdb_idl_txn *ovnsb_idl_txn; > > struct ovsdb_idl_index *sbrec_port_binding_by_name; > > struct ovsdb_idl_index *sbrec_learned_route_by_datapath; > > + const struct sbrec_chassis *chassis; > > > > /* Contains struct advertise_datapath_entry */ > > const struct hmap *announce_routes; > > diff --git a/tests/ovn-inc-proc-graph-dump.at b/tests/ > ovn-inc-proc-graph-dump.at > > index 3fe7b8fbd..0a514ffe5 100644 > > --- a/tests/ovn-inc-proc-graph-dump.at > > +++ b/tests/ovn-inc-proc-graph-dump.at > > @@ -460,6 +460,8 @@ digraph "Incremental-Processing-Engine" { > > route_table_notify [[style=filled, shape=box, fillcolor=white, > label="route_table_notify"]]; > > route_exchange_status [[style=filled, shape=box, fillcolor=white, > label="route_exchange_status"]]; > > route_exchange [[style=filled, shape=box, fillcolor=white, > label="route_exchange"]]; > > + OVS_open_vswitch -> route_exchange [[label=""]]; > > + SB_chassis -> route_exchange [[label=""]]; > > route -> route_exchange [[label=""]]; > > SB_learned_route -> route_exchange [[label="engine_noop_handler"]]; > > SB_port_binding -> route_exchange [[label="engine_noop_handler"]]; > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index 67f03e3be..b0a023791 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -20593,3 +20593,128 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query > port patch-.*/d > > /connection dropped.*/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) > > +# +----+---+ > > There's also a ls-dummy in the test, not depicted here. It might > be useful to tag the DGPs and their chassis here. > I agree > > > +# | > > +# +------+-------+ > > +# |local-bgp-port| (in VRF 10) > > +# +--------------+ > > This is a bit confusing... local-bgp-port is actually a LSP on > "public" and it has the same IP as the lr-frr port towards "public". > > > + > > +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 > > All comments should be sentences, they should end with period. That's > the case for more comments in this patch. > I agree > > > +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 > > As mentioned in my review of v0, I'd call this "vrf" instead of "id. > > Sorry, I forget > Also, we need: > > VRF_RESERVE([$vrf]) > > I agree. I'll adjust > > + > > +# 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=false \ > > + -- 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 > > + > > + > > Nit: one empty line is enough. > > Agree > > +# 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 > > + > > +# 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' > > + > > I think we should check that both DGPs are bound and with > dynamic routing enabled: > I agree > > check_row_count Port_Binding 1 logical_port=cr-lrp-dgp-dummy > '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 > > + > > +# 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 > > This was hard to decipher for me... what happens here is that the > cr-lrp-local-bgp-port now ends up with: > > _uuid : cce1f38d-51b3-4734-b451-f53910f3e137 > options : {always-redirect="true", > distributed-port=lrp-local-bgp-port, dynamic-routing="true", > dynamic-routing-port-name=bgpvrf1002} > logical_port : cr-lrp-local-bgp-port > chassis : 85f900e2-a456-42ff-a90f-682bf0bb6ba6 > > And cr-lrp-dgp-dummy with: > > _uuid : 586ead78-a51b-4b6e-a25a-f45936a2f2c2 > options : {always-redirect="true", > distributed-port=lrp-dgp-dummy, dynamic-routing="true"} > logical_port : cr-lrp-dgp-dummy > chassis : 85f900e2-a456-42ff-a90f-682bf0bb6ba6 > > And because lr-frr has at least one logical router port with > dynamic-routing-port-name set (cr-lrp-local-bgp-port) we ignore dynamic > routing on all the other ports (that don't have dynamic-routing-port-name > set). > > So cr-lrp-dgp-dummy should not be used for dynamic routing > anymore. > When I added this test, we could configure dynamic-routing just by logical_router, so it'll copy the dynamic-routing for other LRPs. > > However, the actual remaining learned route is: > # ovn-sbctl list learned_route > _uuid : e850df2d-0644-4aad-a606-94c998399672 > datapath : 8ce61f1b-e838-42b9-a26a-014b310eebcc > external_ids : {} > ip_prefix : "10.10.3.1" > logical_port : a0838191-c9aa-4d6d-a539-741e11d97bdd > nexthop : "20.0.0.25" > > And that's on port: > _uuid : a0838191-c9aa-4d6d-a539-741e11d97bdd > options : {chassis-redirect-port=cr-lrp-local-bgp-port, > peer=public-lr-frr} > logical_port : lrp-local-bgp-port > chassis : [] > > AKA on DGP: > _uuid : cce1f38d-51b3-4734-b451-f53910f3e137 > options : {always-redirect="true", > distributed-port=lrp-local-bgp-port, dynamic-routing="true", > dynamic-routing-port-name=bgpvrf1002} > logical_port : cr-lrp-local-bgp-port > chassis : 85f900e2-a456-42ff-a90f-682bf0bb6ba6 > > So the route that had initially been learned on cr-lrp-dgp-dummy > has now been removed. That's correct. > > BUT, the remaining route should also be deleted, if I'm not wrong. > > You're right, both routes must be deleted. But, here I configured dynamic-routing-port-name with wrong value in the test. > The port it's learned on has dynamic-routing="true" and > dynamic-routing-port-name set. But the dynamic-routing-port-name > is set to bgpvrf1002. And the DGP is bound to the local chassis. > But if there's no bgpvrf1002 interface on the local chassis the > route shouldn't be learned (see the docs in ovn-nb(5)). > So I think what would be correct to check for at this point is that > all learned routes are removed from the SB database. > > Then, after that check, we should add another LSP, bound to bgpvrf1002, > or set dynamic-routing-port-mapping like we do in other tests, e.g.: > > check ovs-vsctl set Open_vSwitch . > external-ids:dynamic-routing-port-mapping="bgpvrf1002=lo" > And add the Linux route on the "lo" device: > check ip route add10.10.3.1 via 20.0.0.25 dev lo onlink vrf vrf-$id proto > zebra > Now, because the route is learnt on "dev lo" and because ovn-controller > reads the mapping "lo <-> bgpvrf1002" it should create Learned_Routes > for all DGPs that have dynamic-routing-port-name=bgpvrf1002. > > Or am I missing something? > > Actually, I'm thinking to use local-bgp-port. check ovn-nbctl --wait=hv set Logical_Router_Port lrp-local-bgp-port options:dynamic-routing-port-name=local-bgp-port > > + > > +# Stop ovn-controller > > +OVN_CLEANUP_CONTROLLER([hv1], [], [], [lr-frr]) > > This is unstable in CI, it sometimes fails with: > > I see the same problem in other tests, they fix it with something like: check ovn-nbctl --wait=hv ls-del ls-int1 check ovn-nbctl --wait=hv ls-del ls-int2 check ovn-nbctl --wait=hv ls-del ls check ovn-nbctl --wait=hv lr-del lr Regards, Lucas > Checking after recompute for > ovn-nbctl --wait=hv sync > ./ovn-macros.at:898: "$@" > ./system-ovn.at:21001: wc -l < related-ports-diff > Ignoring lr-frr i.e. tunnel_key 0x2 > ./system-ovn.at:21001: wc -l < flow-diff-$hv > --- - 2026-02-13 12:26:26.530011416 +0000 > +++ > /workspace/ovn-tmp/tests/system-kmod-testsuite.dir/at-groups/272/stdout > 2026-02-13 12:26:26.527566777 +0000 > @@ -1,2 +1,2 @@ > -0 > +2 > > related-ports-diff: > 272. system-ovn.at:21001: 272. dynamic-routing - BGP learned routes with > router filter name and multiple DGPs -- parallelization=yes -- > ovn_monitor_all=yes (system-ovn.at:21001): FAILED (system-ovn.at:21001) > > > +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 > > +]) > > Thanks, > Dumitru > > -- _‘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
