[ovs-dev] [PATCH v2 2/2] ovn: Test for full logical flow processing in ovn-controller

Jakub Sitnicki jkbs at redhat.com
Fri Jun 1 18:08:11 UTC 2018


On Fri, 1 Jun 2018 10:50:07 -0700
Han Zhou <zhouhan at gmail.com> wrote:

> On Fri, Jun 1, 2018 at 10:03 AM, Jakub Sitnicki <jkbs at redhat.com> wrote:
> >
> > On Fri, 1 Jun 2018 09:21:28 -0700
> > Han Zhou <zhouhan at gmail.com> wrote:
> >  
> > > On Fri, Jun 1, 2018 at 6:49 AM, Jakub Sitnicki <jkbs at redhat.com> wrote:  
> > > >
> > > > On Thu, 31 May 2018 10:14:50 -0700
> > > > Han Zhou <zhouhan at gmail.com> wrote:
> > > >  
> > > > > > +# vec_sub VEC_A VEC_B
> > > > > > +#
> > > > > > +# Subtracts two vectors:
> > > > > > +#
> > > > > > +#     VEC_A = [a1, a2, ...]
> > > > > > +#     VEC_B = [b1, b2, ...]
> > > > > > +#     OUT = [(a1 - b1), (a2 - b2), ...]
> > > > > > +#
> > > > > > +# VEC_A and VEC_B must be lists of values separated by a  
> character
> > > from  
> > > > > $IFS.  
> > > > > > +vec_sub() {
> > > > > > +    local a b i j
> > > > > > +
> > > > > > +    i=0
> > > > > > +    for a in $1; do
> > > > > > +        j=0
> > > > > > +        for b in $2; do
> > > > > > +            if test $i -eq $j; then
> > > > > > +                expr $a - $b
> > > > > > +                break
> > > > > > +            fi
> > > > > > +            j=`expr $j + 1`
> > > > > > +        done
> > > > > > +        i=`expr $i + 1`
> > > > > > +    done  
> > > > >
> > > > > The loop is O(n^2) while ideally it can be O(n). I think it is not  
> a big
> > > > > deal since it is testing, and I don't have better idea how to  
> achieve
> > > O(n)  
> > > > > while keeping the current convenient input parameters. Just to  
> confirm
> > > it  
> > > > > is not supposed to be used in scalability testing environment to  
> > > calculate  
> > > > > counters for thousands of sandboxes, right?  
> > > >
> > > > Oh, no, I would not be brave enough to go testing 1000's of sandboxes
> > > > with shell scripts. For that scale I would rewrite the whole test to
> > > > Python probably.
> > > >
> > > > This is the best I could come up with, w/o going into Bash arrays. It
> > > > is intended only for the scale we test at in the automated test suite.
> > > > So just a few sandboxes as most.
> > > >  
> > > Agree. It is totally fine for this test suite.
> > >
> > > How about my other comment:  
> > > > > > +    # Bind port $lp  
> > > > > +    OVN_CONTROLLER_EXPECT_HIT(  
> > >  
> > > > > For port binding, we should expect recompute on the HV where the  
> port
> > > is bound to, and expect no recompute on all other HVs.
> > >  
> > > > > +        [hv1 hv2], [lflow_run],
> > > > > +        [as $hv ovs-vsctl add-port br-int $vif -- set Interface  
> $vif
> > > external-ids:iface-id=$lp]  
> > > > > +    )  
> > >
> > > For example, after binding on hv1, we should expect lflow_run counter
> > > increased on hv1, but not on hv2. We want to ensure incremental
> > > port-binding processing is taking effect on hv2.  
> OVN_CONTROLLER_EXPECT_HIT
> > > checks the total count, but doesn't tell the differences for each HV. I
> > > think we can split this test into two.  
> >
> > Apologies, I've missed your other comment.
> >
> > Yes, I saw what you're describing in with a PoC script I did before
> > rewriting it in Autotest:
> >
> >  
> https://github.com/jsitnicki/tools/blob/master/net/ovn/example_count_lflow_run.txt
> >
> > I was considering making the EXPECT_HIT check less coarse but decided to
> > keep it simple at first. It's certainly something I would like to
> > improve.
> >  
> > > And I just thought about another problem in this port-binding test. It  
> is
> > > better to combine with the wait-until ... up="true" test, and do a
> > > ovn-nbctl --wait=hv sync, so that we are sure the ovn-controller already
> > > processed the port-binding. Otherwise, the test could easily fail due to
> > > timing issue.  
> >
> > Good point, I did not think of that. Having the EXPECT_{HIT,NO_HIT}
> > accept a batch of commands could be doable with some m4_foreach magic.
> > I will need to give it a try.  
> 
> Shouldn't ";" separated commands as the "cmd" parameter just work?

Great idea! I will try it out. The less m4 macros the better. ;-)

> 
> >  
> > > BTW, can I fold your patch into my incremental patch series so that I  
> can
> > > use it and add more test cases on top of it? I am not sure about the
> > > practice in this community when two or more people are working on  
> patches
> > > that depends on each other. Can I just keep everything in your patch
> > > (including Signed-off-by) and also add a comment that the patch is from
> > > you, so that when the committer (most likely blp) will commit with the
> > > correct Author in github?  
> >
> > Yes, sure. Feel free to pull these patches into your series. I'm
> > glad they will be of use.
> >
> > Meanwhile, I will try to add the two enhancements we're discussing
> > here.
> >  
> 
> That's great, thanks!
> I just sent out v3 of my series with address set and port group incremental
> processing:
> https://patchwork.ozlabs.org/project/openvswitch/list/?series=48060
> I will wait for your enhancements and fold your patch in v4.

OK, sounds good.

Thanks,
Jakub

> 
> Thanks,
> Han



More information about the dev mailing list