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

Reply via email to