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? 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
