| Message ID | 20180531104111.15790-3-jkbs@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ovn: Check for effects of incremental processing | expand |
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>
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
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
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
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
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
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])
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