Check whether patches returned by Gerrit queries in fact match the query.
If the Pre-CQ launcher picks up a CL while the CQ is committing
the CL, it may catch a race condition where a new patchset has been
created and committed by the CQ, but the CL is still treated as if
it matches the query (which it doesn't, anymore).
See https://gerrit.chromium.org/gerrit/#/c/48712 for an example.
To fix this, we add logic to the CQ to check whether the currentPatchSet
for the CL still matches the query. This only works when we don't have a
custom query.
In cases where the user has specified a custom query for testing, we
really don't care about checking for this race condition, because this
is just for testing purposes and it would be difficult to implement
verification of arbitrary queries.
BUG=chromium:235459
TEST=Test run of mario-paladin.
Change-Id: I3e4eafa22300631310e56429aabe9b01dd1a7440
Reviewed-on: https://gerrit.chromium.org/gerrit/49191
Commit-Queue: David James <davidjames@chromium.org>
Reviewed-by: David James <davidjames@chromium.org>
Tested-by: David James <davidjames@chromium.org>
Reviewed-on: https://gerrit.chromium.org/gerrit/49260
Reviewed-by: Ryan Cui <rcui@chromium.org>
diff --git a/buildbot/constants.py b/buildbot/constants.py
index 1d2a250..6d8ea5e 100644
--- a/buildbot/constants.py
+++ b/buildbot/constants.py
@@ -250,6 +250,10 @@
'AND CommitQueue=+1 '
'AND NOT ( CodeReview=-2 OR Verified=-1 )')
+# Default filter rules for verifying that Gerrit returned results that matched
+# our query. This used for working around Gerrit bugs.
+DEFAULT_CQ_READY_FIELDS = {'SUBM': '0', 'CRVW': '2', 'VRIF': '1', 'COMR': '1'}
+
# Some files need permissions set for several distinct groups. A google storage
# acl (xml) file will be necessary in those cases. Make available well known
# locations and standardize.
diff --git a/buildbot/validation_pool.py b/buildbot/validation_pool.py
index 1536ac4..2f6a2f7 100644
--- a/buildbot/validation_pool.py
+++ b/buildbot/validation_pool.py
@@ -939,6 +939,35 @@
self.pre_cq))
@classmethod
+ def FilterNonMatchingChanges(cls, changes):
+ """Filter out changes that don't actually match our query.
+
+ Generally, Gerrit should only return patches that match our query. However,
+ there are race conditions (bugs in Gerrit) where the final patch won't
+ match our query.
+
+ Here's an example problem that this code fixes: If the Pre-CQ launcher
+ picks up a CL while the CQ is committing the CL, it may catch a race
+ condition where a new patchset has been created and committed by the CQ,
+ but the CL is still treated as if it matches the query (which it doesn't,
+ anymore).
+
+ Arguments:
+ changes: List of changes to filter.
+
+ Returns:
+ List of changes that match our query.
+ """
+ for change in changes:
+ # Check that the user (or chrome-bot) uploaded a new change under our
+ # feet while Gerrit was in the middle of answering our query.
+ for field, value in constants.DEFAULT_CQ_READY_FIELDS.iteritems():
+ if not change.HasApproval(field, value):
+ break
+ else:
+ yield change
+
+ @classmethod
def AcquirePreCQPool(cls, *args, **kwargs):
"""See ValidationPool.__init__ for arguments."""
kwargs.setdefault('pre_cq', True)
@@ -998,6 +1027,10 @@
raw_changes = helper.Query(changes_query, sort='lastUpdated')
raw_changes.reverse()
+ # Verify the results match the query, to prevent race conditions.
+ if changes_query == constants.DEFAULT_CQ_READY_QUERY:
+ raw_changes = cls.FilterNonMatchingChanges(raw_changes)
+
changes, non_manifest_changes = ValidationPool._FilterNonCrosProjects(
raw_changes, git.ManifestCheckout.Cached(repo.directory))
pool.changes.extend(changes)
diff --git a/lib/patch.py b/lib/patch.py
index 94f2e8e..dad53b7 100644
--- a/lib/patch.py
+++ b/lib/patch.py
@@ -1102,9 +1102,9 @@
# status - Current state of this change. Can be one of
# ['NEW', 'SUBMITTED', 'MERGED', 'ABANDONED'].
self.status = patch_dict['status']
- approvals = self.patch_dict['currentPatchSet'].get('approvals', [])
+ self._approvals = self.patch_dict['currentPatchSet'].get('approvals', [])
self.approval_timestamp = \
- max(x['grantedOn'] for x in approvals) if approvals else 0
+ max(x['grantedOn'] for x in self._approvals) if self._approvals else 0
def __reduce__(self):
"""Used for pickling to re-create patch object."""
@@ -1122,6 +1122,21 @@
"""Returns whether the patch has already been merged in Gerrit."""
return self.status == 'MERGED'
+ def HasApproval(self, field, value):
+ """Return whether the current patchset has the specified approval.
+
+ Args:
+ field: Which field to check.
+ 'SUBM': Whether patch was submitted.
+ 'VRIF': Whether patch was verified.
+ 'CRVW': Whether patch was approved.
+ 'COMR': Whether patch was marked ready.
+ value: The expected value of the specified field.
+ """
+ # All approvals default to '0', so use that if there's no matches.
+ type_approvals = [x['value'] for x in self._approvals if x['type'] == field]
+ return value in (type_approvals or ['0'])
+
def _EnsureId(self, commit_message):
"""Ensure we have a usable Change-Id, validating what we received
from gerrit against what the commit message states."""
