[ovs-dev] [PATCH V2 4/4] tests: Fix fail of OVS_APP_EXIT_AND_WAIT on Windows

Fri Jun 3 02:07:50 UTC 2016

----- Original Message -----
> From: "Joe Stringer" <joe at ovn.org>
> To: "Lance Richardson" <lrichard at redhat.com>
> Cc: dev at openvswitch.org, "Paul Boca" <pboca at cloudbasesolutions.com>
> Sent: Thursday, June 2, 2016 9:41:23 PM
> Subject: Re: [ovs-dev] [PATCH V2 4/4] tests: Fix fail of OVS_APP_EXIT_AND_WAIT on Windows
> 
> On 2 June 2016 at 01:28, Paul Boca <pboca at cloudbasesolutions.com> wrote:
> > Hi!
> >
> > Thanks for review!
> > Please see my comments inline.
> 
> Thanks for looking into the windows testsuite failures :-)
> 
> More below.
> 
> >> -----Original Message-----
> >> From: Joe Stringer [mailto:joe at ovn.org]
> >> Sent: Thursday, June 2, 2016 4:53 AM
> >> To: Paul Boca
> >> Cc: dev at openvswitch.org
> >> Subject: Re: [ovs-dev] [PATCH V2 4/4] tests: Fix fail of
> >> OVS_APP_EXIT_AND_WAIT on Windows
> >>
> >> On 1 June 2016 at 04:46, Paul Boca <pboca at cloudbasesolutions.com> wrote:
> >> > The problem is that on some cases it gets called with the socket
> >> > name instead of the service name.
> >> >
> >> > Signed-off-by: Paul-Daniel Boca <pboca at cloudbasesolutions.com>
> >>
> >> Looking at commit 0c473314294930a47a18d380e0bbcdf7b02a16f2 which
> >> introduced this macro, it seems like simply skipping these pieces
> >> could cause some tests to intermittently fail, because "ovs-appctl ...
> >> exit" does not wait until the process has terminated; it simply tells
> >> the process to terminate itself, then returns.
> > [Paul Boca] The problem with OVS_APP_EXIT_AND_WAIT is that it gets called
> > with "`pwd`"/unixctl,
> > the macro tries to get the pid from the wrong file "$1.pid", this expands
> > to "`pwd`"/unixctl.pid
> > which doesn't exist. Also the macro tries to wait for the pid inside
> > "`pwd`"/unixctl.pid.
> > I'm looking at commit d9c8c57c48a22b145cf1b5e78839ac0a16e039e9 which
> > replaces
> > "ovs-appctl -t `pwd`/unixctl exit" with
> > OVS_APP_EXIT_AND_WAIT([`pwd`/unixctl]) which it
> > doesn't do the right thing in this case, for other situations this works
> > well.
> >
> > Here you can see one of the unixctl used:
> > https://github.com/openvswitch/ovs/commit/d9c8c57c48a22b145cf1b5e78839ac0a16e039e9
> > #diff-b878790402a4b86a273e4b8c75b9bea6R86
> >
> >>
> >> What needs to happen differently on windows between validating that a
> >> process has exited vs. a service has exited?
> > [Paul Boca] The macro does the right thing except the case above.
> > If a pid file name is sent to the macro, with the previous version, it
> > works well.
> 
> Bear with me, I'm a little confused - firstly, about why the TMPPID
> stuff is supposed to work at all, and secondly why the behaviour
> differs between platforms.
> 
> One one hand, if the user of OVS_APP_EXIT_AND_WAIT() passes
> `pwd`/unixctl, wouldn't the path then become
> $OVS_RUNDIR/`pwd`/unixctl.pid ? That doesn't seem like a valid path to
> read to acquire a PID. In fact, when I removed the "2>/dev/null" from
> the TMPPID line, I found some tests that would quietly log that the
> file doesn't exist - and rightly so. This suggests that the
> $OVS_RUNDIR part of the cat command should be removed.
> 
> On the other hand, if that command returned effectively an empty
> string, then I'd expect the kill command would become something like
> "kill -0", which should print the usage and return a non-zero exit
> code. Because the exit code is nonzero, the OVS_WAIT_WHILE should exit
> immediately, so the test should continue. But this doesn't seem to be
> the behaviour on windows? Maybe one of my assumptions here is a bit
> off.
> 
> Lance, it seems like the "$OVS_RUNDIR" part of these commands was
> introduced by some of the changes you made around gcov support,
> specifically commits d9c8c57c48a2 ("tests: consistently use
> OVS_APP_EXIT_AND_WAIT() for daemon termination") and f9b11f2a09b4
> ("tests: Make OVS_APP_EXIT_AND_WAIT() wait for process termination").
> As far as I can tell, they're broken for a bunch of the tests. I
> started investigating by applying the diff below, and I can see that
> there's a lot of failures locating the pidfiles. Would you be able to
> take a look?
>

Sure, I'll take a look in the morning and report back.

   Lance

> diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at
> index e5710a07c192..2411fab2bb3f 100644
> --- a/tests/ovs-macros.at
> +++ b/tests/ovs-macros.at
> @@ -131,11 +131,18 @@ m4_define([OVS_WAIT_WHILE],
>   [OVS_WAIT([if $1; then return 1; else return 0; fi], [$2])])
> 
> dnl OVS_APP_EXIT_AND_WAIT(DAEMON)
> dnl
> dnl Ask the daemon named DAEMON to exit, via ovs-appctl, and then waits for
> it
> dnl to exit.
> m4_define([OVS_APP_EXIT_AND_WAIT],
> -  [TMPPID=$(cat "$OVS_RUNDIR"/$1.pid 2>/dev/null)
> +  [pidfile="$1.pid"
> +   AT_CHECK([test -e $pidfile])
> +   TMPPID=$(cat $pidfile)
>    AT_CHECK([ovs-appctl -t $1 exit])
>    OVS_WAIT_WHILE([kill -0 $TMPPID 2>/dev/null])])
> 
> @@ -144,7 +151,9 @@ dnl
> dnl Ask the daemon named DAEMON to exit, via ovs-appctl (using the target
> dnl argument), and then waits for it to exit.
> m4_define([OVS_APP_EXIT_AND_WAIT_BY_TARGET],
> -  [TMPPID=`cat "$OVS_RUNDIR"/$1.pid 2>/dev/null`
> +  [pidfile="$OVS_RUNDIR"/$1.pid
> +   AT_CHECK([test -e $pidfile])
> +   TMPPID=$(cat "$pidfile")
>    AT_CHECK([ovs-appctl --target=$OVS_RUNDIR/ovsdb-server.$TMPPID.ctl exit])
>    OVS_WAIT_WHILE([kill -0 $TMPPID 2>/dev/null])])
> 