[Telemetry] Add timeout to telemetry run_tests

[Telemetry] Add timeout to telemetry run_tests

BUG=457446

[nednguyen, 2015-09-03 16:27:53]

Patchset #1 (id:1) has been deleted

[nednguyen, 2015-09-03 16:29:45]

nednguyen@google.com changed reviewers:
+ dpranke@chromium.org

[nednguyen, 2015-09-03 16:29:46]

+Dirk: I may not know what I am doing here. Killing the run_tests process this
way can leave a bunch of subprocesses lingering around. Does tryjob has a step
to clean up the test environment?

[nednguyen, 2015-09-03 16:40:06]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-03 16:40:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1323303002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1323303002/20001

[commit-bot: I haz the power, 2015-09-03 17:14:28]

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

[commit-bot: I haz the power, 2015-09-03 17:14:29]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Dirk Pranke, 2015-09-03 17:51:18]

dpranke@chromium.org changed reviewers:
+ iannucci@chromium.org, maruel@chromium.org

[Dirk Pranke, 2015-09-03 17:54:02]

On 2015/09/03 16:29:46, nednguyen wrote:
> +Dirk: I may not know what I am doing here. Killing the run_tests process this
> way can leave a bunch of subprocesses lingering around. Does tryjob has a step
> to clean up the test environment?

On 2015/09/03 16:29:46, nednguyen wrote:
> +Dirk: I may not know what I am doing here. Killing the run_tests process this
> way can leave a bunch of subprocesses lingering around. Does tryjob has a step
> to clean up the test environment?

At one point the bots would kill stray processes, but it looks like maybe we
don't do that by default in a recipe-based world.

I do think even subprocess32 may have issues w/ leaving child processes around.

Perhaps maruel or iannucci have suggestions.

You'll also need to get someone from the OSPO to approve checking in
subprocess32, unless we've already done this elsewhere (maruel might know),
and then fix the checklicenses step.

You may not want to make this timeout unconditional, in case you need to be able
to run for longer than 10 minutes locally.

I also wonder if we can push this into a higher layer to just configure the step
that runs this test to abort after 10 minutes instead.

[M-A Ruel, 2015-09-03 17:56:49]

maruel@chromium.org changed reviewers:
+ kbr@chromium.org

[M-A Ruel, 2015-09-03 17:56:49]

The solution is to migrate telemetry to Swarming ASAP. Not do patchwork. You'd
have to properly handle signals instead, it's not trivial and Swarming already
handle this.

Ken had volunteered to work on this but it's been a while I haven't asked for
progress. With the current time of year we are now at, it's likely not going to
be done for another ~1m. I may do it myself.

[nednguyen, 2015-09-03 17:58:08]

On 2015/09/03 17:54:02, Dirk Pranke wrote:
> On 2015/09/03 16:29:46, nednguyen wrote:
> > +Dirk: I may not know what I am doing here. Killing the run_tests process
this
> > way can leave a bunch of subprocesses lingering around. Does tryjob has a
step
> > to clean up the test environment?
> 
> On 2015/09/03 16:29:46, nednguyen wrote:
> > +Dirk: I may not know what I am doing here. Killing the run_tests process
this
> > way can leave a bunch of subprocesses lingering around. Does tryjob has a
step
> > to clean up the test environment?
> 
> At one point the bots would kill stray processes, but it looks like maybe we
> don't do that by default in a recipe-based world.
> 
> I do think even subprocess32 may have issues w/ leaving child processes
around.
> 
> Perhaps maruel or iannucci have suggestions.
> 
> You'll also need to get someone from the OSPO to approve checking in
> subprocess32, unless we've already done this elsewhere (maruel might know),
> and then fix the checklicenses step.
> 
> You may not want to make this timeout unconditional, in case you need to be
able
> to run for longer than 10 minutes locally.
> 
> I also wonder if we can push this into a higher layer to just configure the
step
> that runs this test to abort after 10 minutes instead.

Yeah, I think that would be the best approach. However, I don't know anything
about trybot recipe.

[nednguyen, 2015-09-03 17:59:38]

nednguyen@google.com changed reviewers:
+ prasadv@chromium.org

[nednguyen, 2015-09-03 17:59:38]

+Ken, Prasad: do you know how to setup the CQ try job so that it timeout the
tests after x minutes?

[Ken Russell (switch to Gerrit), 2015-09-09 18:15:37]

On 2015/09/03 17:56:49, M-A Ruel wrote:
> The solution is to migrate telemetry to Swarming ASAP. Not do patchwork. You'd
> have to properly handle signals instead, it's not trivial and Swarming already
> handle this.
> 
> Ken had volunteered to work on this but it's been a while I haven't asked for
> progress. With the current time of year we are now at, it's likely not going
to
> be done for another ~1m. I may do it myself.

I'd appreciate help finishing the task of isolating the Telemetry tests and
getting those isolates invoked via chromium_trybot's steps (
http://crbug.com/507796 ). Apologies, I forgot about this task.

[Ken Russell (switch to Gerrit), 2015-09-09 18:17:15]

On 2015/09/03 17:59:38, nednguyen wrote:
> +Ken, Prasad: do you know how to setup the CQ try job so that it timeout the
> tests after x minutes?

Recipes and/or buildbot are already supposed to time out and kill steps after
600 seconds of no output; please confirm with iannucci@.

The problem is likely that the subprocess killing mechanism there is not robust
enough. I doubt that this change will improve the situation and agree that this
approach is likely to lead to lingering subprocesses especially on Windows.
Also, the fixed 10 minute timeout is undesirable.

As I understand it, Swarming has multiple layers of robustness in this area,
including forcibly rebooting the machine upon test failure, which will prevent
stuck subprocesses from lingering around. I agree with maruel@ that the best
path forward is to swarm all of the Telemetry based tests.

[Dirk Pranke, 2016-08-31 18:59:37]

I think we probably don't need this any more, and timeouts should probably be
handled differently regardless, so I'm closing this CL. We can discuss elsewhere
if we still want to make timeout-related improvements (I have some stuff I want
to do in typ that'll make things way better).

[nednguyen, 2016-08-31 20:47:01]

On 2016/08/31 18:59:37, Dirk Pranke wrote:
> I think we probably don't need this any more, and timeouts should probably be
> handled differently regardless, so I'm closing this CL. We can discuss
elsewhere
> if we still want to make timeout-related improvements (I have some stuff I
want
> to do in typ that'll make things way better).

sgtm