Hi Jacob, Thanks for your review. Em sex., 18 de set. de 2026 às 18:11, Jacob Tanenbaum <[email protected]> escreveu:
> 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? > > Yes, we can. I'll send this adjustment in a new version. > +} >> + >> 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. > I agree. Regards, Lucas > > > >> +# 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 > -- _‘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
