On 11/4/25 5:27 PM, Lorenzo Bianconi wrote:
> When dynamic-routing-redistribute option is set to connected-as-host for
> logical_router_port where dynamic-routing is enabled, virtual port ips
> are advertise without taking into account if the port is bound locally.
> Fix the issue setting the advertised route tracked_port to the virtual
> parent port in order to allow the CMS to mange the host route priority
> or to not advertised the host route if the port is not local.
>
> Reported-at: https://issues.redhat.com/browse/FDP-1940
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
> Changes since v1:
> - Remove dynamic-routing-redistribute-local-only check in ovn-northd.
> - Improve commit message.
> ---
Hi Lorenzo,
Thanks for v2! The code change looks good to me. I have another
suggestion for improving the test, please see below.
> northd/en-advertised-route-sync.c | 44 +++++++++++++++++++--
> tests/system-ovn.at | 66 +++++++++++++++++++++++++++++++
> 2 files changed, 106 insertions(+), 4 deletions(-)
>
> diff --git a/northd/en-advertised-route-sync.c
> b/northd/en-advertised-route-sync.c
> index c737376ae..b5ce0c291 100644
> --- a/northd/en-advertised-route-sync.c
> +++ b/northd/en-advertised-route-sync.c
> @@ -129,6 +129,7 @@ advertised_route_table_sync(
> const struct sbrec_advertised_route_table *sbrec_advertised_route_table,
> const struct hmap *routes,
> const struct hmap *dynamic_routes,
> + const struct hmap *ls_ports,
> struct advertised_route_sync_data *data);
>
> enum engine_input_handler_result
> @@ -241,6 +242,7 @@ en_advertised_route_sync_run(struct engine_node *node,
> void *data OVS_UNUSED)
> = engine_get_input_data("routes", node);
> struct dynamic_routes_data *dynamic_routes_data
> = engine_get_input_data("dynamic_routes", node);
> + struct northd_data *northd_data = engine_get_input_data("northd", node);
> const struct engine_context *eng_ctx = engine_get_context();
> const struct sbrec_advertised_route_table *sbrec_advertised_route_table =
> EN_OVSDB_GET(engine_get_input("SB_advertised_route", node));
> @@ -251,7 +253,7 @@ en_advertised_route_sync_run(struct engine_node *node,
> void *data OVS_UNUSED)
> sbrec_advertised_route_table,
> &routes_data->parsed_routes,
> &dynamic_routes_data->routes,
> - routes_sync_data);
> + &northd_data->ls_ports, routes_sync_data);
>
> stopwatch_stop(ADVERTISED_ROUTE_SYNC_RUN_STOPWATCH_NAME, time_msec());
> return EN_UPDATED;
> @@ -561,10 +563,37 @@ publish_lport_addresses(struct hmap *sync_routes,
> }
> }
>
> +static void
> +publish_host_routes_for_virtual_ports(
> + struct ovn_port *port,
> + const struct hmap *ls_ports,
> + const struct ovn_datapath *advertising_od,
> + const struct ovn_port *advertising_op,
> + struct hmap *sync_routes)
> +{
> + ovs_assert(port->sb);
> +
> + const char *virtual_parent = port->sb->virtual_parent;
> + if (!virtual_parent) {
> + return;
> + }
> +
> + struct ovn_port *vp = ovn_port_find(ls_ports, virtual_parent);
> + if (!vp) {
> + return;
> + }
> +
> + for (size_t i = 0; i < port->n_lsp_addrs; i++) {
> + publish_lport_addresses(sync_routes, advertising_od, advertising_op,
> + &port->lsp_addrs[i], vp);
> + }
> +}
> +
> /* Collect all IP addresses connected to the out_port of a route.
> * This traverses all LSPs on the LS connected to the out_port. */
> static void
> publish_host_routes(struct hmap *sync_routes,
> + const struct hmap *ls_ports,
> const struct ovn_datapath *advertising_od,
> const struct ovn_port *advertising_op,
> struct advertised_route_sync_data *data)
> @@ -597,6 +626,10 @@ publish_host_routes(struct hmap *sync_routes,
> const struct ovn_port *lrp = port->peer;
> publish_lport_addresses(sync_routes, advertising_od,
> advertising_op, &lrp->lrp_networks, lrp);
> + } else if (port->nbsp && !strcmp(port->nbsp->type, "virtual")) {
> + publish_host_routes_for_virtual_ports(port, ls_ports,
> + advertising_od,
> + advertising_op,
> sync_routes);
> } else {
> /* This is just a plain LSP */
> for (size_t i = 0; i < port->n_lsp_addrs; i++) {
> @@ -657,6 +690,7 @@ advertise_routes_as_host_prefix(
> struct advertised_route_sync_data *data,
> struct uuidset *host_route_lrps,
> struct hmap *sync_routes,
> + const struct hmap *ls_ports,
> const struct ovn_datapath *advertising_od,
> const struct ovn_port *advertising_op,
> enum route_source source
> @@ -673,7 +707,8 @@ advertise_routes_as_host_prefix(
> }
>
> uuidset_insert(host_route_lrps, &advertising_op->nbrp->header_.uuid);
> - publish_host_routes(sync_routes, advertising_od, advertising_op, data);
> + publish_host_routes(sync_routes, ls_ports, advertising_od,
> + advertising_op, data);
> return true;
> }
>
> @@ -726,6 +761,7 @@ advertised_route_table_sync(
> const struct sbrec_advertised_route_table *sbrec_advertised_route_table,
> const struct hmap *routes,
> const struct hmap *dynamic_routes,
> + const struct hmap *ls_ports,
> struct advertised_route_sync_data *data)
> {
> struct hmap sync_routes = HMAP_INITIALIZER(&sync_routes);
> @@ -744,8 +780,8 @@ advertised_route_table_sync(
> }
>
> if (advertise_routes_as_host_prefix(data, &host_route_lrps,
> - &sync_routes, route->od,
> - route->out_port,
> + &sync_routes, ls_ports,
> + route->od, route->out_port,
> route->source)) {
> continue;
> }
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 373a87657..941e1fb87 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -15754,6 +15754,7 @@ AT_CLEANUP
>
> OVN_FOR_EACH_NORTHD([
> AT_SETUP([dynamic-routing - DGP])
> +AT_SKIP_IF([test "$HAVE_ARPING" = no])
>
> VRF_RESERVE([1337])
> VRF_RESERVE([1338])
> @@ -16090,7 +16091,72 @@ blackhole 198.51.100.0/24 proto ovn metric 1000
> # When we now stop the ovn-controller it will remove the VRF.
> start_daemon ovn-controller
> OVS_WAIT_UNTIL([test "$(ovn-appctl -t ovn-controller debug/status)" ==
> "running"])
> +
> +# Create a virutal port "vip" with parent set to "vif4".
> +check ovn-nbctl lsp-add public vif4 \
> + -- lsp-set-addresses vif4 "00:00:ff:ff:ff:04 192.0.2.21"
> +ADD_NAMESPACES(vif4)
> +ADD_VETH(vif4, vif4, br-int, "192.0.2.30/24", "00:00:ff:ff:ff:04",
> "192.0.2.21")
> +check ovn-nbctl lsp-add public vip \
> + -- lsp-set-addresses vip "00:00:00:00:01:88 192.0.2.30" \
> + -- lsp-set-type vip virtual \
> + -- set logical_switch_port vip options:virtual-ip=192.0.2.30 \
> + -- set logical_switch_port vip options:virtual-parents=vif4 \
> + -- set logical_router_port internet-public
> options:dynamic-routing-redistribute-local-only=true
We have no test for the redistribute-local-only=false case.
> check ovn-nbctl --wait=hv sync
> +
> +OVN_ROUTE_EQUAL([ovnvrf1338], [dnl
> +blackhole 192.0.2.2 proto ovn metric 100
> +blackhole 192.0.2.3 proto ovn metric 100
> +blackhole 192.0.2.10 proto ovn metric 100
> +blackhole 192.0.2.20 proto ovn metric 100
> +blackhole 192.0.2.21 proto ovn metric 100
> +blackhole 198.51.100.0/24 proto ovn metric 1000
> +233.252.0.0/24 via 192.168.10.10 dev lo onlink
> +233.253.0.0/24 via 192.168.20.20 dev hv1-mll onlink])
> +
> +# Bind "vip" port locally and check the virtual IP is added in the VRF.
> +NS_EXEC([vif4], [arping -U -c 1 -w 2 -I vif4 192.0.2.30])
> +wait_column "vif4" Port_Binding virtual_parent logical_port=vip
> +wait_for_ports_up vip
> +OVN_ROUTE_EQUAL([ovnvrf1338], [dnl
> +blackhole 192.0.2.2 proto ovn metric 100
> +blackhole 192.0.2.3 proto ovn metric 100
> +blackhole 192.0.2.10 proto ovn metric 100
> +blackhole 192.0.2.20 proto ovn metric 100
> +blackhole 192.0.2.21 proto ovn metric 100
> +blackhole 192.0.2.30 proto ovn metric 100
> +blackhole 198.51.100.0/24 proto ovn metric 1000
> +233.252.0.0/24 via 192.168.10.10 dev lo onlink
> +233.253.0.0/24 via 192.168.20.20 dev hv1-mll onlink])
> +
> +check ovn-sbctl clear Port_Binding vip virtual-parent
> +wait_column "" Port_Binding virtual_parent logical_port=vip
This wait is not needed.
> +OVN_ROUTE_EQUAL([ovnvrf1338], [dnl
> +blackhole 192.0.2.2 proto ovn metric 100
> +blackhole 192.0.2.3 proto ovn metric 100
> +blackhole 192.0.2.10 proto ovn metric 100
> +blackhole 192.0.2.20 proto ovn metric 100
> +blackhole 192.0.2.21 proto ovn metric 100
> +blackhole 198.51.100.0/24 proto ovn metric 1000
> +233.252.0.0/24 via 192.168.10.10 dev lo onlink
> +233.253.0.0/24 via 192.168.20.20 dev hv1-mll onlink])
> +
> +# Rebind "vip" locally.
> +NS_EXEC([vif4], [arping -U -c 1 -w 2 -I vif4 192.0.2.30])
> +wait_column "vif4" Port_Binding virtual_parent logical_port=vip
> +wait_for_ports_up vip
> +OVN_ROUTE_EQUAL([ovnvrf1338], [dnl
> +blackhole 192.0.2.2 proto ovn metric 100
> +blackhole 192.0.2.3 proto ovn metric 100
> +blackhole 192.0.2.10 proto ovn metric 100
> +blackhole 192.0.2.20 proto ovn metric 100
> +blackhole 192.0.2.21 proto ovn metric 100
> +blackhole 192.0.2.30 proto ovn metric 100
> +blackhole 198.51.100.0/24 proto ovn metric 1000
> +233.252.0.0/24 via 192.168.10.10 dev lo onlink
> +233.253.0.0/24 via 192.168.20.20 dev hv1-mll onlink])
> +
> OVN_CLEANUP_CONTROLLER([hv1])
> AT_CHECK([ip vrf | grep -q ovnvrf1338], [1], [])
>
I can squash the following incremental in, if it looks good to you too.
It adds the redistribute-local-only=false test.
Please let me know what you think.
Thanks,
Dumitru
---
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 941e1fb874..cc672d43a0 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -16092,17 +16092,20 @@ blackhole 198.51.100.0/24 proto ovn metric 1000
start_daemon ovn-controller
OVS_WAIT_UNTIL([test "$(ovn-appctl -t ovn-controller debug/status)" ==
"running"])
-# Create a virutal port "vip" with parent set to "vif4".
+# Create a virutal port "vip" with parent set to "vif4", "vif5".
check ovn-nbctl lsp-add public vif4 \
-- lsp-set-addresses vif4 "00:00:ff:ff:ff:04 192.0.2.21"
+check ovn-nbctl lsp-add public vif5 \
+ -- lsp-set-addresses vif5 "00:00:ff:ff:ff:05 192.0.2.22"
ADD_NAMESPACES(vif4)
ADD_VETH(vif4, vif4, br-int, "192.0.2.30/24", "00:00:ff:ff:ff:04",
"192.0.2.21")
check ovn-nbctl lsp-add public vip \
-- lsp-set-addresses vip "00:00:00:00:01:88 192.0.2.30" \
-- lsp-set-type vip virtual \
-- set logical_switch_port vip options:virtual-ip=192.0.2.30 \
- -- set logical_switch_port vip options:virtual-parents=vif4 \
- -- set logical_router_port internet-public
options:dynamic-routing-redistribute-local-only=true
+ -- set logical_switch_port vip options:virtual-parents=vif4,vif5 \
+ -- set logical_router_port internet-public \
+ options:dynamic-routing-redistribute-local-only=true
check ovn-nbctl --wait=hv sync
OVN_ROUTE_EQUAL([ovnvrf1338], [dnl
@@ -16131,7 +16134,6 @@ blackhole 198.51.100.0/24 proto ovn metric 1000
233.253.0.0/24 via 192.168.20.20 dev hv1-mll onlink])
check ovn-sbctl clear Port_Binding vip virtual-parent
-wait_column "" Port_Binding virtual_parent logical_port=vip
OVN_ROUTE_EQUAL([ovnvrf1338], [dnl
blackhole 192.0.2.2 proto ovn metric 100
blackhole 192.0.2.3 proto ovn metric 100
@@ -16157,6 +16159,53 @@ blackhole 198.51.100.0/24 proto ovn metric 1000
233.252.0.0/24 via 192.168.10.10 dev lo onlink
233.253.0.0/24 via 192.168.20.20 dev hv1-mll onlink])
+# Simulate "vip" bound to a different chassis.
+check ovn-sbctl clear Port_Binding vip virtual-parent
+wait_column "" Port_Binding chassis logical_port=vip
+check ovn-sbctl set Port_Binding vip virtual_parent=vif5
+OVN_ROUTE_EQUAL([ovnvrf1338], [dnl
+blackhole 192.0.2.2 proto ovn metric 100
+blackhole 192.0.2.3 proto ovn metric 100
+blackhole 192.0.2.10 proto ovn metric 100
+blackhole 192.0.2.20 proto ovn metric 100
+blackhole 192.0.2.21 proto ovn metric 100
+blackhole 198.51.100.0/24 proto ovn metric 1000
+233.252.0.0/24 via 192.168.10.10 dev lo onlink
+233.253.0.0/24 via 192.168.20.20 dev hv1-mll onlink])
+
+# Check with dynamic-routing-redistribute-local-only=false.
+check ovn-nbctl --wait=hv set logical_router_port internet-public \
+ options:dynamic-routing-redistribute-local-only=false
+OVN_ROUTE_EQUAL([ovnvrf1338], [dnl
+blackhole 192.0.2.1 proto ovn metric 1000
+blackhole 192.0.2.2 proto ovn metric 100
+blackhole 192.0.2.3 proto ovn metric 100
+blackhole 192.0.2.10 proto ovn metric 100
+blackhole 192.0.2.20 proto ovn metric 100
+blackhole 192.0.2.21 proto ovn metric 100
+blackhole 192.0.2.22 proto ovn metric 1000
+blackhole 192.0.2.30 proto ovn metric 1000
+blackhole 198.51.100.0/24 proto ovn metric 1000
+233.252.0.0/24 via 192.168.10.10 dev lo onlink
+233.253.0.0/24 via 192.168.20.20 dev hv1-mll onlink])
+
+# Rebind "vip" locally.
+NS_EXEC([vif4], [arping -U -c 1 -w 2 -I vif4 192.0.2.30])
+wait_column "vif4" Port_Binding virtual_parent logical_port=vip
+wait_for_ports_up vip
+OVN_ROUTE_EQUAL([ovnvrf1338], [dnl
+blackhole 192.0.2.1 proto ovn metric 1000
+blackhole 192.0.2.2 proto ovn metric 100
+blackhole 192.0.2.3 proto ovn metric 100
+blackhole 192.0.2.10 proto ovn metric 100
+blackhole 192.0.2.20 proto ovn metric 100
+blackhole 192.0.2.21 proto ovn metric 100
+blackhole 192.0.2.22 proto ovn metric 1000
+blackhole 192.0.2.30 proto ovn metric 100
+blackhole 198.51.100.0/24 proto ovn metric 1000
+233.252.0.0/24 via 192.168.10.10 dev lo onlink
+233.253.0.0/24 via 192.168.20.20 dev hv1-mll onlink])
+
OVN_CLEANUP_CONTROLLER([hv1])
AT_CHECK([ip vrf | grep -q ovnvrf1338], [1], [])
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
