On Fri, Sep 25, 2026 at 5:47 PM Jacob Tanenbaum via dev < [email protected]> wrote:
> This patch looks good to me, good find! > > > On Fri, Sep 25, 2026 at 9:08 AM Lucas Vargas Dias <[email protected] > > > wrote: > > > northd_handle_lr_changes() falls back to a full recompute when a deleted > > logical router still has ports, since the ovn_ports it built can only be > > torn down by a recompute - ovn_datapath_destroy() asserts on them. > > > > That check only looked at 'deleted_lr', the IDL's copy of the row from > > before the deletion. If the LRP is removed in one transaction and the > > router deleted in another, and northd processes both in the same > > iteration, the row it sees no longer lists the port while the datapath > > still holds it, and northd aborts: > > > > |util|EMER|northd/northd.c:644: assertion hmap_is_empty(&od->ports) > > failed in destroy_ports_for_datapath() > > > > Check od->ports as well, so the fallback is driven by the ports northd > > actually built. The existing 'n_ports' check is kept: the deleted row > > can also list ports northd never created. > > > > Add a test for both deletion orders, including the two-transaction one > > that aborts today. > > > > Fixes: cef4ef0cd680 ("northd: Creation and deletion of routers in > > en-northd engine node.") > > Signed-off-by: Lucas Vargas Dias <[email protected]> > > --- > > northd/northd.c | 9 ++++ > > tests/ovn-northd.at | 108 ++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 117 insertions(+) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 4eb2ea44b..779c38666 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -5682,7 +5682,16 @@ northd_handle_lr_changes(const struct northd_input > > *ni, > > goto fail; > > } > > > > + /* 'deleted_lr' is the copy of the row from before the deletion, > > so > > + * its port list can disagree with what northd actually built, > in > > + * either direction: it reports ports this run never created (an > > + * LRP added and deleted along with its router in the same > > + * iteration), and it misses ports northd still has (an LRP > > removed > > + * in an earlier transaction of this same iteration). Bail out > on > > + * both, as the ports northd built can only be torn down by a > > + * recompute (see destroy_ports_for_datapath()). */ > nit: I don't think the comment is necessary. > > if (deleted_lr->copp || > > + !hmap_is_empty(&od->ports) || > > deleted_lr->n_ports > 0 || > > deleted_lr->n_policies > 0 || > > deleted_lr->n_static_routes > 0) { > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 6572b1318..695ffbdc1 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -21053,6 +21053,114 @@ OVN_CLEANUP_NORTHD > > AT_CLEANUP > > ]) > > > > +OVN_FOR_EACH_NORTHD_NO_HV([ > > +AT_SETUP([LRP and its logical router deleted in the same iteration]) > > +ovn_start > > + > > +# A logical router without ports is deleted incrementally... > > +check ovn-nbctl --wait=sb lr-add lr0 > > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > +check ovn-nbctl --wait=sb lr-del lr0 > > +check_engine_compute northd incremental > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > + > > +# ...but if the LRP is deleted in the same iteration as its logical > > router, > > +# the deleted Logical_Router row still reports the port (the IDL keeps > the > > +# contents the row had before the deletion), so > northd_handle_lr_changes() > > +# can't handle it and has to fall back to a full recompute. > > +check ovn-nbctl lr-add lr1 > > +check ovn-nbctl --wait=sb lrp-add lr1 lr1-p0 00:00:00:00:00:01 > > 10.0.0.1/24 > > + > > +lr1_dp=$(fetch_column datapath_binding _uuid external_ids:name=lr1) > > +AT_CHECK([test "$lr1_dp" != ""]) > > +AT_CHECK([test "$(fetch_column port_binding _uuid logical_port=lr1-p0)" > > != ""]) > > + > > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > +check ovn-nbctl --wait=sb lrp-del lr1-p0 -- lr-del lr1 > > +check_engine_compute northd recompute > > + > > +# Nothing of the router may be left behind in the SB. > > +AT_CHECK([test "$(fetch_column datapath_binding _uuid > > external_ids:name=lr1)" = ""]) > > +AT_CHECK([test "$(fetch_column port_binding _uuid logical_port=lr1-p0)" > = > > ""]) > > +AT_CHECK([ovn-sbctl --bare --columns logical_datapath list Logical_Flow > | > > dnl > > + grep -c "$lr1_dp"], [1], [0 > > +]) > > +AT_CHECK([ovn-sbctl --bare --columns datapaths list Logical_DP_Group | > dnl > > + grep -c "$lr1_dp"], [1], [0 > > +]) > > + > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > + > > +# The port list of the deleted row is what is checked, not just the > ports > > +# northd built: here the LRP has an unparseable MAC, so > > join_logical_ports() > > +# skipped it and the datapath has no ports at all, but the deleted row > > still > > +# reports it and the deletion falls back to a recompute. > > +check ovn-nbctl lr-add lr2 > > +check_uuid ovn-nbctl --wait=sb --id=@lrp create Logical_Router_Port \ > > + name=lr2-p0 mac=bogus -- add Logical_Router lr2 ports @lrp > > + > > +AT_CHECK([test "$(fetch_column port_binding _uuid logical_port=lr2-p0)" > = > > ""]) > > + > > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > +check ovn-nbctl --wait=sb lr-del lr2 > > +check_engine_compute northd recompute > > + > > +AT_CHECK([test "$(fetch_column datapath_binding _uuid > > external_ids:name=lr2)" = ""]) > > + > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > + > > +# The same, with the LRP peered to a logical switch and the whole chain > > +# (LSP, LRP and logical router) removed in a single transaction. > > +check ovn-nbctl ls-add sw0 > > +check ovn-nbctl lr-add lr3 > > +check ovn-nbctl lrp-add lr3 lr3-sw0 00:00:00:00:00:02 10.0.1.1/24 > > +check ovn-nbctl lsp-add sw0 sw0-lr3 \ > > + -- lsp-set-type sw0-lr3 router \ > > + -- lsp-set-addresses sw0-lr3 router \ > > + -- lsp-set-options sw0-lr3 router-port=lr3-sw0 > > +check ovn-nbctl --wait=sb sync > > + > > +lr3_dp=$(fetch_column datapath_binding _uuid external_ids:name=lr3) > > +AT_CHECK([test "$lr3_dp" != ""]) > > + > > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > +check ovn-nbctl --wait=sb lsp-del sw0-lr3 -- lrp-del lr3-sw0 -- lr-del > lr3 > > +check_engine_compute northd recompute > > + > > +AT_CHECK([test "$(fetch_column datapath_binding _uuid > > external_ids:name=lr3)" = ""]) > > +AT_CHECK([test "$(fetch_column port_binding _uuid logical_port=lr3-sw0)" > > = ""]) > > +AT_CHECK([test "$(fetch_column port_binding _uuid logical_port=sw0-lr3)" > > = ""]) > > +AT_CHECK([ovn-sbctl --bare --columns logical_datapath list Logical_Flow > | > > dnl > > + grep -c "$lr3_dp"], [1], [0 > > +]) > > + > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > + > > +# The two halves of the deletion can also reach northd as two separate > > +# transactions of a single iteration. The deleted row then doesn't > report > > +# the port anymore, but northd still holds the ovn_port it built for it, > > so > > +# the deletion has to fall back to a recompute all the same. > > +check ovn-nbctl lr-add lr4 > > +check ovn-nbctl --wait=sb lrp-add lr4 lr4-p0 00:00:00:00:00:04 > > 10.0.2.1/24 > > + > > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > +sleep_northd > > +check ovn-nbctl lrp-del lr4-p0 > > +check ovn-nbctl lr-del lr4 > > +wake_up_northd > > + > > +check ovn-nbctl --wait=sb sync > > +check_engine_compute northd recompute > > + > > +AT_CHECK([test "$(fetch_column datapath_binding _uuid > > external_ids:name=lr4)" = ""]) > > +AT_CHECK([test "$(fetch_column port_binding _uuid logical_port=lr4-p0)" > = > > ""]) > > + > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > + > > +OVN_CLEANUP_NORTHD > > +AT_CLEANUP > > +]) > > + > > OVN_FOR_EACH_NORTHD_NO_HV([ > > AT_SETUP([Synced logical switch and router incremental procesesing]) > > 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 > > > > Acked-by: Jacob Tanenbaum <[email protected]> > > Jacob > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thank you Lucas, Bekei and Jacob, applied to main with that nit addressed and backported down to 26.03. Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
