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

Lance Richardson lrichard at redhat.com
Fri Jun 3 18:51:54 UTC 2016



----- Original Message -----
> From: "Ben Pfaff" <blp at ovn.org>
> To: "Lance Richardson" <lrichard at redhat.com>
> Cc: "Joe Stringer" <joe at ovn.org>, dev at openvswitch.org, "Paul Boca" <pboca at cloudbasesolutions.com>
> Sent: Friday, June 3, 2016 2:31:30 PM
> Subject: Re: [ovs-dev] [PATCH V2 4/4] tests: Fix fail of OVS_APP_EXIT_AND_WAIT on Windows
> 
> On Fri, Jun 03, 2016 at 12:49:54PM -0400, Lance Richardson wrote:
> > ----- Original Message -----
> > > From: "Ben Pfaff" <blp at ovn.org>
> > > To: "Lance Richardson" <lrichard at redhat.com>
> > > Cc: "Joe Stringer" <joe at ovn.org>, dev at openvswitch.org, "Paul Boca"
> > > <pboca at cloudbasesolutions.com>
> > > Sent: Friday, June 3, 2016 12:38:56 PM
> > > Subject: Re: [ovs-dev] [PATCH V2 4/4] tests: Fix fail of
> > > OVS_APP_EXIT_AND_WAIT on Windows
> > > 
> > > On Fri, Jun 03, 2016 at 12:00:11PM -0400, Lance Richardson wrote:
> > > > OK, it appears that I was probably a little too focused on the ~250
> > > > locations where OVS_APP_EXIT_AND_WAIT() is invoked with a daemon
> > > > name (where the TMPPID stuff works fine) and not enough to the ~8
> > > > locations where it's invoked with the path of a unixctl socket.
> > > > 
> > > > Ben's suggestion to have the Windows-specfic kill() function emulate
> > > > the kill command's handling of "kill -0" should make this macro
> > > > behave as it does on other systems, but I'm wondering if it might
> > > > also be good to create a separate macro for the unixctl case. Ideally
> > > > there would be a way to derive the pid from the unixctl socket path,
> > > > but I believe we can't assume the existence of things like fuser or
> > > > lsof even on Linux or BSD systems. Or maybe this variant could
> > > > take both the unixctl socket path and pid file path as parameters.
> > > 
> > > The "kill -0" change is a tangent.  It solves a particular problem, but
> > > not the root of this issue.
> > > 
> > > It's a mistake to pass a unixctl socket name to OVS_APP_EXIT_AND_WAIT.
> > 
> > Right.
> > 
> > > A new macro is one possible solution.  However, looking at a few of the
> > > cases where OVS_APP_EXIT_AND_WAIT is being passed a unixctl socket, I'm
> > > not sure why we couldn't just pass a pidfile instead.  Some of the users
> > > don't have a pidfile, but why not just add one?
> > > 
> > 
> > I'm fine with that, the main difference between creating a new macro and
> > adding a parameter to the existing one is the number of invocation sites
> > that would need to be modified... 8 or so if a new macro is created and
> > over 250 if a parameter is added to the existing macro.  It's more work
> > in the short term to add a parameter, but it does have a cleaner end
> > result.
> > 
> > I'll go ahead and start working a patch set up to add a pidfile parameter
> > to OVS_APP_EXIT_AND_WAIT.
> 
> I don't think we need a new parameter to the existing function.  I think
> that we just need those 8 cases to create pidfiles, e.g.
> 
> diff --git a/tests/ovsdb-monitor.at b/tests/ovsdb-monitor.at
> index 37383fa..48275f0 100644
> --- a/tests/ovsdb-monitor.at
> +++ b/tests/ovsdb-monitor.at
> @@ -24,23 +24,25 @@ m4_define([OVSDB_CHECK_MONITOR],
>     m4_foreach([txn], [$3],
>       [AT_CHECK([ovsdb-tool transact db 'txn'], [0], [ignore], [ignore])])
>     AT_CAPTURE_FILE([ovsdb-server-log])
> -   AT_CHECK([ovsdb-server --detach --no-chdir --pidfile="`pwd`"/server-pid
> --remote=punix:socket --unixctl="`pwd`"/unixctl
> --log-file="`pwd`"/ovsdb-server-log db >/dev/null 2>&1],
> +   AT_CHECK([ovsdb-server --detach --no-chdir --pidfile
> --remote=punix:socket --log-file="`pwd`"/ovsdb-server-log db >/dev/null
> 2>&1],
>              [0], [], [])
> +   on_exit "kill `cat ovsdb-server.pid`"
>     if test "$IS_WIN32" = "yes"; then
> -     AT_CHECK([ovsdb-client -vjsonrpc --pidfile="`pwd`"/client-pid -d json
> monitor --format=csv unix:socket $4 $5 $8 > output &],
> -              [0], [ignore], [ignore], [kill `cat server-pid`])
> +     AT_CHECK([ovsdb-client -vjsonrpc --pidfile -d json monitor --format=csv
> unix:socket $4 $5 $8 > output &],
> +              [0], [ignore], [ignore])
>       sleep 1
>     else
> -     AT_CHECK([ovsdb-client -vjsonrpc --detach --no-chdir
> --pidfile="`pwd`"/client-pid -d json monitor --format=csv unix:socket $4 $5
> $8 > output],
> -            [0], [ignore], [ignore], [kill `cat server-pid`])
> +     AT_CHECK([ovsdb-client -vjsonrpc --detach --no-chdir --pidfile -d json
> monitor --format=csv unix:socket $4 $5 $8 > output],
> +            [0], [ignore], [ignore])
>     fi
> +   on_exit "kill `cat ovsdb-client.pid`"
>     m4_foreach([txn], [$6],
>       [AT_CHECK([ovsdb-client transact unix:socket 'txn'], [0],
> -                     [ignore], [ignore], [kill `cat server-pid
> client-pid`])])
> +                     [ignore], [ignore])])
>     AT_CHECK([ovsdb-client transact unix:socket '[["$4"]]'], [0],
> -            [ignore], [ignore], [kill `cat server-pid client-pid`])
> -   OVS_APP_EXIT_AND_WAIT(["`pwd`"/unixctl])
> -   OVS_WAIT_UNTIL([test ! -e client-pid])
> +            [ignore], [ignore])
> +   OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +   OVS_WAIT_UNTIL([test ! -e ovsdb-client.pid])
>     AT_CHECK([${PERL} $srcdir/ovsdb-monitor-sort.pl < output | ${PERL}
>     $srcdir/uuidfilt.pl], [0], [$7], [ignore])
>     AT_CLEANUP])
>  
> Some cases are a little harder and might benefit from something new, but
> other cases, like the above, just seem to underuse or misuse existing
> infrastructure.
> 

Another thought I had while looking at this: since OVS_APP_EXIT_AND_WAIT_BY_TARGET
is already in place, and since it differs from OVS_APP_EXIT_AND_WAIT only in
(1) using a unixctl socket for the target and (2) using a specific rule for
the name of the pidfile, we could rework OVS_APP_EXIT_AND_WAIT_BY_TARGET to
take a unixctl socket and a pidfile for parameters and use the reworked
version for both purposes.

OVS_APP_EXIT_AND_WAIT_BY_TARGET is currently only used in 3 spots.

I *think* this would be a fairly small set of changes, although from the above
diff it seems some cleanup is needed in addition to fixing this specific
problem.

   Lance



More information about the dev mailing list