validation_pool: pool.changes -> pool.candidates, pool.applied.

Validation pool had a list of changes, and one would know if they were
applied or not by knowing if ApplyPoolIntoRepo had been called.

This change breaks pool.changes into a pool.candidates (the list of
changes being considered for application), and pool.applied (the list of
changes that have been appled to the current repo).

The point is that it's now safe to call ApplyPoolIntoRepo() more than
once. In particular, that function now accepts a filter argument. This
means we can apply changes which match a filter, do some work, then come
back and apply the rest of the changes.

A follow on change will start applying manifest changes first, doing
work to include those changes in the manifest used by the CQ, and then
to apply the rest of the changes on top of the patched manifest.

BUG=chromium:549844
TEST=cbuildbot/run_tests

Change-Id: I2275248d19d6ba64e255258eeeea771d827a8022
Reviewed-on: https://chromium-review.googlesource.com/311393
Commit-Ready: Don Garrett <dgarrett@chromium.org>
Tested-by: Don Garrett <dgarrett@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>

7 files changed, 105 insertions, 79 deletions

diff --git a/cbuildbot/lkgm_manager.py b/cbuildbot/lkgm_manager.py
index 947b7b25e..a2303f0d9 100644
--- a/cbuildbot/lkgm_manager.py
+++ b/cbuildbot/lkgm_manager.py
@@ -269,7 +269,7 @@ class LKGMManager(manifest_version.BuildSpecsManager):
           not config_lib.IsPFQType(self.build_type)):
         return None
 
-      self._AddPatchesToManifest(new_manifest, validation_pool.changes)
+      self._AddPatchesToManifest(new_manifest, validation_pool.applied)
 
       # Add info about the last known good version to the manifest. This will
       # be used by slaves to calculate what artifacts from old builds are safe
@@ -288,7 +288,7 @@ class LKGMManager(manifest_version.BuildSpecsManager):
 
         # If we don't have any valid changes to test, make sure the checkout
         # is at least different.
-        if ((not validation_pool or not validation_pool.changes) and
+        if ((not validation_pool or not validation_pool.applied) and
             not self.force and self.HasCheckoutBeenBuilt()):
           return None
 
diff --git a/cbuildbot/stages/completion_stages.py b/cbuildbot/stages/completion_stages.py
index e1467d3f0..e18eaf6d0 100644
--- a/cbuildbot/stages/completion_stages.py
+++ b/cbuildbot/stages/completion_stages.py
@@ -640,7 +640,7 @@ class CommitQueueCompletionStage(MasterSlaveSyncCompletionStage):
     messages = self._GetFailedMessages(failing)
     self.SendInfraAlertIfNeeded(failing, inflight, no_stat)
 
-    changes = self.sync_stage.pool.changes
+    changes = self.sync_stage.pool.applied
 
     do_partial_submission = self._ShouldSubmitPartialPool()
 
diff --git a/cbuildbot/stages/completion_stages_unittest.py b/cbuildbot/stages/completion_stages_unittest.py
index 7e900be31..af5ac8714 100644
--- a/cbuildbot/stages/completion_stages_unittest.py
+++ b/cbuildbot/stages/completion_stages_unittest.py
@@ -352,7 +352,7 @@ class BaseCommitQueueCompletionStageTest(
     """
     sync_stage = sync_stages.CommitQueueSyncStage(self._run)
     sync_stage.pool = mock.MagicMock()
-    sync_stage.pool.changes = self.changes
+    sync_stage.pool.applied = self.changes
     sync_stage.pool.tree_was_open = tree_was_open
 
     sync_stage.pool.handle_failure_mock = self.PatchObject(
diff --git a/cbuildbot/stages/sync_stages.py b/cbuildbot/stages/sync_stages.py
index 8eb6ce77f..e682f427a 100644
--- a/cbuildbot/stages/sync_stages.py
+++ b/cbuildbot/stages/sync_stages.py
@@ -1058,7 +1058,7 @@ class CommitQueueSyncStage(MasterSlaveLKGMSyncStage):
     else:
       ManifestVersionedSyncStage.PerformStage(self)
 
-    self.WriteChangesToMetadata(self.pool.changes)
+    self.WriteChangesToMetadata(self.pool.applied)
 
 
 class PreCQSyncStage(SyncStage):
@@ -1091,14 +1091,14 @@ class PreCQSyncStage(SyncStage):
     self.pool = validation_pool.ValidationPool.AcquirePreCQPool(
         self._run.config.overlays, self._build_root,
         self._run.buildnumber, self._run.config.name,
-        dryrun=self._run.options.debug_forced, changes=self.patches,
+        dryrun=self._run.options.debug_forced, candidates=self.patches,
         builder_run=self._run)
     self.pool.ApplyPoolIntoRepo()
 
-    if len(self.pool.changes) == 0 and self.patches:
+    if len(self.pool.applied) == 0 and self.patches:
       cros_build_lib.Die('No changes have been applied.')
 
-    changes = self.pool.changes or self.patches
+    changes = self.pool.applied or self.patches
     self.WriteChangesToMetadata(changes)
 
 class PreCQLauncherStage(SyncStage):
diff --git a/cbuildbot/stages/sync_stages_unittest.py b/cbuildbot/stages/sync_stages_unittest.py
index e2dc8297b..f79a9069f 100644
--- a/cbuildbot/stages/sync_stages_unittest.py
+++ b/cbuildbot/stages/sync_stages_unittest.py
@@ -526,38 +526,38 @@ class MasterCQSyncTest(MasterCQSyncTestCase):
   def testCommitNonManifestChange(self):
     """See MasterCQSyncTestCase"""
     changes = self._testCommitNonManifestChange()
-    self.assertItemsEqual(self.sync_stage.pool.changes, changes)
+    self.assertItemsEqual(self.sync_stage.pool.candidates, changes)
     self.assertItemsEqual(self.sync_stage.pool.non_manifest_changes, [])
 
   def testFailedCommitOfNonManifestChange(self):
     """See MasterCQSyncTestCase"""
     changes = self._testFailedCommitOfNonManifestChange()
-    self.assertItemsEqual(self.sync_stage.pool.changes, changes)
+    self.assertItemsEqual(self.sync_stage.pool.candidates, changes)
     self.assertItemsEqual(self.sync_stage.pool.non_manifest_changes, [])
 
   def testCommitManifestChange(self):
     """See MasterCQSyncTestCase"""
     changes = self._testCommitManifestChange()
-    self.assertItemsEqual(self.sync_stage.pool.changes, changes)
+    self.assertItemsEqual(self.sync_stage.pool.candidates, changes)
     self.assertItemsEqual(self.sync_stage.pool.non_manifest_changes, [])
 
   def testCommitManifestChangeWithoutPreCQ(self):
     """Changes get ignored if they aren't approved by pre-cq."""
     self._testCommitManifestChange(pre_cq_status=None)
-    self.assertItemsEqual(self.sync_stage.pool.changes, [])
+    self.assertItemsEqual(self.sync_stage.pool.candidates, [])
     self.assertItemsEqual(self.sync_stage.pool.non_manifest_changes, [])
 
   def testCommitManifestChangeWithoutPreCQAndOldPatches(self):
     """Changes get tested without pre-cq if the approval_timestamp is old."""
     changes = self._testCommitManifestChange(pre_cq_status=None,
                                              approval_timestamp=0)
-    self.assertItemsEqual(self.sync_stage.pool.changes, changes)
+    self.assertItemsEqual(self.sync_stage.pool.candidates, changes)
     self.assertItemsEqual(self.sync_stage.pool.non_manifest_changes, [])
 
   def testDefaultSync(self):
     """See MasterCQSyncTestCase"""
     changes = self._testDefaultSync()
-    self.assertItemsEqual(self.sync_stage.pool.changes, changes)
+    self.assertItemsEqual(self.sync_stage.pool.candidates, changes)
     self.assertItemsEqual(self.sync_stage.pool.non_manifest_changes, [])
 
   def testReload(self):
@@ -565,7 +565,7 @@ class MasterCQSyncTest(MasterCQSyncTestCase):
     # Use zero patches because mock patches can't be pickled.
     changes = self.PerformSync(num_patches=0, runs=0)
     self.ReloadPool()
-    self.assertItemsEqual(self.sync_stage.pool.changes, changes)
+    self.assertItemsEqual(self.sync_stage.pool.candidates, changes)
     self.assertItemsEqual(self.sync_stage.pool.non_manifest_changes, [])
 
   def testTreeClosureBlocksCommit(self):
@@ -579,7 +579,7 @@ class MasterCQSyncTest(MasterCQSyncTestCase):
     gerrit.GerritHelper.Query.assert_called_with(
         mock.ANY, constants.THROTTLED_CQ_READY_QUERY[0],
         sort='lastUpdated')
-    self.assertItemsEqual(self.sync_stage.pool.changes, changes)
+    self.assertItemsEqual(self.sync_stage.pool.candidates, changes)
     self.assertItemsEqual(self.sync_stage.pool.non_manifest_changes, [])
 
 
diff --git a/cbuildbot/validation_pool.py b/cbuildbot/validation_pool.py
index 15f18e9d0..4c0bde02f 100644
--- a/cbuildbot/validation_pool.py
+++ b/cbuildbot/validation_pool.py
@@ -1120,9 +1120,9 @@ class ValidationPool(object):
 
 
   def __init__(self, overlays, build_root, build_number, builder_name,
-               is_master, dryrun, changes=None, non_os_changes=None,
+               is_master, dryrun, candidates=None, non_os_changes=None,
                conflicting_changes=None, pre_cq_trybot=False,
-               tree_was_open=True, _applied=None, builder_run=None):
+               tree_was_open=True, applied=None, builder_run=None):
     """Initializes an instance by setting default variables to instance vars.
 
     Generally use AcquirePool as an entry pool to a pool rather than this
@@ -1136,7 +1136,7 @@ class ValidationPool(object):
       is_master: True if this is the master builder for the Commit Queue.
       dryrun: If set to True, do not submit anything to Gerrit.
     Optional Args:
-      changes: List of changes for this validation pool.
+      candidates: List of changes to consider validating.
       non_os_changes: List of changes that are part of this validation
         pool but aren't part of the cros checkout.
       conflicting_changes: Changes that failed to apply but we're keeping around
@@ -1144,8 +1144,7 @@ class ValidationPool(object):
       pre_cq_trybot: If set to True, this is a Pre-CQ trybot. (Note: The Pre-CQ
         launcher is NOT considered a Pre-CQ trybot.)
       tree_was_open: Whether the tree was open when the pool was created.
-      applied: List of CLs that have been applied to the current repo. Not
-        yet used, but needs to be here for pickling compatibility.
+      applied: List of CLs that have been applied to the current repo.
       builder_run: BuilderRun instance used to fetch cidb handle and metadata
         instance. Please note due to the pickling logic, this MUST be the last
         kwarg listed.
@@ -1167,7 +1166,9 @@ class ValidationPool(object):
       raise ValueError("Invalid builder_name: %r" % (builder_name,))
 
     for changes_name, changes_value in (
-        ('changes', changes), ('non_os_changes', non_os_changes)):
+        ('candidates', candidates),
+        ('non_os_changes', non_os_changes),
+        ('applied', applied)):
       if not changes_value:
         continue
       if not all(isinstance(x, cros_patch.GitRepoPatch) for x in changes_value):
@@ -1195,8 +1196,17 @@ class ValidationPool(object):
       self.queue = 'The Commit Queue'
 
     # See optional args for types of changes.
-    self.changes = changes or []
+    self.candidates = candidates or []
     self.non_manifest_changes = non_os_changes or []
+
+    # TODO(dgarrett): Remove this if block after pickle changes settle down.
+    if applied is None:
+      # If we received a pickle from an older version, it will use the
+      # default applied value. All candidates were already applied.
+      applied = self.candidates[:]
+
+    self.applied = applied or []
+
     # Note, we hold onto these CLs since they conflict against our current CLs
     # being tested; if our current ones succeed, we notify the user to deal
     # w/ the conflict.  If the CLs we're testing fail, then there is no
@@ -1241,11 +1251,12 @@ class ValidationPool(object):
         (
             self._overlays,
             self.build_root, self._build_number, self._builder_name,
-            self.is_master, self.dryrun, self.changes,
+            self.is_master, self.dryrun, self.candidates,
             self.non_manifest_changes,
             self.changes_that_failed_to_apply_earlier,
             self.pre_cq_trybot,
-            self.tree_was_open))
+            self.tree_was_open,
+            self.applied))
 
   @classmethod
   @failures_lib.SetFailureType(failures_lib.BuilderFailure)
@@ -1298,14 +1309,14 @@ class ValidationPool(object):
 
       changes, non_manifest_changes = ValidationPool._FilterNonCrosProjects(
           changes, git.ManifestCheckout.Cached(self.build_root))
-      self.changes.extend(changes)
+      self.candidates.extend(changes)
       self.non_manifest_changes.extend(non_manifest_changes)
 
     # Filter out unwanted changes.
-    self.changes, self.non_manifest_changes = change_filter(
-        self, self.changes, self.non_manifest_changes)
+    self.candidates, self.non_manifest_changes = change_filter(
+        self, self.candidates, self.non_manifest_changes)
 
-    return self.changes or self.non_manifest_changes
+    return self.candidates or self.non_manifest_changes
 
   @classmethod
   def AcquirePool(cls, overlays, repo, build_number, builder_name, query,
@@ -1454,7 +1465,7 @@ class ValidationPool(object):
         attr_dict[name] = pc.getAttribute(name)
       patch = cros_patch.GerritFetchOnlyPatch.FromAttrDict(attr_dict)
 
-      self.changes.append(patch)
+      self.candidates.append(patch)
 
   @classmethod
   def AcquirePoolFromManifest(cls, manifest, overlays, repo, build_number,
@@ -1619,6 +1630,8 @@ class ValidationPool(object):
   def FilterChangesForThrottledTree(self):
     """Apply Throttled Tree logic to select patch candidates.
 
+    This should be called before any CLs are applied.
+
     If the tree is throttled, we only test a random subset of our candidate
     changes. Call this to select that subset, and throw away unrelated changes.
 
@@ -1627,12 +1640,14 @@ class ValidationPool(object):
     if self.tree_was_open:
       return
 
+    # By filtering the candidates before any calls to Apply, we can make sure
+    # that repeated calls to Apply always consider the same list of candidates.
     fail_streak = self._GetFailStreak()
-    test_pool_size = max(1, len(self.changes) / (2**fail_streak))
-    random.shuffle(self.changes)
-    self.changes = self.changes[:test_pool_size]
+    test_pool_size = max(1, len(self.candidates) / (2**fail_streak))
+    random.shuffle(self.candidates)
+    self.candidates = self.candidates[:test_pool_size]
 
-  def ApplyPoolIntoRepo(self, manifest=None):
+  def ApplyPoolIntoRepo(self, manifest=None, filter_fn=lambda p: True):
     """Applies changes from pool into the directory specified by the buildroot.
 
     This method applies changes in the order specified. If the build
@@ -1640,6 +1655,13 @@ class ValidationPool(object):
     order. Otherwise, the changes should already be listed in an order
     that will not break the dependency order.
 
+    It is safe to call this method more than once, probably with different
+    filter functions. A given patch will never be applied more than  once.
+
+    Args:
+      manifest: A manifest object to use for mapping projects to repositories.
+      filter_fn: Takes a patch argument, returns bool for 'should apply'.
+
     Returns:
       True if we managed to apply any changes.
     """
@@ -1652,9 +1674,12 @@ class ValidationPool(object):
 
     if self.is_master:
       try:
+        candidates = [c for c in self.candidates if
+                      c not in self.applied and filter_fn(c)]
+
         # pylint: disable=E1123
         applied, failed_tot, failed_inflight = patch_series.Apply(
-            self.changes, manifest=manifest)
+            candidates, manifest=manifest)
       except (KeyboardInterrupt, RuntimeError, SystemExit):
         raise
       except Exception as e:
@@ -1667,9 +1692,9 @@ class ValidationPool(object):
             'commit queue does not go into an infinite loop retrying '
             'patches.' % (e,)
         )
-        links = cros_patch.GetChangesAsString(self.changes)
+        links = cros_patch.GetChangesAsString(self.candidates)
         logging.error('%s\nAffected Patches are: %s', msg, links)
-        errors = [InternalCQError(patch, msg) for patch in self.changes]
+        errors = [InternalCQError(patch, msg) for patch in self.candidates]
         self._HandleApplyFailure(errors)
         raise
 
@@ -1691,7 +1716,7 @@ class ValidationPool(object):
       # Slaves do not need to create transactions and should simply
       # apply the changes serially, based on the order that the
       # changes were listed on the manifest.
-      for change in self.changes:
+      for change in self.candidates:
         try:
           # pylint: disable=E1123
           patch_series.ApplyChange(change, manifest=manifest)
@@ -1733,9 +1758,9 @@ class ValidationPool(object):
         self._HandleFailedToApplyDueToInflightConflict(x.patch)
 
     self.changes_that_failed_to_apply_earlier.extend(failed_inflight)
-    self.changes = applied
+    self.applied.extend(applied)
 
-    return bool(self.changes)
+    return bool(applied)
 
   @staticmethod
   def Load(filename, builder_run=None):
@@ -2323,7 +2348,7 @@ class ValidationPool(object):
     # a CQ run (since the submit state has changed, we have no way of
     # knowing).  They *likely* will still fail, but this approach tries
     # to minimize wasting the developers time.
-    submitted, errors = self.SubmitChanges(self.changes,
+    submitted, errors = self.SubmitChanges(self.applied,
                                            check_tree_open=check_tree_open,
                                            throttled_ok=throttled_ok,
                                            reason=reason)
@@ -2453,7 +2478,7 @@ class ValidationPool(object):
         not sane, none of the changes will have their CommitReady bit modified.
     """
     if changes is None:
-      changes = self.changes
+      changes = self.applied
 
     logging.info('Validation timed out for all changes.')
     base_msg = ('%(queue)s timed out while verifying your change in '
@@ -2518,7 +2543,7 @@ class ValidationPool(object):
         self._InsertCLActionToDatabase(change, constants.CL_ACTION_VERIFIED)
 
     # Process the changes in parallel.
-    inputs = [[change] for change in self.changes]
+    inputs = [[change] for change in self.applied]
     parallel.RunTasksInProcessPool(ProcessChange, inputs)
 
   def _HandleCouldNotSubmit(self, change, error=''):
@@ -2675,7 +2700,7 @@ class ValidationPool(object):
         status. If not None, this implies there were infrastructure issues.
     """
     if changes is None:
-      changes = self.changes
+      changes = self.applied
 
     candidates = []
 
diff --git a/cbuildbot/validation_pool_unittest.py b/cbuildbot/validation_pool_unittest.py
index 3fab2d100..fbf47e1f1 100644
--- a/cbuildbot/validation_pool_unittest.py
+++ b/cbuildbot/validation_pool_unittest.py
@@ -652,7 +652,7 @@ def MakePool(overlays=constants.PUBLIC_OVERLAYS, build_number=1,
              builder_name='foon', is_master=True, dryrun=True,
              fake_db=None, **kwargs):
   """Helper for creating ValidationPool objects for tests."""
-  kwargs.setdefault('changes', [])
+  kwargs.setdefault('candidates', [])
   build_root = kwargs.pop('build_root', '/fake_root')
 
   builder_run = FakeBuilderRun(fake_db)
@@ -785,7 +785,7 @@ class ValidationFailureOrTimeout(MoxBase):
 
   def setUp(self):
     self._patches = self.GetPatches(3)
-    self._pool = MakePool(changes=self._patches, fake_db=self.fake_db)
+    self._pool = MakePool(applied=self._patches, fake_db=self.fake_db)
 
     self.PatchObject(
         triage_lib.CalculateSuspects, 'FindSuspects',
@@ -878,8 +878,8 @@ class TestCoreLogic(MoxBase):
     return cros_patch.ApplyPatchException(patch, inflight=inflight)
 
   def GetPool(self, changes, applied=(), tot=(), inflight=(), **kwargs):
-
-    pool = self.MakePool(changes=changes, fake_db=self.fake_db, **kwargs)
+    pool = self.MakePool(
+        candidates=changes, applied=[], fake_db=self.fake_db, **kwargs)
     applied = list(applied)
     tot = [self.MakeFailure(x, inflight=False) for x in tot]
     inflight = [self.MakeFailure(x, inflight=True) for x in inflight]
@@ -909,7 +909,7 @@ class TestCoreLogic(MoxBase):
     """Verifies that slave calls ApplyChange() directly for each patch."""
     slave_pool = self.MakePool(is_master=False)
     patches = self.GetPatches(3)
-    slave_pool.changes = patches
+    slave_pool.candidates = patches
     for patch in patches:
       # pylint: disable=E1120, E1123
       validation_pool.PatchSeries.ApplyChange(patch, manifest=mox.IgnoreArg())
@@ -920,7 +920,7 @@ class TestCoreLogic(MoxBase):
 
   def runApply(self, pool, result):
     self.assertEqual(result, pool.ApplyPoolIntoRepo())
-    self.assertEqual(pool.changes, pool._test_data[1])
+    self.assertEqual(pool.applied, pool._test_data[1])
     failed_inflight = pool.changes_that_failed_to_apply_earlier
     expected_inflight = set(pool._test_data[3])
     # Intersect the results, since it's possible there were results failed
@@ -928,8 +928,6 @@ class TestCoreLogic(MoxBase):
     self.assertEqual(set(failed_inflight).intersection(expected_inflight),
                      expected_inflight)
 
-    self.assertEqual(pool.changes, pool._test_data[1])
-
   def testPatchSeriesInteraction(self):
     """Verify the interaction between PatchSeries and ValidationPool.
 
@@ -991,7 +989,7 @@ class TestCoreLogic(MoxBase):
     pool = self.MakePool(dryrun=False, handlers=True)
     patches = self.GetPatches(3)
     failed = self.GetPatches(3)
-    pool.changes = patches[:]
+    pool.applied = patches[:]
     # While we don't do anything w/ these patches, that's
     # intentional; we're verifying that it isn't submitted
     # if there is a failure.
@@ -1124,6 +1122,9 @@ class TestCoreLogic(MoxBase):
     """Test that CQ doesn't loop due to unhandled Exceptions."""
     pool, patches, _failed = self._setUpSubmit()
 
+    pool.candidates = pool.applied
+    pool.applied = []
+
     class MyException(Exception):
       """"Unique Exception used for testing."""
 
@@ -1371,11 +1372,11 @@ class TestCoreLogic(MoxBase):
     self.mox.ReplayAll()
 
     # Perform test.
-    slave_pool = self.MakePool(changes=patches, tree_was_open=True)
+    slave_pool = self.MakePool(candidates=patches, tree_was_open=True)
     slave_pool.FilterChangesForThrottledTree()
 
     # Validate results.
-    self.assertEqual(len(slave_pool.changes), 4)
+    self.assertEqual(len(slave_pool.candidates), 4)
     self.mox.VerifyAll()
     self.mox.ResetAll()
 
@@ -1388,11 +1389,11 @@ class TestCoreLogic(MoxBase):
     self.mox.ReplayAll()
 
     # Perform test.
-    slave_pool = self.MakePool(changes=patches, tree_was_open=False)
+    slave_pool = self.MakePool(candidates=patches, tree_was_open=False)
     slave_pool.FilterChangesForThrottledTree()
 
     # Validate results.
-    self.assertEqual(len(slave_pool.changes), 2)
+    self.assertEqual(len(slave_pool.candidates), 2)
     self.mox.VerifyAll()
     self.mox.ResetAll()
 
@@ -1405,11 +1406,11 @@ class TestCoreLogic(MoxBase):
     self.mox.ReplayAll()
 
     # Perform test.
-    slave_pool = self.MakePool(changes=patches, tree_was_open=False)
+    slave_pool = self.MakePool(candidates=patches, tree_was_open=False)
     slave_pool.FilterChangesForThrottledTree()
 
     # Validate results.
-    self.assertEqual(len(slave_pool.changes), 1)
+    self.assertEqual(len(slave_pool.candidates), 1)
     self.mox.VerifyAll()
     self.mox.ResetAll()
 
@@ -1422,11 +1423,11 @@ class TestCoreLogic(MoxBase):
     self.mox.ReplayAll()
 
     # Perform test.
-    slave_pool = self.MakePool(changes=patches, tree_was_open=False)
+    slave_pool = self.MakePool(candidates=patches, tree_was_open=False)
     slave_pool.FilterChangesForThrottledTree()
 
     # Validate results.
-    self.assertEqual(len(slave_pool.changes), 1)
+    self.assertEqual(len(slave_pool.candidates), 1)
     self.mox.VerifyAll()
 
 
@@ -1495,7 +1496,7 @@ sys.stdout.write(validation_pool_unittest.TestPickling.%s)
         constants.PUBLIC_OVERLAYS,
         '/fake/pathway', 1,
         'testing', True, True,
-        changes=changes, non_os_changes=non_os,
+        candidates=changes, non_os_changes=non_os,
         conflicting_changes=conflicting)
     return pickle.dumps([pool, changes, non_os, conflicting])
 
@@ -1510,7 +1511,7 @@ sys.stdout.write(validation_pool_unittest.TestPickling.%s)
       for s_item, v_item in zip(source, value):
         assert getter(s_item).id == getter(v_item).id
         assert getter(s_item).remote == getter(v_item).remote
-    _f(pool.changes, changes)
+    _f(pool.candidates, changes)
     _f(pool.non_manifest_changes, non_os)
     _f(pool.changes_that_failed_to_apply_earlier, conflicting,
        getter=lambda s: getattr(s, 'patch', s))
@@ -1640,8 +1641,8 @@ class TestCreateDisjointTransactions(MoxBase):
       # the specified length, ignoring any remaining patches.
       expected_plans = [txn[:max_txn_length] for txn in txns]
 
-    pool = MakePool(changes=patches)
-    plans = pool.CreateDisjointTransactions(None, pool.changes,
+    pool = MakePool(candidates=patches)
+    plans = pool.CreateDisjointTransactions(None, pool.candidates,
                                             max_txn_length=max_txn_length)
 
     # If the dependencies are circular, the order of the patches is not
@@ -1665,7 +1666,7 @@ class TestCreateDisjointTransactions(MoxBase):
     notify = self.PatchObject(validation_pool.ValidationPool,
                               'SendNotification')
     remove = self.PatchObject(gerrit.GerritHelper, 'RemoveReady')
-    pool = MakePool(changes=changes)
+    pool = MakePool(candidates=changes)
     plans = pool.CreateDisjointTransactions(None, changes,
                                             max_txn_length=max_txn_length)
     self.assertEqual(plans, [])
@@ -1764,7 +1765,7 @@ class BaseSubmitPoolTestCase(MoxBase):
     self.patches = self.GetPatches(2)
 
   def SetUpPatchPool(self, failed_to_apply=False):
-    pool = MakePool(changes=self.patches, dryrun=False)
+    pool = MakePool(candidates=self.patches, dryrun=False)
     if failed_to_apply:
       # Create some phony errors and add them to the pool.
       errors = []
@@ -1788,7 +1789,7 @@ class BaseSubmitPoolTestCase(MoxBase):
     with mock.patch.object(git.ManifestCheckout, 'Cached', new=mock_manifest):
       if not self.ALL_BUILDS_PASSED:
         actually_rejected = sorted(pool.SubmitPartialPool(
-            pool.changes, mock.ANY, dict(), [], [], [], reason=reason))
+            pool.candidates, mock.ANY, dict(), [], [], [], reason=reason))
       else:
         _, actually_rejected = pool.SubmitChanges(self.patches, reason=reason)
 
@@ -2027,17 +2028,17 @@ class LoadManifestTest(cros_test_lib.TempDirTestCase):
       f.flush()
       self.pool.AddPendingCommitsIntoPool(f.name)
 
-    self.assertEqual(self.pool.changes[0].owner_email, 'foo@chromium.org')
-    self.assertEqual(self.pool.changes[0].tracking_branch, 'master')
-    self.assertEqual(self.pool.changes[0].remote, 'cros')
-    self.assertEqual(self.pool.changes[0].gerrit_number, '17000')
-    self.assertEqual(self.pool.changes[0].project, 'chromiumos/taco/bar')
-    self.assertEqual(self.pool.changes[0].project_url,
+    self.assertEqual(self.pool.candidates[0].owner_email, 'foo@chromium.org')
+    self.assertEqual(self.pool.candidates[0].tracking_branch, 'master')
+    self.assertEqual(self.pool.candidates[0].remote, 'cros')
+    self.assertEqual(self.pool.candidates[0].gerrit_number, '17000')
+    self.assertEqual(self.pool.candidates[0].project, 'chromiumos/taco/bar')
+    self.assertEqual(self.pool.candidates[0].project_url,
                      'https://base_url/chromiumos/taco/bar')
-    self.assertEqual(self.pool.changes[0].change_id,
+    self.assertEqual(self.pool.candidates[0].change_id,
                      'Ieeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee1')
-    self.assertEqual(self.pool.changes[0].commit,
+    self.assertEqual(self.pool.candidates[0].commit,
                      '1ddddddddddddddddddddddddddddddddddddddd')
-    self.assertEqual(self.pool.changes[0].fail_count, 2)
-    self.assertEqual(self.pool.changes[0].pass_count, 0)
-    self.assertEqual(self.pool.changes[0].total_fail_count, 3)
+    self.assertEqual(self.pool.candidates[0].fail_count, 2)
+    self.assertEqual(self.pool.candidates[0].pass_count, 0)
+    self.assertEqual(self.pool.candidates[0].total_fail_count, 3)