This patch looks good, a few minor questions below On Thu, Sep 17, 2026 at 3:26 PM Lucas Vargas Dias <[email protected]> wrote:
> Until now any change to a logical switch port of type "virtual" fell back > to a full northd recompute, because lsp_can_be_inc_processed() only allowed > plain VIF and remote ports. Enable incremental processing for creation and > deletion of virtual ports as well. > > The per-port lflows of a virtual port (the "bind_vport" ARP/ND responder > flows) are anchored on the port's own lflow_ref and are already handled by > the generic created/deleted tracked-port paths in the lflow engine. The > dependency on the virtual parents is likewise already handled (existing > virtual ports are re-tracked when a parent VIF is created or deleted). > > Some virtual ports, however, have dependencies that live outside of their > own lflow_ref and that the incremental LSP path does not keep in sync. For > those we detect the situation and fall back to a full recompute: > > - A distributed NAT rule whose logical_port is the virtual port adds a > S_ROUTER_IN_GW_REDIRECT drop flow owned by the router datapath's > lflow_ref (see build_lrouter_nat_defrag_and_lb()). > > - Dynamic routing on a connected router advertises host routes for a > virtual port based on its SB "virtual_parent", i.e. once the port is > claimed. The claim reaches northd as an "up"-only change on the NB > port, which the incremental path otherwise ignores, so the advertised > route engine would not be re-run. The fallback is therefore also > applied on the update path, before the "ignore_lsp_down" shortcut. > > Add tests covering incremental create/delete of a virtual port as well as > the recompute fallbacks for the distributed-NAT and dynamic-routing cases. > > Assisted-by: Claude Opus 4.8, ClaudeCode > Signed-off-by: Lucas Vargas Dias <[email protected]> > --- > northd/northd.c | 77 +++++++++++++++++++++++++- > tests/ovn-northd.at | 129 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 204 insertions(+), 2 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index af37ebd35..f42b7fd69 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -1286,6 +1286,12 @@ lsp_is_remote(const struct > nbrec_logical_switch_port *nbsp) > return !strcmp(nbsp->type, "remote"); > } > > +static bool > +lsp_is_virtual(const struct nbrec_logical_switch_port *nbsp) > +{ > + return !strcmp(nbsp->type, "virtual"); > +} > + > static bool > lsp_is_localnet(const struct nbrec_logical_switch_port *nbsp) > { > @@ -4700,8 +4706,10 @@ destroy_northd_tracked_data(struct northd_data *nd) > static bool > lsp_can_be_inc_processed(const struct nbrec_logical_switch_port *nbsp) > { > - /* Support only normal VIF, remote and localport ports for now. */ > - if (nbsp->type[0] && !lsp_is_remote(nbsp) && !lsp_is_localport(nbsp)) > { > + /* Support only normal VIF, remote, localport, and virtual ports for > + * now. */ > + if (nbsp->type[0] && !lsp_is_remote(nbsp) && !lsp_is_localport(nbsp) > && > + !lsp_is_virtual(nbsp)) { > return false; > } > > @@ -4746,6 +4754,48 @@ lsp_can_be_inc_processed(const struct > nbrec_logical_switch_port *nbsp) > return true; > } > > +/* A logical switch port of type "virtual" can have dependencies that live > + * outside of its own 'lflow_ref' and that the incremental LSP path does > not > + * keep in sync. When any such dependency is present on a logical router > + * connected to the port's logical switch, the caller must fall back to a > full > + * recompute. The known dependencies are: > + * > + * - A distributed NAT rule whose 'logical_port' is this virtual port. > It > + * adds a S_ROUTER_IN_GW_REDIRECT drop flow (see > + * build_lrouter_nat_defrag_and_lb()) owned by the router datapath's > + * 'lflow_ref', not by the virtual port. > + * > + * - Dynamic routing on the connected router. Host routes for a virtual > port > + * are advertised based on its SB 'virtual_parent' (i.e. once the port > is > + * claimed, see publish_host_routes_for_virtual_ports()). The claim > reaches > + * northd as an "up"-only change on the NB port, which the incremental > path > + * ignores, so the advertised-route engine would not be re-run. */ > +static bool > +virtual_lsp_needs_recompute(struct ovn_datapath *od, const char *lport) > +{ > + struct ovn_port *rp; > + VECTOR_FOR_EACH (&od->router_ports, rp) { > + struct ovn_port *lrp = rp->peer; > + if (!lrp || !lrp->od || !lrp->od->nbr) { > + continue; > + } > + > + if (lrp->od->dynamic_routing) { > + return true; > + } > + > + const struct nbrec_logical_router *nbr = lrp->od->nbr; > + for (size_t i = 0; i < nbr->n_nat; i++) { > + const struct nbrec_nat *nat = nbr->nat[i]; > + if (nat->logical_port && !strcmp(nat->logical_port, lport) && > + is_nat_distributed(nat, lrp->od)) { > + return true; > + } > + } > + } > + return false; > This returns true if a logical router has the option dynamic-router=true without checking if the LRP actually advertises. Could we check lrp->dynamic_routing_redestribute and only recompute when the router would actually advertse the virtual port's routes? > +} > + > static bool > ls_port_has_changed(const struct nbrec_logical_switch_port *new) > { > @@ -5043,6 +5093,13 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn > *ovnsb_idl_txn, > if (!lsp_can_be_inc_processed(new_nbsp)) { > goto fail; > } > + if (lsp_is_virtual(new_nbsp) && > + virtual_lsp_needs_recompute(od, new_nbsp->name)) { > + /* The new virtual port has a dependency on a > connected > + * router that can't be handled incrementally. Fall > back > + * to recompute. */ > + goto fail; > + } > op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports, > new_nbsp->name, new_nbsp, od, > ni->sbrec_mirror_table, > @@ -5061,6 +5118,15 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn > *ovnsb_idl_txn, > !lsp_can_be_inc_processed(new_nbsp)) { > goto fail; > } > + if (lsp_is_virtual(new_nbsp) && > + virtual_lsp_needs_recompute(od, new_nbsp->name)) { > + /* This virtual port has a dependency on a connected > router > + * that can't be handled incrementally. In > particular a > + * claim (which reaches northd as an "up"-only > change) must > + * re-run the advertised-route engine. Fall back to > + * recompute. */ > + goto fail; > + } > const struct sbrec_port_binding *sb = op->sb; > if (sset_contains(&nd->svc_monitor_lsps, new_nbsp->name)) > { > /* This port is used for svc monitor, which may be > impacted > @@ -5118,6 +5184,13 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn > *ovnsb_idl_txn, > if (!op->lsp_can_be_inc_processed) { > goto fail; > } > + if (lsp_is_virtual(op->nbsp) && > + virtual_lsp_needs_recompute(op->od, op->key)) { > + /* This virtual port has a dependency on a connected > router > + * that can't be regenerated incrementally. Fall back to > + * recompute. */ > + goto fail; > + } > if (sset_contains(&nd->svc_monitor_lsps, op->key)) { > /* This port was used for svc monitor, which may be > * impacted by this deletion. Fallback to recompute. */ > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index e0f9bd82f..748d8a438 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -12248,6 +12248,135 @@ ignored_dp=ls0 > AT_CLEANUP > ]) > > +AT_SETUP([Virtual port incremental processing]) > +AT_KEYWORDS([incremental processing]) > +ovn_start > + > +check ovn-nbctl ls-add sw0 > +check ovn-nbctl --wait=sb lsp-add sw0 vif0 \ > + -- lsp-set-addresses vif0 "00:00:00:00:00:01 10.0.0.4" > + > +# Creating a virtual port should be incrementally processed. > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > +check ovn-nbctl --wait=sb lsp-add sw0 vip0 \ > + -- lsp-set-type vip0 virtual \ > + -- set Logical_Switch_Port vip0 \ > + options:virtual-ip=10.0.0.10 options:virtual-parents=vif0 > +check_engine_compute northd incremental > +check_engine_compute lflow incremental > + > +# The bind_vport flow for the virtual port is present. > +AT_CHECK([ovn-sbctl dump-flows sw0 | grep ls_in_arp_rsp | grep bind_vport > \ > + | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_arp_rsp ), priority=100 , match=(inport == "vif0" > && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || > (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("vip0", > inport); next;) > +]) > + > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > + > +# Deleting a virtual port should be incrementally processed. > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > +check ovn-nbctl --wait=sb lsp-del vip0 > +check_engine_compute northd incremental > +check_engine_compute lflow incremental > + > +# The bind_vport flow for the virtual port is gone. > +AT_CHECK([ovn-sbctl dump-flows sw0 | grep ls_in_arp_rsp | grep bind_vport > \ > + | ovn_strip_lflows], [0], []) > + > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > + > +OVN_CLEANUP_NORTHD > +AT_CLEANUP > + > +AT_SETUP([Virtual port incremental processing fallback with distributed > NAT]) > +AT_KEYWORDS([incremental processing]) > +ovn_start > + > +check ovn-sbctl chassis-add gw1 geneve 127.0.0.1 > + > +check ovn-nbctl ls-add sw0 > +check ovn-nbctl lr-add lr0 > +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 > +check ovn-nbctl --wait=sb lsp-add-router-port sw0 sw0-lr0 lr0-sw0 > + > +# Distributed gateway port so that dnat_and_snat NATs become distributed. > +check ovn-nbctl ls-add public > +check ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24 > +check ovn-nbctl --wait=sb lsp-add-router-port public public-lr0 lr0-public > +check ovn-nbctl lsp-add-localnet-port public ln-public public > +check ovn-nbctl --wait=sb lrp-set-gateway-chassis lr0-public gw1 20 > + > +# Parent VIF and the virtual port. No NAT references it yet, so creating > it is > +# incrementally processed. > +check ovn-nbctl --wait=sb lsp-add sw0 vif0 \ > + -- lsp-set-addresses vif0 "00:00:20:20:12:01 10.0.0.4" > +check ovn-nbctl --wait=sb lsp-add sw0 vip0 \ > + -- lsp-set-type vip0 virtual \ > + -- set Logical_Switch_Port vip0 \ > + options:virtual-ip=10.0.0.5 options:virtual-parents=vif0 > + > +# Distributed dnat_and_snat whose logical_port is the virtual port 'vip0'. > +# The router's S_ROUTER_IN_GW_REDIRECT drop flow for this NAT depends on > +# 'vip0' being a virtual port, but that flow is owned by the router > datapath's > +# lflow_ref, not by 'vip0'. > +check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat \ > + 172.168.0.110 10.0.0.5 vip0 30:54:00:00:00:03 > + > +# Deleting the virtual port must fall back to recompute. > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > +check ovn-nbctl --wait=sb lsp-del vip0 > +check_engine_compute northd recompute > + > Minor nit: the lflow engine is not checked here, unlike in the test above. The northd node will drive the lflow node, so checking it is not required but asserting the lflow state would make these two fallback tests more complete. > +# Re-creating the virtual port (still referenced by the NAT) must also > fall > +# back to recompute. > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > +check ovn-nbctl --wait=sb lsp-add sw0 vip0 \ > + -- lsp-set-type vip0 virtual \ > + -- set Logical_Switch_Port vip0 \ > + options:virtual-ip=10.0.0.5 options:virtual-parents=vif0 > +check_engine_compute northd recompute > + > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > + > +OVN_CLEANUP_NORTHD > +AT_CLEANUP > + > +AT_SETUP([Virtual port incremental processing fallback with dynamic > routing]) > +AT_KEYWORDS([incremental processing]) > +ovn_start > + > +check ovn-nbctl ls-add sw0 > +check ovn-nbctl lr-add lr0 \ > + -- set Logical_Router lr0 options:dynamic-routing=true > +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 \ > + -- set Logical_Router_Port lr0-sw0 \ > + options:dynamic-routing-redistribute=connected-as-host > +check ovn-nbctl --wait=sb lsp-add-router-port sw0 sw0-lr0 lr0-sw0 > + > +check ovn-nbctl --wait=sb lsp-add sw0 vif0 \ > + -- lsp-set-addresses vif0 "00:00:00:00:00:01 10.0.0.4" > + > +# Creating a virtual port on a switch connected to a dynamic-routing > router > +# must fall back to recompute: advertised host routes for virtual ports > depend > +# on the SB "virtual_parent" (claim), which the incremental path does not > +# track. > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > +check ovn-nbctl --wait=sb lsp-add sw0 vip0 \ > + -- lsp-set-type vip0 virtual \ > + -- set Logical_Switch_Port vip0 \ > + options:virtual-ip=10.0.0.5 options:virtual-parents=vif0 > +check_engine_compute northd recompute > + > +# Deleting it must also fall back to recompute. > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > +check ovn-nbctl --wait=sb lsp-del vip0 > +check_engine_compute northd recompute > + > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > + > +OVN_CLEANUP_NORTHD > +AT_CLEANUP > + > OVN_FOR_EACH_NORTHD_NO_HV([ > AT_SETUP([SB Port binding incremental processing]) > ovn_start > -- > 2.43.0 > > > -- > > > > > _'Esta mensagem é direcionada apenas para os endereços constantes no > cabeçalho inicial. Se você não está listado nos endereços constantes no > cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa > mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas > estão > imediatamente anuladas e proibidas'._ > > > * **'Apesar do Magazine Luiza tomar > todas as precauções razoáveis para assegurar que nenhum vírus esteja > presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por > quaisquer perdas ou danos causados por esse e-mail ou por seus anexos'.* > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Jacob _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
