The datapath-group bitmap stored in a lflow_ref_node records the set of
datapaths (and the total datapath count, hence the bitmap length) that
the ref contributed to a shared logical flow, so that unlinking the ref
later releases exactly the right per-datapath refcounts.
lflow_table_add_lflow__() asserted that, when re-linking such a ref node,
the datapath-group bitmap length matched the current one. This held as
long as the number of datapaths of a given type stayed constant between
unlink and re-link. With incremental processing of datapath
creation/deletion this is no longer true: adding (or removing) a
datapath changes sparse_array_len() and therefore the bitmap length used
by dp-group flows (e.g. shared load balancer flows), which made northd
abort with:
lflow-mgr.c: assertion lrn->dpgrp_bitmap_len == dp_bitmap_len failed
Re-record the bitmap every time the ref node is linked, so that a
subsequent unlink releases exactly the datapaths acquired at link time.
Do so unconditionally: neither the length nor the contents is a reliable
indicator of the other, as sparse_array_len() is the highest used index
plus one and deleting any but the last datapath therefore changes the
contents while leaving the length untouched. A stale record is worse
than the assertion it replaced: dp_refcnt_release() treats an index it
holds no refcount for as released and clears it from the flow's datapath
group, so the flow silently disappears from a datapath that should have
it (or lingers on one that should not).
As a newly created ref node is never linked, the bitmap is now recorded
in that one place only, and the clone done when the node is created is
dropped.
Fixes: 3166f87b3991 ("northd: Fix lflow ref node's reference counting.")
Assisted-by: Claude Opus 4.8, Claude Code
Signed-off-by: Lucas Vargas Dias <[email protected]>
---
northd/lflow-mgr.c | 22 ++++++++++++++++-----
tests/ovn-northd.at | 47 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 64 insertions(+), 5 deletions(-)
diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
index ce9c4f854..9241e53e8 100644
--- a/northd/lflow-mgr.c
+++ b/northd/lflow-mgr.c
@@ -773,19 +773,31 @@ lflow_table_add_lflow__(struct lflow_table *lflow_table,
lrn->lflow = lflow;
lrn->lflow_ref = lflow_ref;
lrn->dpgrp_lflow = !sdp;
- if (lrn->dpgrp_lflow) {
- lrn->dpgrp_bitmap = bitmap_clone(dp_bitmap, dp_bitmap_len);
- lrn->dpgrp_bitmap_len = dp_bitmap_len;
- } else {
+ if (!lrn->dpgrp_lflow) {
lrn->dp_index = sdp->index;
}
+ /* A dp group bitmap is recorded below, when the ref node is
+ * linked. */
ovs_list_insert(&lflow->referenced_by, &lrn->ref_list_node);
hmap_insert(&lflow_ref->lflow_ref_nodes, &lrn->ref_node, hash);
}
if (!lrn->linked) {
if (lrn->dpgrp_lflow) {
- ovs_assert(lrn->dpgrp_bitmap_len == dp_bitmap_len);
+ /* Record the datapath group whose refcounts are acquired
+ * below, so that a subsequent unlink releases exactly those.
+ * Both the bitmap contents (which datapaths share this flow)
+ * and its length (the total number of datapaths of this type)
+ * can change while this ref node is unlinked, e.g. because a
+ * datapath was created or deleted by incremental processing.
+ *
+ * Re-clone unconditionally: neither is a reliable indicator
+ * of the other. sparse_array_len() is the highest used index
+ * plus one, so deleting any but the last datapath leaves the
+ * length unchanged while the contents change. */
+ bitmap_free(lrn->dpgrp_bitmap);
+ lrn->dpgrp_bitmap = bitmap_clone(dp_bitmap, dp_bitmap_len);
+ lrn->dpgrp_bitmap_len = dp_bitmap_len;
size_t index;
BITMAP_FOR_EACH_1 (index, dp_bitmap_len, dp_bitmap) {
/* Allocate a reference counter only if already used. */
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 0cc242428..f13eda112 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -16419,6 +16419,53 @@ OVN_CLEANUP_NORTHD
AT_CLEANUP
])
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([Datapath count change with shared dp-group flow])
+AT_KEYWORDS([incremental processing])
+
+ovn_start
+
+# Two logical routers sharing the same load balancer produce an identical
+# "ct_lb_mark" flow on both router datapaths. northd collapses those into a
+# single dp-group logical flow whose datapath-group bitmap length equals the
+# current number of logical-router datapaths.
+check ovn-nbctl lb-add lb0 10.0.0.100:80 192.168.1.10:8080,192.168.1.11:8080
+check ovn-nbctl lr-add lr0
+check ovn-nbctl lr-add lr1
+check ovn-nbctl lr-lb-add lr0 lb0
+check ovn-nbctl lr-lb-add lr1 lb0
+check ovn-nbctl --wait=sb sync
+
+AT_CHECK([ovn-sbctl lflow-list | grep -c \
+"ct_lb_mark(backends=192.168.1.10:8080,192.168.1.11:8080)"], [0], [dnl
+2
+])
+
+# Incrementally create an (empty) logical router. This grows the number of
+# logical-router datapaths, and hence the dp-group bitmap length used by the
+# shared load balancer flow, without recomputing the lflow engine node.
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-nbctl --wait=sb lr-add lr2
+check_engine_stats northd norecompute compute
+check_engine_stats lflow norecompute compute
+
+# Reprocess the load balancer. This unlinks and re-links the shared flow's
+# ref nodes at the new (larger) datapath-group bitmap length. Before the fix
+# this hit "assertion lrn->dpgrp_bitmap_len == dp_bitmap_len failed" and
+# aborted northd.
+check ovn-nbctl --wait=sb set load_balancer lb0 \
+ vips:'"10.0.0.101:80"'='"192.168.1.12:8080"'
+
+# northd must still be running and the shared flow must be intact.
+AT_CHECK([ovn-sbctl lflow-list | grep -c \
+"ct_lb_mark(backends=192.168.1.10:8080,192.168.1.11:8080)"], [0], [dnl
+2
+])
+
+OVN_CLEANUP_NORTHD
+AT_CLEANUP
+])
+
AT_SETUP([RBAC -- Recover builtin role and permissions])
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
