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

Message ID 20180531104111.15790-3-jkbs@redhat.com
State Superseded
Headers show
Series
  • ovn: Check for effects of incremental processing
Related show

Commit Message

Jakub Sitnicki May 31, 2018, 10:41 a.m.
Add a test that performs typical operations of creating & destroying
logical routers, switches, ports, address sets and ACLs while checking
if they trigger full logical flow processing in the ovn-controller.
This way confirm that incremental processing is taking effect when we
expect it to.

Place the new test in a separate module - tests/ovn-performance.at,
instead of the usual tests/ovn.at as it doesn't test OVN's functionality
but rather a performance aspect of ovn-controller.

Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
---
 tests/automake.mk        |   3 +-
 tests/ovn-performance.at | 287 +++++++++++++++++++++++++++++++++++++++++++++++
 tests/testsuite.at       |   1 +
 3 files changed, 290 insertions(+), 1 deletion(-)
 create mode 100644 tests/ovn-performance.at

Comments

Han Zhou May 31, 2018, 5:14 p.m. | #1
Hi Jakub,

Thanks a lot for implement this nice testing utility for incremental
processing. I looks very useful and convenient!
Please find my comment/question inlined.

On Thu, May 31, 2018 at 3:41 AM, Jakub Sitnicki <jkbs@redhat.com> wrote:
>
> Add a test that performs typical operations of creating & destroying
> logical routers, switches, ports, address sets and ACLs while checking
> if they trigger full logical flow processing in the ovn-controller.
> This way confirm that incremental processing is taking effect when we
> expect it to.
>
> Place the new test in a separate module - tests/ovn-performance.at,
> instead of the usual tests/ovn.at as it doesn't test OVN's functionality
> but rather a performance aspect of ovn-controller.
>
> Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
> ---
>  tests/automake.mk        |   3 +-
>  tests/ovn-performance.at | 287
+++++++++++++++++++++++++++++++++++++++++++++++
>  tests/testsuite.at       |   1 +
>  3 files changed, 290 insertions(+), 1 deletion(-)
>  create mode 100644 tests/ovn-performance.at
>
> diff --git a/tests/automake.mk b/tests/automake.mk
> index c420b29f3..1e009389c 100644
> --- a/tests/automake.mk
> +++ b/tests/automake.mk
> @@ -105,7 +105,8 @@ TESTSUITE_AT = \
>         tests/ovn-controller-vtep.at \
>         tests/mcast-snooping.at \
>         tests/packet-type-aware.at \
> -       tests/nsh.at
> +       tests/nsh.at \
> +       tests/ovn-performance.at
>
>  SYSTEM_KMOD_TESTSUITE_AT = \
>         tests/system-common-macros.at \
> diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
> new file mode 100644
> index 000000000..d2a795a15
> --- /dev/null
> +++ b/tests/ovn-performance.at
> @@ -0,0 +1,287 @@
> +#
> +# Tests targeting performance of OVN components.
> +#
> +
> +m4_divert_push([PREPARE_TESTS])
> +
> +# 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?

> +}
> +
> +# vec_sum VEC
> +#
> +# Sums elements of a vector:
> +#
> +#     VEC = [e1, e2, ...]
> +#     OUT = e1 + e2 + ...
> +#
> +# VEC must be a list of values separated by a character from $IFS.
> +vec_sum() {
> +    local s=0
> +
> +    for a in $1; do
> +        s=`expr $s + $a`
> +    done
> +    echo $s
> +}
> +
> +# read_counters SANDBOXES APPLICATION COUNTER
> +#
> +# Prints out the coverage COUNTER for the APPLICATION in each of the
SANDBOXES.
> +#
> +# SANDBOXES must be a list of strings separated by a character from $IFS.
> +read_counters() {
> +    local sims="$1" app="$2" counter="$3"
> +
> +    for sim in $sims; do
> +        as $sim ovs-appctl -t "$app" coverage/read-count "$counter" ||
return 1
> +    done
> +}
> +
> +# counter_delta_per_sim SANDBOXES APPLICATION COUNTER COMMAND
> +#
> +# Runs the COMMAND and reports the COUNTER change registered during the
command
> +# run for the given APPLICATION in each of the SANDBOXES.
> +counter_delta_per_sim() {
> +    local sims="$1" app="$2" counter="$3" cmd="$4"
> +    local before after diff
> +
> +    before=$(read_counters "$sims" "$app" "$counter") || return 1
> +    $cmd || return 1
> +    after=$(read_counters "$sims" "$app" "$counter") || return 1
> +
> +    vec_sub "$after" "$before"
> +}
> +
> +# counter_delta_per_sim SANDBOXES APPLICATION COUNTER COMMAND
> +#
> +# Same as counter_delta_per_sim but reports the sum of COUNTERs across
all
> +# SANDBOXES.
> +counter_delta_in_total() {
> +    vec_sum "$(counter_delta_per_sim "$@")"
> +}
> +
> +m4_divert_pop([PREPARE_TESTS])
> +
> +# CHECK_COUNTER_DELTA_IS_ZERO SANDBOXES APPLICATION COUNTER COMMAND
> +#
> +# Runs the COMMAND and checks if the COUNTER value for the APPLICATION
in all of
> +# the SANDBOXES did not change.
> +m4_define([CHECK_COUNTER_DELTA_IS_ZERO],[
> +    count=`counter_delta_in_total "$1" "$2" "$3" "$4"`
> +    rc=$?
> +    AT_CHECK([test $rc -eq 0 -a $count -eq 0])
> +])
> +
> +# CHECK_COUNTER_DELTA_IS_NOT_ZERO SANDBOXES APPLICATION COUNTER COMMAND
> +#
> +# Runs the COMMAND and checks if there COUNTER value for the APPLICATION
in
> +# any of the SANDBOXES has changed.
> +m4_define([CHECK_COUNTER_DELTA_IS_NOT_ZERO],[
> +    count=`counter_delta_in_total "$1" "$2" "$3" "$4"`
> +    rc=$?
> +    AT_CHECK([test $rc -eq 0 -a $count -ne 0])
> +])
> +
> +# OVN_CONTROLLER_EXPECT_HIT SANDBOXES COUNTER COMMAND
> +#
> +# Checks if the COUNTER value has changed for any of the ovn-controller
> +# processes from the SANDBOXES when the COMMAND was run.
> +m4_define([OVN_CONTROLLER_EXPECT_HIT],[
> +    CHECK_COUNTER_DELTA_IS_NOT_ZERO([$1], [ovn-controller], [$2], [$3])
> +])
> +
> +# OVN_CONTROLLER_EXPECT_NO_HIT SANDBOXES COUNTER COMMAND
> +#
> +# Checks if the COUNTER value has not changed for any of the
ovn-controller
> +# processes from the SANDBOXES when the COMMAND was run.
> +m4_define([OVN_CONTROLLER_EXPECT_NO_HIT],[
> +    CHECK_COUNTER_DELTA_IS_ZERO([$1], [ovn-controller], [$2], [$3])
> +])
> +
> +AT_SETUP([ovn -- ovn-controller incremental processing])
> +# Check which operations the trigger full logical flow processing.
> +#
> +# Create and destroy logical routers, switches, ports, address sets and
ACLs
> +# while counting calls to lflow_run() in ovn-controller.
> +
> +ovn_start
> +net_add n1
> +for i in 1 2; do
> +    sim_add hv$i
> +    as hv$i
> +    ovs-vsctl add-br br-phys
> +    ovn_attach n1 br-phys 192.168.0.$i
> +done
> +
> +# Add router lr1
> +OVN_CONTROLLER_EXPECT_HIT(
> +    [hv1 hv2], [lflow_run],
> +    [ovn-nbctl --wait=hv lr-add lr1]
> +)
> +
> +for i in 1 2; do
> +    ls=ls$i
> +    lsp=$ls-lr1
> +    lrp=lr1-$ls
> +
> +    # Add switch $ls
> +    OVN_CONTROLLER_EXPECT_HIT(
> +        [hv1 hv2], [lflow_run],
> +        [ovn-nbctl --wait=hv ls-add $ls]
> +    )
> +    OVN_CONTROLLER_EXPECT_NO_HIT(
> +        [hv1 hv2], [lflow_run],
> +        [ovn-nbctl --wait=hv add Logical_Switch $ls other_config
subnet=10.0.$i.0/24]
> +    )
> +
> +    # Add router port to $ls
> +    OVN_CONTROLLER_EXPECT_HIT(
> +        [hv1 hv2], [lflow_run],
> +        [ovn-nbctl --wait=hv lrp-add lr1 $lrp 02:00:00:00:0$i:01
10.0.$i.1/24]
> +    )
> +    OVN_CONTROLLER_EXPECT_NO_HIT(
> +        [hv1 hv2], [lflow_run],
> +        [ovn-nbctl --wait=hv lsp-add $ls $lsp]
> +    )
> +    OVN_CONTROLLER_EXPECT_HIT(
> +        [hv1 hv2], [lflow_run],
> +        [ovn-nbctl --wait=hv lsp-set-type $lsp router]
> +    )
> +    OVN_CONTROLLER_EXPECT_HIT(
> +        [hv1 hv2], [lflow_run],
> +        [ovn-nbctl --wait=hv lsp-set-addresses $lsp router]
> +    )
> +done
> +
> +for i in 1 2; do
> +    as=as$i
> +    hv=hv$i
> +    ls=ls$i
> +    lp=lp$i
> +    vif=vif$i
> +
> +    # Add port $lp
> +    OVN_CONTROLLER_EXPECT_NO_HIT(
> +        [hv1 hv2], [lflow_run],
> +        [ovn-nbctl --wait=hv lsp-add $ls $lp]
> +    )
> +    OVN_CONTROLLER_EXPECT_NO_HIT(
> +        [hv1 hv2], [lflow_run],
> +        [ovn-nbctl --wait=hv lsp-set-addresses $lp "dynamic"]
> +    )
> +    OVN_CONTROLLER_EXPECT_NO_HIT(
> +        [hv1 hv2], [lflow_run],
> +        [ovn-nbctl --wait=hv wait-until Logical_Switch_Port $lp
'dynamic_addresses!=[]']
> +    )
> +    OVN_CONTROLLER_EXPECT_NO_HIT(
> +        [hv1 hv2], [lflow_run],
> +        [ovn-nbctl --wait=hv get Logical_Switch_Port $lp
dynamic_addresses]
> +    )
> +
> +    # Add address set $as
> +    OVN_CONTROLLER_EXPECT_HIT(
> +        [hv1 hv2], [lflow_run],
> +        [ovn-nbctl --wait=hv create Address_Set name="$as"]
> +    )
> +    OVN_CONTROLLER_EXPECT_HIT(
> +        [hv1 hv2], [lflow_run],
> +        [ovn-nbctl --wait=hv add Address_Set "$as" addresses
"10.0.$i.10"]
> +    )
> +
> +    # Add ACLs for port $lp
> +    OVN_CONTROLLER_EXPECT_NO_HIT(
> +        [hv1 hv2], [lflow_run],
> +        [ovn-nbctl --wait=hv acl-add $ls to-lport 1001 'outport ==
"'$lp'" && ip4.src == $'$as' allow']
> +    )
> +    OVN_CONTROLLER_EXPECT_NO_HIT(
> +        [hv1 hv2], [lflow_run],
> +        [ovn-nbctl --wait=hv acl-add $ls to-lport 1000 'outport ==
"'$lp'" drop']
> +    )
> +
> +    # 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]
> +    )
> +
> +    # Wait for port $lp
> +    OVN_CONTROLLER_EXPECT_NO_HIT(
> +        [hv1 hv2], [lflow_run],
> +        [ovn-nbctl --wait=hv wait-until Logical_Switch_Port $lp
'up=true']
> +    )
> +done
> +
> +for i in 1 2; do
> +  as=as$i
> +  ls=ls$i
> +  lp=lp$i
> +
> +  # Delete port $lp
> +  OVN_CONTROLLER_EXPECT_HIT(
> +      [hv1 hv2], [lflow_run],
> +      [ovn-nbctl --wait=hv lsp-del $lp]
> +  )
> +
> +  # Delete ACLs for port $lp
> +  OVN_CONTROLLER_EXPECT_NO_HIT(
> +      [hv1 hv2], [lflow_run],
> +      [ovn-nbctl --wait=hv acl-del $ls to-lport 1001 'outport == "'$lp'"
&& ip4.src == $'$as' allow']
> +  )
> +  OVN_CONTROLLER_EXPECT_NO_HIT(
> +      [hv1 hv2], [lflow_run],
> +      [ovn-nbctl --wait=hv acl-del $ls to-lport 1000 'outport == "'$lp'"
drop']
> +  )
> +
> +  # Delete address set $as
> +  OVN_CONTROLLER_EXPECT_HIT(
> +      [hv1 hv2], [lflow_run],
> +      [ovn-nbctl --wait=hv remove Address_Set "$as" addresses
"10.0.$i.10"]
> +  )
> +  OVN_CONTROLLER_EXPECT_HIT(
> +      [hv1 hv2], [lflow_run],
> +      [ovn-nbctl --wait=hv destroy Address_Set "$as"]
> +  )
> +done
> +
> +for i in 1 2; do
> +  ls=ls$i
> +
> +  # Delete switch $ls
> +  OVN_CONTROLLER_EXPECT_HIT(
> +      [hv1 hv2], [lflow_run],
> +      [ovn-nbctl ls-del $ls]
> +  )
> +done
> +
> +# Delete router lr1
> +OVN_CONTROLLER_EXPECT_HIT(
> +    [hv1 hv2], [lflow_run],
> +    [ovn-nbctl lr-del lr1]
> +)
> +
> +OVN_CLEANUP([hv1], [hv2])
> +
> +AT_CLEANUP
> diff --git a/tests/testsuite.at b/tests/testsuite.at
> index 15c385e2c..c769770b1 100644
> --- a/tests/testsuite.at
> +++ b/tests/testsuite.at
> @@ -80,3 +80,4 @@ m4_include([tests/ovn-controller-vtep.at])
>  m4_include([tests/mcast-snooping.at])
>  m4_include([tests/packet-type-aware.at])
>  m4_include([tests/nsh.at])
> +m4_include([tests/ovn-performance.at])
> --
> 2.14.3
>

Acked-by: Han Zhou <hzhou8@ebay.com>
Jakub Sitnicki June 1, 2018, 1:49 p.m. | #2
On Thu, 31 May 2018 10:14:50 -0700
Han Zhou <zhouhan@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.

Thanks for the review,
-Jakub
Han Zhou June 1, 2018, 4:21 p.m. | #3
On Fri, Jun 1, 2018 at 6:49 AM, Jakub Sitnicki <jkbs@redhat.com> wrote:
>
> On Thu, 31 May 2018 10:14:50 -0700
> Han Zhou <zhouhan@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.

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.

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?

Thanks,
Han
Jakub Sitnicki June 1, 2018, 5:03 p.m. | #4
On Fri, 1 Jun 2018 09:21:28 -0700
Han Zhou <zhouhan@gmail.com> wrote:

> On Fri, Jun 1, 2018 at 6:49 AM, Jakub Sitnicki <jkbs@redhat.com> wrote:
> >
> > On Thu, 31 May 2018 10:14:50 -0700
> > Han Zhou <zhouhan@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. 
 
> 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.

Thanks,
Jakub
Han Zhou June 1, 2018, 5:50 p.m. | #5
On Fri, Jun 1, 2018 at 10:03 AM, Jakub Sitnicki <jkbs@redhat.com> wrote:
>
> On Fri, 1 Jun 2018 09:21:28 -0700
> Han Zhou <zhouhan@gmail.com> wrote:
>
> > On Fri, Jun 1, 2018 at 6:49 AM, Jakub Sitnicki <jkbs@redhat.com> wrote:
> > >
> > > On Thu, 31 May 2018 10:14:50 -0700
> > > Han Zhou <zhouhan@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?

>
> > 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.

Thanks,
Han
Jakub Sitnicki June 1, 2018, 6:08 p.m. | #6
On Fri, 1 Jun 2018 10:50:07 -0700
Han Zhou <zhouhan@gmail.com> wrote:

> On Fri, Jun 1, 2018 at 10:03 AM, Jakub Sitnicki <jkbs@redhat.com> wrote:
> >
> > On Fri, 1 Jun 2018 09:21:28 -0700
> > Han Zhou <zhouhan@gmail.com> wrote:
> >  
> > > On Fri, Jun 1, 2018 at 6:49 AM, Jakub Sitnicki <jkbs@redhat.com> wrote:  
> > > >
> > > > On Thu, 31 May 2018 10:14:50 -0700
> > > > Han Zhou <zhouhan@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

Patch

diff --git a/tests/automake.mk b/tests/automake.mk
index c420b29f3..1e009389c 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -105,7 +105,8 @@  TESTSUITE_AT = \
 	tests/ovn-controller-vtep.at \
 	tests/mcast-snooping.at \
 	tests/packet-type-aware.at \
-	tests/nsh.at
+	tests/nsh.at \
+	tests/ovn-performance.at
 
 SYSTEM_KMOD_TESTSUITE_AT = \
 	tests/system-common-macros.at \
diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
new file mode 100644
index 000000000..d2a795a15
--- /dev/null
+++ b/tests/ovn-performance.at
@@ -0,0 +1,287 @@ 
+#
+# Tests targeting performance of OVN components.
+#
+
+m4_divert_push([PREPARE_TESTS])
+
+# 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
+}
+
+# vec_sum VEC
+#
+# Sums elements of a vector:
+#
+#     VEC = [e1, e2, ...]
+#     OUT = e1 + e2 + ...
+#
+# VEC must be a list of values separated by a character from $IFS.
+vec_sum() {
+    local s=0
+
+    for a in $1; do
+        s=`expr $s + $a`
+    done
+    echo $s
+}
+
+# read_counters SANDBOXES APPLICATION COUNTER
+#
+# Prints out the coverage COUNTER for the APPLICATION in each of the SANDBOXES.
+#
+# SANDBOXES must be a list of strings separated by a character from $IFS.
+read_counters() {
+    local sims="$1" app="$2" counter="$3"
+
+    for sim in $sims; do
+        as $sim ovs-appctl -t "$app" coverage/read-count "$counter" || return 1
+    done
+}
+
+# counter_delta_per_sim SANDBOXES APPLICATION COUNTER COMMAND
+#
+# Runs the COMMAND and reports the COUNTER change registered during the command
+# run for the given APPLICATION in each of the SANDBOXES.
+counter_delta_per_sim() {
+    local sims="$1" app="$2" counter="$3" cmd="$4"
+    local before after diff
+
+    before=$(read_counters "$sims" "$app" "$counter") || return 1
+    $cmd || return 1
+    after=$(read_counters "$sims" "$app" "$counter") || return 1
+
+    vec_sub "$after" "$before"
+}
+
+# counter_delta_per_sim SANDBOXES APPLICATION COUNTER COMMAND
+#
+# Same as counter_delta_per_sim but reports the sum of COUNTERs across all
+# SANDBOXES.
+counter_delta_in_total() {
+    vec_sum "$(counter_delta_per_sim "$@")"
+}
+
+m4_divert_pop([PREPARE_TESTS])
+
+# CHECK_COUNTER_DELTA_IS_ZERO SANDBOXES APPLICATION COUNTER COMMAND
+#
+# Runs the COMMAND and checks if the COUNTER value for the APPLICATION in all of
+# the SANDBOXES did not change.
+m4_define([CHECK_COUNTER_DELTA_IS_ZERO],[
+    count=`counter_delta_in_total "$1" "$2" "$3" "$4"`
+    rc=$?
+    AT_CHECK([test $rc -eq 0 -a $count -eq 0])
+])
+
+# CHECK_COUNTER_DELTA_IS_NOT_ZERO SANDBOXES APPLICATION COUNTER COMMAND
+#
+# Runs the COMMAND and checks if there COUNTER value for the APPLICATION in
+# any of the SANDBOXES has changed.
+m4_define([CHECK_COUNTER_DELTA_IS_NOT_ZERO],[
+    count=`counter_delta_in_total "$1" "$2" "$3" "$4"`
+    rc=$?
+    AT_CHECK([test $rc -eq 0 -a $count -ne 0])
+])
+
+# OVN_CONTROLLER_EXPECT_HIT SANDBOXES COUNTER COMMAND
+#
+# Checks if the COUNTER value has changed for any of the ovn-controller
+# processes from the SANDBOXES when the COMMAND was run.
+m4_define([OVN_CONTROLLER_EXPECT_HIT],[
+    CHECK_COUNTER_DELTA_IS_NOT_ZERO([$1], [ovn-controller], [$2], [$3])
+])
+
+# OVN_CONTROLLER_EXPECT_NO_HIT SANDBOXES COUNTER COMMAND
+#
+# Checks if the COUNTER value has not changed for any of the ovn-controller
+# processes from the SANDBOXES when the COMMAND was run.
+m4_define([OVN_CONTROLLER_EXPECT_NO_HIT],[
+    CHECK_COUNTER_DELTA_IS_ZERO([$1], [ovn-controller], [$2], [$3])
+])
+
+AT_SETUP([ovn -- ovn-controller incremental processing])
+# Check which operations the trigger full logical flow processing.
+#
+# Create and destroy logical routers, switches, ports, address sets and ACLs
+# while counting calls to lflow_run() in ovn-controller.
+
+ovn_start
+net_add n1
+for i in 1 2; do
+    sim_add hv$i
+    as hv$i
+    ovs-vsctl add-br br-phys
+    ovn_attach n1 br-phys 192.168.0.$i
+done
+
+# Add router lr1
+OVN_CONTROLLER_EXPECT_HIT(
+    [hv1 hv2], [lflow_run],
+    [ovn-nbctl --wait=hv lr-add lr1]
+)
+
+for i in 1 2; do
+    ls=ls$i
+    lsp=$ls-lr1
+    lrp=lr1-$ls
+
+    # Add switch $ls
+    OVN_CONTROLLER_EXPECT_HIT(
+        [hv1 hv2], [lflow_run],
+        [ovn-nbctl --wait=hv ls-add $ls]
+    )
+    OVN_CONTROLLER_EXPECT_NO_HIT(
+        [hv1 hv2], [lflow_run],
+        [ovn-nbctl --wait=hv add Logical_Switch $ls other_config subnet=10.0.$i.0/24]
+    )
+
+    # Add router port to $ls
+    OVN_CONTROLLER_EXPECT_HIT(
+        [hv1 hv2], [lflow_run],
+        [ovn-nbctl --wait=hv lrp-add lr1 $lrp 02:00:00:00:0$i:01 10.0.$i.1/24]
+    )
+    OVN_CONTROLLER_EXPECT_NO_HIT(
+        [hv1 hv2], [lflow_run],
+        [ovn-nbctl --wait=hv lsp-add $ls $lsp]
+    )
+    OVN_CONTROLLER_EXPECT_HIT(
+        [hv1 hv2], [lflow_run],
+        [ovn-nbctl --wait=hv lsp-set-type $lsp router]
+    )
+    OVN_CONTROLLER_EXPECT_HIT(
+        [hv1 hv2], [lflow_run],
+        [ovn-nbctl --wait=hv lsp-set-addresses $lsp router]
+    )
+done
+
+for i in 1 2; do
+    as=as$i
+    hv=hv$i
+    ls=ls$i
+    lp=lp$i
+    vif=vif$i
+
+    # Add port $lp
+    OVN_CONTROLLER_EXPECT_NO_HIT(
+        [hv1 hv2], [lflow_run],
+        [ovn-nbctl --wait=hv lsp-add $ls $lp]
+    )
+    OVN_CONTROLLER_EXPECT_NO_HIT(
+        [hv1 hv2], [lflow_run],
+        [ovn-nbctl --wait=hv lsp-set-addresses $lp "dynamic"]
+    )
+    OVN_CONTROLLER_EXPECT_NO_HIT(
+        [hv1 hv2], [lflow_run],
+        [ovn-nbctl --wait=hv wait-until Logical_Switch_Port $lp 'dynamic_addresses!=[]']
+    )
+    OVN_CONTROLLER_EXPECT_NO_HIT(
+        [hv1 hv2], [lflow_run],
+        [ovn-nbctl --wait=hv get Logical_Switch_Port $lp dynamic_addresses]
+    )
+
+    # Add address set $as
+    OVN_CONTROLLER_EXPECT_HIT(
+        [hv1 hv2], [lflow_run],
+        [ovn-nbctl --wait=hv create Address_Set name="$as"]
+    )
+    OVN_CONTROLLER_EXPECT_HIT(
+        [hv1 hv2], [lflow_run],
+        [ovn-nbctl --wait=hv add Address_Set "$as" addresses "10.0.$i.10"]
+    )
+
+    # Add ACLs for port $lp
+    OVN_CONTROLLER_EXPECT_NO_HIT(
+        [hv1 hv2], [lflow_run],
+        [ovn-nbctl --wait=hv acl-add $ls to-lport 1001 'outport == "'$lp'" && ip4.src == $'$as' allow']
+    )
+    OVN_CONTROLLER_EXPECT_NO_HIT(
+        [hv1 hv2], [lflow_run],
+        [ovn-nbctl --wait=hv acl-add $ls to-lport 1000 'outport == "'$lp'" drop']
+    )
+
+    # Bind port $lp
+    OVN_CONTROLLER_EXPECT_HIT(
+        [hv1 hv2], [lflow_run],
+        [as $hv ovs-vsctl add-port br-int $vif -- set Interface $vif external-ids:iface-id=$lp]
+    )
+
+    # Wait for port $lp
+    OVN_CONTROLLER_EXPECT_NO_HIT(
+        [hv1 hv2], [lflow_run],
+        [ovn-nbctl --wait=hv wait-until Logical_Switch_Port $lp 'up=true']
+    )
+done
+
+for i in 1 2; do
+  as=as$i
+  ls=ls$i
+  lp=lp$i
+
+  # Delete port $lp
+  OVN_CONTROLLER_EXPECT_HIT(
+      [hv1 hv2], [lflow_run],
+      [ovn-nbctl --wait=hv lsp-del $lp]
+  )
+
+  # Delete ACLs for port $lp
+  OVN_CONTROLLER_EXPECT_NO_HIT(
+      [hv1 hv2], [lflow_run],
+      [ovn-nbctl --wait=hv acl-del $ls to-lport 1001 'outport == "'$lp'" && ip4.src == $'$as' allow']
+  )
+  OVN_CONTROLLER_EXPECT_NO_HIT(
+      [hv1 hv2], [lflow_run],
+      [ovn-nbctl --wait=hv acl-del $ls to-lport 1000 'outport == "'$lp'" drop']
+  )
+
+  # Delete address set $as
+  OVN_CONTROLLER_EXPECT_HIT(
+      [hv1 hv2], [lflow_run],
+      [ovn-nbctl --wait=hv remove Address_Set "$as" addresses "10.0.$i.10"]
+  )
+  OVN_CONTROLLER_EXPECT_HIT(
+      [hv1 hv2], [lflow_run],
+      [ovn-nbctl --wait=hv destroy Address_Set "$as"]
+  )
+done
+
+for i in 1 2; do
+  ls=ls$i
+
+  # Delete switch $ls
+  OVN_CONTROLLER_EXPECT_HIT(
+      [hv1 hv2], [lflow_run],
+      [ovn-nbctl ls-del $ls]
+  )
+done
+
+# Delete router lr1
+OVN_CONTROLLER_EXPECT_HIT(
+    [hv1 hv2], [lflow_run],
+    [ovn-nbctl lr-del lr1]
+)
+
+OVN_CLEANUP([hv1], [hv2])
+
+AT_CLEANUP
diff --git a/tests/testsuite.at b/tests/testsuite.at
index 15c385e2c..c769770b1 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -80,3 +80,4 @@  m4_include([tests/ovn-controller-vtep.at])
 m4_include([tests/mcast-snooping.at])
 m4_include([tests/packet-type-aware.at])
 m4_include([tests/nsh.at])
+m4_include([tests/ovn-performance.at])