Refactoring scripts that wait for buildbot jobs to complete.

scripts/slave/recipe_modules/auto_bisect/bisector.py

Index: scripts/slave/recipe_modules/auto_bisect/bisector.py
diff --git a/scripts/slave/recipe_modules/auto_bisect/bisector.py b/scripts/slave/recipe_modules/auto_bisect/bisector.py
index 13636398fe4bf3ae8d6dfe1148b8d057fb1b85cf..521e8cce43722212024ff55d3b8078a642768201 100644
--- a/scripts/slave/recipe_modules/auto_bisect/bisector.py
+++ b/scripts/slave/recipe_modules/auto_bisect/bisector.py
@@ -453,46 +453,61 @@ class Bisector(object):
     A revision is considered to make progress when a build file is uploaded to
     the appropriate bucket, or when buildbot test job is complete.
     """
-    gsutil_path = self.api.m.gsutil.get_gsutil_path()
-    name = 'Waiting for revision ' + revision_list[0].revision_string
-    if len(revision_list) > 1:
-      name += ' and %d other revision(s).' % (len(revision_list) - 1)
+
+    revision_mapping = {}
+    gs_jobs = []
+    buildbot_jobs = []
+
+    for revision in revision_list:
+      url = revision.get_next_url()
+      buildbot_job = revision.get_buildbot_locator()
+      if url:
+        gs_jobs.append({'type': 'gs', 'location': url})
+        revision_mapping[url] = revision
+      if buildbot_job:
+        buildbot_job['type'] = 'buildbot'

[prasadv, 2015/09/17 22:33:34]

Just curious, why 'type' is not added in get_buildbot_locator()?
why do you want to do this here?

[RobertoCN, 2015/09/19 00:32:54]

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.

+        buildbot_jobs.append(buildbot_job)
+        revision_mapping[buildbot_job['job_name']] = revision
+
+    jobs_config = {'jobs': buildbot_jobs + gs_jobs}
+
     script = self.api.resource('wait_for_any.py')
-    args_list = [gsutil_path]
-    url_mapping = {r.get_next_url(): r for r in revision_list}
-    bb_locator_mapping = {r.get_buildbot_locator(): r for r in revision_list}
-    bb_locator_list = bb_locator_mapping.keys()
-    url_list = url_mapping.keys()
-    args_list += [url for url in url_list if url and url is not None]
-    args_list += [entry for entry in bb_locator_list
-                  if entry and entry is not None]
-    args_list.append(
-        '--timeout=%d' % (
-            self.get_build_timeout_minutes() * 60))
+    args_list = [self.api.m.gsutil.get_gsutil_path()] if gs_jobs else []
+
     try:
+      step_name = 'Waiting for revision ' + revision_list[0].revision_string
+      if len(revision_list) > 1:
+        step_name += ' and %d other revision(s).' % (len(revision_list) - 1)
       step_result = self.api.m.python(
-          str(name),
+          str(step_name),
           script,
           args_list,
-          stdout=self.api.m.raw_io.output())
+          stdout=self.api.m.json.output(),
+          stdin=self.api.m.json.input(jobs_config))
     except self.api.m.step.StepFailure:  # pragma: no cover
       # StepFailure is interpreted as a timeout.
       for revision in revision_list:
         revision.status = revision_state.RevisionState.FAILED
       return None

[prasadv, 2015/09/17 22:33:34]

When this happens what should be the behavior of wait_for_any?

[RobertoCN, 2015/09/19 00:32:54]

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.

[RobertoCN, 2015/09/19 00:32:54]

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)

-    step_output = step_result.stdout or 'Build finished: Unknown'
-    for line in step_output.splitlines():
-      if line.startswith('Build finished: '):
-        finished_url = line[len('Build finished: '):].strip()
-        return url_mapping.get(finished_url)
-      if line.startswith('Failed build url: '):  # pragma: no cover
-        url = line[len('Failed build url: '):].strip()
-        step_result.presentation.links['Failed build'] = url

[prasadv, 2015/09/17 22:33:34]

This is missing.

[RobertoCN, 2015/09/19 00:32:54]

Done.

-      if line.startswith('Build failed: '):  # pragma: no cover
-        failed_build_locator = line[len('Build failed: '):].strip()
-        failed_build = bb_locator_mapping.get(failed_build_locator)
-        failed_build.status = revision_state.RevisionState.FAILED
-        return failed_build
+    if not step_result.stdout:
+      return None
+    step_results = step_result.stdout
+    failed_jobs = step_results.get('failed', [])
+    completed_jobs = step_results.get('completed', [])
+    assert failed_jobs or completed_jobs
+    # Marked all failed builds as failed
+    for job in failed_jobs:
+      last_failed_revision = revision_mapping[str(job.get(
+          'location', job.get('job_name')))]
+      last_failed_revision.status = revision_state.RevisionState.FAILED
+
+    # Return a completed job if availavle
+    for job in completed_jobs:
+      return revision_mapping[str(job.get(
+          'location', job.get('job_name')))]
+
+    # Or return any of the failed revisions
+    return last_failed_revision
 
   def wait_for_any(self, revision_list):
     """Waits for any of the revisions in the list to finish its job(s)."""
@@ -560,11 +575,6 @@ class Bisector(object):
       # Reasonable fallback
       return 'linux_perf_tester'
 
-  def get_build_timeout_minutes(self):
-    if 'win' in self.get_perf_tester_name():
-      return 4 * 60
-    return 2 * 60
-
   def get_builder_bot_for_this_platform(self):
     # TODO(prasadv): We should refactor these codes to remove hard coded values.
     bot_name = self.get_perf_tester_name()