| Message ID | 20221202135137.1728564-11-xsimonar@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Fixes to multiple Unit and System Tests | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Hi Xavier. I think I may be a bit "slow" myself. It's not clear why these changes help slower systems. My guess is that `xxd` is faster than running `printf` in a loop, maybe? If that's the case, then is there a chance that you could see the same issue on a slow system that doesn't have xxd? And why does the sandbox cleanup change help? On 12/2/22 08:51, Xavier Simonart wrote: > When running unit tests on a s390x VM with a x86 host, > many tests fail due to the very slow speed of the system. > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > --- > tests/atlocal.in | 3 +++ > tests/network-functions.at | 18 ++++++++++++------ > tests/ovn.at | 7 ++++++- > 3 files changed, 21 insertions(+), 7 deletions(-) > > diff --git a/tests/atlocal.in b/tests/atlocal.in > index 0b9a31276..02e9ce9bb 100644 > --- a/tests/atlocal.in > +++ b/tests/atlocal.in > @@ -166,6 +166,9 @@ fi > # Set HAVE_TCPDUMP > find_command tcpdump > > +# Set HAVE_XXD > +find_command xxd > + > # Set HAVE_LFTP > find_command lftp > > diff --git a/tests/network-functions.at b/tests/network-functions.at > index c583bc31e..a2481c55c 100644 > --- a/tests/network-functions.at > +++ b/tests/network-functions.at > @@ -128,12 +128,18 @@ OVS_START_SHELL_HELPERS > # hex_to_binary HEXDIGITS > # > # Converts the pairs of HEXDIGITS into bytes and prints them on stdout. > -hex_to_binary() { > - printf $(while test -n "$1"; do > - printf '\\%03o' 0x$(expr "$1" : '\(..\)') > - set -- "${1##??}" > - done) > -} > +if test x$HAVE_XXD = xno; then > + hex_to_binary() { > + printf $(while test -n "$1"; do > + printf '\\%03o' 0x$(expr "$1" : '\(..\)') > + set -- "${1##??}" > + done) > + } > +else > + hex_to_binary() { > + echo $1 | xxd -r -p > + } > +fi > > # tcpdump_hex TITLE PACKET > # > diff --git a/tests/ovn.at b/tests/ovn.at > index dcc74649e..11f4cf2e6 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -4420,7 +4420,12 @@ for i in 1 2 3; do > done > > # Gracefully terminate daemons > -OVN_CLEANUP([hv1],[hv2],[vtep]) > + > +OVN_CLEANUP_SBOX([hv1]) > +OVN_CLEANUP_SBOX([hv2]) > +OVS_WAIT_UNTIL([test `as vtep ovs-vsctl list-ports vtep_bfd | wc -l` -eq 0]) > +OVN_CLEANUP([vtep]) > + > OVN_CLEANUP_VSWITCH([hv3]) > > AT_CLEANUP
Hi Mark On Mon, Dec 12, 2022 at 10:33 PM Mark Michelson <mmichels@redhat.com> wrote: > Hi Xavier. I think I may be a bit "slow" myself. It's not clear why > these changes help slower systems. > > My guess is that `xxd` is faster than running `printf` in a loop, maybe? > On the system I was using (a s390 VM running on a x86_64 host ) each iteration of the loop took around 0.3 sec IIRC. So, a 100 bytes packet was taking 30 seconds. I do not exactly remember xxd speed, but it was probably 0.3 sec for the whole loop. If that's the case, then is there a chance that you could see the same > issue on a slow system that doesn't have xxd? > > Definitely. The fix uses xxd. And makes sure that, if xxd is not installed, that it runs as before. In other words, I do not have a solution for a slow system w/o xxd. > And why does the sandbox cleanup change help? > Cleanup was timeouting while exiting vtep (vtep is already quite slow on a normal system, so it is not a surprise to see some timeout here). By splitting OVN_CLEANUP in pieces, and adding OVS_WAIT_UNTIL, we give much more time to vtep to properly stop. Another way to fix this, more general (and more readable), would be to add a OVS_EXIT_TIMEOUT (in a similar way as OVS_CTL_TIMEOUT) so that we can configure the timeout used in OVN_CLEANUP. What do you think? Thanks Xavier > > On 12/2/22 08:51, Xavier Simonart wrote: > > When running unit tests on a s390x VM with a x86 host, > > many tests fail due to the very slow speed of the system. > > > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > > --- > > tests/atlocal.in | 3 +++ > > tests/network-functions.at | 18 ++++++++++++------ > > tests/ovn.at | 7 ++++++- > > 3 files changed, 21 insertions(+), 7 deletions(-) > > > > diff --git a/tests/atlocal.in b/tests/atlocal.in > > index 0b9a31276..02e9ce9bb 100644 > > --- a/tests/atlocal.in > > +++ b/tests/atlocal.in > > @@ -166,6 +166,9 @@ fi > > # Set HAVE_TCPDUMP > > find_command tcpdump > > > > +# Set HAVE_XXD > > +find_command xxd > > + > > # Set HAVE_LFTP > > find_command lftp > > > > diff --git a/tests/network-functions.at b/tests/network-functions.at > > index c583bc31e..a2481c55c 100644 > > --- a/tests/network-functions.at > > +++ b/tests/network-functions.at > > @@ -128,12 +128,18 @@ OVS_START_SHELL_HELPERS > > # hex_to_binary HEXDIGITS > > # > > # Converts the pairs of HEXDIGITS into bytes and prints them on stdout. > > -hex_to_binary() { > > - printf $(while test -n "$1"; do > > - printf '\\%03o' 0x$(expr "$1" : '\(..\)') > > - set -- "${1##??}" > > - done) > > -} > > +if test x$HAVE_XXD = xno; then > > + hex_to_binary() { > > + printf $(while test -n "$1"; do > > + printf '\\%03o' 0x$(expr "$1" : '\(..\)') > > + set -- "${1##??}" > > + done) > > + } > > +else > > + hex_to_binary() { > > + echo $1 | xxd -r -p > > + } > > +fi > > > > # tcpdump_hex TITLE PACKET > > # > > diff --git a/tests/ovn.at b/tests/ovn.at > > index dcc74649e..11f4cf2e6 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -4420,7 +4420,12 @@ for i in 1 2 3; do > > done > > > > # Gracefully terminate daemons > > -OVN_CLEANUP([hv1],[hv2],[vtep]) > > + > > +OVN_CLEANUP_SBOX([hv1]) > > +OVN_CLEANUP_SBOX([hv2]) > > +OVS_WAIT_UNTIL([test `as vtep ovs-vsctl list-ports vtep_bfd | wc -l` > -eq 0]) > > +OVN_CLEANUP([vtep]) > > + > > OVN_CLEANUP_VSWITCH([hv3]) > > > > AT_CLEANUP > >
On 12/14/22 03:18, Xavier Simonart wrote: > Hi Mark > > > On Mon, Dec 12, 2022 at 10:33 PM Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> wrote: > > Hi Xavier. I think I may be a bit "slow" myself. It's not clear why > these changes help slower systems. > > My guess is that `xxd` is faster than running `printf` in a loop, > maybe? > > On the system I was using (a s390 VM running on a x86_64 host ) each > iteration of the loop took around 0.3 sec IIRC. > So, a 100 bytes packet was taking 30 seconds. > I do not exactly remember xxd speed, but it was probably 0.3 sec for the > whole loop. > > If that's the case, then is there a chance that you could see the same > issue on a slow system that doesn't have xxd? > > Definitely. The fix uses xxd. And makes sure that, if xxd is not > installed, that it runs as before. In other words, I do not have a > solution for a slow system w/o xxd. > > And why does the sandbox cleanup change help? > > Cleanup was timeouting while exiting vtep (vtep is already quite slow on > a normal system, so it is not a surprise to see some timeout here). > By splitting OVN_CLEANUP in pieces, and adding OVS_WAIT_UNTIL, we give > much more time to vtep to properly stop. Thanks for the explanations, Xavier. Acked-by: Mark Michelson > Another way to fix this, more general (and more readable), would be to > add a OVS_EXIT_TIMEOUT (in a similar way as OVS_CTL_TIMEOUT) so that we > can configure the timeout used in OVN_CLEANUP. > What do you think? That sounds like a good idea, but also not something that should block this change. If you want to add something like that as a future improvement, feel free. > > Thanks > Xavier > > > On 12/2/22 08:51, Xavier Simonart wrote: > > When running unit tests on a s390x VM with a x86 host, > > many tests fail due to the very slow speed of the system. > > > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com > <mailto:xsimonar@redhat.com>> > > --- > > tests/atlocal.in <http://atlocal.in> | 3 +++ > > tests/network-functions.at <http://network-functions.at> | 18 > ++++++++++++------ > > tests/ovn.at <http://ovn.at> | 7 ++++++- > > 3 files changed, 21 insertions(+), 7 deletions(-) > > > > diff --git a/tests/atlocal.in <http://atlocal.in> > b/tests/atlocal.in <http://atlocal.in> > > index 0b9a31276..02e9ce9bb 100644 > > --- a/tests/atlocal.in <http://atlocal.in> > > +++ b/tests/atlocal.in <http://atlocal.in> > > @@ -166,6 +166,9 @@ fi > > # Set HAVE_TCPDUMP > > find_command tcpdump > > > > +# Set HAVE_XXD > > +find_command xxd > > + > > # Set HAVE_LFTP > > find_command lftp > > > > diff --git a/tests/network-functions.at > <http://network-functions.at> b/tests/network-functions.at > <http://network-functions.at> > > index c583bc31e..a2481c55c 100644 > > --- a/tests/network-functions.at <http://network-functions.at> > > +++ b/tests/network-functions.at <http://network-functions.at> > > @@ -128,12 +128,18 @@ OVS_START_SHELL_HELPERS > > # hex_to_binary HEXDIGITS > > # > > # Converts the pairs of HEXDIGITS into bytes and prints them on > stdout. > > -hex_to_binary() { > > - printf $(while test -n "$1"; do > > - printf '\\%03o' 0x$(expr "$1" : '\(..\)') > > - set -- "${1##??}" > > - done) > > -} > > +if test x$HAVE_XXD = xno; then > > + hex_to_binary() { > > + printf $(while test -n "$1"; do > > + printf '\\%03o' 0x$(expr "$1" : '\(..\)') > > + set -- "${1##??}" > > + done) > > + } > > +else > > + hex_to_binary() { > > + echo $1 | xxd -r -p > > + } > > +fi > > > > # tcpdump_hex TITLE PACKET > > # > > diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at > <http://ovn.at> > > index dcc74649e..11f4cf2e6 100644 > > --- a/tests/ovn.at <http://ovn.at> > > +++ b/tests/ovn.at <http://ovn.at> > > @@ -4420,7 +4420,12 @@ for i in 1 2 3; do > > done > > > > # Gracefully terminate daemons > > -OVN_CLEANUP([hv1],[hv2],[vtep]) > > + > > +OVN_CLEANUP_SBOX([hv1]) > > +OVN_CLEANUP_SBOX([hv2]) > > +OVS_WAIT_UNTIL([test `as vtep ovs-vsctl list-ports vtep_bfd | wc > -l` -eq 0]) > > +OVN_CLEANUP([vtep]) > > + > > OVN_CLEANUP_VSWITCH([hv3]) > > > > AT_CLEANUP >
On Fri, Dec 2, 2022 at 2:53 PM Xavier Simonart <xsimonar@redhat.com> wrote: > When running unit tests on a s390x VM with a x86 host, > many tests fail due to the very slow speed of the system. > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > --- > tests/atlocal.in | 3 +++ > tests/network-functions.at | 18 ++++++++++++------ > tests/ovn.at | 7 ++++++- > 3 files changed, 21 insertions(+), 7 deletions(-) > > diff --git a/tests/atlocal.in b/tests/atlocal.in > index 0b9a31276..02e9ce9bb 100644 > --- a/tests/atlocal.in > +++ b/tests/atlocal.in > @@ -166,6 +166,9 @@ fi > # Set HAVE_TCPDUMP > find_command tcpdump > > +# Set HAVE_XXD > +find_command xxd > + > # Set HAVE_LFTP > find_command lftp > > diff --git a/tests/network-functions.at b/tests/network-functions.at > index c583bc31e..a2481c55c 100644 > --- a/tests/network-functions.at > +++ b/tests/network-functions.at > @@ -128,12 +128,18 @@ OVS_START_SHELL_HELPERS > # hex_to_binary HEXDIGITS > # > # Converts the pairs of HEXDIGITS into bytes and prints them on stdout. > -hex_to_binary() { > - printf $(while test -n "$1"; do > - printf '\\%03o' 0x$(expr "$1" : '\(..\)') > - set -- "${1##??}" > - done) > -} > +if test x$HAVE_XXD = xno; then > + hex_to_binary() { > + printf $(while test -n "$1"; do > + printf '\\%03o' 0x$(expr "$1" : '\(..\)') > + set -- "${1##??}" > + done) > + } > +else > + hex_to_binary() { > + echo $1 | xxd -r -p > + } > +fi > > # tcpdump_hex TITLE PACKET > # > diff --git a/tests/ovn.at b/tests/ovn.at > index dcc74649e..11f4cf2e6 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -4420,7 +4420,12 @@ for i in 1 2 3; do > done > > # Gracefully terminate daemons > -OVN_CLEANUP([hv1],[hv2],[vtep]) > + > +OVN_CLEANUP_SBOX([hv1]) > +OVN_CLEANUP_SBOX([hv2]) > +OVS_WAIT_UNTIL([test `as vtep ovs-vsctl list-ports vtep_bfd | wc -l` -eq > 0]) > +OVN_CLEANUP([vtep]) > + > OVN_CLEANUP_VSWITCH([hv3]) > > AT_CLEANUP > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
I pushed this change to main and all branches from 22.03 up to 23.03. On 2/10/23 02:58, Ales Musil wrote: > On Fri, Dec 2, 2022 at 2:53 PM Xavier Simonart <xsimonar@redhat.com> wrote: > >> When running unit tests on a s390x VM with a x86 host, >> many tests fail due to the very slow speed of the system. >> >> Signed-off-by: Xavier Simonart <xsimonar@redhat.com> >> --- >> tests/atlocal.in | 3 +++ >> tests/network-functions.at | 18 ++++++++++++------ >> tests/ovn.at | 7 ++++++- >> 3 files changed, 21 insertions(+), 7 deletions(-) >> >> diff --git a/tests/atlocal.in b/tests/atlocal.in >> index 0b9a31276..02e9ce9bb 100644 >> --- a/tests/atlocal.in >> +++ b/tests/atlocal.in >> @@ -166,6 +166,9 @@ fi >> # Set HAVE_TCPDUMP >> find_command tcpdump >> >> +# Set HAVE_XXD >> +find_command xxd >> + >> # Set HAVE_LFTP >> find_command lftp >> >> diff --git a/tests/network-functions.at b/tests/network-functions.at >> index c583bc31e..a2481c55c 100644 >> --- a/tests/network-functions.at >> +++ b/tests/network-functions.at >> @@ -128,12 +128,18 @@ OVS_START_SHELL_HELPERS >> # hex_to_binary HEXDIGITS >> # >> # Converts the pairs of HEXDIGITS into bytes and prints them on stdout. >> -hex_to_binary() { >> - printf $(while test -n "$1"; do >> - printf '\\%03o' 0x$(expr "$1" : '\(..\)') >> - set -- "${1##??}" >> - done) >> -} >> +if test x$HAVE_XXD = xno; then >> + hex_to_binary() { >> + printf $(while test -n "$1"; do >> + printf '\\%03o' 0x$(expr "$1" : '\(..\)') >> + set -- "${1##??}" >> + done) >> + } >> +else >> + hex_to_binary() { >> + echo $1 | xxd -r -p >> + } >> +fi >> >> # tcpdump_hex TITLE PACKET >> # >> diff --git a/tests/ovn.at b/tests/ovn.at >> index dcc74649e..11f4cf2e6 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -4420,7 +4420,12 @@ for i in 1 2 3; do >> done >> >> # Gracefully terminate daemons >> -OVN_CLEANUP([hv1],[hv2],[vtep]) >> + >> +OVN_CLEANUP_SBOX([hv1]) >> +OVN_CLEANUP_SBOX([hv2]) >> +OVS_WAIT_UNTIL([test `as vtep ovs-vsctl list-ports vtep_bfd | wc -l` -eq >> 0]) >> +OVN_CLEANUP([vtep]) >> + >> OVN_CLEANUP_VSWITCH([hv3]) >> >> AT_CLEANUP >> -- >> 2.31.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Looks good to me, thanks. > > Acked-by: Ales Musil <amusil@redhat.com> >
diff --git a/tests/atlocal.in b/tests/atlocal.in index 0b9a31276..02e9ce9bb 100644 --- a/tests/atlocal.in +++ b/tests/atlocal.in @@ -166,6 +166,9 @@ fi # Set HAVE_TCPDUMP find_command tcpdump +# Set HAVE_XXD +find_command xxd + # Set HAVE_LFTP find_command lftp diff --git a/tests/network-functions.at b/tests/network-functions.at index c583bc31e..a2481c55c 100644 --- a/tests/network-functions.at +++ b/tests/network-functions.at @@ -128,12 +128,18 @@ OVS_START_SHELL_HELPERS # hex_to_binary HEXDIGITS # # Converts the pairs of HEXDIGITS into bytes and prints them on stdout. -hex_to_binary() { - printf $(while test -n "$1"; do - printf '\\%03o' 0x$(expr "$1" : '\(..\)') - set -- "${1##??}" - done) -} +if test x$HAVE_XXD = xno; then + hex_to_binary() { + printf $(while test -n "$1"; do + printf '\\%03o' 0x$(expr "$1" : '\(..\)') + set -- "${1##??}" + done) + } +else + hex_to_binary() { + echo $1 | xxd -r -p + } +fi # tcpdump_hex TITLE PACKET # diff --git a/tests/ovn.at b/tests/ovn.at index dcc74649e..11f4cf2e6 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -4420,7 +4420,12 @@ for i in 1 2 3; do done # Gracefully terminate daemons -OVN_CLEANUP([hv1],[hv2],[vtep]) + +OVN_CLEANUP_SBOX([hv1]) +OVN_CLEANUP_SBOX([hv2]) +OVS_WAIT_UNTIL([test `as vtep ovs-vsctl list-ports vtep_bfd | wc -l` -eq 0]) +OVN_CLEANUP([vtep]) + OVN_CLEANUP_VSWITCH([hv3]) AT_CLEANUP
When running unit tests on a s390x VM with a x86 host, many tests fail due to the very slow speed of the system. Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- tests/atlocal.in | 3 +++ tests/network-functions.at | 18 ++++++++++++------ tests/ovn.at | 7 ++++++- 3 files changed, 21 insertions(+), 7 deletions(-)