On Tue, Jul 19, 2016 at 02:18:11PM -0500, Ryan Moats wrote:
>
>
> Ben Pfaff <b...@ovn.org> wrote on 07/19/2016 01:39:43 PM:
>
> > From: Ben Pfaff <b...@ovn.org>
> > To: Ryan Moats/Omaha/IBM@IBMUS
> > Cc: dev@openvswitch.org
> > Date: 07/19/2016 01:41 PM
> > Subject: Re: [ovs-dev, v23, 2/2] ovn-controller: Add incremental
> > processing to lflow_run and physical_run
> >
> > On Tue, Jul 19, 2016 at 08:10:05AM -0500, Ryan Moats wrote:
> > >
> > >
> > > Ben Pfaff <b...@ovn.org> wrote on 07/19/2016 12:57:23 AM:
> > >
> > > > From: Ben Pfaff <b...@ovn.org>
> > > > To: Ryan Moats/Omaha/IBM@IBMUS
> > > > Cc: dev@openvswitch.org
> > > > Date: 07/19/2016 12:57 AM
> > > > Subject: Re: [ovs-dev, v23, 2/2] ovn-controller: Add incremental
> > > > processing to lflow_run and physical_run
> > > >
> > > > On Mon, Jul 18, 2016 at 04:21:17PM -0500, Ryan Moats wrote:
> > > > > This code changes to allow incremental processing of the
> > > > > logical flow and physical binding tables whenver possible.
> > > > >
> > > > > Note: flows created by physical_run for multicast_groups are
> > > > > *NOT* handled incrementally due to to be solved issues
> > > > > with GWs and local routers.
> > > > >
> > > > > Signed-off-by: Ryan Moats <rmo...@us.ibm.com>
> > > >
> > > > With this applied, I get a number of apparently consistent test
> failures
> > > > (literally no passes in 5+ runs) that I don't see, at least not
> > > > consistently, if I revert it:
> > > >
> > > > OVN end-to-end tests
> > > >
> > > > 2106: ovn -- vtep: 3 HVs, 1 VIFs/HV, 1 GW, 1 LS FAILED
> > > (ovn.at:1307)
> > > > 2107: ovn -- 3 HVs, 1 VIFs/HV, 1 software GW, 1 LS FAILED
> > > (ovn.at:1473)
> > > > 2108: ovn -- 3 HVs, 3 LS, 3 lports/LS, 1 LR FAILED
> > > (ovn.at:1887)
> > > > 2110: ovn -- 2 HVs, 2 LS, 1 lport/LS, 2 peer LRs FAILED
> > > (ovn.at:2416)
> > > > 2115: ovn -- 2 HVs, 3 LRs connected via LS, static routes FAILED
> > > > (ovn.at:3053)
> > > >
> > > > Do these pass reasonably often for you? (Maybe my changes to the
> > > > previous patch screwed something up?)
> > > >
> > >
> > > First, not all of the above cases failed for me even with a rebase of
> the
> > > last patch onto what is currently in master - the last two passed for
> me
> > > on either the first and second run, so I'm looking at the first three
> for
> > > the rest of this message.
> > >
> > > Double checking by going back to my original first patch verified that
> all
> > > of the tests in question passed. Going through and checking each of
> the
> > > changes that you made to the first patch, the following is the diff I
> > > needed
> > > to make things work again:
> > >
> > > diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> > > index d10dcc6..e113213 100644
> > > --- a/ovn/controller/ofctrl.c
> > > +++ b/ovn/controller/ofctrl.c
> > > @@ -553,7 +553,10 @@ ofctrl_add_flow(uint8_t table_id, uint16_t
> priority,
> > > ovn_flow_lookup(&match_flow_table, f, &existing);
> > > struct ovn_flow *d;
> > > LIST_FOR_EACH (d, list_node, &existing) {
> > > - if (uuid_equals(&f->uuid, &d->uuid)) {
> > > + if (f->table_id == d->table_id && f->priority == d->priority
> > > + && ofpacts_equal(f->ofpacts, f->ofpacts_len,
> > > + d->ofpacts, d->ofpacts_len)
> > > + && uuid_equals(&f->uuid, &d->uuid)) {
> > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
> 1);
> > > char *s = ovn_flow_to_string(f);
> > > VLOG_WARN_RL(&rl, "found duplicate flow %s for parent
> > > "UUID_FMT,
> > >
> > > Hopefully that will get you past the test failures and you can push
> > > both it and the rest of the last incremental processing patch.
> >
> > OK...
> >
> > Help me understand why that change is correct.
> >
> > I deleted the table_id and priority checks because ovn_flow_lookup()
> > only returns flows where those are equal anyhow, so they seemed
> > completely redundant. That leaves the actions comparison. Why would we
> > report a duplicate only if the actions are the same? It's actually
> > worse, isn't it, if the actions are different, because then the flows
> > aren't just duplicates, they're inconsistent with each other.
>
> Actually, you are correct in that I'm doing something I shouldn't, but
> your solution is also doing something it shouldn't. Yes, my keeping
> rules that are inconsistent is wrong, but discarding rules where
> the action changes (which is what you proposed) doesn't work either.
>
> How about if we split the difference? First, verify that the uuid
> is the same, then check the actions. If they are the same, spawn a
> warning, and if they aren't replace the old actions with the new ones.
> This appears to pass unit tests as well (and I think it's more correct),
> in which case the difference becomes:
>
> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> index d10dcc6..0b8326e 100644
> --- a/ovn/controller/ofctrl.c
> +++ b/ovn/controller/ofctrl.c
> @@ -554,13 +554,21 @@ ofctrl_add_flow(uint8_t table_id, uint16_t priority,
> struct ovn_flow *d;
> LIST_FOR_EACH (d, list_node, &existing) {
> if (uuid_equals(&f->uuid, &d->uuid)) {
> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> - char *s = ovn_flow_to_string(f);
> - VLOG_WARN_RL(&rl, "found duplicate flow %s for parent
> "UUID_FMT,
> - s, UUID_ARGS(&f->uuid));
> - ovn_flow_destroy(f);
> - free(s);
> - return;
> + if (ofpacts_equal(f->ofpacts, f->ofpacts_len,
> + d->ofpacts, d->ofpacts_len)) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
> 1);
> + char *s = ovn_flow_to_string(f);
> + VLOG_WARN_RL(&rl, "found duplicate flow %s for parent
> "UUID_FMT
> + s, UUID_ARGS(&f->uuid));
> + ovn_flow_destroy(f);
> + free(s);
> + return;
> + } else {
> + /* Update the action. */
> + free(d->ofpacts);
> + d->ofpacts = xmemdup(f->ofpacts, f->ofpacts_len);
> + d->ofpacts_len = f->ofpacts_len;
> + }
> }
> }
>
I'm OK with updating the actions, but I'd prefer to warn about it anyway
because I still think it's a bug; if the "new" actions happen to work
better than the "old" actions, I think that's just a coincidence.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
