Checking for failure in requested builds.

Checking for failure in requested builds.

When the bisect requests a build that fails, rather than time out waiting
for it, we now query buildbot and handle failures.

R=prasadv,qyearsley
BUG=591536
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299430

[qyearsley, 2016-03-04 22:06:28]

Overall idea looks good I think, but the _is_build_failed method can be made
simpler and more readable; a few comments and questions:

https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/revision_state.py (right):

https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:255:
os.environ.get('BUILDBOT_URL', 'http://localhost:8041/'),
Why http://localhost:8041/?

https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:257: if
self.build_number is None:
Does this mean no build started yet, or we're not sure what the build number is
yet?

https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:277: break
Is build.get('properties') expected to be a list of 2-element lists? e.g.
[
 ['k', 'v'],
 ['build_archive_url', 'http://example.com/'],
]

If so, you can convert it to a dict, e.g.

>>> dict([['k', 'v'], ['k', 'v2']])
{'k', 'v2'}

So, this could be changed to something like:

build_properties = dict(build.get('properties', []))
if build_properties.get('build_archive_url') == self.build_url:
  current_build = build
  self.build_number = build_number

https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:293: current_build =
json.loads(fetch_result.stdout or '{}')
This section of code (where the URL is put together, and a request for build
details JSON is made and then that JSON is parsed) is repeated -- it should be
extracted to a separate method (which takes a base URL and build number, and
returns the parsed data).

https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:294: return
current_build.get('results') in [2, 3, 4]
These could be set as constants, ideally with some reference as to where they
come from. e.g.

# Buildbot job result codes.
# See http://docs.buildbot.net/current/developer/results.html
SUCCESS, WARNINGS, FAILURE, SKIPPED, EXCEPTION = range(5)

I'm not sure if that's correct though.

[RobertoCN, 2016-03-14 00:59:36]

Can I haz stamp?

[RobertoCN, 2016-03-14 01:00:25]

https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/revision_state.py (right):

https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:255:
os.environ.get('BUILDBOT_URL', 'http://localhost:8041/'),
On 2016/03/04 at 22:06:28, qyearsley wrote:
> Why http://localhost:8041/?

The reasonable assumption if this env variable is not set, is that this is a
workstation running a local master. I added a comment to that effect.

https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:257: if
self.build_number is None:
On 2016/03/04 at 22:06:28, qyearsley wrote:
> Does this mean no build started yet, or we're not sure what the build number
is yet?

It means we don't know the build number yet. The build number is not assigned
until the build is started however.

https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:277: break
On 2016/03/04 at 22:06:28, qyearsley wrote:
> Is build.get('properties') expected to be a list of 2-element lists? e.g.
> [
>  ['k', 'v'],
>  ['build_archive_url', 'http://example.com/'],
> ]
> 
> If so, you can convert it to a dict, e.g.
> 
> >>> dict([['k', 'v'], ['k', 'v2']])
> {'k', 'v2'}
> 
> So, this could be changed to something like:
> 
> build_properties = dict(build.get('properties', []))
> if build_properties.get('build_archive_url') == self.build_url:
>   current_build = build
>   self.build_number = build_number

Thanks! I don't know how I didn't see it.
Done.

https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:293: current_build =
json.loads(fetch_result.stdout or '{}')
On 2016/03/04 at 22:06:28, qyearsley wrote:
> This section of code (where the URL is put together, and a request for build
details JSON is made and then that JSON is parsed) is repeated -- it should be
extracted to a separate method (which takes a base URL and build number, and
returns the parsed data).

Done.

https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:294: return
current_build.get('results') in [2, 3, 4]
On 2016/03/04 at 22:06:28, qyearsley wrote:
> These could be set as constants, ideally with some reference as to where they
come from. e.g.
> 
> # Buildbot job result codes.
> # See http://docs.buildbot.net/current/developer/results.html
> SUCCESS, WARNINGS, FAILURE, SKIPPED, EXCEPTION = range(5)
> 
> I'm not sure if that's correct though.

Works for me.

[RobertoCN, 2016-03-16 20:09:49]

On 2016/03/14 01:00:25, RobertoCN wrote:
>
https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
> File scripts/slave/recipe_modules/auto_bisect/revision_state.py (right):
> 
>
https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
> scripts/slave/recipe_modules/auto_bisect/revision_state.py:255:
> os.environ.get('BUILDBOT_URL', 'http://localhost:8041/'),
> On 2016/03/04 at 22:06:28, qyearsley wrote:
> > Why http://localhost:8041/
> 
> The reasonable assumption if this env variable is not set, is that this is a
> workstation running a local master. I added a comment to that effect.
> 
>
https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
> scripts/slave/recipe_modules/auto_bisect/revision_state.py:257: if
> self.build_number is None:
> On 2016/03/04 at 22:06:28, qyearsley wrote:
> > Does this mean no build started yet, or we're not sure what the build number
> is yet?
> 
> It means we don't know the build number yet. The build number is not assigned
> until the build is started however.
> 
>
https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
> scripts/slave/recipe_modules/auto_bisect/revision_state.py:277: break
> On 2016/03/04 at 22:06:28, qyearsley wrote:
> > Is build.get('properties') expected to be a list of 2-element lists? e.g.
> > [
> >  ['k', 'v'],
> >  ['build_archive_url', 'http://example.com/'],
> > ]
> > 
> > If so, you can convert it to a dict, e.g.
> > 
> > >>> dict([['k', 'v'], ['k', 'v2']])
> > {'k', 'v2'}
> > 
> > So, this could be changed to something like:
> > 
> > build_properties = dict(build.get('properties', []))
> > if build_properties.get('build_archive_url') == self.build_url:
> >   current_build = build
> >   self.build_number = build_number
> 
> Thanks! I don't know how I didn't see it.
> Done.
> 
>
https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
> scripts/slave/recipe_modules/auto_bisect/revision_state.py:293: current_build
=
> json.loads(fetch_result.stdout or '{}')
> On 2016/03/04 at 22:06:28, qyearsley wrote:
> > This section of code (where the URL is put together, and a request for build
> details JSON is made and then that JSON is parsed) is repeated -- it should be
> extracted to a separate method (which takes a base URL and build number, and
> returns the parsed data).
> 
> Done.
> 
>
https://codereview.chromium.org/1758603004/diff/1/scripts/slave/recipe_module...
> scripts/slave/recipe_modules/auto_bisect/revision_state.py:294: return
> current_build.get('results') in [2, 3, 4]
> On 2016/03/04 at 22:06:28, qyearsley wrote:
> > These could be set as constants, ideally with some reference as to where
they
> come from. e.g.
> > 
> > # Buildbot job result codes.
> > # See http://docs.buildbot.net/current/developer/results.html
> > SUCCESS, WARNINGS, FAILURE, SKIPPED, EXCEPTION = range(5)
> > 
> > I'm not sure if that's correct though.
> 
> Works for me.

Ping.

[prasadv, 2016-03-16 22:02:30]

lgtm

[RobertoCN, 2016-03-16 22:17:40]

The CQ bit was checked by robertocn@chromium.org

[commit-bot: I haz the power, 2016-03-16 22:17:46]

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

[qyearsley, 2016-03-17 17:24:39]

lgtm

[qyearsley, 2016-03-17 17:24:44]

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

[commit-bot: I haz the power, 2016-03-17 17:24:59]

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

[commit-bot: I haz the power, 2016-03-17 17:33:05]

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

[commit-bot: I haz the power, 2016-03-17 17:33:06]

Dry run: Try jobs failed on following builders:
  Build Presubmit on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Build%20Presubmit/build...)

[RobertoCN, 2016-03-22 22:34:39]

The CQ bit was checked by robertocn@chromium.org

[RobertoCN, 2016-03-22 22:34:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
prasadv@chromium.org, qyearsley@chromium.org
Link to the patchset: https://codereview.chromium.org/1758603004/#ps60001
(title: "Updating expectations after rebase and sync. Yet again.")

[commit-bot: I haz the power, 2016-03-22 22:34:46]

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

[commit-bot: I haz the power, 2016-03-22 22:38:47]

Description was changed from

==========
Checking for failure in requested builds.

When the bisect requests a build that fails, rather than time out waiting
for it, we now query buildbot and handle failures.

R=prasadv,qyearsley
BUG=591536
==========

to

==========
Checking for failure in requested builds.

When the bisect requests a build that fails, rather than time out waiting
for it, we now query buildbot and handle failures.

R=prasadv,qyearsley
BUG=591536

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

[commit-bot: I haz the power, 2016-03-22 22:38:48]

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