On Wed, Jul 27, 2016 at 8:34 AM, Ryan Moats <[email protected]> wrote:
> "dev" <[email protected]> wrote on 07/27/2016 12:44:38 AM: > > > From: Babu Shanmugam <[email protected]> > > To: Russell Bryant <[email protected]> > > Cc: ovs dev <[email protected]> > > 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" <[email protected]> > > > > > > > > On Wednesday 27 July 2016 06:43 AM, Russell Bryant wrote: > > > On Tue, Jul 26, 2016 at 6:46 AM, <[email protected] > > > <mailto:[email protected] <[email protected]>>> wrote: > > > > > > From: Babu Shanmugam <[email protected] > > > <mailto:[email protected] <[email protected]>>> > > > > > > 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 <[email protected] > > > <mailto:[email protected] <[email protected]>>> > > > > > > > > > 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 > > [email protected] > > 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 _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
