Wait cloudtail termination in goma module

Wait cloudtail termination in goma module

This is follow up CL for https://codereview.chromium.org/2375843005/

BUG=

Committed: https://chromium.googlesource.com/chromium/tools/build/+/5ba4397bb3ab875a37501ec8eb0de33d1b4a1514

[tikuta, 2016-10-03 02:38:21]

tikuta@chromium.org changed reviewers:
+ phajdan.jr@chromium.org, shinyak@chromium.org, ukai@chromium.org,
vadimsh@chromium.org, yyanagisawa@chromium.org

[tikuta, 2016-10-03 02:38:31]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 02:38:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 02:45:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 02:45:10]

Dry run: This issue passed the CQ dry run.

[ukai, 2016-10-03 02:52:52]

https://codereview.chromium.org/2386073002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2386073002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:32: if e.errno ==
errno.ESRCH:
kill may get errno.EPERM and I think we should return false for it too?
0 may not cause EINVAL?

https://codereview.chromium.org/2386073002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:67: if os.name ==
'nt':
factor out a func to wait for pid?

https://codereview.chromium.org/2386073002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:70: while True:
might be better to abandon if it tried >5 sec or so, and kill with SIGKILL?

[Yoshisato Yanagisawa, 2016-10-03 04:49:05]

https://codereview.chromium.org/2386073002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2386073002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:34: raise e
So, in this case, we do not guarantee cloud tail died?

[Vadim Sh., 2016-10-03 19:07:06]

https://codereview.chromium.org/2386073002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2386073002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:32: if e.errno ==
errno.ESRCH:
On 2016/10/03 02:52:52, ukai wrote:
> kill may get errno.EPERM and I think we should return false for it too?
> 0 may not cause EINVAL?

+1 for EPERM handling and returning false

https://codereview.chromium.org/2386073002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:67: if os.name ==
'nt':
On 2016/10/03 02:52:52, ukai wrote:
> factor out a func to wait for pid?

+1

https://codereview.chromium.org/2386073002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:70: while True:
On 2016/10/03 02:52:52, ukai wrote:
> might be better to abandon if it tried >5 sec or so, and kill with SIGKILL?

+1

also nit

while is_running_posix(pid):
  time.sleep(1)

[tikuta, 2016-10-04 03:45:26]

https://codereview.chromium.org/2386073002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2386073002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:32: if e.errno ==
errno.ESRCH:
On 2016/10/03 19:07:06, Vadim Sh. wrote:
> On 2016/10/03 02:52:52, ukai wrote:
> > kill may get errno.EPERM and I think we should return false for it too?
> > 0 may not cause EINVAL?
> 
> +1 for EPERM handling and returning false

Done.

https://codereview.chromium.org/2386073002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:34: raise e
On 2016/10/03 04:49:05, Yoshisato Yanagisawa wrote:
> So, in this case, we do not guarantee cloud tail died?

I send SIGKILL

https://codereview.chromium.org/2386073002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:67: if os.name ==
'nt':
On 2016/10/03 19:07:06, Vadim Sh. wrote:
> On 2016/10/03 02:52:52, ukai wrote:
> > factor out a func to wait for pid?
> 
> +1

Done.

https://codereview.chromium.org/2386073002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:70: while True:
On 2016/10/03 19:07:06, Vadim Sh. wrote:
> On 2016/10/03 02:52:52, ukai wrote:
> > might be better to abandon if it tried >5 sec or so, and kill with SIGKILL?
> 
> +1
> 
> also nit
> 
> while is_running_posix(pid):
>   time.sleep(1)

Done.

[tikuta, 2016-10-04 03:45:31]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 03:45:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 03:52:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 03:52:48]

Dry run: This issue passed the CQ dry run.

[ukai, 2016-10-04 04:58:44]

lgtm

https://codereview.chromium.org/2386073002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2386073002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:29: try:
docstring?

https://codereview.chromium.org/2386073002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:30: os.kill(pid,
signal.SIGINT)
os.kill(pid, 0) ?

https://codereview.chromium.org/2386073002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:38: def
wait_termination(pid):
docstring?

https://codereview.chromium.org/2386073002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:39: os.kill(pid,
signal.SIGINT)
no exception is expected here?

[tikuta, 2016-10-04 05:12:07]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 05:12:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 05:15:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 05:15:51]

Dry run: This issue passed the CQ dry run.

[tikuta, 2016-10-04 05:18:08]

https://codereview.chromium.org/2386073002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2386073002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:29: try:
On 2016/10/04 04:58:44, ukai wrote:
> docstring?

Done.

https://codereview.chromium.org/2386073002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:30: os.kill(pid,
signal.SIGINT)
On 2016/10/04 04:58:44, ukai wrote:
> os.kill(pid, 0) ?

Done.

https://codereview.chromium.org/2386073002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:38: def
wait_termination(pid):
On 2016/10/04 04:58:44, ukai wrote:
> docstring?

Done.

https://codereview.chromium.org/2386073002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:39: os.kill(pid,
signal.SIGINT)
On 2016/10/04 04:58:44, ukai wrote:
> no exception is expected here?

If exception occurs, I kill cloudtail and raise same exception.

[ukai, 2016-10-05 01:08:32]

lgtm

https://codereview.chromium.org/2386073002/diff/80001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2386073002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:29: """Return
True if process of pid is running."""
Args:
Returns:
Raises:

https://codereview.chromium.org/2386073002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:41: """Wait
termination of pid."""
Send SIGINT and wait ..
?

Args:
Raises:

[Yoshisato Yanagisawa, 2016-10-05 02:15:43]

lgtm w/ nit

https://codereview.chromium.org/2386073002/diff/80001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2386073002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:52: for _ in
range(10):
nit xrange?

https://codereview.chromium.org/2386073002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:58: os.kill(pid,
signal.SIGKILL)
not sure but no need to print you killed the process in this case?

[tikuta, 2016-10-05 04:12:59]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 04:13:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tikuta, 2016-10-05 04:15:41]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 04:15:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tikuta, 2016-10-05 04:16:28]

https://codereview.chromium.org/2386073002/diff/80001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2386073002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:29: """Return
True if process of pid is running."""
On 2016/10/05 01:08:32, ukai wrote:
> Args:
> Returns:
> Raises:

Done.

https://codereview.chromium.org/2386073002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:41: """Wait
termination of pid."""
On 2016/10/05 01:08:32, ukai wrote:
> Send SIGINT and wait ..
> ?
> 
> Args:
> Raises:

Done.

https://codereview.chromium.org/2386073002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:52: for _ in
range(10):
On 2016/10/05 02:15:42, Yoshisato Yanagisawa wrote:
> nit xrange?

Done.

https://codereview.chromium.org/2386073002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:58: os.kill(pid,
signal.SIGKILL)
On 2016/10/05 02:15:42, Yoshisato Yanagisawa wrote:
> not sure but no need to print you killed the process in this case?

Done.
It is needed.

[commit-bot: I haz the power, 2016-10-05 04:22:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 04:22:59]

Dry run: This issue passed the CQ dry run.

[shinyak, 2016-10-05 07:48:24]

lgtm

[tikuta, 2016-10-07 05:48:05]

The CQ bit was checked by tikuta@chromium.org

[tikuta, 2016-10-07 05:48:05]

The patchset sent to the CQ was uploaded after l-g-t-m from ukai@chromium.org,
yyanagisawa@chromium.org
Link to the patchset: https://codereview.chromium.org/2386073002/#ps120001
(title: "add print")

[commit-bot: I haz the power, 2016-10-07 05:48:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 05:52:06]

Description was changed from

==========
Wait cloudtail termination in goma module

This is follow up CL for https://codereview.chromium.org/2375843005/

BUG=
==========

to

==========
Wait cloudtail termination in goma module

This is follow up CL for https://codereview.chromium.org/2375843005/

BUG=

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/5ba4397bb3ab875a3750...
==========

[commit-bot: I haz the power, 2016-10-07 05:52:07]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/tools/build/+/5ba4397bb3ab875a3750...

[tikuta, 2016-10-07 23:46:55]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2399403005/ by tikuta@chromium.org.

The reason for reverting is:
https://build.chromium.org/p/tryserver.client.pdfium/builders/win_xfa_clang/b...

Traceback (most recent call last):
  File
"E:\b\build\scripts/slave\recipe_modules\goma\resources\cloudtail_utils.py",
line 111, in <module>
    sys.exit(main())
  File
"E:\b\build\scripts/slave\recipe_modules\goma\resources\cloudtail_utils.py",
line 108, in main
    wait_termination(int(f.read()))
  File
"E:\b\build\scripts/slave\recipe_modules\goma\resources\cloudtail_utils.py",
line 69, in wait_termination
    os.waitpid(pid, 0)
OSError: [Errno 10] No child processes
step returned non-zero exit code: 1

Need to ignore OSError ECHILD?.