cidb: eliminate CL status files

This CL eliminates use of CL status files and status counters.

For the CQ, the status counters were used in generating summary text
that included the number of prior failures (and in the past also in
identifying suspects, but this is no longer the case). The counters have
been replaced by counting the number of occurences of KICKED_OUT actions
for the change.

For the pre-CQ, status files were used to coordinate and track state, by
the pre-cq-launcher, the pre-cq-group, and master-paladin. These are
replaced by status transition actions that are recorded to cidb. The
pre-cq status of a given change is the status corresponding to the most
recent pre-cq status action.

 - Rename PRE_CQ_BUILDER_NAME for consistency.
 - Rename GetCLStatus to GetCLPreCQStatus, update all its callers, and
   rewrite it to use the last known pre-CQ action for a change as its
   pre-cq status.
 - Rename UpdateCLStatus to UpdateCLPreCQStatus, update all its callers
   (deleting callers that were CQ-specific), and rewrite it to only
   record a status transition action to cidb.
 - Delete _FindPreviouslyFailedChanges, which was not used anywhere.
 - Do not update a change's pass_count, this was only used when printing
   CL summary links, and not clear why this should ever be nonzero.
 - Delete CLStatusMock, instead use FakeCIDBConnection in unit
   tests and do not mock out the CL status methods.

BUG=chromium:410546
TEST=`git grep PRE_CQ_BUILDER_NAME` -> no results
`git grep GetCLStatus` -> no results
`git grep UpdateCLStatus` -> no results
`git grep _FindPreviouslyFailedChanges` -> no results
New unit tests of GetCLActionCount, UpdateCLPreCQStatus, and
GetCLPreCQStatus.
Existing unit tests fixed and passing.
Remote trybots.
Local trybots.

Change-Id: I91752207782ff7278e0a4ada4388fcf3509b1860
Reviewed-on: https://chromium-review.googlesource.com/221956
Reviewed-by: Aviv Keshet <akeshet@chromium.org>
Tested-by: Aviv Keshet <akeshet@chromium.org>

3 files changed, 122 insertions, 110 deletions

diff --git a/cbuildbot/stages/report_stages_unittest.py b/cbuildbot/stages/report_stages_unittest.py
index 4da178c00..a9926e849 100755
--- a/cbuildbot/stages/report_stages_unittest.py
+++ b/cbuildbot/stages/report_stages_unittest.py
@@ -112,8 +112,6 @@ class ReportStageTest(generic_stages_unittest.AbstractStageTest):
       self.StartPatcher(mock.patch.object(*cmd, autospec=True))
 
     self.StartPatcher(BuilderRunMock())
-    self.cq = sync_stages_unittest.CLStatusMock()
-    self.StartPatcher(self.cq)
     self.sync_stage = None
 
     # Set up a general purpose cidb mock. Tests with more specific
diff --git a/cbuildbot/stages/sync_stages.py b/cbuildbot/stages/sync_stages.py
index 47d661120..5826bc718 100644
--- a/cbuildbot/stages/sync_stages.py
+++ b/cbuildbot/stages/sync_stages.py
@@ -19,7 +19,6 @@ from chromite.cbuildbot import failures_lib
 from chromite.cbuildbot import constants
 from chromite.cbuildbot import lkgm_manager
 from chromite.cbuildbot import manifest_version
-from chromite.cbuildbot import metadata_lib
 from chromite.cbuildbot import repository
 from chromite.cbuildbot import tree_status
 from chromite.cbuildbot import trybot_patch_pool
@@ -701,7 +700,7 @@ class CommitQueueSyncStage(MasterSlaveLKGMSyncStage):
     # First, look for changes that were tested by the Pre-CQ.
     changes_to_test = []
     for change in changes:
-      status = pool.GetCLStatus(PRE_CQ, change)
+      status = pool.GetCLPreCQStatus(change)
       if status == validation_pool.ValidationPool.STATUS_PASSED:
         changes_to_test.append(change)
 
@@ -818,23 +817,23 @@ class PreCQSyncStage(SyncStage):
     if len(self.pool.changes) == 0:
       cros_build_lib.Die('No changes have been applied.')
 
-    # Mark changes with pre-cq status inflight in database.
-    # This will be replaced by a call to UpdateCLPreCQStatus in a future CL.
+    # Mark changes that are not passed with pre-cq status inflight.
     if (cidb.CIDBConnectionFactory.IsCIDBSetup() and
         cidb.CIDBConnectionFactory.GetCIDBConnectionForBuilder()):
       build_id = self._run.attrs.metadata.GetValue('build_id')
-      db = cidb.CIDBConnectionFactory.GetCIDBConnectionForBuilder()
       for change in self.pool.changes:
-        db.InsertCLActions(
-            build_id,
-            [metadata_lib.GetCLActionTuple(
-                change, constants.CL_ACTION_PRE_CQ_INFLIGHT)])
+        current_status = validation_pool.ValidationPool.GetCLPreCQStatus(
+            change)
+        if current_status != validation_pool.ValidationPool.STATUS_PASSED:
+          validation_pool.ValidationPool.UpdateCLPreCQStatus(
+              change, validation_pool.ValidationPool.STATUS_INFLIGHT,
+              build_id)
 
 
 class PreCQLauncherStage(SyncStage):
   """Scans for CLs and automatically launches Pre-CQ jobs to test them."""
 
-  # The CL is currently being tested by a Pre-CQ builder.
+  # The CL is currently being tested by a Pre-CQ trybot.
   STATUS_INFLIGHT = validation_pool.ValidationPool.STATUS_INFLIGHT
 
   # The CL has passed the Pre-CQ.
@@ -959,16 +958,14 @@ class PreCQLauncherStage(SyncStage):
 
             pool.SendNotification(change, '%(details)s', details=msg)
             pool.RemoveCommitReady(change)
-            pool.UpdateCLStatus(PRE_CQ, change, self.STATUS_FAILED,
-                                self._run.options.debug,
-                                build_id=self._build_id)
+            pool.UpdateCLPreCQStatus(change, self.STATUS_FAILED,
+                                     self._build_id)
             self.retried.discard(change)
           else:
             # Try the change again.
             self.retried.add(change)
-            pool.UpdateCLStatus(PRE_CQ, change, self.STATUS_WAITING,
-                                self._run.options.debug,
-                                build_id=self._build_id)
+            pool.UpdateCLPreCQStatus(change, self.STATUS_WAITING,
+                                     self._build_id)
       elif status == self.STATUS_INFLIGHT:
         # Once a Pre-CQ run actually starts, it'll set the status to
         # STATUS_INFLIGHT.
@@ -987,18 +984,16 @@ class PreCQLauncherStage(SyncStage):
 
           pool.SendNotification(change, '%(details)s', details=msg)
           pool.RemoveCommitReady(change)
-          pool.UpdateCLStatus(PRE_CQ, change, self.STATUS_FAILED,
-                              self._run.options.debug,
-                              build_id=self._build_id)
+          pool.UpdateCLPreCQStatus(change, self.STATUS_FAILED,
+                                   self._build_id)
       elif status == self.STATUS_FAILED:
         # The Pre-CQ run failed for this change. It's possible that we got
         # unlucky and this change was just marked as 'Not Ready' by a bot. To
         # test this, mark the CL as 'waiting' for now. If the CL is still marked
         # as 'Ready' next time we check, we'll know the CL is truly still ready.
         busy.add(change)
-        pool.UpdateCLStatus(PRE_CQ, change, self.STATUS_WAITING,
-                            self._run.options.debug,
-                            build_id=self._build_id)
+        pool.UpdateCLPreCQStatus(change, self.STATUS_WAITING,
+                                 self._build_id)
         self._PrintPatchStatus(change, status)
       elif status == self.STATUS_PASSED:
         passed.add(change)
@@ -1016,7 +1011,7 @@ class PreCQLauncherStage(SyncStage):
       pool: ValidationPool corresponding to |plan|.
       plan: The list of patches to test in the Pre-CQ run.
     """
-    cmd = ['cbuildbot', '--remote', constants.PRE_CQ_BUILDER_NAME,
+    cmd = ['cbuildbot', '--remote', constants.PRE_CQ_GROUP_CONFIG,
            '--timeout', str(self.INFLIGHT_DELAY * 60)]
     if self._run.options.debug:
       cmd.append('--debug')
@@ -1025,10 +1020,9 @@ class PreCQLauncherStage(SyncStage):
       self._PrintPatchStatus(patch, 'testing')
     cros_build_lib.RunCommand(cmd, cwd=self._build_root)
     for patch in plan:
-      if pool.GetCLStatus(PRE_CQ, patch) != self.STATUS_PASSED:
-        pool.UpdateCLStatus(PRE_CQ, patch, self.STATUS_LAUNCHING,
-                            self._run.options.debug,
-                            build_id=self._build_id)
+      if pool.GetCLPreCQStatus(patch) != self.STATUS_PASSED:
+        pool.UpdateCLPreCQStatus(patch, self.STATUS_LAUNCHING,
+                                 self._build_id)
 
   def GetDisjointTransactionsToTest(self, pool, changes, status_map):
     """Get the list of disjoint transactions to test.
@@ -1082,7 +1076,7 @@ class PreCQLauncherStage(SyncStage):
     # Get change status.
     status_map = {}
     for change in changes:
-      status = pool.GetCLStatus(PRE_CQ, change)
+      status = pool.GetCLPreCQStatus(change)
       status_map[change] = status
 
     # Launch trybots for manifest changes.
diff --git a/cbuildbot/stages/sync_stages_unittest.py b/cbuildbot/stages/sync_stages_unittest.py
index 7bf23012b..20a2d8b61 100755
--- a/cbuildbot/stages/sync_stages_unittest.py
+++ b/cbuildbot/stages/sync_stages_unittest.py
@@ -26,11 +26,12 @@ from chromite.cbuildbot.stages import sync_stages
 from chromite.cbuildbot.stages import generic_stages_unittest
 from chromite.lib import cros_build_lib_unittest
 from chromite.lib import cros_test_lib
+from chromite.lib import cidb
+from chromite.lib import fake_cidb
 from chromite.lib import gerrit
 from chromite.lib import git_unittest
 from chromite.lib import gob_util
 from chromite.lib import osutils
-from chromite.lib import partial_mock
 from chromite.lib import timeout_util
 
 
@@ -127,7 +128,6 @@ class MockPatch(mock.MagicMock):
 
 class BaseCQTestCase(generic_stages_unittest.StageTest):
   """Helper class for testing the CommitQueueSync stage"""
-  PALADIN_BOT_ID = None
   MANIFEST_CONTENTS = '<manifest/>'
 
   def setUp(self):
@@ -160,9 +160,16 @@ class BaseCQTestCase(generic_stages_unittest.StageTest):
     self.PatchObject(validation_pool.ValidationPool, 'ReloadChanges',
                      side_effect=lambda x: x)
 
+    # Create and set up a fake cidb instance.
+    self.fake_db = fake_cidb.FakeCIDBConnection()
+    cidb.CIDBConnectionFactory.SetupMockCidb(self.fake_db)
+
     self.sync_stage = None
     self._Prepare()
 
+  def tearDown(self):
+    cidb.CIDBConnectionFactory.ClearMock()
+
   def _Prepare(self, bot_id=None, **kwargs):
     super(BaseCQTestCase, self)._Prepare(bot_id, **kwargs)
 
@@ -185,6 +192,9 @@ class BaseCQTestCase(generic_stages_unittest.StageTest):
       runs: The maximum number of times to allow validation_pool.AcquirePool
             to wait for additional changes. runs=0 means never wait for
             additional changes. Default: 0.
+
+    Returns:
+      A list of MockPatch objects which were created and used in PerformSync.
     """
     p = MockPatch(remote=remote, tracking_branch=tracking_branch)
     my_patches = [p] * num_patches
@@ -207,6 +217,8 @@ class BaseCQTestCase(generic_stages_unittest.StageTest):
                      side_effect=exit_it)
     self.sync_stage.PerformStage()
 
+    return my_patches
+
   def ReloadPool(self):
     """Save the pool to disk and reload it."""
     with tempfile.NamedTemporaryFile() as f:
@@ -243,24 +255,40 @@ class MasterCQSyncTestCase(BaseCQTestCase):
                      return_value=self.manifest_path, autospec=True)
 
   def _testCommitNonManifestChange(self, **kwargs):
-    """Test the commit of a non-manifest change."""
+    """Test the commit of a non-manifest change.
+
+    Returns:
+      List of MockPatch objects that were used in PerformSync
+    """
     # Setting tracking_branch=foo makes this a non-manifest change.
     kwargs.setdefault('committed', True)
-    self.PerformSync(tracking_branch='foo', **kwargs)
+    return self.PerformSync(tracking_branch='foo', **kwargs)
 
   def _testFailedCommitOfNonManifestChange(self):
-    """Test that the commit of a non-manifest change fails."""
-    self._testCommitNonManifestChange(committed=False)
+    """Test that the commit of a non-manifest change fails.
+
+    Returns:
+      List of MockPatch objects that were used in PerformSync
+    """
+    return self._testCommitNonManifestChange(committed=False)
 
   def _testCommitManifestChange(self, **kwargs):
-    """Test committing a change to a project that's part of the manifest."""
+    """Test committing a change to a project that's part of the manifest.
+
+    Returns:
+      List of MockPatch objects that were used in PerformSync
+    """
     self.PatchObject(validation_pool.ValidationPool, '_FilterNonCrosProjects',
                      side_effect=lambda x, _: (x, []))
-    self.PerformSync(**kwargs)
+    return self.PerformSync(**kwargs)
 
   def _testDefaultSync(self):
-    """Test basic ability to sync with standard options."""
-    self.PerformSync()
+    """Test basic ability to sync with standard options.
+
+    Returns:
+      List of MockPatch objects that were used in PerformSync
+    """
+    return self.PerformSync()
 
 
 class MasterCQSyncTest(MasterCQSyncTestCase):
@@ -268,19 +296,19 @@ class MasterCQSyncTest(MasterCQSyncTestCase):
 
   def testCommitNonManifestChange(self):
     """See MasterCQSyncTestCase"""
-    self._testCommitNonManifestChange()
+    return self._testCommitNonManifestChange()
 
   def testFailedCommitOfNonManifestChange(self):
     """See MasterCQSyncTestCase"""
-    self._testFailedCommitOfNonManifestChange()
+    return self._testFailedCommitOfNonManifestChange()
 
   def testCommitManifestChange(self):
     """See MasterCQSyncTestCase"""
-    self._testCommitManifestChange()
+    return self._testCommitManifestChange()
 
   def testDefaultSync(self):
     """See MasterCQSyncTestCase"""
-    self._testDefaultSync()
+    return self._testDefaultSync()
 
   def testReload(self):
     """Test basic ability to sync and reload the patches from disk."""
@@ -300,99 +328,85 @@ class MasterCQSyncTest(MasterCQSyncTestCase):
         mock.ANY, constants.THROTTLED_CQ_READY_QUERY,
         sort='lastUpdated')
 
-
-class CLStatusMock(partial_mock.PartialMock):
-  """Partial mock for CLStatus methods in ValidationPool."""
-
-  TARGET = 'chromite.cbuildbot.validation_pool.ValidationPool'
-  ATTRS = ('GetCLStatus', 'GetCLStatusCount', 'UpdateCLStatus',)
-
-  def __init__(self, treat_launching_as_inflight=False):
-    """CLStatusMock constructor.
-
-    Args:
-      treat_launching_as_inflight: When getting a CL's status via
-        GetCLStatus, treat any change with status LAUNCHING as if
-        it has status INFLIGHT. This simulates pre-cq tryjobs getting
-        immediately launched. Default: False.
-    """
-    partial_mock.PartialMock.__init__(self)
-    self.calls = {}
-    self.status = {}
-    self.status_count = {}
-    self._treat_launching_as_inflight = treat_launching_as_inflight
-
-  def GetCLStatus(self, _bot, change):
-    status = self.status.get(change)
-    if (self._treat_launching_as_inflight and
-        status == validation_pool.ValidationPool.STATUS_LAUNCHING):
-      return validation_pool.ValidationPool.STATUS_INFLIGHT
-    return status
-
-  def GetCLStatusCount(self, _bot, change, count, latest_patchset_only=True):
-    # pylint: disable=W0613
-    return self.status_count.get(change, 0)
-
-  def UpdateCLStatus(self, _bot, change, status, dry_run, build_id=None):
-    # pylint: disable=W0613
-    self.calls[status] = self.calls.get(status, 0) + 1
-    self.status[change] = status
-    self.status_count[change] = self.status_count.get(change, 0) + 1
-
-
 class PreCQLauncherStageTest(MasterCQSyncTestCase):
   """Tests for the PreCQLauncherStage."""
 
-  BOT_ID = 'pre-cq-launcher'
+  BOT_ID = constants.PRE_CQ_LAUNCHER_CONFIG
   STATUS_LAUNCHING = validation_pool.ValidationPool.STATUS_LAUNCHING
   STATUS_WAITING = validation_pool.ValidationPool.STATUS_WAITING
   STATUS_FAILED = validation_pool.ValidationPool.STATUS_FAILED
   STATUS_READY_TO_SUBMIT = validation_pool.ValidationPool.STATUS_READY_TO_SUBMIT
+  STATUS_INFLIGHT = validation_pool.ValidationPool.STATUS_INFLIGHT
 
   def setUp(self):
     self.PatchObject(time, 'sleep', autospec=True)
 
-  def _PrepareValidationPoolMock(self, auto_launch=False):
-    # pylint: disable-msg=W0201
-    self.pre_cq = CLStatusMock(treat_launching_as_inflight=auto_launch)
-    self.StartPatcher(self.pre_cq)
+  def _PrepareAutoLaunch(self):
+    """Cause CLs with launching status to be automatically launched."""
+    # Mock out UpdateCLPreCQStatus so that when a "Launching" action is
+    # recorded, automatically pretend to start a new build which records
+    # an "Inflight" action for the same change.
+    original_method = validation_pool.ValidationPool.UpdateCLPreCQStatus
+
+    def new_method(change, status, build_id):
+      original_method(change, status, build_id)
+      if (status == self.STATUS_LAUNCHING):
+        new_build_id = self.fake_db.InsertBuild('Pre cq group',
+                                                constants.WATERFALL_TRYBOT,
+                                                1,
+                                                constants.PRE_CQ_GROUP_CONFIG,
+                                                'bot-hostname')
+        original_method(change, self.STATUS_INFLIGHT,
+                        new_build_id)
+
+    self.PatchObject(validation_pool.ValidationPool, 'UpdateCLPreCQStatus',
+                     side_effect=new_method)
+
 
   def _Prepare(self, bot_id=None, **kwargs):
-    super(PreCQLauncherStageTest, self)._Prepare(bot_id, **kwargs)
+    build_id = self.fake_db.InsertBuild(
+        constants.PRE_CQ_LAUNCHER_NAME, constants.WATERFALL_INTERNAL, 1,
+        constants.PRE_CQ_LAUNCHER_CONFIG, 'bot-hostname')
+
+    super(PreCQLauncherStageTest, self)._Prepare(
+        bot_id, build_id=build_id, **kwargs)
 
     self.sync_stage = sync_stages.PreCQLauncherStage(self._run)
 
   def testCommitNonManifestChange(self):
     """See MasterCQSyncTestCase"""
-    self._PrepareValidationPoolMock()
     self._testCommitNonManifestChange()
 
   def testFailedCommitOfNonManifestChange(self):
     """See MasterCQSyncTestCase"""
-    self._PrepareValidationPoolMock()
     self._testFailedCommitOfNonManifestChange()
 
   def testCommitManifestChange(self):
     """See MasterCQSyncTestCase"""
-    self._PrepareValidationPoolMock()
     self._testCommitManifestChange()
 
   def testDefaultSync(self):
     """See MasterCQSyncTestCase"""
-    self._PrepareValidationPoolMock()
     self._testDefaultSync()
 
   def testTreeClosureIsOK(self):
     """Test that tree closures block commits."""
-    self._PrepareValidationPoolMock()
     self._testCommitNonManifestChange(tree_open=False)
 
   def testLaunchTrybot(self):
     """Test launching a trybot."""
-    self._PrepareValidationPoolMock()
-    self._testCommitManifestChange()
-    self.assertEqual(self.pre_cq.status.values(), [self.STATUS_LAUNCHING])
-    self.assertEqual(self.pre_cq.calls.keys(), [self.STATUS_LAUNCHING])
+    change = self._testCommitManifestChange()[0]
+
+    self.assertEqual(validation_pool.ValidationPool.GetCLPreCQStatus(change),
+                     self.STATUS_LAUNCHING)
+
+  def testLaunchTrybotWithAutolaunch(self):
+    """Test launching a trybot with auto-launch."""
+    self._PrepareAutoLaunch()
+    change = self._testCommitManifestChange()[0]
+
+    self.assertEqual(validation_pool.ValidationPool.GetCLPreCQStatus(change),
+                     self.STATUS_INFLIGHT)
 
   def runTrybotTest(self, launching=0, waiting=0, failed=0, runs=0):
     """Helper function for testing PreCQLauncher.
@@ -402,16 +416,24 @@ class PreCQLauncherStageTest(MasterCQSyncTestCase):
     LAUNCHING, WAITING, and FAILED |launching|, |waiting|, and |failed| times
     respectively.
     """
-    self._testCommitManifestChange(runs=runs)
-    self.assertEqual(self.pre_cq.calls.get(self.STATUS_LAUNCHING, 0), launching)
-    self.assertEqual(self.pre_cq.calls.get(self.STATUS_WAITING, 0), waiting)
-    self.assertEqual(self.pre_cq.calls.get(self.STATUS_FAILED, 0), failed)
-    self.assertEqual(sum(self.pre_cq.calls.values()),
-                     launching + waiting + failed)
+    change = self._testCommitManifestChange(runs=runs)[0]
+    # Count the number of recorded actions corresponding to launching, watiting,
+    # and failed, and ensure they are correct.
+    validation_pool.ValidationPool.ClearActionCache()
+
+    expected = (launching, waiting, failed)
+    actions = (constants.CL_ACTION_PRE_CQ_LAUNCHING,
+               constants.CL_ACTION_PRE_CQ_WAITING,
+               constants.CL_ACTION_PRE_CQ_FAILED)
+
+    for exp, action in zip(expected, actions):
+      self.assertEqual(
+          exp,
+          validation_pool.ValidationPool.GetCLActionCount(
+              change, [constants.PRE_CQ_LAUNCHER_CONFIG], action))
 
   def testLaunchTrybotTimesOutOnce(self):
     """Test what happens when a trybot launch times out."""
-    self._PrepareValidationPoolMock()
     it = itertools.chain([True], itertools.repeat(False))
     self.PatchObject(sync_stages.PreCQLauncherStage, '_HasLaunchTimedOut',
                      side_effect=it)
@@ -419,14 +441,13 @@ class PreCQLauncherStageTest(MasterCQSyncTestCase):
 
   def testLaunchTrybotTimesOutTwice(self):
     """Test what happens when a trybot launch times out."""
-    self._PrepareValidationPoolMock()
     self.PatchObject(sync_stages.PreCQLauncherStage, '_HasLaunchTimedOut',
                      return_value=True)
     self.runTrybotTest(launching=2, waiting=1, failed=1, runs=3)
 
   def testInflightTrybotTimesOutOnce(self):
     """Test what happens when an inflight trybot times out."""
-    self._PrepareValidationPoolMock(auto_launch=True)
+    self._PrepareAutoLaunch()
     it = itertools.chain([True], itertools.repeat(False))
     self.PatchObject(sync_stages.PreCQLauncherStage, '_HasInflightTimedOut',
                      side_effect=it)
@@ -434,8 +455,7 @@ class PreCQLauncherStageTest(MasterCQSyncTestCase):
 
   def testSubmit(self):
     """Test submission of patches."""
-    self._PrepareValidationPoolMock()
-    self.PatchObject(validation_pool.ValidationPool, 'GetCLStatus',
+    self.PatchObject(validation_pool.ValidationPool, 'GetCLPreCQStatus',
                      return_value=self.STATUS_READY_TO_SUBMIT)
     m = self.PatchObject(validation_pool.ValidationPool, 'SubmitChanges')
     self.runTrybotTest(runs=1)