----- Original Message ----- > From: "Joe Stringer" <j...@ovn.org> > To: "Lance Richardson" <lrich...@redhat.com> > Cc: dev@openvswitch.org, "Paul Boca" <pb...@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 <pb...@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:j...@ovn.org] > >> Sent: Thursday, June 2, 2016 4:53 AM > >> To: Paul Boca > >> Cc: dev@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 <pb...@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 <pb...@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])]) > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
