From 81d84e7c15d46a7e4b3557824552b941e4c60182 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Sat, 5 Mar 2022 11:16:15 -0800 Subject: [PATCH] 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 --- doc/source/config/queue.rst | 27 ++++---- doc/source/drivers/gerrit.rst | 7 ++ ...t-submit-whole-topic-5fef75db6ca450e8.yaml | 20 ++++++ tests/base.py | 34 +++++++++- tests/fixtures/zuul-gerrit-github.conf | 1 + tests/unit/test_circular_dependencies.py | 39 +++++++++++ tests/unit/test_web.py | 4 +- zuul/driver/gerrit/gerritconnection.py | 64 ++++++++++++++++++- 8 files changed, 178 insertions(+), 18 deletions(-) create mode 100644 releasenotes/notes/gerrit-submit-whole-topic-5fef75db6ca450e8.yaml diff --git a/doc/source/config/queue.rst b/doc/source/config/queue.rst index bc24bb4fbf..c18bf89395 100644 --- a/doc/source/config/queue.rst +++ b/doc/source/config/queue.rst @@ -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 diff --git a/doc/source/drivers/gerrit.rst b/doc/source/drivers/gerrit.rst index 48908b74b6..5cb1f5d11f 100644 --- a/doc/source/drivers/gerrit.rst +++ b/doc/source/drivers/gerrit.rst @@ -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 ------------------------ diff --git a/releasenotes/notes/gerrit-submit-whole-topic-5fef75db6ca450e8.yaml b/releasenotes/notes/gerrit-submit-whole-topic-5fef75db6ca450e8.yaml new file mode 100644 index 0000000000..c78d1bef84 --- /dev/null +++ b/releasenotes/notes/gerrit-submit-whole-topic-5fef75db6ca450e8.yaml @@ -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. diff --git a/tests/base.py b/tests/base.py index 262e2d595a..7778ba7636 100644 --- a/tests/base.py +++ b/tests/base.py @@ -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()] diff --git a/tests/fixtures/zuul-gerrit-github.conf b/tests/fixtures/zuul-gerrit-github.conf index 1b2813dd48..0d03bacc20 100644 --- a/tests/fixtures/zuul-gerrit-github.conf +++ b/tests/fixtures/zuul-gerrit-github.conf @@ -20,6 +20,7 @@ driver=gerrit server=review.example.com user=jenkins sshkey=fake_id_rsa_path +password=badpassword [connection github] driver=github diff --git a/tests/unit/test_circular_dependencies.py b/tests/unit/test_circular_dependencies.py index 0b48a66a9d..713408ee98 100644 --- a/tests/unit/test_circular_dependencies.py +++ b/tests/unit/test_circular_dependencies.py @@ -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" diff --git a/tests/unit/test_web.py b/tests/unit/test_web.py index 9be7efd2bf..87570e2705 100644 --- a/tests/unit/test_web.py +++ b/tests/unit/test_web.py @@ -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, diff --git a/zuul/driver/gerrit/gerritconnection.py b/zuul/driver/gerrit/gerritconnection.py index 704ca2d2b3..66d8079660 100644 --- a/zuul/driver/gerrit/gerritconnection.py +++ b/zuul/driver/gerrit/gerritconnection.py @@ -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(