Hi Lorenzo, Thanks for the patch!
Nit: "binded" is not the correct form; it should be "bound". On 10/31/25 11:41 PM, Lorenzo Bianconi wrote: > Introduce the capability to add the virtual port ip to the local > vrf just if the port is binded locally. In order to enable this > feature, it is necessary to set the following options on the > logical_router_port where dynamic-routing is enabled: > > - options:dynamic-routing-redistribute=connected-as-host > - options:dynamic-routing-redistribute-local-only=true I don't think this is correct. It's not a new feature, it's just a bug with the "connected-as-host" implementation. For virtual port IPs we already advertise them when "connected-as-host" is set, we just don't take into account whether the parent port is locally bound or not. By default if an advertised route has "tracked_port" set we advertise it with a better metric on the chassis where the tracked_port is bound. And we advertise it with a worse metric on other chassis. If redistribute-local-only=true (default: false), we skip the "worse metric" route. I think the problem your patch should be fixing is only the fact that the advertised route for the virtual IP doesn't have a tracked_port set. Please see below. > > Reported-at: https://issues.redhat.com/browse/FDP-1940 > Signed-off-by: Lorenzo Bianconi <[email protected]> > --- > northd/en-advertised-route-sync.c | 50 ++++++++++++++++-- > tests/system-ovn.at | 84 +++++++++++++++++++++++++++++++ > 2 files changed, 130 insertions(+), 4 deletions(-) > > diff --git a/northd/en-advertised-route-sync.c > b/northd/en-advertised-route-sync.c > index c737376ae..5a3e0db37 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,43 @@ 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) > +{ > + struct ovn_port *vp = port; > + if (smap_get_bool(&advertising_op->nbrp->options, > + "dynamic-routing-redistribute-local-only", false)) { The 'dynamic-routing-redistribute-local-only' option is intended to inform ovn-controller if it should announce non-local routes or not. It's weird that we're checking its value in ovn-northd. In my opinion all this function should do is: if && port->sb->virtual_parent && (virtual_parent exists) then publish_lport_addresses(...., &port->lsp_addrs[i], vp); fi In essence this translates to 1. if a virtual port has no parent set, we don't advertise its IP. 2. if a virtual port has parent set, we advertise its IP and set Advertised_Route.tracked_port=parent Then in ovn-controller (this code already exists and works like this): a. if redistribute-local-only=false and tracked_port is not local: we advertise the route with a worse metric b. if redistribute-local-only=false and tracked_port is local: we advertise the route with a better metric c. if redistribute-local-only=true and tracked_port is not local: we don't advertise the route d. if redistribute-local-only=true and tracked_port is local: we advertise the route > + if (!port->sb) { > + return; > + } All ovn_port records at this point must have a non-null port->sb. If you want you can add an "ovs_assert(port->sb)" but we don't need the check. > + > + const char *virtual_parent = port->sb->virtual_parent; > + if (!virtual_parent) { > + return; > + } > + > + 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 +632,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 +696,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 +713,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 +767,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 +786,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 78d12cbdc..e7969278a 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,90 @@ 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 > 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 > +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]) > + > +# Verify we continue to announce vip IP if > +# dynamic-routing-redistribute-local-only is set to false. > +check ovn-nbctl set logical_router_port internet-public > options:dynamic-routing-redistribute-local-only=false There's a small race here because we don't call "ovn-nbctl --wait=hv sync". > +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.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.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]) > + > OVN_CLEANUP_CONTROLLER([hv1]) > AT_CHECK([ip vrf | grep -q ovnvrf1338], [1], []) > Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
