650865 - add pid check for runtestsremote to cleanup.py run

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P4)

Description

[Mike Taylor [:bear]]

when bug 650828 lands add check for these pids to the cleanup.py step so that it can be noticed *before* the tests start

Comment 1

[Mike Taylor [:bear]]

Attachment: add pidfile parameter to remote test calls

Comment 2

[Mike Taylor [:bear]]

Attachment: add pidfile check to cleanup.py step

Comment 3

[Aki Sasaki (not active)]

Comment on attachment 527282 [details] [diff] [review]
add pidfile parameter to remote test calls

Have you tried this in staging?

By my reading of process/factory.py, basedir is getting set to the slavedir, which is /builds/tegra-###.  You're setting the pidfile to /builds/remotereftest.pid ? Or am I reading this wrong?

Comment 4

[Aki Sasaki (not active)]

Comment on attachment 527283 [details] [diff] [review]
add pidfile check to cleanup.py step

>+pidDir    = os.path.join(cwd, '..')

I want to doublecheck that this is aligned with the builder (see previous comment).

>+for f in ('runtestsremote', 'remotereftest'):
>+    pidFile = os.path.join(pidDir, '%s.pid' % f)
>+    if os.path.exists(pidFile):
>+        print "pidfile from prior test run found, trying to kill"
>+        stopProcess(pidFile, f)
>+        if not os.path.exists(pidFile):
>+            setFlag(errorFile, "Remote Device Error: process from previous test run present [%s]" % f)
>+            sys.exit(2)

if *not* os.path.exists(pidFile) it's an error?
I think this is a bug.

Comment 5

[Mike Taylor [:bear]]

(In reply to comment #4)
> Comment on attachment 527283 [details] [diff] [review]
> add pidfile check to cleanup.py step
> 
> >+pidDir    = os.path.join(cwd, '..')
> 
> I want to doublecheck that this is aligned with the builder (see previous
> comment).

yea, I was unsure of this and when I checked the tinderbox output I saw that the basedir property was shown as /builds/tegra-###/test

I'll be running this in staging to find out

> 
> >+for f in ('runtestsremote', 'remotereftest'):
> >+    pidFile = os.path.join(pidDir, '%s.pid' % f)
> >+    if os.path.exists(pidFile):
> >+        print "pidfile from prior test run found, trying to kill"
> >+        stopProcess(pidFile, f)
> >+        if not os.path.exists(pidFile):
> >+            setFlag(errorFile, "Remote Device Error: process from previous test run present [%s]" % f)
> >+            sys.exit(2)
> 
> if *not* os.path.exists(pidFile) it's an error?
> I think this is a bug.

the "if not" part is because after running stopProcess, the pid file is still around, then we were unable to .... doh!  good catch

Comment 6

[Mike Taylor [:bear]]

Attachment: add pidfile parameter to remote test calls

changed path to pidfile to move it to /builds/tegra-###

Comment 7

[Mike Taylor [:bear]]

Attachment: add pidfile check to cleanup.py step

adjusted cleanup.py to pidfile path change and also sneaking in a bugfix to stopProcess()

Comment 8

[Aki Sasaki (not active)]

Comment on attachment 527381 [details] [diff] [review]
add pidfile parameter to remote test calls

Per IRC, basedir is being set to /builds/tegra-###/test, so the pidfiles will go in /builds/tegra-###. r=me

Comment 9

[Mike Taylor [:bear]]

Attachment: add pidfile check to cleanup.py step

Comment 10

[Mike Taylor [:bear]]

Comment on attachment 527381 [details] [diff] [review]
add pidfile parameter to remote test calls

committed changeset 1495:a8404b0bcbe4

Comment 11

[Mike Taylor [:bear]]

Comment on attachment 527387 [details] [diff] [review]
add pidfile check to cleanup.py step

committed changeset 1371:4c03d0266f86

this change is waiting for the buildbotcustom change to land before it will be active

Comment 12

[Mike Taylor [:bear]]

buildbotcustom change landed in reconfig done 21 Apr 2011