Bug 69976

Summary: Kill old run-webkit-tests processes in kill-process on Mac
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Tools / TestsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: dpranke, eric, lforschler, simon.fraser, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 63651    
Bug Blocks:    
Attachments:
Description Flags
adds a new system call to kill-old-processes
none
Fixed indentation and typo
dpranke: review-
new patch
none
new patch 2 none

Description Ryosuke Niwa 2011-10-12 16:57:01 PDT
We should kill all python instances executing run-webkit-tests.
Comment 1 Ryosuke Niwa 2011-10-12 17:00:17 PDT
Created attachment 110771 [details]
adds a new system call to kill-old-processes
Comment 2 Eric Seidel (no email) 2011-10-12 17:04:35 PDT
Comment on attachment 110771 [details]
adds a new system call to kill-old-processes

Um.  You want to call this once for each task?  Makes no sense.
Comment 3 Tony Chang 2011-10-12 17:08:13 PDT
What about the buildbot process?
Comment 4 Ryosuke Niwa 2011-10-12 17:08:25 PDT
(In reply to comment #2)
> (From update of attachment 110771 [details])
> Um.  You want to call this once for each task?  Makes no sense.

I don't quite get what you mean. We need to kill all python instances running run-webkit-tests (in particular child processes created by manager.py). How else can we accomplish this?
Comment 5 Ryosuke Niwa 2011-10-12 17:09:02 PDT
(In reply to comment #3)
> What about the buildbot process?

The bug 63651 will enable kill-old-processes on Mac so this will be a buildbot step.
Comment 6 Tony Chang 2011-10-12 17:09:52 PDT
(In reply to comment #5)
> (In reply to comment #3)
> > What about the buildbot process?
> 
> The bug 63651 will enable kill-old-processes on Mac so this will be a buildbot step.

I mean, buildbot is a python process.  You probably don't want to kill it.
Comment 7 Ryosuke Niwa 2011-10-12 17:11:07 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > What about the buildbot process?
> > 
> > The bug 63651 will enable kill-old-processes on Mac so this will be a buildbot step.
> 
> I mean, buildbot is a python process.  You probably don't want to kill it.

Right, that's why I'm explicitly looking for python process with run-webkit-test string:
grep -P '.+/Python .+(run_webkit_tests|run-webkit-tests)'
Comment 8 Tony Chang 2011-10-12 17:12:48 PDT
Comment on attachment 110771 [details]
adds a new system call to kill-old-processes

View in context: https://bugs.webkit.org/attachment.cgi?id=110771&action=review

> Tools/BuildSlaveSupport/kill-old-processes:80
> +            # Kill all instances of pyhton executing run-webkit-tests

typo: pyhton
Comment 9 Eric Seidel (no email) 2011-10-12 17:16:27 PDT
Comment on attachment 110771 [details]
adds a new system call to kill-old-processes

View in context: https://bugs.webkit.org/attachment.cgi?id=110771&action=review

> Tools/BuildSlaveSupport/kill-old-processes:81
> +            os.system("aux | grep -P '.+/Python .+(run_webkit_tests|run-webkit-tests)' | grep -v grep | awk '{print $2}' | xargs kill")

You're runing this in a loop.  Once for every "task" in the array.  WHy?
Comment 10 Ryosuke Niwa 2011-10-12 17:17:56 PDT
Comment on attachment 110771 [details]
adds a new system call to kill-old-processes

View in context: https://bugs.webkit.org/attachment.cgi?id=110771&action=review

>> Tools/BuildSlaveSupport/kill-old-processes:81
>> +            os.system("aux | grep -P '.+/Python .+(run_webkit_tests|run-webkit-tests)' | grep -v grep | awk '{print $2}' | xargs kill")
> 
> You're runing this in a loop.  Once for every "task" in the array.  WHy?

Oh oops, that's definitely a typo!  Will fix.

This is why I hate python :(
Comment 11 Ryosuke Niwa 2011-10-12 17:22:05 PDT
Created attachment 110776 [details]
Fixed indentation and typo
Comment 12 Dirk Pranke 2011-10-12 17:32:41 PDT
Comment on attachment 110776 [details]
Fixed indentation and typo

View in context: https://bugs.webkit.org/attachment.cgi?id=110776&action=review

> Tools/BuildSlaveSupport/kill-old-processes:81
> +        os.system("aux | grep -P '.+/Python .+(run_webkit_tests|run-webkit-tests)' | grep -v grep | awk '{print $2}' | xargs kill")

presumably this should be "ps aux" at the beginning?
Comment 13 Dirk Pranke 2011-10-12 17:34:12 PDT
(In reply to comment #12)
> (From update of attachment 110776 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110776&action=review
> 
> > Tools/BuildSlaveSupport/kill-old-processes:81
> > +        os.system("aux | grep -P '.+/Python .+(run_webkit_tests|run-webkit-tests)' | grep -v grep | awk '{print $2}' | xargs kill")
> 
> presumably this should be "ps aux" at the beginning?

Also, if you add this, can you remove 'httpd' from tasksToKill? NRWT should restart httpd itself correctly on the next run, if it started the last one.
Comment 14 Ryosuke Niwa 2011-10-12 17:41:46 PDT
Created attachment 110780 [details]
new patch
Comment 15 Ryosuke Niwa 2011-10-12 17:43:06 PDT
Created attachment 110781 [details]
new patch 2

Oops, wrong file.
Comment 16 WebKit Review Bot 2011-10-12 18:33:05 PDT
Comment on attachment 110781 [details]
new patch 2

Clearing flags on attachment: 110781

Committed r97330: <http://trac.webkit.org/changeset/97330>
Comment 17 WebKit Review Bot 2011-10-12 18:33:10 PDT
All reviewed patches have been landed.  Closing bug.