Refactoring scripts that wait for buildbot jobs to complete.

Refactoring scripts that wait for buildbot jobs to complete.

R=prasadv
BUG=534583
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=296830

[RobertoCN, 2015-09-16 22:43:51]

PTAL

[prasadv, 2015-09-17 22:33:35]

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (left):

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:490:
step_result.presentation.links['Failed build'] = url
This is missing.

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:468: buildbot_job['type'] =
'buildbot'
Just curious, why 'type' is not added in get_buildbot_locator()?
why do you want to do this here?

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:491: return None
When this happens what should be the behavior of wait_for_any?

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/perf_revision_state.py (right):

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/perf_revision_state.py:151: job_name =
self.build_job_name
I think at any given point a revision can either be building or testing, not
both at same time. right?
Then in that case why do want to have different job_names
variables(build_job_name and test_job_name)?

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/resources/check_buildbot.py
(left):

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/resources/check_buildbot.py:53: print
'Failed build url: %(url)s\nBuild failed: %(locator)s' % {
We need this build url to be a part of step link, we should do this both for
completed and failed builds.

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/resources/wait_for_any.py (right):

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/resources/wait_for_any.py:17:
COMPLETED, FAILED, TIMED_OUT, NOT_EVEN_STARTED = 0, 0, 2, 3
COMPLETED == FAILED, shouldn't we mark the failed build as red.
How about NOT_EVEN_STARTED -> PENDING?

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/resources/wait_for_any.py:67: #
Building a nested dictionary so that we check at most once per builder.
May be we should elaborate these comments.

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/resources/wait_for_any.py:102:
"chromium.perf".
May be tryserver.chromium.perf be more relevant

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/resources/wait_for_any.py:117: 2 if the
one-week timeout is triggered.
What if the case where buildbot job succeeds but there is no gs:// file?
Should we still wait for one week to fail the job.

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/resources/wait_for_any.py:200: return
FAILED
How are we handling pending jobs, I mean jobs in queue.

[RobertoCN, 2015-09-19 00:32:55]

Done with all feedback, please take a look.

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (left):

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:490:
step_result.presentation.links['Failed build'] = url
On 2015/09/17 22:33:34, prasadv wrote:
> This is missing.

Done.

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:468: buildbot_job['type'] =
'buildbot'
On 2015/09/17 22:33:34, prasadv wrote:
> Just curious, why 'type' is not added in get_buildbot_locator()?
> why do you want to do this here?

Because the 'type': 'gs' is just 2 lines above. But I guess it makes more sense
to add it where the dictionary is originally created.

Moved it there.

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:491: return None
On 2015/09/17 22:33:34, prasadv wrote:
> When this happens what should be the behavior of wait_for_any?
If wait_for_any.py errors out, all revisions will be marked as failed. The
wait_for_any function will then return the first of the failed revisions.

If wait_for_any.py doesn't error out, but it returns nothing, then we still
check all the revisions. This is mostly so that recipe simulation test passes.
(We don't want to hardcode a fake result for every single time wait_for_any.py
runs in the simulation)

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:491: return None
On 2015/09/17 22:33:34, prasadv wrote:
> When this happens what should be the behavior of wait_for_any?

Changed it so that we keep the step red and proceed when one of the builds
failed.

Also adding the link to both failed and succeeded builds.

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/perf_revision_state.py (right):

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/perf_revision_state.py:151: job_name =
self.build_job_name
On 2015/09/17 22:33:34, prasadv wrote:
> I think at any given point a revision can either be building or testing, not
> both at same time. right?
> Then in that case why do want to have different job_names
> variables(build_job_name and test_job_name)?
There's no need to have them separate anymore. I just merged them.

Done.

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/resources/check_buildbot.py
(left):

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/resources/check_buildbot.py:53: print
'Failed build url: %(url)s\nBuild failed: %(locator)s' % {
On 2015/09/17 22:33:34, prasadv wrote:
> We need this build url to be a part of step link, we should do this both for
> completed and failed builds.

Done.

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/resources/wait_for_any.py (right):

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/resources/wait_for_any.py:17:
COMPLETED, FAILED, TIMED_OUT, NOT_EVEN_STARTED = 0, 0, 2, 3
On 2015/09/17 22:33:35, prasadv wrote:
> COMPLETED == FAILED, shouldn't we mark the failed build as red.
> How about NOT_EVEN_STARTED -> PENDING?

Completed now != failed.
NOT_EVEN_STARTED, means that the script bailed before checking, renamed it to
BAD_ARGS for clarity.

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/resources/wait_for_any.py:67: #
Building a nested dictionary so that we check at most once per builder.
On 2015/09/17 22:33:35, prasadv wrote:
> May be we should elaborate these comments.

Done.

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/resources/wait_for_any.py:102:
"chromium.perf".
On 2015/09/17 22:33:35, prasadv wrote:
> May be tryserver.chromium.perf be more relevant

Done.

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/resources/wait_for_any.py:117: 2 if the
one-week timeout is triggered.
On 2015/09/17 22:33:35, prasadv wrote:
> What if the case where buildbot job succeeds but there is no gs:// file?
> Should we still wait for one week to fail the job.

In that case it will still return 0. We won't wait a week. Note that regardless
of whether we find that the job completed or the file in the gs location first,
we sill still return 0 as soon as either happens.

https://codereview.chromium.org/1339613005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/resources/wait_for_any.py:200: return
FAILED
On 2015/09/17 22:33:35, prasadv wrote:
> How are we handling pending jobs, I mean jobs in queue.

They are considered in progress, as they are neither completed nor failed.

This applies to jobs that are in the queue and jobs that are in progress, jobs
in this category won't be in buildbot_results.

Adding a comment to clarify

[RobertoCN, 2015-09-22 22:16:05]

Local tests now pass, ptal.

[prasadv, 2015-09-22 23:00:15]

lgtm

https://codereview.chromium.org/1339613005/diff/80001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/1339613005/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:463: 
Delete this blank line.

[RobertoCN, 2015-09-23 00:01:35]

https://codereview.chromium.org/1339613005/diff/80001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/1339613005/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:463: 
On 2015/09/22 23:00:15, prasadv wrote:
> Delete this blank line.

Done.

[RobertoCN, 2015-09-23 00:01:41]

The CQ bit was checked by robertocn@chromium.org

[RobertoCN, 2015-09-23 00:01:42]

The patchset sent to the CQ was uploaded after l-g-t-m from prasadv@chromium.org
Link to the patchset: https://codereview.chromium.org/1339613005/#ps100001
(title: "removing blank line")

[commit-bot: I haz the power, 2015-09-23 00:02:02]

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

[commit-bot: I haz the power, 2015-09-23 00:06:26]

Committed patchset #6 (id:100001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=296830