zuul/zuul
Code Issues Proposed changes

Add detail to "depends on a change that failed to merge"

Browse Source 
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
This commit is contained in:
James E. Blair
2022-08-11 18:44:07 -07:00
parent d35b9f8c73
commit 80a45b77b5
6 changed files with 136 additions and 65 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/unit/test_circular_dependencies.py
+34
View File
@@ -13,6 +13,7 @@
# You should have received a copy of the GNU General Public License
# along with this software. If not, see <http://www.gnu.org/licenses/>.
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]))
zuul/manager/__init__.py
+39 -20
View File
@@ -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:
zuul/manager/dependent.py
+28 -20
View File
@@ -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'):
zuul/manager/independent.py
+16 -13
View File
@@ -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)
zuul/model.py
+12 -9
View File
@@ -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 '<Bundle 0x{:x} {}'.format(id(self), self.items)
zuul/reporter/__init__.py
+7 -3
View File
@@ -191,9 +191,13 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
def _formatItemReportFailure(self, item, with_jobs=True):
if item.cannotMergeBundle():
msg = 'This change is part of a bundle that failed to merge.\n'
msg = 'This change is part of a bundle that can not merge.\n'
if isinstance(item.bundle.cannot_merge, str):
msg += '\n' + item.bundle.cannot_merge + '\n'
elif item.dequeued_needing_change:
msg = 'This change depends on a change that failed to merge.\n'
if isinstance(item.dequeued_needing_change, str):
msg += '\n' + item.dequeued_needing_change + '\n'
elif item.dequeued_missing_requirements:
msg = ('This change is unable to merge '
'due to a missing merge requirement.\n')
@@ -250,8 +254,8 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
def _formatItemReportOtherBundleItems(self, item):
related_changes = item.pipeline.manager.resolveChangeReferences(
item.change.needs_changes)
return "Related changes:\n{}".format("\n".join(
c.url for c in related_changes if c is not item.change))
return "Related changes:\n{}\n".format("\n".join(
f' - {c.url}' for c in related_changes if c is not item.change))
def _getItemReportJobsFields(self, item):
# Extract the report elements from an item