[ovs-dev] [PATCH] ovn-controller: update_ct_zone operates always on empty set

Russell Bryant russell at ovn.org
Wed Jul 27 21:09:12 UTC 2016


On Wed, Jul 27, 2016 at 8:34 AM, Ryan Moats <rmoats at us.ibm.com> wrote:

> "dev" <dev-bounces at openvswitch.org> wrote on 07/27/2016 12:44:38 AM:
>
> > From: Babu Shanmugam <bschanmu at redhat.com>
> > To: Russell Bryant <russell at ovn.org>
> > Cc: ovs dev <dev at openvswitch.org>
> > Date: 07/27/2016 12:45 AM
> > Subject: Re: [ovs-dev] [PATCH] ovn-controller: update_ct_zone
> > operates always on empty set
> > Sent by: "dev" <dev-bounces at openvswitch.org>
> >
> >
> >
> > On Wednesday 27 July 2016 06:43 AM, Russell Bryant wrote:
> > > On Tue, Jul 26, 2016 at 6:46 AM, <bschanmu at redhat.com
> > > <mailto:bschanmu at redhat.com <bschanmu at redhat.com>>> wrote:
> > >
> > >     From: Babu Shanmugam <bschanmu at redhat.com
> > >     <mailto:bschanmu at redhat.com <bschanmu at redhat.com>>>
> > >
> > >     Commit 263064a (Convert binding_run to incremental processing.)
> > >     removed the usage
> > >     of all_lports from binding_run, but it is infact used in the
> > >     context of the caller,
> > >     especially by update_ct_zones().
> > >
> > >     Without this change, update_ct_zones operates on an empty set
> always.
> > >
> > >     Signed-off-by: Babu Shanmugam <bschanmu at redhat.com
> > >     <mailto:bschanmu at redhat.com <bschanmu at redhat.com>>>
> > >
> > >
> > > Ouch. This is a really bad regression.  If I understand correctly,
> > > we're not setting a ct zone ID for any logical ports.  All are just
> > > using the default zone of 0.
> > >
> > Yes Russell, your understanding is correct.
> > > We should think about a good way to test OVN's use of conntrack zones
> > > to ensure that entries end up in separate zones for separate ports.  A
> > > good test for that may require userspace conntrack support, though.
> > >  Another test we could do now would be looking at the flows in table 0
> > > and ensuring that the input flow for each port has a different
> > > conntrack zone ID assigned.  That feels like kind of a hack, though.
> > I agree that we need more test cases. I could not spend much time to
> > figure out a proper approach for a test case. I will have a look at it.
> >
> > Thank you,
> > Babu
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> <http://patchwork.ozlabs.org/patch/653288/>
> http://patchwork.ozlabs.org/patch/653288/ replicates the
> all_lports code from binding.c prior to commit 263064a (I literally
> rolled a repo back to the commit before that one and then did a
> code inspection and copy/paste). Now, I still don't have a test case
> that shows the revert is fixed (because I frankly don't know how to
> write this one), but I believe that with the above patch set we are
> no longer using only the default zone.
>
Thanks.  Re-adding a full loop over all port bindings seems pretty
undesirable.  I think we can iterate on this original patch and get a full
solution without a ton of extra effort.

-- 
Russell Bryant



More information about the dev mailing list