"dev" <dev-boun...@openvswitch.org> wrote on 07/26/2016 08:13:00 PM:
> From: Russell Bryant <russ...@ovn.org>
> To: Babu Shanmugam <bscha...@redhat.com>
> Cc: ovs dev <dev@openvswitch.org>
> Date: 07/26/2016 08:13 PM
> Subject: Re: [ovs-dev] [PATCH] ovn-controller: update_ct_zone
> operates always on empty set
> Sent by: "dev" <dev-boun...@openvswitch.org>
>
> On Tue, Jul 26, 2016 at 6:46 AM, <bscha...@redhat.com> wrote:
>
> > From: Babu Shanmugam <bscha...@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 <bscha...@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.
>
> 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.
>
> ---
> > ovn/controller/binding.c | 4 +++-
> > ovn/controller/binding.h | 3 ++-
> > ovn/controller/ovn-controller.c | 2 +-
> > 3 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> > index e83c1d5..7bc6fb4 100644
> > --- a/ovn/controller/binding.c
> > +++ b/ovn/controller/binding.c
> > @@ -230,7 +230,8 @@ consider_local_datapath(struct controller_ctx *ctx,
> >
> > void
> > binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge
> > *br_int,
> > - const char *chassis_id, struct hmap *local_datapaths)
> > + const char *chassis_id, struct hmap *local_datapaths,
> > + struct sset *all_lports)
> > {
> > const struct sbrec_chassis *chassis_rec;
> > const struct sbrec_port_binding *binding_rec;
> > @@ -292,6 +293,7 @@ binding_run(struct controller_ctx *ctx, const
struct
> > ovsrec_bridge *br_int,
> > }
> > }
> >
> > + sset_clone(all_lports, &local_ids);
> >
>
> I don't think this is quite sufficient. It's missing, at least:
>
> - the IDs of sub-ports
>
> - localnet ports
>
> The old handling of building up all_lports ensure those got added.
>
Yes, I'm inclined to put the all_lports code back in and persist it in
a similar way to how local_ids is persisted.
I just checked and the only test that I'm seeing consistently fail
is the LB test Guru pointed out previously, I think we've got a
problem in our test code. While I'd be happy to write the test case,
I'm not sure I understand it, so can somebody give me a pointer to an
existing test case or draft something that can be used to help fix this
and avoid the regression in the future?
Ryan
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
