On 7/20/26 4:50 PM, Lucas Vargas Dias wrote: > Hi Mark, Hi Lucas,
I didn't review this patch or the new version yet but maybe it makes sense to group all these new incremental processing port updates into a single patch series, we now have 3 kind of similar patches posted independently for review: - [ovs-dev] [PATCH ovn v1] northd: Incrementally process virtual port creation and deletion. https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ - [ovs-dev] [PATCH ovn v0] northd: Enable incremental processing for localport ports. https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ - [ovs-dev] [PATCH ovn v0] northd: Incrementally process logical switch ports of type "router". https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ We (OVN maintainers) are currently scrambling to review as many non-bugfix patches as possible before the upcoming soft freeze for 26.09.0 (scheduled July 23rd). Grouping these together would slightly improve our lives. Also, related, code reviews of other people's patches are ALWAYS welcome so if you have time to review some of the other patches that are waiting for code review, that's always great: https://patchwork.ozlabs.org/project/ovn/list/ Best regards, Dumitru > 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’. >> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
