From 39aded45178520fba6190f81660025573951a6e4 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Sat, 25 Jun 2022 15:17:18 -0700 Subject: Fix merging with submitWholeTopic Previously support for Gerrit's submitWholeTopic feature was added so that when it is enabled, changes that are submitted together are treated as circular dependencies in Zuul. However, this feature did not work in a gating pipeline because when that setting is enabled, Gerrit requires all changes to be mergable at once so that it can attempt to atomically merge all of them. That means that every change would need a Verified+2 vote before any change can be submitted. Zuul leaves the vote and submits each change one at a time. (Note, this does not affect the emulated submitWholeTopic feature in Zuul, since in that case, Gerrit will merge each change alone.) To correct this, a two-phase option is added to reporters. In phase1, reporters will report all information about the change but not submit. In phase2, they will only submit. In the cases where we are about to submit a successful bundle, we enable the two-phase option and report the entire bundle without submitting first, then proceed to submit each change in the bundle in sequence as normal. In Gerrit, if submitWholeTopic is enabled, this will cause all changes to be submitted as soon as the first one is, but that's okay because we handle the case where we try to submit a change and it is already submitted. The fake Gerrit used in the Zuul unit tests is updated to match the real Gerrit in these cases. If submitWholeTopic is enabled, it will return a 409 to submit requests if the whole topic is not able to be submitted. One unit test of failed bundle reporting is adjusted since we will now report the buildset result to all changes before potentially reporting a second time if the bundle later fails to merge. While this does mean that some changes will have extra report messages, it actually makes the behavior consistent (before, some changes would have 2 reports and some would have only 1; now all changes will have 2 reports: the expected result and then a second report of the unexpected merge failure). All reporters are updated to handle the two-phase reporting. Since all reporters have different API methods to leave comments and merge changes, this won't actually cause any extra API calls even for reporters which don't need two-phase reporting. Non-merging reporters (MQTT, SMTP, etc) simply ignore phase2. Change-Id: Ibf377ab5b7141fe60ecfd5cbbb296bb4f9c24265 --- zuul/driver/gerrit/gerritconnection.py | 139 +++++++++++++++++---------------- zuul/driver/gerrit/gerritreporter.py | 5 +- 2 files changed, 75 insertions(+), 69 deletions(-) (limited to 'zuul/driver/gerrit') diff --git a/zuul/driver/gerrit/gerritconnection.py b/zuul/driver/gerrit/gerritconnection.py index 005f62b9a..6b12b4cd2 100644 --- a/zuul/driver/gerrit/gerritconnection.py +++ b/zuul/driver/gerrit/gerritconnection.py @@ -1203,16 +1203,17 @@ class GerritConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection): self.event_queue.put(event) def review(self, item, message, submit, labels, checks_api, - file_comments, zuul_event_id=None): + file_comments, phase1, phase2, zuul_event_id=None): if self.session: meth = self.review_http else: meth = self.review_ssh return meth(item, message, submit, labels, checks_api, - file_comments, zuul_event_id=zuul_event_id) + file_comments, phase1, phase2, + zuul_event_id=zuul_event_id) def review_ssh(self, item, message, submit, labels, checks_api, - file_comments, zuul_event_id=None): + file_comments, phase1, phase2, zuul_event_id=None): log = get_annotated_logger(self.log, zuul_event_id) if checks_api: log.error("Zuul is configured to report to the checks API, " @@ -1221,23 +1222,24 @@ class GerritConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection): change = item.change project = change.project.name cmd = 'gerrit review --project %s' % project - if message: - b_len = len(message.encode('utf-8')) - if b_len >= GERRIT_HUMAN_MESSAGE_LIMIT: - log.info("Message truncated %d > %d" % - (b_len, GERRIT_HUMAN_MESSAGE_LIMIT)) - message = ("%s... (truncated)" % - message[:GERRIT_HUMAN_MESSAGE_LIMIT - 20]) - cmd += ' --message %s' % shlex.quote(message) - if submit: + if phase1: + if message: + b_len = len(message.encode('utf-8')) + if b_len >= GERRIT_HUMAN_MESSAGE_LIMIT: + log.info("Message truncated %d > %d" % + (b_len, GERRIT_HUMAN_MESSAGE_LIMIT)) + message = ("%s... (truncated)" % + message[:GERRIT_HUMAN_MESSAGE_LIMIT - 20]) + cmd += ' --message %s' % shlex.quote(message) + for key, val in labels.items(): + if val is True: + cmd += ' --%s' % key + else: + cmd += ' --label %s=%s' % (key, val) + if self.version >= (2, 13, 0): + cmd += ' --tag autogenerated:zuul:%s' % (item.pipeline.name) + if phase2 and submit: cmd += ' --submit' - for key, val in labels.items(): - if val is True: - cmd += ' --%s' % key - else: - cmd += ' --label %s=%s' % (key, val) - if self.version >= (2, 13, 0): - cmd += ' --tag autogenerated:zuul:%s' % (item.pipeline.name) changeid = '%s,%s' % (change.number, change.patchset) cmd += ' %s' % changeid out, err = self._ssh(cmd, zuul_event_id=zuul_event_id) @@ -1290,8 +1292,13 @@ class GerritConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection): time.sleep(x * 10) def review_http(self, item, message, submit, labels, - checks_api, file_comments, zuul_event_id=None): + checks_api, file_comments, phase1, phase2, + zuul_event_id=None): change = item.change + changeid = "%s~%s~%s" % ( + urllib.parse.quote(str(change.project), safe=''), + urllib.parse.quote(str(change.branch), safe=''), + change.id) log = get_annotated_logger(self.log, zuul_event_id) b_len = len(message.encode('utf-8')) if b_len >= GERRIT_HUMAN_MESSAGE_LIMIT: @@ -1299,53 +1306,51 @@ class GerritConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection): (b_len, GERRIT_HUMAN_MESSAGE_LIMIT)) message = ("%s... (truncated)" % message[:GERRIT_HUMAN_MESSAGE_LIMIT - 20]) - data = dict(message=message, - strict_labels=False) - if change.is_current_patchset: - if labels: - data['labels'] = labels - if file_comments: - if self.version >= (2, 15, 0): - file_comments = copy.deepcopy(file_comments) - url = item.formatStatusUrl() - for comments in itertools.chain(file_comments.values()): - for comment in comments: - comment['robot_id'] = 'zuul' - comment['robot_run_id'] = \ - item.current_build_set.uuid - if url: - comment['url'] = url - data['robot_comments'] = file_comments - else: - data['comments'] = file_comments - if self.version >= (2, 13, 0): - data['tag'] = 'autogenerated:zuul:%s' % (item.pipeline.name) - changeid = "%s~%s~%s" % ( - urllib.parse.quote(str(change.project), safe=''), - urllib.parse.quote(str(change.branch), safe=''), - change.id) - if checks_api: - self.report_checks(log, item, changeid, checks_api) - if (message or data.get('labels') or data.get('comments') - or data.get('robot_comments')): - for x in range(1, 4): - try: - self.post('changes/%s/revisions/%s/review' % - (changeid, change.commit), - data) - break - except HTTPConflictException: - log.exception("Conflict submitting data to gerrit.") - break - except HTTPBadRequestException: - log.exception( - "Bad request submitting check data to gerrit.") - break - except Exception: - log.exception( - "Error submitting data to gerrit, attempt %s", x) - time.sleep(x * 10) - if change.is_current_patchset and submit: + data = dict(strict_labels=False) + if phase1: + data['message'] = message + if change.is_current_patchset: + if labels: + data['labels'] = labels + if file_comments: + if self.version >= (2, 15, 0): + file_comments = copy.deepcopy(file_comments) + url = item.formatStatusUrl() + for comments in itertools.chain( + file_comments.values()): + for comment in comments: + comment['robot_id'] = 'zuul' + comment['robot_run_id'] = \ + item.current_build_set.uuid + if url: + comment['url'] = url + data['robot_comments'] = file_comments + else: + data['comments'] = file_comments + if self.version >= (2, 13, 0): + data['tag'] = 'autogenerated:zuul:%s' % (item.pipeline.name) + if checks_api: + self.report_checks(log, item, changeid, checks_api) + if (message or data.get('labels') or data.get('comments') + or data.get('robot_comments')): + for x in range(1, 4): + try: + self.post('changes/%s/revisions/%s/review' % + (changeid, change.commit), + data) + break + except HTTPConflictException: + log.exception("Conflict submitting data to gerrit.") + break + except HTTPBadRequestException: + log.exception( + "Bad request submitting check data to gerrit.") + break + except Exception: + log.exception( + "Error submitting data to gerrit, attempt %s", x) + time.sleep(x * 10) + if phase2 and change.is_current_patchset and submit: for x in range(1, 4): try: self.post('changes/%s/submit' % (changeid,), {}) diff --git a/zuul/driver/gerrit/gerritreporter.py b/zuul/driver/gerrit/gerritreporter.py index fbee0e0ec..984b8742a 100644 --- a/zuul/driver/gerrit/gerritreporter.py +++ b/zuul/driver/gerrit/gerritreporter.py @@ -35,7 +35,7 @@ class GerritReporter(BaseReporter): self._checks_api = action.pop('checks-api', None) self._labels = action - def report(self, item): + def report(self, item, phase1=True, phase2=True): """Send a message to gerrit.""" log = get_annotated_logger(self.log, item.event) @@ -70,7 +70,8 @@ class GerritReporter(BaseReporter): return self.connection.review(item, message, self._submit, self._labels, self._checks_api, - comments, zuul_event_id=item.event) + comments, phase1, phase2, + zuul_event_id=item.event) def getSubmitAllowNeeds(self): """Get a list of code review labels that are allowed to be -- cgit v1.2.1