Commit 0e2bcf70ac4f moved the release of the dp group referenced by an
lflow from sync_lflow_to_sb() to do_ovn_lflow_add(), so that the group is
already free when a new one is looked up and the SB row can be reused.
However, do_ovn_lflow_add() is only called when the lflow is generated
again. An lflow that is merely unlinked from one of its lflow_refs, and
that survives because other lflow_refs still reference it, never goes
through do_ovn_lflow_add(): only its dp group bitmap shrinks. Such an
lflow is synced with a different dp group (or with a single datapath),
and the reference to the previous group is never dropped.
The leaked group stays in the 'dp_groups' map with a non zero refcount
while no lflow uses it anymore, so its SB Logical_DP_Group row is garbage
collected. Any lflow that later needs that same set of datapaths finds
the leaked group, fails to look its row up and northd logs:
SB Logical flow [...]'s logical_dp_group column is not set (which is
unexpected). It should have been referencing the dp group [...]
and falls back to a full recompute.
Release the group in lflow_ref_unlink_lflows(), where the bitmap actually
shrinks. Like the release done by do_ovn_lflow_add(), this happens before
any lflow of the round is synced, which is what allows
ovn_dp_group_create() to modify the SB row of a group that just became
free instead of inserting a new row and leaving the old one to be deleted.
Releasing at sync time would fix the leak but not that churn: a group is
usually referenced by several lflows, so it is still in 'dp_groups' when
the first of them is synced.
Fixes: 0e2bcf70ac4f ("northd: Change ovn_dp_groups to decrement refcount in
do_ovn_lflow_add.")
Assisted-by: Claude Opus 5, Claude Code
Signed-off-by: Lucas Vargas Dias <[email protected]>
---
northd/en-lflow.c | 6 ++-
northd/lflow-mgr.c | 39 ++++++++++++++++---
northd/lflow-mgr.h | 2 +-
northd/northd.c | 16 ++++----
tests/ovn-northd.at | 94 +++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 140 insertions(+), 17 deletions(-)
diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index 8cb987777..e8e6cdaa2 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -294,7 +294,8 @@ lflow_group_ecmp_route_change_handler(struct engine_node
*node,
HMAPX_FOR_EACH (hmapx_node,
&group_ecmp_route_data->trk_data.deleted_datapath_routes) {
route_node = hmapx_node->data;
- lflow_ref_unlink_lflows(route_node->lflow_ref);
+ lflow_ref_unlink_lflows(route_node->lflow_ref,
+ lflow_data->lflow_table);
bool handled = lflow_ref_sync_lflows(
route_node->lflow_ref, lflow_data->lflow_table,
@@ -312,7 +313,8 @@ lflow_group_ecmp_route_change_handler(struct engine_node
*node,
&group_ecmp_route_data->trk_data.crupdated_datapath_routes;
HMAPX_FOR_EACH (hmapx_node, crupdated_datapath_routes) {
route_node = hmapx_node->data;
- lflow_ref_unlink_lflows(route_node->lflow_ref);
+ lflow_ref_unlink_lflows(route_node->lflow_ref,
+ lflow_data->lflow_table);
build_route_data_flows_for_lrouter(
route_node->od, lflow_data->lflow_table,
route_node, lflow_input.bfd_ports);
diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
index ce9c4f854..f51df9ce0 100644
--- a/northd/lflow-mgr.c
+++ b/northd/lflow-mgr.c
@@ -657,26 +657,53 @@ lflow_ref_destroy(struct lflow_ref *lflow_ref)
* from the lrn->lflow's dpg bitmap
*/
void
-lflow_ref_unlink_lflows(struct lflow_ref *lflow_ref)
+lflow_ref_unlink_lflows(struct lflow_ref *lflow_ref,
+ struct lflow_table *lflow_table)
{
struct lflow_ref_node *lrn;
HMAP_FOR_EACH (lrn, ref_node, &lflow_ref->lflow_ref_nodes) {
+ struct ovn_lflow *lflow = lrn->lflow;
+ bool dpg_bitmap_changed = false;
+
if (lrn->dpgrp_lflow) {
size_t index;
BITMAP_FOR_EACH_1 (index, lrn->dpgrp_bitmap_len,
lrn->dpgrp_bitmap) {
- if (dp_refcnt_release(&lrn->lflow->dp_refcnts_map, index)) {
- dynamic_bitmap_set0(&lrn->lflow->dpg_bitmap, index);
+ if (dp_refcnt_release(&lflow->dp_refcnts_map, index)) {
+ dynamic_bitmap_set0(&lflow->dpg_bitmap, index);
+ dpg_bitmap_changed = true;
}
}
} else {
- if (dp_refcnt_release(&lrn->lflow->dp_refcnts_map,
+ if (dp_refcnt_release(&lflow->dp_refcnts_map,
lrn->dp_index)) {
- dynamic_bitmap_set0(&lrn->lflow->dpg_bitmap, lrn->dp_index);
+ dynamic_bitmap_set0(&lflow->dpg_bitmap, lrn->dp_index);
+ dpg_bitmap_changed = true;
}
}
+ /* The lflow doesn't apply to the same set of datapaths anymore, so
+ * the dp group it is referencing is not the one it will be synced
+ * with. Drop the reference now, before any lflow of this round is
+ * synced: as long as the group is still referenced it stays in
+ * 'dp_groups' and ovn_dp_group_create() refuses to modify its SB
+ * row, inserting a new Logical_DP_Group row and deleting the one
+ * that just became free instead of reusing it.
+ *
+ * This is the counterpart of the release done by
+ * do_ovn_lflow_add(), which covers the lflows that are generated
+ * again. An lflow that is only unlinked from one of its lflow_refs
+ * and survives because other lflow_refs still reference it never
+ * goes through do_ovn_lflow_add(). */
+ if (dpg_bitmap_changed && lflow->dpg) {
+ enum ovn_datapath_type dp_type =
+ ovn_stage_to_datapath_type(lflow->stage);
+ ovn_dp_group_release(&lflow_table->dp_groups[dp_type],
+ lflow->dpg);
+ lflow->dpg = NULL;
+ }
+
lrn->linked = false;
}
}
@@ -690,7 +717,7 @@ lflow_ref_resync_flows(struct lflow_ref *lflow_ref,
const struct sbrec_logical_flow_table *sbflow_table,
const struct sbrec_logical_dp_group_table *dpgrp_table)
{
- lflow_ref_unlink_lflows(lflow_ref);
+ lflow_ref_unlink_lflows(lflow_ref, lflow_table);
return lflow_ref_sync_lflows__(lflow_ref, lflow_table, ovnsb_txn,
dps,
ovn_internal_version_changed, sbflow_table,
diff --git a/northd/lflow-mgr.h b/northd/lflow-mgr.h
index 84d0b3e67..6253be685 100644
--- a/northd/lflow-mgr.h
+++ b/northd/lflow-mgr.h
@@ -57,7 +57,7 @@ struct lflow_ref;
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 *);
+void lflow_ref_unlink_lflows(struct lflow_ref *, struct lflow_table *);
bool lflow_ref_resync_flows(struct lflow_ref *,
struct lflow_table *lflow_table,
struct ovsdb_idl_txn *ovnsb_txn,
diff --git a/northd/northd.c b/northd/northd.c
index 9f33b50cf..7753de970 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -20997,7 +20997,7 @@ lflow_handle_northd_lr_changes(struct ovsdb_idl_txn
*ovnsb_txn,
HMAPX_FOR_EACH (hmapx_node, &tracked_lrs->crupdated) {
struct ovn_datapath *od = hmapx_node->data;
- lflow_ref_unlink_lflows(od->datapath_lflows);
+ lflow_ref_unlink_lflows(od->datapath_lflows, lflows);
build_lswitch_and_lrouter_iterate_by_lr(od, &lsi);
}
@@ -21052,7 +21052,7 @@ lflow_handle_northd_port_changes(struct ovsdb_idl_txn
*ovnsb_txn,
/* Make sure 'op' is an lsp and not lrp. */
ovs_assert(op->nbsp);
/* Clear old lflows. */
- lflow_ref_unlink_lflows(op->lflow_ref);
+ lflow_ref_unlink_lflows(op->lflow_ref, lflows);
/* Generate new lflows. */
struct ds match = DS_EMPTY_INITIALIZER;
@@ -21072,7 +21072,7 @@ lflow_handle_northd_port_changes(struct ovsdb_idl_txn
*ovnsb_txn,
if (handled) {
/* Now regenerate the stateful lflows for 'op' */
/* Clear old lflows. */
- lflow_ref_unlink_lflows(op->stateful_lflow_ref);
+ lflow_ref_unlink_lflows(op->stateful_lflow_ref, lflows);
build_lbnat_lflows_iterate_by_lsp(op,
lflow_input->lr_stateful_table,
&match, &actions, lflows);
@@ -21164,7 +21164,7 @@ lflow_handle_northd_lb_changes(struct ovsdb_idl_txn
*ovnsb_txn,
lb_dps = hmapx_node->data;
/* unlink old lflows. */
- lflow_ref_unlink_lflows(lb_dps->lflow_ref);
+ lflow_ref_unlink_lflows(lb_dps->lflow_ref, lflows);
/* Generate new lflows. */
struct ds match = DS_EMPTY_INITIALIZER;
@@ -21220,7 +21220,7 @@ lflow_handle_lr_stateful_changes(struct ovsdb_idl_txn
*ovnsb_txn,
HMAPX_FOR_EACH (hmapx_node, &trk_data->crupdated) {
lr_stateful_rec = hmapx_node->data;
/* Unlink old lflows. */
- lflow_ref_unlink_lflows(lr_stateful_rec->lflow_ref);
+ lflow_ref_unlink_lflows(lr_stateful_rec->lflow_ref, lflows);
/* Generate new lflows. */
build_lr_stateful_flows(lr_stateful_rec, lflow_input->lr_datapaths,
@@ -21245,7 +21245,7 @@ lflow_handle_lr_stateful_changes(struct ovsdb_idl_txn
*ovnsb_txn,
lr_stateful_rec->lr_index);
struct ovn_port *op;
HMAP_FOR_EACH (op, dp_node, &od->ports) {
- lflow_ref_unlink_lflows(op->stateful_lflow_ref);
+ lflow_ref_unlink_lflows(op->stateful_lflow_ref, lflows);
build_lbnat_lflows_iterate_by_lrp(op,
lflow_input->lr_stateful_table,
@@ -21265,7 +21265,7 @@ lflow_handle_lr_stateful_changes(struct ovsdb_idl_txn
*ovnsb_txn,
}
if (op->peer && op->peer->nbsp) {
- lflow_ref_unlink_lflows(op->peer->stateful_lflow_ref);
+ lflow_ref_unlink_lflows(op->peer->stateful_lflow_ref, lflows);
build_lbnat_lflows_iterate_by_lsp(
op->peer, lflow_input->lr_stateful_table, &match, &actions,
@@ -21307,7 +21307,7 @@ lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn
*ovnsb_txn,
ovs_assert(od->nbs && uuid_equals(&od->nbs->header_.uuid,
&ls_stateful_rec->nbs_uuid));
- lflow_ref_unlink_lflows(ls_stateful_rec->lflow_ref);
+ lflow_ref_unlink_lflows(ls_stateful_rec->lflow_ref, lflows);
/* Generate new lflows. */
build_ls_stateful_flows(ls_stateful_rec, od,
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 8c8d7852e..aa281c1a2 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -23647,3 +23647,97 @@ AT_CHECK([as northd ovn-appctl -t ovn-northd
inc-engine/enable-stopwatch nonexis
OVN_CLEANUP_NORTHD
AT_CLEANUP
])
+
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([Datapath group reuse after an lflow loses a datapath])
+ovn_start
+
+# All four switches have ACLs, so the logical flows that only depend on
+# "the switch has ACLs" are shared by the four of them. ls1 and ls2 use
+# tier 0 while ls3 and ls4 use tier 1, which changes the actions of the
+# egress "acl action" flows. Hence three datapath groups are expected:
+# {ls1, ls2}, {ls3, ls4} and {ls1, ls2, ls3, ls4}.
+check ovn-nbctl ls-add ls1
+check ovn-nbctl ls-add ls2
+check ovn-nbctl ls-add ls3
+check ovn-nbctl ls-add ls4
+check ovn-nbctl acl-add ls1 to-lport 1000 ip4 allow
+check ovn-nbctl acl-add ls2 to-lport 1000 ip6 allow
+check ovn-nbctl --tier=1 acl-add ls3 to-lport 1000 tcp allow
+check ovn-nbctl --wait=sb --tier=1 acl-add ls4 to-lport 1000 udp allow
+
+acl1=$(fetch_column nb:ACL _uuid match=ip4)
+check_row_count Logical_DP_Group 3
+
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+
+# Move ls1's ACL to tier 1. ls1 stops generating the egress "acl action"
+# flows it shared with ls2, so those flows are left with a single datapath
+# and the {ls1, ls2} datapath group becomes unused and is deleted. northd
+# has to drop the reference it holds to that group, otherwise the group is
+# leaked in the in-memory dp group table, still pointing to the SB row that
+# has just been deleted.
+check ovn-nbctl --wait=sb set ACL $acl1 tier=1
+check_engine_stats lflow norecompute compute
+check_row_count Logical_DP_Group 2
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+
+# Move ls1's ACL back to tier 0. The {ls1, ls2} datapath group is needed
+# again: a leaked group would be picked up here and northd would complain
+# about its dangling SB reference.
+check ovn-nbctl --wait=sb set ACL $acl1 tier=0
+check_engine_stats lflow norecompute compute
+check_row_count Logical_DP_Group 3
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+AT_CHECK([grep -q "logical_dp_group column is not set" \
+ northd/ovn-northd.log], [1])
+
+OVN_CLEANUP_NORTHD
+AT_CLEANUP
+])
+
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([Datapath group SB row reuse when an lflow loses a datapath])
+ovn_start
+
+# ls1, ls2 and ls3 have tier 0 ACLs, ls4 has a tier 1 ACL. The flows that
+# only depend on "the switch has ACLs" are shared by the four of them, and
+# the egress "acl action" flows of tier 0 are shared by ls1, ls2 and ls3.
+# ls4 is the only user of the tier 1 flows, so those are synced with a
+# single datapath. Hence two datapath groups: {ls1, ls2, ls3} and
+# {ls1, ls2, ls3, ls4}.
+check ovn-nbctl ls-add ls1
+check ovn-nbctl ls-add ls2
+check ovn-nbctl ls-add ls3
+check ovn-nbctl ls-add ls4
+check ovn-nbctl acl-add ls1 to-lport 1000 ip4 allow
+check ovn-nbctl acl-add ls2 to-lport 1000 ip6 allow
+check ovn-nbctl acl-add ls3 to-lport 1000 tcp allow
+check ovn-nbctl --wait=sb --tier=1 acl-add ls4 to-lport 1000 udp allow
+
+acl3=$(fetch_column nb:ACL _uuid match=tcp)
+check_row_count Logical_DP_Group 2
+ovn-sbctl --bare --columns _uuid list Logical_DP_Group | sort > dpg_uuids
+
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+
+# Move ls3's ACL to tier 1. The tier 0 "acl action" flows are left with
+# {ls1, ls2}: they are not generated again (ls1 and ls2 didn't change), they
+# are only unlinked from ls3's lflow_ref. The {ls1, ls2, ls3} group becomes
+# unused, so its SB row is free and northd is expected to modify it in place
+# instead of inserting a new row and deleting this one.
+check ovn-nbctl --wait=sb set ACL $acl3 tier=1
+check_engine_stats lflow norecompute compute
+check_row_count Logical_DP_Group 3
+
+ovn-sbctl --bare --columns _uuid list Logical_DP_Group | sort > dpg_uuids2
+AT_CHECK([comm -23 dpg_uuids dpg_uuids2], [0], [])
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+OVN_CLEANUP_NORTHD
+AT_CLEANUP
+])
+
--
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
