Refactoring scripts that wait for buildbot jobs to complete.

scripts/slave/recipe_modules/auto_bisect/bisector.py

 # Copyright 2015 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import json
 import re
 
 from . import bisect_results
 from . import depot_config
 from . import revision_state
@@ skipping 435 matching lines @@
     """Waits for all revisions in list to finish."""
     while any([r.in_progress for r in revision_list]):
       revision_list.remove(self.wait_for_any(revision_list))
 
   def sleep_until_next_revision_ready(self, revision_list):
     """Produces a single step that sleeps until any revision makes progress.
 
     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)."""
     while True:
       if not revision_list or not any(
           r.in_progress or r.tested for r in revision_list):  # pragma: no cover
         break
 
       finished_revision = self.sleep_until_next_revision_ready(revision_list)
 
@@ skipping 47 matching lines @@
     recipe_tester_name = self.bisect_config.get('recipe_tester_name')
     original_bot_name = self.bisect_config.get('original_bot_name', '')
     if recipe_tester_name:
       return recipe_tester_name
     elif 'win' in original_bot_name:  # pragma: no cover
       return 'win64_nv_tester'
     else:  # pragma: no cover
       # 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()
     if 'win' in bot_name:
       if any(b in bot_name for b in ['x64', 'gpu']):
         return 'winx64_bisect_builder'
       return 'win_perf_bisect_builder'
 
     if 'android' in bot_name:
       if 'nexus9' in bot_name:
@@ skipping 28 matching lines @@
 
     We have seen on several occasions that the local master branch gets reset
     to previous revisions and also detached head states. Running this should
     take care of either situation.
     """
     # TODO(robertocn): Investigate what causes the states mentioned in the
     # docstring in the first place.
     self.api.m.git('update-ref', 'refs/heads/master',
                    'refs/remotes/origin/master')
     self.api.m.git('checkout', 'master', cwd=self.api.m.path['checkout'])