> 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. Regards, Lorenzo > >
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
