On 11/5/25 10:28 AM, Lorenzo Bianconi wrote: >> 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], []) > > Hi Dumitru, > > the changes seem fine to me. Thanks. >
Thanks for checking! Applied to main, 25.09 and 25.03. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
