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.")
>
> 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?
> + 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.
> };
> - 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.
> +
> +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.
> +
> +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?
+
> +# 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
