Move setting builder status to inflight to the end of the stage

This is desirable for the reasons below:
  1. When a slave fails midway through the sync stage, we often
     have to delete the status file in GS in order to relaunch
     the slave. By moving the status change to the end of the
     stage, we avoid hitting this situation again.

  2. If a builder did not start ("inflight" was never set), we
     consider it an infrastructure failure. If the builder set
     status to inflight then timed out, we think the CLs may be
     at fault. However, as mentioned in (1), we have sync failures
     often due to gerrit/git problems, which are valid infra
     failures. Moving the status change to the end of the stage
     helps CQ categorize the failure more accurately.

  3. CQ slave records CLs it picks up in the sync stage. If the
     build  fails before the recording completes, we can detect that
     the pickup information is inaccurate (and should not be used)
     if the builder status is not set.

BUG=chromium:422639
TEST=`cbuildbot/run_tests`

Change-Id: I23e9999b016baea7d40cf62ef77b85057a7d4f33
Reviewed-on: https://chromium-review.googlesource.com/227654
Tested-by: Yu-Ju Hong <yjhong@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>
Commit-Queue: Yu-Ju Hong <yjhong@chromium.org>

5 files changed, 16 insertions, 28 deletions

diff --git a/cbuildbot/lkgm_manager.py b/cbuildbot/lkgm_manager.py
index 84f909668..02d064a54 100644
--- a/cbuildbot/lkgm_manager.py
+++ b/cbuildbot/lkgm_manager.py
@@ -412,7 +412,7 @@ class LKGMManager(manifest_version.BuildSpecsManager):
       raise manifest_version.GenerateBuildSpecException(last_error)
 
   def CreateFromManifest(self, manifest, retries=manifest_version.NUM_RETRIES,
-                         dashboard_url=None, build_id=None):
+                         build_id=None):
     """Sets up an lkgm_manager from the given manifest.
 
     This method sets up an LKGM manager and publishes a new manifest to the
@@ -424,7 +424,6 @@ class LKGMManager(manifest_version.BuildSpecsManager):
         is named with the given version we want to create a new manifest from
         i.e R20-1920.0.1-rc7.xml where R20-1920.0.1-rc7 is the version.
       retries: Number of retries for updating the status.
-      dashboard_url: Optional url linking to builder dashboard for this build.
       build_id: Optional integer cidb build id of the build publishing the
                 manifest.
 
@@ -445,7 +444,6 @@ class LKGMManager(manifest_version.BuildSpecsManager):
         version = os.path.splitext(os.path.basename(manifest))[0]
         logging.info('Publishing filtered build spec')
         self.PublishManifest(new_manifest, version, build_id=build_id)
-        self.SetInFlight(version, dashboard_url=dashboard_url)
         self.current_version = version
         return self.GetLocalManifest(version)
       except cros_build_lib.RunCommandError as e:
@@ -455,12 +453,10 @@ class LKGMManager(manifest_version.BuildSpecsManager):
     else:
       raise manifest_version.GenerateBuildSpecException(last_error)
 
-  def GetLatestCandidate(self, dashboard_url=None, timeout=10 * 60):
+  def GetLatestCandidate(self, timeout=10 * 60):
     """Gets and syncs to the next candiate manifest.
 
     Args:
-      retries: Number of retries for updating the status
-      dashboard_url: Optional url linking to builder dashboard for this build.
       timeout: The timeout in seconds.
 
     Returns:
@@ -501,7 +497,6 @@ class LKGMManager(manifest_version.BuildSpecsManager):
 
     if version_to_build:
       logging.info('Starting build spec: %s', version_to_build)
-      self.SetInFlight(version_to_build, dashboard_url=dashboard_url)
       self.current_version = version_to_build
 
       # Actually perform the sync.
diff --git a/cbuildbot/lkgm_manager_unittest.py b/cbuildbot/lkgm_manager_unittest.py
index 896797d63..6573f516f 100755
--- a/cbuildbot/lkgm_manager_unittest.py
+++ b/cbuildbot/lkgm_manager_unittest.py
@@ -307,7 +307,6 @@ class LKGMManagerTest(cros_test_lib.MoxTempDirTestCase):
                              'InitializeManifestVariables')
     self.mox.StubOutWithMock(lkgm_manager.LKGMManager, 'CheckoutSourceCode')
     self.mox.StubOutWithMock(lkgm_manager.LKGMManager, 'PushSpecChanges')
-    self.mox.StubOutWithMock(lkgm_manager.LKGMManager, 'SetInFlight')
 
     my_info = lkgm_manager._LKGMCandidateInfo('1.2.3')
     most_recent_candidate = lkgm_manager._LKGMCandidateInfo('1.2.3-rc12')
@@ -318,8 +317,6 @@ class LKGMManagerTest(cros_test_lib.MoxTempDirTestCase):
     lkgm_manager.LKGMManager.GetCurrentVersionInfo().AndReturn(my_info)
     lkgm_manager.LKGMManager.InitializeManifestVariables(my_info)
 
-    lkgm_manager.LKGMManager.SetInFlight(most_recent_candidate.VersionString(),
-                                         dashboard_url=None)
     repository.RepoRepository.Sync(
         self._GetPathToManifest(most_recent_candidate))
 
@@ -339,7 +336,6 @@ class LKGMManagerTest(cros_test_lib.MoxTempDirTestCase):
                              'InitializeManifestVariables')
     self.mox.StubOutWithMock(lkgm_manager.LKGMManager, 'CheckoutSourceCode')
     self.mox.StubOutWithMock(lkgm_manager.LKGMManager, 'PushSpecChanges')
-    self.mox.StubOutWithMock(lkgm_manager.LKGMManager, 'SetInFlight')
 
     my_info = lkgm_manager._LKGMCandidateInfo('1.2.4')
     most_recent_candidate = lkgm_manager._LKGMCandidateInfo('1.2.4-rc12',
@@ -350,8 +346,6 @@ class LKGMManagerTest(cros_test_lib.MoxTempDirTestCase):
     lkgm_manager.LKGMManager.GetCurrentVersionInfo().AndReturn(my_info)
     lkgm_manager.LKGMManager.InitializeManifestVariables(my_info)
 
-    lkgm_manager.LKGMManager.SetInFlight(most_recent_candidate.VersionString(),
-                                         dashboard_url=None)
     repository.RepoRepository.Sync(
         self._GetPathToManifest(most_recent_candidate))
 
diff --git a/cbuildbot/manifest_version.py b/cbuildbot/manifest_version.py
index 45cbed8fc..3b32a9467 100644
--- a/cbuildbot/manifest_version.py
+++ b/cbuildbot/manifest_version.py
@@ -755,13 +755,11 @@ class BuildSpecsManager(object):
     """Syncs the cros source to the latest git hashes for the branch."""
     self.cros_source.Sync(self.manifest)
 
-  def GetNextBuildSpec(self, retries=NUM_RETRIES, dashboard_url=None,
-                       build_id=None):
+  def GetNextBuildSpec(self, retries=NUM_RETRIES, build_id=None):
     """Returns a path to the next manifest to build.
 
     Args:
       retries: Number of retries for updating the status.
-      dashboard_url: Optional url linking to builder dashboard for this build.
       build_id: Optional integer cidb id of this build, which will be used to
                 annotate the manifest-version commit if one is created.
 
@@ -791,7 +789,6 @@ class BuildSpecsManager(object):
         else:
           version = self.latest_unprocessed
 
-        self.SetInFlight(version, dashboard_url=dashboard_url)
         self.current_version = version
         return self.GetLocalManifest(version)
       except cros_build_lib.RunCommandError as e:
diff --git a/cbuildbot/stages/sync_stages.py b/cbuildbot/stages/sync_stages.py
index 6050db604..503d47266 100644
--- a/cbuildbot/stages/sync_stages.py
+++ b/cbuildbot/stages/sync_stages.py
@@ -514,9 +514,7 @@ class ManifestVersionedSyncStage(SyncStage):
 
     build_id = self._run.attrs.metadata.GetDict().get('build_id')
 
-    to_return = self.manifest_manager.GetNextBuildSpec(
-        dashboard_url=self.ConstructDashboardURL(),
-        build_id=build_id)
+    to_return = self.manifest_manager.GetNextBuildSpec(build_id=build_id)
     previous_version = self.manifest_manager.GetLatestPassingSpec()
     target_version = self.manifest_manager.current_version
 
@@ -591,6 +589,13 @@ class ManifestVersionedSyncStage(SyncStage):
         next_manifest, filter_cros=self._run.options.local) as new_manifest:
       self.ManifestCheckout(new_manifest)
 
+    # Set the status inflight at the end of the ManifestVersionedSync
+    # stage. This guarantees that all syncing has completed.
+    if self.manifest_manager:
+      self.manifest_manager.SetInFlight(
+          self.manifest_manager.current_version,
+          dashboard_url=self.ConstructDashboardURL())
+
 
 class MasterSlaveLKGMSyncStage(ManifestVersionedSyncStage):
   """Stage that generates a unique manifest file candidate, and sync's to it.
@@ -664,12 +669,10 @@ class MasterSlaveLKGMSyncStage(ManifestVersionedSyncStage):
           chrome_version=self._chrome_version,
           build_id=build_id)
       if MasterSlaveLKGMSyncStage.sub_manager:
-        MasterSlaveLKGMSyncStage.sub_manager.CreateFromManifest(
-            manifest, dashboard_url=self.ConstructDashboardURL())
+        MasterSlaveLKGMSyncStage.sub_manager.CreateFromManifest(manifest)
       return manifest
     else:
       return self.manifest_manager.GetLatestCandidate(
-          dashboard_url=self.ConstructDashboardURL(),
           timeout=self.LATEST_CANDIDATE_TIMEOUT_SECONDS)
 
   def GetLatestChromeVersion(self):
@@ -809,12 +812,10 @@ class CommitQueueSyncStage(MasterSlaveLKGMSyncStage):
                                                           build_id=build_id)
       if MasterSlaveLKGMSyncStage.sub_manager:
         MasterSlaveLKGMSyncStage.sub_manager.CreateFromManifest(
-            manifest, dashboard_url=self.ConstructDashboardURL(),
-            build_id=build_id)
+            manifest, build_id=build_id)
 
     else:
-      manifest = self.manifest_manager.GetLatestCandidate(
-          dashboard_url=self.ConstructDashboardURL())
+      manifest = self.manifest_manager.GetLatestCandidate()
       if manifest:
         if self._run.config.build_before_patching:
           pre_build_passed = self.RunPrePatchBuild()
diff --git a/cbuildbot/stages/sync_stages_unittest.py b/cbuildbot/stages/sync_stages_unittest.py
index 91220b2ee..2c073f541 100755
--- a/cbuildbot/stages/sync_stages_unittest.py
+++ b/cbuildbot/stages/sync_stages_unittest.py
@@ -57,6 +57,7 @@ class ManifestVersionedSyncStageTest(generic_stages_unittest.AbstractStageTest):
     self.incr_type = 'branch'
     self.next_version = 'next_version'
     self.sync_stage = None
+    self.PatchObject(manifest_version.BuildSpecsManager, 'SetInFlight')
 
     repo = repository.RepoRepository(
       self.source_repo, self.tempdir, self.branch)
@@ -91,7 +92,6 @@ class ManifestVersionedSyncStageTest(generic_stages_unittest.AbstractStageTest):
     sync_stages.ManifestVersionedSyncStage.Initialize()
     self.manager.GetNextBuildSpec(
         build_id=MOCK_BUILD_ID,
-        dashboard_url=self.sync_stage.ConstructDashboardURL()
         ).AndReturn(self.next_version)
     self.manager.GetLatestPassingSpec().AndReturn(None)
 
@@ -138,6 +138,7 @@ class BaseCQTestCase(generic_stages_unittest.StageTest):
     """Setup patchers for specified bot id."""
     # Mock out methods as needed.
     self.PatchObject(lkgm_manager, 'GenerateBlameList')
+    self.PatchObject(lkgm_manager.LKGMManager, 'SetInFlight')
     self.PatchObject(repository.RepoRepository, 'ExportManifest',
                      return_value=self.MANIFEST_CONTENTS, autospec=True)
     self.StartPatcher(git_unittest.ManifestMock())