[ovs-dev] [PATCH ovn v3 4/4] lflow: Consistent conjunction id generation.

[Han Zhou]

On Fri, Nov 19, 2021 at 11:13 AM Numan Siddique <numans at ovn.org> wrote:
>
> On Thu, Nov 11, 2021 at 5:39 PM Han Zhou <hzhou at ovn.org> wrote:
> >
> > When a logical flow translation uses conjunction ids, it is better to
> > keep the conjuntion ids consistent between iterations, so that the
> > generated OVS flows don't change unnecessarily and avoid reinstalling
> > unchanged flows to OVS. The problem was partially solved when
> > lflow-cache was enabled but there was a bug for a corner case so the
> > lflow-cache based approach was reverted in a previous commit.
> >
> > This patch takes an approach that generates conjunction ids based on the
> > logical flow's uuid, which ensures the same ids being used when
> > processing the same lflow in most cases. There is a very little chance
> > that different logial flows happen to have the same 32-bit prefix in
> > their uuids, but the probability is really low. For the very unlikely
> > situation, the conflicts are handled properly and correctness is
> > ensured.
> >
> > A new module lflow-conj-ids is created to manage the id allocations and
> > maintaining the usage of each ids and their owner lflows.
> >
> > Unlike the previously lflow-cache based approach, this approach works
> > equally well when lflow-cache is not in use.
> >
> > Signed-off-by: Han Zhou <hzhou at ovn.org>
>
> Hi  Han,
>
> Few minor comments.
>
> 1.  I think it would be nice to reorganize the newly added file -
> controller/lflow-conj-ids.c to
> be organized with public functions first and then static functions as
> per the coding
> guidelines -
https://github.com/ovn-org/ovn/blob/main/Documentation/internals/contributing/coding-style.rst
>
Ack. Thanks for reminding.

> 2.  I'd suggest to add a debug function in
> controller/lflow-conj-ids.c which would dump
>      the allocated conj ids and the lflow conj_id mapping.  And the
> test file can verify the dump is as expected.
>     What do you think ?  I'm fine if you think this can be a follow up
patch.
>      Also if required, we can add appctl command to dump the conj id
> allocations.
>
Sounds good. And yes I think it can be a follow-up patch. I will merge it
for now and try to add the suggested debug command before the 21.12 release.

Thanks,
Han