Em qua., 7 de jan. de 2026 às 13:15, Lucas Vargas Dias < [email protected]> escreveu:
> Hi > Thanks for the review. > > Em qua., 7 de jan. de 2026 às 03:34, Ales Musil <[email protected]> > escreveu: > >> >> >> On Tue, Jan 6, 2026 at 1:14 PM Lucas Vargas Dias via dev < >> [email protected]> wrote: >> >> Hi Lucas, >> >> thank you for the patch, I have a couple of small comments down below. >> >> If the LRPs are scheduled in the same chassis for some reason, >>> ovn controller will try to advertise many times the same prefix with the >>> same priority. >>> This generates a high CPU load in ovn-controller and generate a lot of >>> messages >>> like the following: >>> "Add route table_id=1337 dst=42.10.10.14 plen=32 nexthop=(blackhole) >>> failed: File exists" >>> To fix it, just check if prefix was already advertised with the same >>> priority. >>> >> >> nit: Missing Fixes: ccb0b6b9109c ("controller: Introduce route node.") >> > > > Agree > > >> >>> Signed-off-by: Lucas Vargas Dias <[email protected]> >>> --- >>> >> controller/route.c | 22 +++- >>> tests/system-ovn.at | 249 ++++++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 268 insertions(+), 3 deletions(-) >>> >>> diff --git a/controller/route.c b/controller/route.c >>> index ecddd0497..8c5ac66f8 100644 >>> --- a/controller/route.c >>> +++ b/controller/route.c >>> @@ -368,7 +368,24 @@ route_run(struct route_ctx_in *r_ctx_in, >>> } >>> } >>> >>> - struct advertise_route_entry *ar = xmalloc(sizeof(*ar)); >>> + struct in6_addr nexthop = IN6_IS_ADDR_V4MAPPED(&prefix) >>> + ? ad->ipv4_nexthop : ad->ipv6_nexthop; >>> + uint32_t hash = advertise_route_hash(&prefix, &nexthop, plen); >>> + bool is_advertised = false; >>> + struct advertise_route_entry *ar; >>> + HMAP_FOR_EACH_WITH_HASH (ar, node, hash, &ad->routes) { >>> + if (ar->priority == priority && >>> + ipv6_addr_equals(&ar->addr, &prefix) && ar->plen == >>> plen && >>> + ipv6_addr_equals(&ar->nexthop, &nexthop)) { >>> + is_advertised = true; >>> + break; >>> + } >>> + } >>> >> >> This would be the second time we do this search directly, the other one >> is in >> route-exchange-netlink.c, could you please make a "advertise_route_find" >> function and use that in both places instead? >> > > Yes, I'll adjust > > >> >> >>> + if (is_advertised) { >>> + continue; >>> + } >>> + >>> + ar = xmalloc(sizeof(*ar)); >>> *ar = (struct advertise_route_entry) { >>> .addr = prefix, >>> .plen = plen, >>> @@ -376,8 +393,7 @@ route_run(struct route_ctx_in *r_ctx_in, >>> .nexthop = IN6_IS_ADDR_V4MAPPED(&prefix) >>> ? ad->ipv4_nexthop : ad->ipv6_nexthop, >>> >> >> nit: We could use the nexthop from above instead of the ternary operator >> here again. >> > > Agree > > >> >> >>> }; >>> - hmap_insert(&ad->routes, &ar->node, >>> - advertise_route_hash(&ar->addr, &ar->nexthop, >>> plen)); >>> + hmap_insert(&ad->routes, &ar->node, hash); >>> } >>> >>> smap_destroy(&port_mapping); >>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >>> index ec3b3735f..0de4acae1 100644 >>> --- a/tests/system-ovn.at >>> +++ b/tests/system-ovn.at >>> @@ -17035,6 +17035,95 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query >>> port patch-.*/d >>> AT_CLEANUP >>> ]) >>> >>> +OVN_FOR_EACH_NORTHD([ >>> +AT_SETUP([dynamic-routing - multiple DGP with same priority]) >>> + >>> +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:dynamic-routing=true >>> \ >>> + options:dynamic-routing-vrf-id=1337 >>> \ >>> + -- lrp-add lr lr-gw1 00:00:00:01:00:10 42.10.10.12/24 >>> \ >>> + -- lrp-set-gateway-chassis lr-gw1 hv1 10 >>> \ >>> + -- lrp-set-options lr-gw1 dynamic-routing-redistribute=nat,lb >>> \ >>> + dynamic-routing-maintain-vrf=true >>> \ >>> + -- lrp-add lr lr-gw2 00:00:00:02:00:10 42.20.10.22/24 >>> \ >>> + -- lrp-set-gateway-chassis lr-gw2 hv1 10 >>> \ >>> + -- lrp-set-options lr-gw2 dynamic-routing-redistribute=nat,lb >>> \ >>> + dynamic-routing-maintain-vrf=true >>> \ >>> + -- lrp-add lr lr-int1 00:00:00:00:01:02 30.0.1.1/24 >>> \ >>> + -- lrp-set-options lr-int1 >>> dynamic-routing-redistribute=connected \ >>> + -- lrp-add lr lr-int2 00:00:00:00:01:02 30.0.2.1/24 >>> \ >>> + -- lrp-set-options lr-int2 >>> dynamic-routing-redistribute=connected \ >>> + -- ls-add ls >>> \ >>> + -- lsp-add-router-port ls ls-lr-gw1 lr-gw1 >>> \ >>> + -- lsp-add-router-port ls ls-lr-gw2 lr-gw2 >>> \ >>> + -- ls-add ls-int1 >>> \ >>> + -- lsp-add-router-port ls-int1 ls-int1-lr lr-int1 >>> \ >>> + -- lsp-add ls-int1 w1 >>> \ >>> + -- lsp-set-addresses w1 "00:00:00:00:00:01 30.0.1.11" >>> \ >>> + -- ls-add ls-int2 >>> \ >>> + -- lsp-add-router-port ls-int2 ls-int2-lr lr-int2 >>> \ >>> + -- lsp-add ls-int2 w2 >>> \ >>> + -- lsp-set-addresses w2 "00:00:00:00:00:02 30.0.2.11" >>> \ >>> + -- lr-nat-add lr dnat_and_snat 42.10.10.23 30.0.1.11 w1 >>> 00:00:00:00:01:11 \ >>> + -- lb-add lb1 42.10.10.14 30.0.1.11 >>> \ >>> + -- lr-lb-add lr lb1 >>> \ >>> + -- lr-nat-add lr dnat_and_snat 42.20.10.23 30.0.2.11 w2 >>> 00:00:00:00:02:11 \ >>> + -- lb-add lb2 42.20.10.24 30.0.2.11 >>> \ >>> + -- lr-lb-add lr lb2 >>> + >>> +check ovs-vsctl add-port br-int w1 \ >>> + -- set interface w1 type=internal external_ids:iface-id=w1 >>> +check ovs-vsctl add-port br-int w2 \ >>> + -- set interface w2 type=internal external_ids:iface-id=w2 >>> +check ovn-nbctl --wait=hv sync >>> +wait_for_ports_up w1 w2 >>> + >>> +lrp1=$(fetch_column port_binding _uuid logical_port="lrp1") >>> +lrp2=$(fetch_column port_binding _uuid logical_port="lrp2") >>> >> >> nit: Those variables are unused. >> > > Agree > > >> >> >>> + >>> +AT_CHECK([ip vrf show ovnvrf1337], [0], [dnl >>> +ovnvrf1337 1337 >>> +]) >>> + >>> +OVN_ROUTE_EQUAL([ovnvrf1337], [dnl >>> +blackhole 30.0.1.0/24 proto ovn metric 1000 >>> +blackhole 30.0.2.0/24 proto ovn metric 1000 >>> +blackhole 42.10.10.14 proto ovn metric 100 >>> +blackhole 42.10.10.23 proto ovn metric 100 >>> +blackhole 42.20.10.23 proto ovn metric 100 >>> +blackhole 42.20.10.24 proto ovn metric 100]) >>> + >>> +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 >>> >> >> What is the purpose of those deletions? It doesn't seem to affect >> the test in any way we should probably remove them. >> > > > Agree > Hi Ales, Test fails sometimes if it doesn't have the deletions. ./system-ovn.at:17038: wc -l < flow-diff-$hv --- - 2026-01-08 15:39:00.994072572 -0300 +++ /home/lucas.vdias/ovn/tests/system-userspace-testsuite.dir/at-groups/231/stdout 2026-01-08 15:39:00.992921540 -0300 @@ -1,2 +1,2 @@ -0 +2 cat tests/system-userspace-testsuite.dir/231/flow-diff-hv1 - cookie=xx, duration=xx, table=39, n_packets=xx, n_bytes=xx, idle_age=xx, priority=80,arp,reg10=0/0x2,metadata=0x1,arp_tpa=42.10.10.23,arp_op=1 actions=load:0x1->NXM_NX_REG15[],resubmit(,41) + cookie=xx, duration=xx, table=39, n_packets=xx, n_bytes=xx, idle_age=xx, priority=80,arp,reg10=0/0x2,metadata=0x1,arp_tpa=42.10.10.23,arp_op=1 actions=load:0x2->NXM_NX_REG15[],resubmit(,41) Regards, Lucas > > >> >> >>> + >>> +OVN_CLEANUP_CONTROLLER([hv1]) >>> + >>> +OVN_CLEANUP_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]) >>> @@ -19752,3 +19841,163 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/.*error >>> receiving.*/d >>> /.*terminating with signal 15.*/d"]) >>> AT_CLEANUP >>> ]) >>> + >>> +OVN_FOR_EACH_NORTHD([ >>> +AT_SETUP([dynamic-routing - BGP learned and advertised routes]) >>> >> >> This test is a good addition however I don't think it actually covers >> anything >> done by this patch. Would you mind please sending it separately? >> > > You're right. I'll send it separately. > > > Regards, > Lucas > > >> >> + >>> +# 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 >>> + >>> +# 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-maintain-vrf=false \ >>> + 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/8 >>> +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 >>> + >>> +# 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 "00:00:00:00:00:03 20.0.0.3" >>> + >>> +# 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/8 >>> +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' >>> + >>> +# 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 >>> +wait_row_count Learned_Route 1 ip_prefix=10.10.3.1 >>> + >>> +# Add a second route to the VRF (simulating BGP learning a route) >>> +ip route add 10.10.4.1 via 20.0.0.25 vrf vrf-$id proto zebra >>> + >>> +# Verify both routes appear in SB database. >>> +wait_row_count Learned_Route 1 ip_prefix=10.10.4.1 >>> + >>> +# Remove one route. >>> +AS_BOX([$(date +%H:%M:%S.%03N) Remove first route]) >>> +check ip route del 10.10.4.1 via 20.0.0.25 vrf vrf-$id proto zebra >>> + >>> +# Verify only one route remains in SB database. >>> +wait_row_count Learned_Route 1 >>> +check_row_count Learned_Route 1 ip_prefix=10.10.3.1 >>> + >>> +# Remove second route. >>> +AS_BOX([$(date +%H:%M:%S.%03N) Remove second route]) >>> +check ip route del 10.10.3.1 via 20.0.0.25 vrf vrf-$id >>> + >>> +# Verify all routes removed from SB database. >>> +wait_row_count Learned_Route 0 >>> + >>> +# Add again 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 >>> +wait_row_count Learned_Route 1 ip_prefix=10.10.3.1 >>> + >>> +# Disable dynamic routing >>> +AS_BOX([$(date +%H:%M:%S.%03N) Disable dynamic-routing]) >>> +check ovn-nbctl --wait=sb set Logical_Router lr-frr >>> options:dynamic-routing=false >>> +check_row_count Port_Binding 0 logical_port=cr-lrp-local-bgp-port >>> 'options:dynamic-routing=true' >>> + >>> +# Add one more route to the VRF (simulating BGP learning a route) >>> +check ip route add 10.10.4.1 via 20.0.0.25 vrf vrf-$id proto zebra >>> + >>> +# Verify learned routes are removed as dynamic-routing=false >>> +wait_row_count Learned_Route 0 >>> + >>> +# Re-enable dynamic routing >>> +check ovn-nbctl --wait=sb set Logical_Router lr-frr >>> options:dynamic-routing=true >>> +check_row_count Port_Binding 1 logical_port=cr-lrp-local-bgp-port >>> 'options:dynamic-routing=true' >>> + >>> +# Verify both routes appear in SB database. >>> +wait_row_count Learned_Route 1 ip_prefix=10.10.3.1 >>> +wait_row_count Learned_Route 1 ip_prefix=10.10.4.1 >>> +check_row_count Learned_Route 2 >>> + >>> +# 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 >>> +]) >>> -- >>> 2.43.0 >>> >>> >>> -- >>> >>> >>> >>> >>> _'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 >>> >>> >> Regards, >> Ales >> > -- _‘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
