Remove the needs_full_sync fallback for IGMP/MLD and IC-learned service
monitor flows. lflow_ref_unlink_and_prune() saves the uuids of orphaned
lflows so that orphaned flows can be deleted without a resync being
required.
The IGMP/MLD and IC-learned service monitor handlers set
trk_data.needs_full_sync, which forced dp_group to fall back to a full
lflow_table_sync_to_sb() instead of the incremental
lflow_ref_sync_lflows() pass.
The full sync, unlike the incremental sync, does not destroy
lflow_ref_nodes that are no longer linked on lflows still referenced
by another lflow_ref. A retained, already-unlinked node can then be
unlinked again by a later lflow_ref_unlink_lflows() call, which
releases its datapath refcnt a second time. The count can drop below
the number of lflow_refs that still use the lflow, emptying its
datapath bitmap so that a later sync deletes an SB row that other
lflow_refs still require (e.g. a load balancer VIP flow shared by two
load balancers on the same datapath).
Drop the fallback and keep these flows incremental.
lflow_ref_unlink_and_prune() already destroys orphaned lflows in
memory; the incremental sync cannot see them because the lflow and
its lflow_ref_node are gone. So record the SB uuid of every
orphaned lflow that already has an SB row in
trk_data.orphaned_sb_uuids and have dp_group_lflow_handler() delete
those rows after the incremental sync pass. This solves a performance
regression in which ordinary logical switch port changes caused the
whole logical flow table to recalculate.
This is safe even when an orphaned lflow is re-added by a later build
(e.g. the per-datapath multicast flood flow): the re-added lflow is a
fresh in-memory object with a new random sb_uuid, so its freshly
inserted SB row is never one of the recorded uuids.
Add a northd regression test for the case where two load balancers
on the same switch share a VIP. Removing the shared VIP from one
load balancer while a datapath group full sync is in progress,
followed by a further change to that load balancer, previously
double-released the shared lflow's datapath refcnt. A later full
sync then pruned the lflow and deleted its SB row even though the
second load balancer still required the VIP.
The test keeps the two changes that must land in a single engine run
(remove the VIP from the first LB and change the port address)
batched into one transaction, so that the multicast node's
recompute forces the datapath group onto the full sync path while
the lflow node stays incremental. It asserts that the shared VIP
flow is still present after the sequence of changes.
Assisted-by: Qwen3.8-27B-FP8, opencode
Fixes: b9809453f546 ("northd: Split en_lflow into compute and sync nodes.")
Signed-off-by: Jacob Tanenbaum <[email protected]>
---
northd/en-dp-group.c | 13 +++++++---
northd/en-lflow.c | 17 +++++++-----
northd/en-lflow.h | 7 ++++-
northd/lflow-mgr.c | 41 +++++++++++++++++++++++++++--
northd/lflow-mgr.h | 6 ++++-
tests/ovn-northd.at | 61 ++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 132 insertions(+), 13 deletions(-)
diff --git a/northd/en-dp-group.c b/northd/en-dp-group.c
index e33e1f407..3aeed361b 100644
--- a/northd/en-dp-group.c
+++ b/northd/en-dp-group.c
@@ -89,9 +89,7 @@ dp_group_lflow_handler(struct engine_node *node,
struct ed_type_global_config *global_config =
engine_get_input_data("global_config", node);
- if (hmapx_is_empty(&lflow_data->trk_data.dirty_lflow_refs) ||
- lflow_data->trk_data.needs_full_sync) {
-
+ if (hmapx_is_empty(&lflow_data->trk_data.dirty_lflow_refs)) {
dp_group_sync_to_sb(node, lflow_data);
return EN_HANDLED_UPDATED;
}
@@ -107,6 +105,15 @@ dp_group_lflow_handler(struct engine_node *node,
return EN_UNHANDLED;
}
}
+
+ /* Delete the SB rows for lflows orphaned in-memory this run (see
+ * lflow_table_delete_orphaned_sb_flows()). This only happens on the
+ * incremental path: the handlers that record orphaned_sb_uuids also
+ * mark a lflow_ref dirty, so the full-sync branch above never sees
+ * non-empty orphaned_sb_uuids. */
+ lflow_table_delete_orphaned_sb_flows(
+ sb_flow_table,
+ &lflow_data->trk_data.orphaned_sb_uuids);
return EN_HANDLED_UPDATED;
}
diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index f2ae40ec8..a73d55486 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -221,14 +221,16 @@ lflow_multicast_igmp_handler(struct engine_node *node,
void *data)
lflow_get_input_data(node, &lflow_input);
lflow_ref_unlink_and_prune(mcast_igmp_data->lflow_ref,
- lflow_data->lflow_table);
+ lflow_data->lflow_table,
+ &lflow_data->trk_data.orphaned_sb_uuids);
build_igmp_lflows(&mcast_igmp_data->igmp_groups,
&lflow_input.ls_datapaths->datapaths,
lflow_data->lflow_table,
mcast_igmp_data->lflow_ref);
- lflow_data->trk_data.needs_full_sync = true;
+ hmapx_add(&lflow_data->trk_data.dirty_lflow_refs,
+ mcast_igmp_data->lflow_ref);
return EN_HANDLED_UPDATED;
}
@@ -297,7 +299,8 @@ lflow_ic_learned_svc_mons_handler(struct engine_node *node,
ic_learned_svc_monitors_data->lflow_ref);
lflow_ref_unlink_and_prune(ic_learned_svc_monitors_data->lflow_ref,
- lflow_data->lflow_table);
+ lflow_data->lflow_table,
+ &lflow_data->trk_data.orphaned_sb_uuids);
build_lswitch_arp_nd_ic_learned_svc_mon(
&svc_mons_data,
@@ -305,7 +308,8 @@ lflow_ic_learned_svc_mons_handler(struct engine_node *node,
lflow_input.svc_monitor_mac,
lflow_data->lflow_table);
- lflow_data->trk_data.needs_full_sync = true;
+ hmapx_add(&lflow_data->trk_data.dirty_lflow_refs,
+ ic_learned_svc_monitors_data->lflow_ref);
return EN_HANDLED_UPDATED;
}
@@ -317,7 +321,7 @@ void *en_lflow_init(struct engine_node *node OVS_UNUSED,
data->lflow_table = lflow_table_alloc();
lflow_table_init(data->lflow_table);
hmapx_init(&data->trk_data.dirty_lflow_refs);
- data->trk_data.needs_full_sync = false;
+ uuidset_init(&data->trk_data.orphaned_sb_uuids);
return data;
}
@@ -327,11 +331,12 @@ void en_lflow_cleanup(void *data_)
struct lflow_data *data = data_;
lflow_table_destroy(data->lflow_table);
hmapx_destroy(&data->trk_data.dirty_lflow_refs);
+ uuidset_destroy(&data->trk_data.orphaned_sb_uuids);
}
void en_lflow_clear_tracked_data(void *data_)
{
struct lflow_data *data = data_;
hmapx_clear(&data->trk_data.dirty_lflow_refs);
- data->trk_data.needs_full_sync = false;
+ uuidset_clear(&data->trk_data.orphaned_sb_uuids);
}
diff --git a/northd/en-lflow.h b/northd/en-lflow.h
index fd3f2427f..c83ef0e4d 100644
--- a/northd/en-lflow.h
+++ b/northd/en-lflow.h
@@ -9,12 +9,17 @@
#include "lib/hmapx.h"
#include "lib/inc-proc-eng.h"
+#include "lib/uuidset.h"
struct lflow_table;
struct lflow_tracked_data {
struct hmapx dirty_lflow_refs; /* lflow_refs changed by handlers. */
- bool needs_full_sync; /* Full lflow_table_sync_to_sb needed. */
+
+ /* SB Logical_Flow row uuids that were orphaned in-memory by
+ * lflow_ref_unlink_and_prune() and must be deleted from SB after the
+ * incremental lflow_ref_sync_lflows() pass. */
+ struct uuidset orphaned_sb_uuids;
};
struct lflow_data {
diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
index deceadb74..195e00e0f 100644
--- a/northd/lflow-mgr.c
+++ b/northd/lflow-mgr.c
@@ -743,10 +743,16 @@ lflow_ref_unlink_lflows(struct lflow_ref *lflow_ref,
* whose referenced_by list is empty (no other lflow_ref references it).
* Unlike lflow_ref_unlink_lflows (which only clears dp bits and sets
* linked=false), this function removes the lrns and orphaned lflows
- * from the in-memory table entirely, without writing to SB. */
+ * from the in-memory table entirely, without writing to SB.
+ *
+ * Because the orphaned lflows are destroyed here (and so are not visible to a
+ * later lflow_ref_sync_lflows() pass), the SB uuid of every orphaned lflow
+ * that already has an SB row is recorded in 'orphaned_sb_uuids' so the caller
+ * can delete those rows from SB after the incremental sync. */
void
lflow_ref_unlink_and_prune(struct lflow_ref *lflow_ref,
- struct lflow_table *lflow_table)
+ struct lflow_table *lflow_table,
+ struct uuidset *orphaned_sb_uuids)
{
lflow_ref_unlink_lflows(lflow_ref, lflow_table);
@@ -756,6 +762,9 @@ lflow_ref_unlink_and_prune(struct lflow_ref *lflow_ref,
lflow_ref_node_destroy(lrn);
if (ovs_list_is_empty(&lflow->referenced_by)) {
+ if (!uuid_is_zero(&lflow->sb_uuid)) {
+ uuidset_insert(orphaned_sb_uuids, &lflow->sb_uuid);
+ }
enum ovn_datapath_type dp_type =
ovn_stage_to_datapath_type(lflow->stage);
ovs_assert(dp_type < DP_MAX);
@@ -767,6 +776,34 @@ lflow_ref_unlink_and_prune(struct lflow_ref *lflow_ref,
}
}
+/* Deletes the SB Logical_Flow rows for lflows that were orphaned in memory
+ * by lflow_ref_unlink_and_prune() this run. lflow_ref_sync_lflows() cannot
+ * see them because the lflow (and its lflow_ref_node) has already been
+ * destroyed, so their sb_uuids were recorded in 'orphaned_sb_uuids' and are
+ * deleted here.
+ *
+ * This is safe even when an orphaned lflow is re-added by a later build
+ * (e.g. the per-datapath multicast flood flow): the re-added lflow is a
+ * fresh in-memory object with a new random sb_uuid (the old one was
+ * destroyed), so its freshly inserted SB row is never in
+ * 'orphaned_sb_uuids'. */
+void
+lflow_table_delete_orphaned_sb_flows(
+ const struct sbrec_logical_flow_table *sb_flow_table,
+ struct uuidset *orphaned_sb_uuids)
+{
+ struct uuidset_node *node;
+
+ UUIDSET_FOR_EACH (node, orphaned_sb_uuids) {
+ const struct sbrec_logical_flow *sbflow =
+ sbrec_logical_flow_table_get_for_uuid(sb_flow_table,
+ &node->uuid);
+ if (sbflow) {
+ sbrec_logical_flow_delete(sbflow);
+ }
+ }
+}
+
bool
lflow_ref_sync_lflows(struct lflow_ref *lflow_ref,
struct lflow_table *lflow_table,
diff --git a/northd/lflow-mgr.h b/northd/lflow-mgr.h
index 678e97214..f4cccb5e1 100644
--- a/northd/lflow-mgr.h
+++ b/northd/lflow-mgr.h
@@ -25,6 +25,7 @@ struct ovsdb_idl_txn;
struct ovn_datapath;
struct ovsdb_idl_row;
struct ovn_lflow;
+struct uuidset;
/* lflow map which stores the logical flows. */
struct lflow_table {
@@ -58,7 +59,10 @@ struct lflow_ref *lflow_ref_create(void);
void lflow_ref_destroy(struct lflow_ref *);
void lflow_ref_clear(struct lflow_ref *lflow_ref);
void lflow_ref_unlink_lflows(struct lflow_ref *, struct lflow_table *);
-void lflow_ref_unlink_and_prune(struct lflow_ref *, struct lflow_table *);
+void lflow_ref_unlink_and_prune(struct lflow_ref *, struct lflow_table *,
+ struct uuidset *);
+void lflow_table_delete_orphaned_sb_flows(
+ const struct sbrec_logical_flow_table *, struct uuidset *);
bool lflow_ref_sync_lflows(struct lflow_ref *,
struct lflow_table *lflow_table,
struct ovsdb_idl_txn *ovnsb_txn,
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 6866068fa..1097fa998 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -24828,4 +24828,65 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
OVN_CLEANUP_NORTHD
AT_CLEANUP
+])
+
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([check shared LB VIP flow survives datapath group full sync])
+AT_KEYWORDS([loadbalancer])
+ovn_start
+
+# Two load balancers on the same switch share the VIP 1.1.1.1:80 with the
+# same backend, so they both reference one logical flow L in ls_in_lb.
+# L's datapath refcnt for sw1 is therefore 2. The bug this test guards
+# against: a full sync of the datapath group retains the lflow_ref_node of
+# an LB that has already dropped the shared VIP, and a later change to that
+# LB unlinks the retained node again, releasing sw1's datapath refcnt a
+# second time. That empties L's datapath bitmap and a later full sync
+# prunes L and deletes its SB row even though lb2 still requires it.
+check ovn-nbctl --wait=hv ls-add sw1
+check ovn-nbctl --wait=hv lsp-add sw1 p1 -- \
+ lsp-set-addresses p1 "aa:aa:aa:aa:aa:01 192.168.10.2"
+
+check ovn-nbctl --wait=hv lb-add lb1 1.1.1.1:80 192.168.10.2:80
+check ovn-nbctl --wait=hv lb-add lb1 2.2.2.2:80 192.168.10.2:80
+check ovn-nbctl --wait=hv lb-add lb2 1.1.1.1:80 192.168.10.2:80
+check ovn-nbctl --wait=hv ls-lb-add sw1 lb1
+check ovn-nbctl --wait=hv ls-lb-add sw1 lb2
+
+# Baseline: the shared VIP flow exists.
+OVS_WAIT_UNTIL([test 1 -le `ovn-sbctl dump-flows sw1 | grep "ip4.dst ==
1.1.1.1" | grep -c .`])
+
+# RUN 1: remove 1.1.1.1 from lb1 (lb1 keeps 2.2.2.2) and change p1's
+# address in the SAME engine run. The port update makes the multicast node
+# recompute, which forces the datapath group to take the FULL sync path.
+# Unlike the incremental sync, the full sync does not destroy the lflow_ref
+# nodes that were already unlinked, so lb1's now-unlinked 1.1.1.1 node is
+# retained. The two changes must land in a single run, so they are batched
+# into one transaction.
+check ovn-nbctl --wait=hv lb-del lb1 1.1.1.1:80 -- \
+ lsp-set-addresses p1 "aa:aa:aa:aa:aa:01 192.168.20.2"
+# The shared VIP flow is still present (lb2 keeps it).
+OVS_WAIT_UNTIL([test 1 -le `ovn-sbctl dump-flows sw1 | grep "ip4.dst ==
1.1.1.1" | grep -c .`])
+
+# RUN 2: change lb1 (append VIP 3.3.3.3). This re-unlinks lb1's lflow_ref.
+# Without the fix this hits the retained 1.1.1.1 node and double-releases
+# sw1's datapath count, emptying the shared flow's datapath bitmap.
+check ovn-nbctl --wait=hv lb-add lb1 3.3.3.3:80 192.168.10.2:80
+
+# RUN 3: another port address change -> needs_full_sync -> full sync.
+# Without the fix this prunes the shared 1.1.1.1 flow (now empty bitmap)
+# and deletes its SB row.
+check ovn-nbctl --wait=hv lsp-set-addresses p1 "aa:aa:aa:aa:aa:01 192.168.30.2"
+
+# REGRESSION ASSERTION: the shared VIP flow must still be present, since lb2
+# still requires 1.1.1.1. Without the fix it was deleted.
+OVS_WAIT_UNTIL([test 1 -le `ovn-sbctl dump-flows sw1 | grep "ip4.dst ==
1.1.1.1" | grep -c .`])
+
+# Sanity: p1's port flows now use the final address 192.168.30.2 (confirms
+# the port changes in runs 1 and 3 were actually applied).
+OVS_WAIT_UNTIL([test 1 -le `ovn-sbctl dump-flows sw1 | grep "192.168.30.2" |
grep -c .`])
+
+OVN_CLEANUP_NORTHD
+AT_CLEANUP
+])
--
2.55.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
