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>

2 files changed, 13 insertions, 11 deletions

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())