On 7/2/25 8:48 PM, Mark Michelson wrote: > On 7/2/25 5:27 AM, Dumitru Ceara wrote: >> Hi Mark, >> >> On 6/9/25 7:35 PM, Mark Michelson via dev wrote: >>> NOTE: This series is built on top of the datapath binding refactor and >>> port binding refactor patches. You're likely to see tests failing on >>> these patches if those series are not merged already. >>> >> >> It seems the logical flow sync refactor (this series) doesn't even apply >> cleanly on top of main without the datapath binding and port binding >> refactor. >> >> https://mail.openvswitch.org/pipermail/ovs-build/2025-June/046904.html >> >> Should we defer it until the binding refactor lands? > > Yes, that's completely fair to do. I posted it because while it depends > on the datapath and port binding refactor patches, my hope was that the > code should be reviewable by applying both datapath and port binding > refactor first. However, the urgency of this has declined somewhat, so > it makes complete sense to just defer this instead of trying to have it > ready overly soon. >
Hi Mark, Thanks for the confirmation! I marked the series as "deferred" in patchwork. Regards, Dumitru >> >>> This series refactors how logical flows are synced, accomplishing two >>> main goals: >>> >>> 1) Syncing of logical flows to the southbound database is handled in a >>> new node, rather than in en-lflow. This allows for syncing of logical >>> flow tables from en-lflow and other nodes. >>> >>> 2) The libraries for building logical flow tables and for syncing >>> logical flows no longer relies on ovn-northd constructs such as >>> ovn_datapath. >>> >>> This series accomplishes the above goals through a series of smaller >>> patches rather than a single giant patch like other refactoring series. >>> >>> Overall, this series gets the job done, but there are a few things I'm >>> not especially happy about: >>> >>> 1) The goal of the new en-lflow-sync node is to be able to sync logical >>> flows from multiple logical flow tables. However, in current OVN, the >>> only logical flow table is the one created by en-lflow. Therefore, while >>> the code *should* be able to handle multiple logical flow tables, it's >>> not proven with this code change. >>> >>> 2) I'm not happy about the non-constness of the inputs to the >>> en-lflow-sync node. In particular, >>> * lflow_refs are created/destroyed in various nodes, then all are >>> populated in en-lflow, and then all are synced in en-lflow-sync. >>> This means that en-lflow and en-lflow-sync are modifying lflow_refs >>> beyond the node that manages their lifetimes. >>> * struct ovn_lflow has a datapath group field (dpg) that is modified >>> in en-lflow-sync. >>> >>> There are ways to fix (2), but to do so in a performant way would be >>> outside the scope of simply trying to refactor when/how logical flows >>> are synced. >>> >>> Mark Michelson (14): >>> northd: Find ovn_datapath from a single hmap. >>> lflow-mgr: Remove the ovn_datapath from ovn_lflow. >>> lflow-mgr: Use one ovn_datapaths arg for ovn_dp_group_create(). >>> lflow-mgr: Pass one ovn_datapaths arg to sync_lflow_to_sb(). >>> lflow_mgr: Pass dp_groups to sync_lflow_to_sb(). >>> lflows: Create a separate node for lflow syncing. >>> en-lflow-sync: Separate syncing from datapaths and from the lflow >>> table. >>> northd: Convert ovn_stage from an enum to a struct. >>> lflow_ref: Allow multiple datapaths per lflow_ref_node. >>> lflows: Separate lflow_ref building and syncing. >>> lflows: Move dp_groups from lflow_data to lflow_sync_data. >>> datapaths: Add a hashvec of datapath bindings. >>> lflow: Remove northd dependency from en-lflow-sync. >>> lflows: Remove ovn_datapath parameter when adding lflows. >>> >>> northd/datapath_sync.c | 52 +++ >>> northd/datapath_sync.h | 33 +- >>> northd/en-datapath-logical-router.c | 23 +- >>> northd/en-datapath-logical-router.h | 10 +- >>> northd/en-datapath-logical-switch.c | 23 +- >>> northd/en-datapath-logical-switch.h | 10 +- >>> northd/en-datapath-sync.c | 36 +- >>> northd/en-learned-route-sync.c | 8 +- >>> northd/en-lflow.c | 309 +++++++++++--- >>> northd/en-lflow.h | 32 ++ >>> northd/en-multicast.c | 3 +- >>> northd/en-northd-output.c | 14 +- >>> northd/en-northd-output.h | 5 +- >>> northd/en-northd.c | 5 +- >>> northd/en-port-binding-chassisredirect.c | 8 +- >>> northd/en-port-binding-logical-router-port.c | 2 +- >>> northd/en-port-binding-logical-switch-port.c | 2 +- >>> northd/en-port-binding-mirror.c | 4 +- >>> northd/en-sync-sb.c | 104 +++-- >>> northd/inc-proc-northd.c | 22 +- >>> northd/lflow-mgr.c | 419 +++++++++---------- >>> northd/lflow-mgr.h | 84 ++-- >>> northd/northd.c | 376 +++++++---------- >>> northd/northd.h | 92 ++-- >>> tests/ovn-northd.at | 125 +++++- >>> 25 files changed, 1112 insertions(+), 689 deletions(-) >>> >> >> Regards, >> Dumitru >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev