From patchwork Fri Jun 3 01:41:23 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joe Stringer X-Patchwork-Id: 629566 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (archives.nicira.com [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id 3rLRf80hqYz9t8M for ; Fri, 3 Jun 2016 11:41:52 +1000 (AEST) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id 8B79B108A4; Thu, 2 Jun 2016 18:41:50 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx1e4.cudamail.com (mx1.cudamail.com [69.90.118.67]) by archives.nicira.com (Postfix) with ESMTPS id 8780B1089F for ; Thu, 2 Jun 2016 18:41:49 -0700 (PDT) Received: from bar5.cudamail.com (unknown [192.168.21.12]) by mx1e4.cudamail.com (Postfix) with ESMTPS id E30EC1E0057 for ; Thu, 2 Jun 2016 19:41:48 -0600 (MDT) X-ASG-Debug-ID: 1464918108-09eadd4ab78d2c0001-byXFYA Received: from mx1-pf1.cudamail.com ([192.168.24.1]) by bar5.cudamail.com with ESMTP id EjiU5mQgUvgeh6ol (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO) for ; Thu, 02 Jun 2016 19:41:48 -0600 (MDT) X-Barracuda-Envelope-From: joe@ovn.org X-Barracuda-RBL-Trusted-Forwarder: 192.168.24.1 Received: from unknown (HELO relay6-d.mail.gandi.net) (217.70.183.198) by mx1-pf1.cudamail.com with ESMTPS (DHE-RSA-AES256-SHA encrypted); 3 Jun 2016 01:41:47 -0000 Received-SPF: pass (mx1-pf1.cudamail.com: SPF record at ovn.org designates 217.70.183.198 as permitted sender) X-Barracuda-Apparent-Source-IP: 217.70.183.198 X-Barracuda-RBL-IP: 217.70.183.198 Received: from mfilter39-d.gandi.net (mfilter39-d.gandi.net [217.70.178.170]) by relay6-d.mail.gandi.net (Postfix) with ESMTP id 3FA41FB886 for ; Fri, 3 Jun 2016 03:41:46 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at mfilter39-d.gandi.net Received: from relay6-d.mail.gandi.net ([IPv6:::ffff:217.70.183.198]) by mfilter39-d.gandi.net (mfilter39-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id ux-xXsFU_Wl6 for ; Fri, 3 Jun 2016 03:41:44 +0200 (CEST) X-Originating-IP: 209.85.218.41 Received: from mail-oi0-f41.google.com (mail-oi0-f41.google.com [209.85.218.41]) (Authenticated sender: joe@ovn.org) by relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 67D65FB883 for ; Fri, 3 Jun 2016 03:41:44 +0200 (CEST) Received: by mail-oi0-f41.google.com with SMTP id w184so105565191oiw.2 for ; Thu, 02 Jun 2016 18:41:44 -0700 (PDT) X-Gm-Message-State: ALyK8tIC7NoTAYdw6GY5aM49nCB44r4Oxl7TUu+TYMKSpdG41eBUc47/KhPrrcNXrUAIfysKdcN0e33TaAoxyQ== X-Received: by 10.202.90.212 with SMTP id o203mr556208oib.117.1464918103163; Thu, 02 Jun 2016 18:41:43 -0700 (PDT) MIME-Version: 1.0 Received: by 10.202.83.23 with HTTP; Thu, 2 Jun 2016 18:41:23 -0700 (PDT) In-Reply-To: <508339EC0242094682895ED3EC4EBA31174DCD03@CBSEX1.cloudbase.local> References: <1464781611-18276-1-git-send-email-pboca@cloudbasesolutions.com> <508339EC0242094682895ED3EC4EBA31174DCD03@CBSEX1.cloudbase.local> X-CudaMail-Envelope-Sender: joe@ovn.org From: Joe Stringer Date: Thu, 2 Jun 2016 18:41:23 -0700 X-Gmail-Original-Message-ID: Message-ID: X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-E1-601090218 X-CudaMail-DTE: 060216 X-CudaMail-Originating-IP: 217.70.183.198 To: Lance Richardson X-ASG-Orig-Subj: [##CM-E1-601090218##]Re: [ovs-dev] [PATCH V2 4/4] tests: Fix fail of OVS_APP_EXIT_AND_WAIT on Windows X-Barracuda-Connect: UNKNOWN[192.168.24.1] X-Barracuda-Start-Time: 1464918108 X-Barracuda-Encrypted: ECDHE-RSA-AES256-GCM-SHA384 X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 Cc: "dev@openvswitch.org" Subject: Re: [ovs-dev] [PATCH V2 4/4] tests: Fix fail of OVS_APP_EXIT_AND_WAIT on Windows X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@openvswitch.org Sender: "dev" On 2 June 2016 at 01:28, Paul Boca 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@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 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 >> >> 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? + 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])]) 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