Hi Mark, Thanks. This version just re-add ovn_dp_group_release in sync_lflow_to_sb. I'll work in a new version to separate the logical datapath group syncing from lflow syncing.
Regards, Lucas Em qua., 19 de ago. de 2026 às 13:29, Mark Michelson <[email protected]> escreveu: > 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’. > > -- _‘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
