Send a SIGKILL to masters after 10 seconds if they don't exit from SIGTERM

Send a SIGKILL to masters after 10 seconds if they don't exit from SIGTERM

BUG=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299527

[dsansome, 2016-03-02 04:47:53]

dsansome@chromium.org changed reviewers:
+ chrishall@chromium.org, pgervais@chromium.org, stip@chromium.org

[Vadim Sh., 2016-03-02 15:41:32]

(just passing by)

Existing implementation has two subtle points that I think might be missing from
yours:
1) It kills process group explicitly, not just root buildbot process. It
somewhat helps to avoid junk processes staying around (e.g. buildbot spawns a
lot of 'git' subprocesses).
2) It works on Mac :) I remember someone complained long time ago when we broke
it for Mac. I'm not sure it's a big deal though.

[pgervais-g, 2016-03-02 16:23:00]

On 2016/03/02 15:41:32, Vadim Sh. wrote:
> (just passing by)
> 
> Existing implementation has two subtle points that I think might be missing
from
> yours:
> 1) It kills process group explicitly, not just root buildbot process. It
> somewhat helps to avoid junk processes staying around (e.g. buildbot spawns a
> lot of 'git' subprocesses).
> 2) It works on Mac :) I remember someone complained long time ago when we
broke
> it for Mac. I'm not sure it's a big deal though.

I second Vadim here: make sure your implementation works on Mac. Some teams use
Macs almost exclusively and it's a pain to find a Linux box simply to run a
master locally.

[dsansome, 2016-03-18 02:11:59]

On 2016/03/02 15:41:32, Vadim Sh. wrote:
> (just passing by)
> 
> Existing implementation has two subtle points that I think might be missing
from
> yours:
> 1) It kills process group explicitly, not just root buildbot process. It
> somewhat helps to avoid junk processes staying around (e.g. buildbot spawns a
> lot of 'git' subprocesses).
> 2) It works on Mac :) I remember someone complained long time ago when we
broke
> it for Mac. I'm not sure it's a big deal though.

Thanks, sorry for the delay!

I've updated this to kill the pgid, and work on mac by using ps -p rather than
checking /proc.

[dsansome, 2016-03-29 03:24:28]

dsansome@chromium.org changed reviewers:
+ dnj@chromium.org

[dsansome, 2016-03-29 03:24:28]

Ping and +dnj

[dnj, 2016-03-29 04:05:07]

lgtm, nice idea.

[dsansome, 2016-03-29 04:13:40]

The CQ bit was checked by dsansome@chromium.org

[dsansome, 2016-03-29 04:13:41]

The patchset sent to the CQ was uploaded after l-g-t-m from dnj@chromium.org
Link to the patchset: https://codereview.chromium.org/1754103002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-29 04:13:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1754103002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1754103002/60001

[commit-bot: I haz the power, 2016-03-29 04:17:32]

Description was changed from

==========
Send a SIGKILL to masters after 10 seconds if they don't exit from SIGTERM

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

to

==========
Send a SIGKILL to masters after 10 seconds if they don't exit from SIGTERM

BUG=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299527
==========

[commit-bot: I haz the power, 2016-03-29 04:17:34]

Committed patchset #4 (id:60001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=299527

[nodir, 2016-04-06 16:56:36]

nodir@chromium.org changed reviewers:
+ nodir@chromium.org

[nodir, 2016-04-06 16:56:36]

it caused https://bugs.chromium.org/p/chromium/issues/detail?id=599874

[ghost stip (do not use), 2016-04-06 20:10:47]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1863083003/ by stip@chromium.org.

The reason for reverting is: In addition to the problems in crbug.com/599874
pointed out by nodir@, I think that this logic is unnecessary given the data in
https://bugs.chromium.org/p/chromium/issues/detail?id=588118. Anything stale was
from well before proper logic was implemented to handle duplicates
(https://chromiumcodereview.appspot.com/1808383002/). Since this may not be
necessary anymore and actively causing trouble, I'm reverting. I'd like to
continue this discussion because I think it's a good issue to have. Perhaps we
can come up with a better way to manage our daemons and ensure only one is
running at a time..