From 80a45b77b52a655b09153baef9b9ee64a8161255 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Thu, 11 Aug 2022 18:44:07 -0700 Subject: Add detail to "depends on a change that failed to merge" The report message "This change depends on a change that failed to merge" (and a similar change for circular dependency bundles) is famously vague. To help users identify the actual problem, include URLs for which change(s) caused the problem so that users may more easily resolve the issue. Change-Id: Id8b9f8cf2c108703e9209e30bdc9a3933f074652 --- tests/unit/test_circular_dependencies.py | 34 ++++++++++++++++++ zuul/manager/__init__.py | 59 +++++++++++++++++++++----------- zuul/manager/dependent.py | 48 +++++++++++++++----------- zuul/manager/independent.py | 29 +++++++++------- zuul/model.py | 21 +++++++----- zuul/reporter/__init__.py | 10 ++++-- 6 files changed, 136 insertions(+), 65 deletions(-) diff --git a/tests/unit/test_circular_dependencies.py b/tests/unit/test_circular_dependencies.py index 2223008d5..292941c13 100644 --- a/tests/unit/test_circular_dependencies.py +++ b/tests/unit/test_circular_dependencies.py @@ -13,6 +13,7 @@ # You should have received a copy of the GNU General Public License # along with this software. If not, see . +import re import textwrap from zuul.model import PromoteEvent @@ -599,8 +600,23 @@ class TestGerritCircularDependencies(ZuulTestCase): self.assertIn("depends on a change that failed to merge", A.messages[-1]) + self.assertTrue(re.search(r'Change http://localhost:\d+/2 is needed', + A.messages[-1])) + self.assertFalse(re.search('Change .*? can not be merged', + A.messages[-1])) + self.assertIn("bundle that failed.", B.messages[-1]) + self.assertFalse(re.search('Change http://localhost:.*? is needed', + B.messages[-1])) + self.assertFalse(re.search('Change .*? can not be merged', + B.messages[-1])) + self.assertIn("bundle that failed.", C.messages[-1]) + self.assertFalse(re.search('Change http://localhost:.*? is needed', + C.messages[-1])) + self.assertFalse(re.search('Change .*? can not be merged', + C.messages[-1])) + self.assertEqual(A.data["status"], "NEW") self.assertEqual(B.data["status"], "NEW") self.assertEqual(C.data["status"], "NEW") @@ -2441,3 +2457,21 @@ class TestGithubCircularDependencies(ZuulTestCase): self.assertEqual(len(B.comments), 2) self.assertFalse(A.is_merged) self.assertFalse(B.is_merged) + + self.assertIn("part of a bundle that can not merge", + A.comments[-1]) + self.assertTrue( + re.search("Change https://github.com/gh/project/pull/1 " + "can not be merged", + A.comments[-1])) + self.assertFalse(re.search('Change .*? is needed', + A.comments[-1])) + + self.assertIn("part of a bundle that can not merge", + B.comments[-1]) + self.assertTrue( + re.search("Change https://github.com/gh/project/pull/1 " + "can not be merged", + B.comments[-1])) + self.assertFalse(re.search('Change .*? is needed', + B.comments[-1])) diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py index a66f5ad22..31c4b54d7 100644 --- a/zuul/manager/__init__.py +++ b/zuul/manager/__init__.py @@ -379,9 +379,16 @@ class PipelineManager(metaclass=ABCMeta): dependency_graph=None): return True - def checkForChangesNeededBy(self, change, change_queue, event, + def getMissingNeededChanges(self, change, change_queue, event, dependency_graph=None): - return True + """Check that all needed changes are ahead in the queue. + + Return a list of any that are missing. If it is not possible + to correct the missing changes, "abort" will be true. + + :returns: (abort, needed_changes) + """ + return False, [] def getFailingDependentItems(self, item, nnfi): return None @@ -690,9 +697,11 @@ class PipelineManager(metaclass=ABCMeta): return queue_config.dependencies_by_topic - def canMergeCycle(self, bundle): - """Check if the cycle still fulfills the pipeline's ready criteria.""" - return True + def getNonMergeableCycleChanges(self, bundle): + + """Return changes in the cycle that do not fulfill + the pipeline's ready criteria.""" + return [] def updateBundle(self, item, change_queue, cycle): if not cycle: @@ -1474,9 +1483,9 @@ class PipelineManager(metaclass=ABCMeta): item.change, item.event) else: meets_reqs = True - needs_met = self.checkForChangesNeededBy(item.change, change_queue, - item.event) is True - if not (meets_reqs and needs_met): + abort, needs_changes = self.getMissingNeededChanges( + item.change, change_queue, item.event) + if not (meets_reqs and not needs_changes): # It's not okay to enqueue this change, we should remove it. log.info("Dequeuing change %s because " "it can no longer merge" % item.change) @@ -1486,7 +1495,12 @@ class PipelineManager(metaclass=ABCMeta): elif not meets_reqs: item.setDequeuedMissingRequirements() else: - item.setDequeuedNeedingChange() + clist = ', '.join([c.url for c in needs_changes]) + if len(needs_changes) > 1: + msg = f'Changes {clist} are needed.' + else: + msg = f'Change {clist} is needed.' + item.setDequeuedNeedingChange(msg) if item.live: try: self.reportItem(item) @@ -1558,17 +1572,22 @@ class PipelineManager(metaclass=ABCMeta): ) # Before starting to merge the cycle items, make sure they # can still be merged, to reduce the chance of a partial merge. - if ( - can_report - and not item.bundle.started_reporting - and not self.canMergeCycle(item.bundle) - ): - item.bundle.cannot_merge = True - failing_reasons.append("cycle can not be merged") - log.debug( - "Dequeuing item %s because cycle can no longer merge", - item - ) + if can_report and not item.bundle.started_reporting: + non_mergeable_cycle_changes = self.getNonMergeableCycleChanges( + item.bundle) + if non_mergeable_cycle_changes: + clist = ', '.join([ + c.url for c in non_mergeable_cycle_changes]) + if len(non_mergeable_cycle_changes) > 1: + msg = f'Changes {clist} can not be merged.' + else: + msg = f'Change {clist} can not be merged.' + item.bundle.cannot_merge = msg + failing_reasons.append("cycle can not be merged") + log.debug( + "Dequeuing item %s because cycle can no longer merge", + item + ) item.bundle.started_reporting = can_report if can_report: diff --git a/zuul/manager/dependent.py b/zuul/manager/dependent.py index d4d4f05dd..db1bf48b1 100644 --- a/zuul/manager/dependent.py +++ b/zuul/manager/dependent.py @@ -56,8 +56,10 @@ class DependentPipelineManager(SharedQueuePipelineManager): return False return True - def canMergeCycle(self, bundle): - """Check if the cycle still fulfills the pipeline's ready criteria.""" + def getNonMergeableCycleChanges(self, bundle): + """Return changes in the cycle that do not fulfill + the pipeline's ready criteria.""" + changes = [] for item in bundle.items: source = item.change.project.source if not source.canMerge( @@ -68,8 +70,8 @@ class DependentPipelineManager(SharedQueuePipelineManager): ): log = get_annotated_logger(self.log, item.event) log.debug("Change %s can no longer be merged", item.change) - return False - return True + changes.append(item.change) + return changes def enqueueChangesBehind(self, change, event, quiet, ignore_requirements, change_queue, history=None, @@ -149,13 +151,17 @@ class DependentPipelineManager(SharedQueuePipelineManager): # Don't enqueue dependencies ahead of a non-change ref. return True - ret = self.checkForChangesNeededBy(change, change_queue, event, - dependency_graph=dependency_graph, - warnings=warnings) - if ret in [True, False]: - return ret - log.debug(" Changes %s must be merged ahead of %s", ret, change) - for needed_change in ret: + abort, needed_changes = self.getMissingNeededChanges( + change, change_queue, event, + dependency_graph=dependency_graph, + warnings=warnings) + if abort: + return False + if not needed_changes: + return True + log.debug(" Changes %s must be merged ahead of %s", + needed_changes, change) + for needed_change in needed_changes: # If the change is already in the history, but the change also has # a git level dependency, we need to enqueue it before the current # change. @@ -169,7 +175,7 @@ class DependentPipelineManager(SharedQueuePipelineManager): return False return True - def checkForChangesNeededBy(self, change, change_queue, event, + def getMissingNeededChanges(self, change, change_queue, event, dependency_graph=None, warnings=None): log = get_annotated_logger(self.log, event) @@ -178,11 +184,12 @@ class DependentPipelineManager(SharedQueuePipelineManager): log.debug("Checking for changes needed by %s:" % change) if not hasattr(change, 'needs_changes'): log.debug(" %s does not support dependencies", type(change)) - return True + return False, [] if not change.needs_changes: log.debug(" No changes needed") - return True + return False, [] changes_needed = [] + abort = False # Ignore supplied change_queue with self.getChangeQueue(change, event) as change_queue: for needed_change in self.resolveChangeReferences( @@ -212,10 +219,12 @@ class DependentPipelineManager(SharedQueuePipelineManager): log.debug(" " + msg) if warnings is not None: warnings.append(msg) - return False + changes_needed.append(needed_change) + abort = True if not needed_change.is_current_patchset: log.debug(" Needed change is not the current patchset") - return False + changes_needed.append(needed_change) + abort = True if self.isChangeAlreadyInQueue(needed_change, change_queue): log.debug(" Needed change is already ahead in the queue") continue @@ -229,10 +238,9 @@ class DependentPipelineManager(SharedQueuePipelineManager): # The needed change can't be merged. log.debug(" Change %s is needed but can not be merged", needed_change) - return False - if changes_needed: - return changes_needed - return True + changes_needed.append(needed_change) + abort = True + return abort, changes_needed def getFailingDependentItems(self, item, nnfi): if not hasattr(item.change, 'needs_changes'): diff --git a/zuul/manager/independent.py b/zuul/manager/independent.py index 7cd14ffd5..b70e9184b 100644 --- a/zuul/manager/independent.py +++ b/zuul/manager/independent.py @@ -49,12 +49,16 @@ class IndependentPipelineManager(PipelineManager): # Don't enqueue dependencies ahead of a non-change ref. return True - ret = self.checkForChangesNeededBy(change, change_queue, event, - dependency_graph=dependency_graph) - if ret in [True, False]: - return ret - log.debug(" Changes %s must be merged ahead of %s" % (ret, change)) - for needed_change in ret: + abort, needed_changes = self.getMissingNeededChanges( + change, change_queue, event, + dependency_graph=dependency_graph) + if abort: + return False + if not needed_changes: + return True + log.debug(" Changes %s must be merged ahead of %s" % ( + needed_changes, change)) + for needed_change in needed_changes: # This differs from the dependent pipeline by enqueuing # changes ahead as "not live", that is, not intended to # have jobs run. Also, pipeline requirements are always @@ -69,22 +73,23 @@ class IndependentPipelineManager(PipelineManager): return False return True - def checkForChangesNeededBy(self, change, change_queue, event, + def getMissingNeededChanges(self, change, change_queue, event, dependency_graph=None): log = get_annotated_logger(self.log, event) if self.pipeline.ignore_dependencies: - return True + return False, [] log.debug("Checking for changes needed by %s:" % change) # Return true if okay to proceed enqueing this change, # false if the change should not be enqueued. if not hasattr(change, 'needs_changes'): log.debug(" %s does not support dependencies" % type(change)) - return True + return False, [] if not change.needs_changes: log.debug(" No changes needed") - return True + return False, [] changes_needed = [] + abort = False for needed_change in self.resolveChangeReferences( change.needs_changes): log.debug(" Change %s needs change %s:" % ( @@ -108,9 +113,7 @@ class IndependentPipelineManager(PipelineManager): continue # This differs from the dependent pipeline check in not # verifying that the dependent change is mergable. - if changes_needed: - return changes_needed - return True + return abort, changes_needed def dequeueItem(self, item): super(IndependentPipelineManager, self).dequeueItem(item) diff --git a/zuul/model.py b/zuul/model.py index 963332826..0d889f557 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -4237,7 +4237,7 @@ class QueueItem(zkobject.ZKObject): pipeline=None, queue=None, change=None, # a ref - dequeued_needing_change=False, + dequeued_needing_change=None, dequeued_missing_requirements=False, current_build_set=None, item_ahead=None, @@ -4405,11 +4405,14 @@ class QueueItem(zkobject.ZKObject): self.current_build_set.updateAttributes( self.pipeline.manager.current_context, result=result) - def warning(self, msg): + def warning(self, msgs): with self.current_build_set.activeContext( self.pipeline.manager.current_context): - self.current_build_set.warning_messages.append(msg) - self.log.info(msg) + if not isinstance(msgs, list): + msgs = [msgs] + for msg in msgs: + self.current_build_set.warning_messages.append(msg) + self.log.info(msg) def freezeJobGraph(self, layout, context, skip_file_matcher, @@ -4583,7 +4586,7 @@ class QueueItem(zkobject.ZKObject): def cannotMergeBundle(self): if self.bundle: - return self.bundle.cannot_merge + return bool(self.bundle.cannot_merge) return False def didMergerFail(self): @@ -4595,7 +4598,7 @@ class QueueItem(zkobject.ZKObject): return [] def wasDequeuedNeedingChange(self): - return self.dequeued_needing_change + return bool(self.dequeued_needing_change) def wasDequeuedMissingRequirements(self): return self.dequeued_missing_requirements @@ -5118,10 +5121,10 @@ class QueueItem(zkobject.ZKObject): self.setResult(fakebuild) return fakebuild - def setDequeuedNeedingChange(self): + def setDequeuedNeedingChange(self, msg): self.updateAttributes( self.pipeline.manager.current_context, - dequeued_needing_change=True) + dequeued_needing_change=msg) self._setAllJobsSkipped() def setDequeuedMissingRequirements(self): @@ -5494,7 +5497,7 @@ class Bundle: self.items = [] self.started_reporting = False self.failed_reporting = False - self.cannot_merge = False + self.cannot_merge = None def __repr__(self): return '