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.
> 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.
>
> 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.
> 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.
> +# |
> +# +------+-------+
> +# |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.
> +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.
Also, we need:
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=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.
> +# 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:
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.
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.
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?
> +
> +# Stop ovn-controller
> +OVN_CLEANUP_CONTROLLER([hv1], [], [], [lr-frr])
This is unstable in CI, it sometimes fails with:
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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
