On Tue, Aug 18, 2026 at 2:59 PM Lucas Vargas Dias <[email protected]> wrote: > > Hi, > > I would like to know your option about this patch, since you created/acked > patch [0]. > Or if is there another way to resolve the Warn log message? > > [0] > https://github.com/ovn-org/ovn/commit/0e2bcf70ac4f769704c62de715b4905720d8ada3 > > Regards, > Lucas
Hi Lucas. First, I haven't had an opportunity to look at your patch. But I figured I could give a bit of background on the commit you linked. The commit you linked was to solve https://redhat.atlassian.net/browse/FDP-2747 . The addition of logical router incremental processing introduced an issue in the southbound database. Originally, we would avoid deleting and re-inserting logical datapath groups by reusing existing ones when possible. But the logical router incremental processing exposed a flaw in the logical flow syncing code. With that addition, we were now deleting and re-inserting logical datapath groups in the SB DB, causing a lot of extra unnecessary work. The issue suggested that the best way to approach this problem is to separate the logical datapath group syncing into a separate engine node that runs after logical flows have been synchronized. This way, the datapath group syncing node would have the full picture of what logical flows exist and what datapaths they apply to. This is not trivial work. This item was assigned to Jacob, and he figured out that by changing the way we were handling the dp group refcount, we could avoid deleting a SB logical datapath group too early. Since this appeared to solve the issue of the unnecessary datapath group deletion/re-insertion, this seemed like a good stop-gap until we could do the proper thing of actually separating the dp group syncing from logical flow syncing. While this solved the issue of the unnecessary churn, it appears to have caused a different problem, which results in unnecessary recomputes in en-lflow. I'll need to take a closer look at your patch and determine if this manages to fix the recompute issue you mentioned while also allowing for reuse of SB logical datapath groups. In the long term, we need to do what the linked issue says: separate the logical datapath group syncing from lflow syncing. It has been difficult to find the time to implement it though. > > Em ter., 18 de ago. de 2026 às 15:52, Lucas Vargas Dias > <[email protected]> escreveu: >> >> 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. When such an lflow is synced, it points to a different dp >> group (or to 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 previous dp group after the new one has been taken. The >> release done by do_ovn_lflow_add() clears 'lflow->dpg', so lflows that >> were regenerated are not released twice. >> >> Fixes: 0e2bcf70ac4f ("northd: Change ovn_dp_groups to decrement refcount in >> do_ovn_lflow_add.") >> Signed-off-by: Lucas Vargas Dias <[email protected]> >> --- >> northd/lflow-mgr.c | 7 +++++++ >> tests/ovn-northd.at | 51 +++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 58 insertions(+) >> >> diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c >> index ce9c4f854..2367156ad 100644 >> --- a/northd/lflow-mgr.c >> +++ b/northd/lflow-mgr.c >> @@ -1264,6 +1264,13 @@ sync_lflow_to_sb(struct ovn_lflow *lflow, >> >> if (pre_sync_dpg != lflow->dpg) { >> ovn_dp_group_use(lflow->dpg); >> + /* The lflow's dp group bitmap may have changed without the lflow >> + * being re-added (e.g. when it was only unlinked from one of its >> + * lflow_refs), in which case do_ovn_lflow_add() didn't get the >> + * chance to drop the reference to the previous dp group. Drop it >> + * here, otherwise the dp group is leaked in 'dp_groups' with a >> + * dangling reference to an SB row that gets garbage collected. */ >> + ovn_dp_group_release(dp_groups, pre_sync_dpg); >> } >> >> lflow->sync_state = LFLOW_SYNCED; >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index 8c8d7852e..19887e6d7 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -23647,3 +23647,54 @@ 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 >> +]) >> -- >> 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
