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