[ovs-dev] [PATCH v3] ovn-controller: Back out incremental processing

[Ryan Moats]

Guru Shetty <guru at ovn.org> wrote on 08/24/2016 03:57:41 PM:

> From: Guru Shetty <guru at ovn.org>
> To: Ryan Moats/Omaha/IBM at IBMUS
> Cc: ovs dev <dev at openvswitch.org>
> Date: 08/24/2016 03:58 PM
> Subject: Re: [ovs-dev] [PATCH v3] ovn-controller: Back out
> incremental processing
>
> On 24 August 2016 at 11:12, Ryan Moats <rmoats at us.ibm.com> wrote:
>
>
>
> Guru Shetty <guru at ovn.org> wrote on 08/24/2016 01:07:47 PM:
>
> > From: Guru Shetty <guru at ovn.org>
> > To: Ryan Moats/Omaha/IBM at IBMUS
> > Cc: ovs dev <dev at openvswitch.org>
> > Date: 08/24/2016 01:08 PM
> > Subject: Re: [ovs-dev] [PATCH v3] ovn-controller: Back out
> > incremental processing
> >
> > On 24 August 2016 at 11:03, Ryan Moats <rmoats at us.ibm.com> wrote:
> >
> > Guru Shetty <guru at ovn.org> wrote on 08/24/2016 12:46:55 PM:
> >
> > > From: Guru Shetty <guru at ovn.org>
> > > To: Ryan Moats/Omaha/IBM at IBMUS
> > > Cc: ovs dev <dev at openvswitch.org>
> > > Date: 08/24/2016 12:47 PM
> > > Subject: Re: [ovs-dev] [PATCH v3] ovn-controller: Back out
> > > incremental processing
> > >
> > > On 23 August 2016 at 22:40, Ryan Moats <rmoats at us.ibm.com> wrote:
> > > As [1] indicates, incremental processing hasn't resulted
> > > in an improvement worth the complexity it has added.
> > > This patch backs out all of the code specific to incremental
> > > processing, along with the persisting of OF flows,
> > > logical ports and multicast groups.
> > >
> > > [1] http://openvswitch.org/pipermail/dev/2016-August/078272.html
> > >
> > > Signed-off-by: Ryan Moats <rmoats at us.ibm.com>
> > >
> > > This is not a full review. But I have a few comments.
> > >
> > > * sparse gives the following warning
> > > ovn/controller/ofctrl.c:675:1: warning: symbol
> > > 'ovn_flow_table_clear' was not declared. Should it be static?
> >
> > hmm... I think I lost something because I remember needing to declare
> > that static...
> >
> > > * struct group_info still has lflow_uuid. Do we need it? It looks to
> > > me that it is not needed.
> >
> > I wanted to keep the change minimal to start with - I will look and
> > see if I can pull this out...
> >
> > > While you are at it, please replace the comment on top of ofctrl_put
> > > around groups to just read:
> > >      * Replaces the group table on the switch, if possible, by the
> > > 'groups->desired_groups'
> >
> > I can do that
> >
> > >
> > > * I notice that conntrack zone allocation for logical ports is still
> > > broken. I am not sure when it broke (but it has been broke for a
> > > long time), but I remember some patches for the fix around it for
> > > incremental processing
> > > For e.g., if you add the following test, you will notice that it
fails.
> >
> > With your permission, I'll add this in to PS 4 and get it fixed
> > You have my permission (I don't know what PS 4 is.). The test is
> > very minimalist and just tries to see whether atleast one logical
> > port has REG13 loaded as an example.

> Thanks, and PS 4 = Patch Set 4 (i.e. the next version)
>
> Just an FYI. If you fix the conntrack zone allocation correctly, 2
> load balancing tests will fail and that should just be because the
> output has an extra zone (which was previously just zero and I had
> missed the implication of it until yesterday.).

Well, I think I have, because I've undone the persistance of
all_lports, local_datapaths, and patched_datapaths in this go around...

I'm spinning v4 of the patch and emailing now, so we can collaborate
on how to fix the tests as part of v5 of the patch...