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 > > >> + >> +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
