Hi Mark, Thanks for your review. I sent a new version to change the callers from ovn_port_set_nb and we can compare the patches.
Regards, Lucas Em seg., 20 de jul. de 2026 às 11:37, Mark Michelson <[email protected]> escreveu: > Hi Lucas, > > It looks like basically none of the callers of ovn_port_set_nb() > already have an ovn_datapath on hand. That makes it more difficult to > try to implement the change I was suggesting. We can stick with your > original patch instead. > > On Fri, Jul 17, 2026 at 3:22 PM Lucas Vargas Dias > <[email protected]> wrote: > > > > I have a question, I'm using ovn_datapath in the function > virtual_lsp_can_be_inc_processed. > > So, I will need to pass ovn_datapath to ovn_port_set_nb. It's called by > many functions. > > What do you think? Is it better to keep this v0 or send a version > adjusting the functions? > > > > Regards, > > Lucas > > > > Em sex., 17 de jul. de 2026 às 15:24, Lucas Vargas Dias > <[email protected]> escreveu: > >> > >> Hi Mark, > >> > >> > >> Em sex., 17 de jul. de 2026 às 15:01, Mark Michelson via dev < > [email protected]> escreveu: > >>> > >>> Hi Lucas, thanks for the patch! > >>> > >>> I like the concept, but I'm curious about something. northd calls > >>> lsp_can_be_inc_processed() on all ovn_ports and caches the result. I > >>> wonder if you could invert the logic of virtual_lsp_needs_recompute() > >>> into a virtual_lsp_can_be_inc_processed() function instead. Then, you > >>> could incorporate this function into lsp_can_be_inc_processed(). This > >>> way, you would not need to make any alterations to > >>> handle_lsp_changes(). > >>> > >>> Would that work, or is there something I'm missing? > >>> > >> > >> I think it works. I'll adjust. > >> > >> Regards, > >> Lucas > >> > >>> > >>> On Thu, Jul 16, 2026 at 3:23 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 | 75 +++++++++++++++++++++++++- > >>> > tests/ovn-northd.at | 129 > ++++++++++++++++++++++++++++++++++++++++++++ > >>> > 2 files changed, 202 insertions(+), 2 deletions(-) > >>> > > >>> > diff --git a/northd/northd.c b/northd/northd.c > >>> > index 3a4afa063..4f4c0733a 100644 > >>> > --- a/northd/northd.c > >>> > +++ b/northd/northd.c > >>> > @@ -1278,6 +1278,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) > >>> > { > >>> > @@ -4620,8 +4626,8 @@ 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 and remote ports for now. */ > >>> > - if (nbsp->type[0] && !lsp_is_remote(nbsp)) { > >>> > + /* Support only normal VIF, remote and virtual ports for now. */ > >>> > + if (nbsp->type[0] && !lsp_is_remote(nbsp) && > !lsp_is_virtual(nbsp)) { > >>> > return false; > >>> > } > >>> > > >>> > @@ -4666,6 +4672,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; > >>> > +} > >>> > + > >>> > static bool > >>> > ls_port_has_changed(const struct nbrec_logical_switch_port *new) > >>> > { > >>> > @@ -4963,6 +5011,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, > >>> > @@ -4981,6 +5036,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 > >>> > @@ -5038,6 +5102,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 86cab3d5b..ed7aef72f 100644 > >>> > --- a/tests/ovn-northd.at > >>> > +++ b/tests/ovn-northd.at > >>> > @@ -12043,6 +12043,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 > >>> > + > >>> > +# 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 > >>> > > >>> > >>> _______________________________________________ > >>> dev mailing list > >>> [email protected] > >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > ‘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’. > > -- _‘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
