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
