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()). */ > 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
