| Message ID | 1425557551-14684-1-git-send-email-stuart.haslam@linaro.org |
|---|---|
| State | Accepted |
| Commit | be9fd23e6d4349347d9be48bbf42c008298a4f1f |
| Headers | show |
On Thu, Mar 05, 2015 at 12:12:31PM +0000, Stuart Haslam wrote: > There's a potential race condition whereby the test case could start > running before the virtual ethernet interfaces are fully brought up. > So replace the the arbitrary delay with a check for the interface's > operational state before kicking off the test. > Can someone review this please? > Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org> > --- > test/validation/odp_pktio_run | 32 ++++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) > > diff --git a/test/validation/odp_pktio_run b/test/validation/odp_pktio_run > index 08288e6..b051be1 100755 > --- a/test/validation/odp_pktio_run > +++ b/test/validation/odp_pktio_run > @@ -34,6 +34,28 @@ IF1=pktio-p1 > # exit codes expected by automake for skipped tests > TEST_SKIPPED=77 > > +# wait for a network interface's operational state to be "up" > +wait_for_iface_up() > +{ > + iface=$1 > + cnt=0 > + > + while [ $cnt -lt 50 ]; do > + read operstate < /sys/class/net/$iface/operstate > + > + if [ $? != 0 ]; then > + break > + elif [ "$operstate" = "up" ]; then > + return 0 > + fi > + > + sleep 0.1 > + cnt=$[$cnt+1] > + done > + > + return 1 > +} > + > setup_env1() > { > ip link show $IF0 2> /dev/null > @@ -59,8 +81,14 @@ setup_env1() > ip link set $IF0 up > ip link set $IF1 up > > - # network needs a little time to come up > - sleep 1 > + # check that the interface has come up before starting the test > + for iface in $IF0 $IF1; do > + wait_for_iface_up $iface > + if [ $? != 0 ]; then > + echo "pktio: interface $iface failed to come up" > + exit 1 > + fi > + done > } > > cleanup_env1() > -- > 2.1.1 >
I simultaneously ran two independent instances of odp with "make check"
and neither had an issue, one had this patch the other did not.
At the same time I used the ./build.sh script from the cleanup branch of
https://git.linaro.org/people/anders.roxell/check-odp.git and it was
pointed at a patched repo
It failed consistently as before
......
PASS: odp_errno
FAIL: odp_pktio_run
make[5]: Entering directory
'/home/mike/git/check-odp/build/odp/testdir/test/validation'
make[5]: Nothing to be done for 'all'.
make[5]: Leaving directory
'/home/mike/git/check-odp/build/odp/testdir/test/validation'
============================================================================
Testsuite summary for OpenDataPlane 1.0.0
============================================================================
# TOTAL: 18
# PASS: 17
# SKIP: 0
# XFAIL: 0
# FAIL: 1
# XPASS: 0
# ERROR: 0
I don't know why this should be because I ran my local instances with the
script
for i in {1..10}; do make clean check -e; done
and the build.sh uses
make check -e - but obviously it does this after building from scratch
downloading the repo etc each time so it does fewer iterations
So I think the new approach is a better idea, and it does not appear to
hurt, but it does not help the original use case of the scrip tin CI.
On 11 March 2015 at 10:22, Stuart Haslam <stuart.haslam@linaro.org> wrote:
> On Thu, Mar 05, 2015 at 12:12:31PM +0000, Stuart Haslam wrote:
> > There's a potential race condition whereby the test case could start
> > running before the virtual ethernet interfaces are fully brought up.
> > So replace the the arbitrary delay with a check for the interface's
> > operational state before kicking off the test.
> >
>
> Can someone review this please?
>
> > Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org>
> > ---
> > test/validation/odp_pktio_run | 32 ++++++++++++++++++++++++++++++--
> > 1 file changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/test/validation/odp_pktio_run
> b/test/validation/odp_pktio_run
> > index 08288e6..b051be1 100755
> > --- a/test/validation/odp_pktio_run
> > +++ b/test/validation/odp_pktio_run
> > @@ -34,6 +34,28 @@ IF1=pktio-p1
> > # exit codes expected by automake for skipped tests
> > TEST_SKIPPED=77
> >
> > +# wait for a network interface's operational state to be "up"
> > +wait_for_iface_up()
> > +{
> > + iface=$1
> > + cnt=0
> > +
> > + while [ $cnt -lt 50 ]; do
> > + read operstate < /sys/class/net/$iface/operstate
> > +
> > + if [ $? != 0 ]; then
> > + break
> > + elif [ "$operstate" = "up" ]; then
> > + return 0
> > + fi
> > +
> > + sleep 0.1
> > + cnt=$[$cnt+1]
> > + done
> > +
> > + return 1
> > +}
> > +
> > setup_env1()
> > {
> > ip link show $IF0 2> /dev/null
> > @@ -59,8 +81,14 @@ setup_env1()
> > ip link set $IF0 up
> > ip link set $IF1 up
> >
> > - # network needs a little time to come up
> > - sleep 1
> > + # check that the interface has come up before starting the test
> > + for iface in $IF0 $IF1; do
> > + wait_for_iface_up $iface
> > + if [ $? != 0 ]; then
> > + echo "pktio: interface $iface failed to come up"
> > + exit 1
> > + fi
> > + done
> > }
> >
> > cleanup_env1()
> > --
> > 2.1.1
> >
>
> --
> Stuart.
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
On 03/11/15 23:47, Mike Holmes wrote:
> for i in {1..10}; do make clean check -e; done
Mike do you run that under root or not? I.e. which pktio in test odp
loop or linux veth?
Maxim.
I never run it as root usually, but I did just now to check. Again build.sh had errors and running the for loop directly did not On 11 March 2015 at 17:43, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 03/11/15 23:47, Mike Holmes wrote: > >> for i in {1..10}; do make clean check -e; done >> > Mike do you run that under root or not? I.e. which pktio in test odp loop > or linux veth? > > Maxim. > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On Wed, Mar 11, 2015 at 04:47:59PM -0400, Mike Holmes wrote: > I simultaneously ran two independent instances of odp with "make check" > and neither had an issue, one had this patch the other did not. > > At the same time I used the ./build.sh script from the cleanup branch of > https://git.linaro.org/people/anders.roxell/check-odp.git and it was > pointed at a patched repo > It failed consistently as before Bah. I can reproduce that failure myself now. That problem is because build.sh runs ./configure && make && make check outside of the source tree, which only works if all of the executables required by "make check" are produced by "make", and that's not the case for odp_pktio_run. I'll have a think about the best way to resolve that. > ...... > > PASS: odp_errno > FAIL: odp_pktio_run > make[5]: Entering directory > '/home/mike/git/check-odp/build/odp/testdir/test/validation' > make[5]: Nothing to be done for 'all'. > make[5]: Leaving directory > '/home/mike/git/check-odp/build/odp/testdir/test/validation' > ============================================================================ > Testsuite summary for OpenDataPlane 1.0.0 > ============================================================================ > # TOTAL: 18 > # PASS: 17 > # SKIP: 0 > # XFAIL: 0 > # FAIL: 1 > # XPASS: 0 > # ERROR: 0 > > > I don't know why this should be because I ran my local instances with the > script > for i in {1..10}; do make clean check -e; done > and the build.sh uses > make check -e - but obviously it does this after building from scratch > downloading the repo etc each time so it does fewer iterations > > So I think the new approach is a better idea, and it does not appear to > hurt, but it does not help the original use case of the scrip tin CI. > I think this patch should still go in though.
On 12 March 2015 at 06:54, Stuart Haslam <stuart.haslam@linaro.org> wrote: > On Wed, Mar 11, 2015 at 04:47:59PM -0400, Mike Holmes wrote: > > I simultaneously ran two independent instances of odp with "make check" > > and neither had an issue, one had this patch the other did not. > > > > At the same time I used the ./build.sh script from the cleanup branch of > > https://git.linaro.org/people/anders.roxell/check-odp.git and it was > > pointed at a patched repo > > It failed consistently as before > > Bah. I can reproduce that failure myself now. > > That problem is because build.sh runs ./configure && make && make check > outside of the source tree, which only works if all of the executables > required by "make check" are produced by "make", and that's not the case > for odp_pktio_run. I'll have a think about the best way to resolve that. > > > ...... > > > > PASS: odp_errno > > FAIL: odp_pktio_run > > make[5]: Entering directory > > '/home/mike/git/check-odp/build/odp/testdir/test/validation' > > make[5]: Nothing to be done for 'all'. > > make[5]: Leaving directory > > '/home/mike/git/check-odp/build/odp/testdir/test/validation' > > > ============================================================================ > > Testsuite summary for OpenDataPlane 1.0.0 > > > ============================================================================ > > # TOTAL: 18 > > # PASS: 17 > > # SKIP: 0 > > # XFAIL: 0 > > # FAIL: 1 > > # XPASS: 0 > > # ERROR: 0 > > > > > > I don't know why this should be because I ran my local instances with the > > script > > for i in {1..10}; do make clean check -e; done > > and the build.sh uses > > make check -e - but obviously it does this after building from scratch > > downloading the repo etc each time so it does fewer iterations > > > > So I think the new approach is a better idea, and it does not appear to > > hurt, but it does not help the original use case of the scrip tin CI. > > > > I think this patch should still go in though. > I agree and you can add my Reviewed-and-tested-by: Mike Holmes < mike.holmes@linaro.org> > > -- > Stuart. >
On 03/11/15 17:22, Stuart Haslam wrote:
> + cnt=$[$cnt+1]
That is not portable, use expr.
./test/validation/odp_pktio_run: 43: [: Illegal number: $[0+1]
Maxim.
On Thu, Mar 12, 2015 at 10:54:49AM +0000, Stuart Haslam wrote: > On Wed, Mar 11, 2015 at 04:47:59PM -0400, Mike Holmes wrote: > > I simultaneously ran two independent instances of odp with "make check" > > and neither had an issue, one had this patch the other did not. > > > > At the same time I used the ./build.sh script from the cleanup branch of > > https://git.linaro.org/people/anders.roxell/check-odp.git and it was > > pointed at a patched repo > > It failed consistently as before > > Bah. I can reproduce that failure myself now. > > That problem is because build.sh runs ./configure && make && make check > outside of the source tree, which only works if all of the executables > required by "make check" are produced by "make", and that's not the case > for odp_pktio_run. I'll have a think about the best way to resolve that. > ./build.sh passes with this patch applied; https://patches.linaro.org/45829/ I was initially confused as to why odp_scheduling_run didn't suffer the same problem, but found that it does and just failed to report it, hence the other 2 patches.
On 03/16/15 14:54, Stuart Haslam wrote: > On Thu, Mar 12, 2015 at 10:54:49AM +0000, Stuart Haslam wrote: >> On Wed, Mar 11, 2015 at 04:47:59PM -0400, Mike Holmes wrote: >>> I simultaneously ran two independent instances of odp with "make check" >>> and neither had an issue, one had this patch the other did not. >>> >>> At the same time I used the ./build.sh script from the cleanup branch of >>> https://git.linaro.org/people/anders.roxell/check-odp.git and it was >>> pointed at a patched repo >>> It failed consistently as before >> Bah. I can reproduce that failure myself now. >> >> That problem is because build.sh runs ./configure && make && make check >> outside of the source tree, which only works if all of the executables >> required by "make check" are produced by "make", and that's not the case >> for odp_pktio_run. I'll have a think about the best way to resolve that. >> > ./build.sh passes with this patch applied; > > https://patches.linaro.org/45829/ > > I was initially confused as to why odp_scheduling_run didn't suffer the > same problem, but found that it does and just failed to report it, hence > the other 2 patches. > So that you need to fix value calculation and send v2. Maxim.
On Mon, Mar 16, 2015 at 03:04:42PM +0300, Maxim Uvarov wrote: > On 03/16/15 14:54, Stuart Haslam wrote: > >On Thu, Mar 12, 2015 at 10:54:49AM +0000, Stuart Haslam wrote: > >>On Wed, Mar 11, 2015 at 04:47:59PM -0400, Mike Holmes wrote: > >>>I simultaneously ran two independent instances of odp with "make check" > >>>and neither had an issue, one had this patch the other did not. > >>> > >>>At the same time I used the ./build.sh script from the cleanup branch of > >>>https://git.linaro.org/people/anders.roxell/check-odp.git and it was > >>>pointed at a patched repo > >>>It failed consistently as before > >>Bah. I can reproduce that failure myself now. > >> > >>That problem is because build.sh runs ./configure && make && make check > >>outside of the source tree, which only works if all of the executables > >>required by "make check" are produced by "make", and that's not the case > >>for odp_pktio_run. I'll have a think about the best way to resolve that. > >> > >./build.sh passes with this patch applied; > > > >https://patches.linaro.org/45829/ > > > >I was initially confused as to why odp_scheduling_run didn't suffer the > >same problem, but found that it does and just failed to report it, hence > >the other 2 patches. > > > So that you need to fix value calculation and send v2. > > Maxim. > Yes, but that's a separate issue, I'll send a v2 of that patch shortly.
diff --git a/test/validation/odp_pktio_run b/test/validation/odp_pktio_run index 08288e6..b051be1 100755 --- a/test/validation/odp_pktio_run +++ b/test/validation/odp_pktio_run @@ -34,6 +34,28 @@ IF1=pktio-p1 # exit codes expected by automake for skipped tests TEST_SKIPPED=77 +# wait for a network interface's operational state to be "up" +wait_for_iface_up() +{ + iface=$1 + cnt=0 + + while [ $cnt -lt 50 ]; do + read operstate < /sys/class/net/$iface/operstate + + if [ $? != 0 ]; then + break + elif [ "$operstate" = "up" ]; then + return 0 + fi + + sleep 0.1 + cnt=$[$cnt+1] + done + + return 1 +} + setup_env1() { ip link show $IF0 2> /dev/null @@ -59,8 +81,14 @@ setup_env1() ip link set $IF0 up ip link set $IF1 up - # network needs a little time to come up - sleep 1 + # check that the interface has come up before starting the test + for iface in $IF0 $IF1; do + wait_for_iface_up $iface + if [ $? != 0 ]; then + echo "pktio: interface $iface failed to come up" + exit 1 + fi + done } cleanup_env1()
There's a potential race condition whereby the test case could start running before the virtual ethernet interfaces are fully brought up. So replace the the arbitrary delay with a check for the interface's operational state before kicking off the test. Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org> --- test/validation/odp_pktio_run | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-)