zuul
/
zuul
Code Issues Proposed changes

Support submitWholeTopic in Gerrit 

This adds a query for changes which are set (by Gerrit) to be submitted
together due to submitWholeTopic being enabled.  In this case, changes
which are of the same topic will be merged simultaneously by Gerrit,
therefore Zuul should treat them as a set of circular dependencies.

The behavior is automatic, since Gerrit will return a set of changes
if the setting is enabled, or the empty list if it is not.  Zuul still
requires that circular dependencies be enabled in any queues where they
appear.

If users have submitWholeTopic enabled in Gerrit but do not have
allow-circular-dependencies enabled, they may begin to see errors.
However, the errors are self-explanatory, and installations such as
these are already not testing what Gerrit will merge, so reporting
the discrepancy is an improvement.

Change-Id: Icf65913a049a9b9ddbedd20cc73bf44ffcc831b8
This commit is contained in:
James E. Blair 2022-03-05 11:16:15 -08:00
parent d05035f6cd
commit 81d84e7c15
8 changed files with 178 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
doc/source/config/queue.rst
View File

@ -46,30 +46,31 @@ Here is an example ``queue`` configuration.
.. attr:: allow-circular-dependencies
:default: false
Define if Zuul is allowed to process circular dependencies between
changes for this queue. All projects that are part of a dependency cycle
must share the same change queue.
Determines whether Zuul is allowed to process circular
dependencies between changes for this queue. All projects that
are part of a dependency cycle must share the same change queue.
In case Zuul detects a dependency cycle it will make sure that every
change also includes all other changes that are part of the cycle.
However each change will still be a normal item in the queue with its own
jobs.
If Zuul detects a dependency cycle it will ensure that every
change also includes all other changes that are part of the
cycle. However each change will still be a normal item in the
queue with its own jobs.
Reporting of success will be postponed until all items in the cycle
succeeded. In case of a failure in any of those items the whole cycle
succeed. In the case of a failure in any of those items the whole cycle
will be dequeued.
An error message will be posted to all items of the cycle in case some
An error message will be posted to all items of the cycle if some
items fail to report (e.g. merge failure when some items were already
merged). In this case the target branch(es) might be in a broken state.
In general, circular dependencies are considered to be an
antipattern since they add extra constraints to continuous
deployment systems. Additionally, due to the lack of atomicity
in merge operations in code review systems, it may be possible
for only part of a cycle to be merged. In that case, manual
interventions (such as reverting a commit, or bypassing gating to
force-merge the remaining commits) may be required.
in merge operations in code review systems (this includes
Gerrit, even with submitWholeTopic set), it may be possible for
only part of a cycle to be merged. In that case, manual
interventions (such as reverting a commit, or bypassing gating
to force-merge the remaining commits) may be required.
.. warning:: If the remote system is able to merge the first but
unable to merge the second or later change in a

7
doc/source/drivers/gerrit.rst
View File

@ -25,6 +25,13 @@ want Zuul to report on. For instance, you may want to grant
or values may be added to Gerrit. Zuul is very flexible and can take
advantage of those.
If ``change.submitWholeTopic`` is configured in Gerrit, Zuul will
honor this by enqueing changes with the same topic as circular
dependencies. However, it is still necessary to enable circular
dependency support in any pipeline queues where such changes may
appear. See :attr:`queue.allow-circular-dependencies` for information
on how to configure this.
Connection Configuration
------------------------

20
releasenotes/notes/gerrit-submit-whole-topic-5fef75db6ca450e8.yaml Normal file
View File

@ -0,0 +1,20 @@
---
features:
- |
Zuul now matches Gerrit's behavior when
``change.submitWholeTopic`` is set in Gerrit.
When this setting is enabled in Gerrit and a change is submitted
(merged), all changes with the same topic are submitted
simultaneously. Zuul will now query for changes which are set to
be submitted together by Gerrit when enqueing them and treat them
as if they are a set of circular dependencies.
If the projects are not part of pipeline queues which are
configured to allow circular dependencies, then Zuul will report
failure on enqueue. Be sure that the submitWholeTopic setting in
Gerrit and the allow-circular-dependencies setting in Zuul match.
This functionality requires an HTTP connection to Gerrit. If only
an SSH connection is available, then changes submitted together
will be ignored.

34
tests/base.py
View File

@ -383,7 +383,8 @@ class FakeGerritChange(object):
def __init__(self, gerrit, number, project, branch, subject,
status='NEW', upstream_root=None, files={},
parent=None, merge_parents=None, merge_files=None):
parent=None, merge_parents=None, merge_files=None,
topic=None):
self.gerrit = gerrit
self.source = gerrit
self.reported = 0
@ -421,6 +422,8 @@ class FakeGerritChange(object):
'submitRecords': [],
'url': '%s/%s' % (self.gerrit.baseurl.rstrip('/'), number)}
if topic:
self.data['topic'] = topic
self.upstream_root = upstream_root
if merge_parents:
self.addMergePatchset(parents=merge_parents,
@ -957,6 +960,7 @@ class GerritWebServer(object):
class Server(http.server.SimpleHTTPRequestHandler):
log = logging.getLogger("zuul.test.FakeGerritConnection")
review_re = re.compile('/a/changes/(.*?)/revisions/(.*?)/review')
together_re = re.compile('/a/changes/(.*?)/submitted_together')
submit_re = re.compile('/a/changes/(.*?)/submit')
pending_checks_re = re.compile(
r'/a/plugins/checks/checks\.pending/\?'
@ -1005,6 +1009,9 @@ class GerritWebServer(object):
m = self.files_re.match(path)
if m:
return self.get_files(m.group(1), m.group(2))
m = self.together_re.match(path)
if m:
return self.get_submitted_together(m.group(1))
m = self.change_search_re.match(path)
if m:
return self.get_changes(m.group(1))
@ -1132,6 +1139,21 @@ class GerritWebServer(object):
self.send_data(data)
self.end_headers()
def get_submitted_together(self, number):
change = fake_gerrit.changes.get(int(number))
if not change:
return self._404()
topic = change.data.get('topic')
if not fake_gerrit._fake_submit_whole_topic:
topic = None
if topic:
results = fake_gerrit._simpleQuery(
f'topic:{topic}', http=True)
else:
results = []
self.send_data(results)
self.end_headers()
def get_changes(self, query):
self.log.debug("simpleQueryHTTP: %s", query)
query = urllib.parse.unquote(query)
@ -1262,6 +1284,7 @@ class FakeGerritConnection(gerritconnection.GerritConnection):
self.fake_checkers = []
self._poller_event = poller_event
self._ref_watcher_event = ref_watcher_event
self._fake_submit_whole_topic = False
def onStop(self):
super().onStop()
@ -1273,14 +1296,15 @@ class FakeGerritConnection(gerritconnection.GerritConnection):
def addFakeChange(self, project, branch, subject, status='NEW',
files=None, parent=None, merge_parents=None,
merge_files=None):
merge_files=None, topic=None):
"""Add a change to the fake Gerrit."""
self.change_number += 1
c = FakeGerritChange(self, self.change_number, project, branch,
subject, upstream_root=self.upstream_root,
status=status, files=files, parent=parent,
merge_parents=merge_parents,
merge_files=merge_files)
merge_files=merge_files,
topic=topic)
self.changes[self.change_number] = c
return c
@ -1431,6 +1455,10 @@ class FakeGerritConnection(gerritconnection.GerritConnection):
queryMethod(change) for change in self.changes.values()
if change.data["lastUpdated"] >= cut_off_time
]
elif query.startswith('topic:'):
topic = query[len('topic:'):].strip()
l = [queryMethod(change) for change in self.changes.values()
if topic in change.data.get('topic', '')]
else:
# Query all open changes
l = [queryMethod(change) for change in self.changes.values()]

1
tests/fixtures/zuul-gerrit-github.conf vendored
View File

@ -20,6 +20,7 @@ driver=gerrit
server=review.example.com
user=jenkins
sshkey=fake_id_rsa_path
password=badpassword
[connection github]
driver=github

39
tests/unit/test_circular_dependencies.py
View File

@ -1410,6 +1410,45 @@ class TestGerritCircularDependencies(ZuulTestCase):
self.assertEqual(B.data['status'], 'NEW')
self.assertEqual(C.data['status'], 'MERGED')
def test_submitted_together(self):
self.fake_gerrit._fake_submit_whole_topic = True
A = self.fake_gerrit.addFakeChange('org/project1', "master", "A",
topic='test-topic')
B = self.fake_gerrit.addFakeChange('org/project2', "master", "B",
topic='test-topic')
self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.assertEqual(len(A.patchsets[-1]["approvals"]), 1)
self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified")
self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "1")
self.assertEqual(len(B.patchsets[-1]["approvals"]), 1)
self.assertEqual(B.patchsets[-1]["approvals"][0]["type"], "Verified")
self.assertEqual(B.patchsets[-1]["approvals"][0]["value"], "1")
# We're about to add approvals to changes without adding the
# triggering events to Zuul, so that we can be sure that it is
# enqueing the changes based on dependencies, not because of
# triggering events. Since it will have the changes cached
# already (without approvals), we need to clear the cache
# first.
for connection in self.scheds.first.connections.connections.values():
connection.maintainCache([], max_age=0)
A.addApproval("Code-Review", 2)
B.addApproval("Code-Review", 2)
A.addApproval("Approved", 1)
self.fake_gerrit.addEvent(B.addApproval("Approved", 1))
self.waitUntilSettled()
self.assertEqual(A.reported, 3)
self.assertEqual(B.reported, 3)
self.assertEqual(A.data["status"], "MERGED")
self.assertEqual(B.data["status"], "MERGED")
class TestGithubCircularDependencies(ZuulTestCase):
config_file = "zuul-gerrit-github.conf"

4
tests/unit/test_web.py
View File

@ -2665,10 +2665,12 @@ class TestWebMulti(BaseTestWeb):
def test_web_connections_list_multi(self):
data = self.get_url('api/connections').json()
port = self.web.connections.connections['gerrit'].web_server.port
url = f'http://localhost:{port}'
gerrit_connection = {
'driver': 'gerrit',
'name': 'gerrit',
'baseurl': 'https://review.example.com',
'baseurl': url,
'canonical_hostname': 'review.example.com',
'server': 'review.example.com',
'port': 29418,

64
zuul/driver/gerrit/gerritconnection.py
View File

@ -881,6 +881,32 @@ class GerritConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
records.append(result)
return [(x.number, x.current_patchset) for x in records]
def _getSubmittedTogether(self, change, event):
if not self.session:
return []
# We could probably ask for everything in one query, but it
# might be extremely large, so just get the change ids here
# and then query the individual changes.
log = get_annotated_logger(self.log, event)
log.debug("Updating %s: Looking for changes submitted together",
change)
ret = []
try:
data = self.get(f'changes/{change.number}/submitted_together')
except Exception:
log.error("Unable to find changes submitted together for %s",
change)
return ret
for c in data:
dep_change = c['_number']
dep_ps = c['revisions'][c['current_revision']]['_number']
if str(dep_change) == str(change.number):
continue
log.debug("Updating %s: Found change %s,%s submitted together",
change, dep_change, dep_ps)
ret.append((dep_change, dep_ps))
return ret
def _updateChange(self, key, change, event, history):
log = get_annotated_logger(self.log, event)
@ -999,6 +1025,41 @@ class GerritConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
log.exception("Failed to get commit-needed change %s,%s",
dep_num, dep_ps)
for (dep_num, dep_ps) in self._getSubmittedTogether(change, event):
try:
log.debug("Updating %s: Getting submitted-together "
"change %s,%s",
change, dep_num, dep_ps)
# Because a submitted-together change may be a cross-repo
# dependency, cause that change to refresh so that it will
# reference the latest patchset of its Depends-On (this
# change). In case the dep is already in history we already
# refreshed this change so refresh is not needed in this case.
refresh = (dep_num, dep_ps) not in history
dep_key = ChangeKey(self.connection_name, None,
'GerritChange', str(dep_num), str(dep_ps))
dep = self._getChange(
dep_key, refresh=refresh, history=history,
event=event)
# Gerrit changes to be submitted together do not
# necessarily get posted with dependency cycles using
# git trees and depends-on. However, they are
# functionally equivalent to a stack of changes with
# cycles using those methods. Here we set
# needs_changes and needed_by_changes as if there were
# a cycle. This ensures Zuul's cycle handling manages
# the submitted together changes properly.
if dep.open and dep not in needs_changes:
compat_needs_changes.append(dep.cache_key)
needs_changes.add(dep.cache_key)
if (dep.open and dep.is_current_patchset
and dep not in needed_by_changes):
compat_needed_by_changes.append(dep.cache_key)
needed_by_changes.add(dep.cache_key)
except Exception:
log.exception("Failed to get commit-needed change %s,%s",
dep_num, dep_ps)
self.updateChangeAttributes(
change,
git_needs_changes=git_needs_changes,
@ -1292,7 +1353,8 @@ class GerritConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
self.post('changes/%s/submit' % (changeid,), {})
break
except HTTPConflictException:
log.exception("Conflict submitting data to gerrit.")
log.info("Conflict submitting data to gerrit, "
"change may already be merged")
break
except HTTPBadRequestException:
log.exception(