Check cycle items are mergeable before reporting

In order to reduce the chance of a partially merged cycle we make sure
all items in the cycle can still be merged prior to starting reporting.

Change-Id: I22a9e8b76325e09b6a0f6bc563840f264c12dfae

tests/unit/test_circular_dependencies.py

@@ -1414,3 +1414,37 @@ class TestGithubCircularDependencies(ZuulTestCase):
         self.assertEqual(len(B.comments), 2)
         self.assertTrue(A.is_merged)
         self.assertTrue(B.is_merged)
+
+    def test_cycle_failed_reporting(self):
+        self.executor_server.hold_jobs_in_build = True
+        A = self.fake_github.openFakePullRequest("gh/project", "master", "A")
+        B = self.fake_github.openFakePullRequest("gh/project1", "master", "B")
+        A.addReview('derp', 'APPROVED')
+        B.addReview('derp', 'APPROVED')
+        B.addLabel("approved")
+
+        # A <-> B
+        A.body = "{}\n\nDepends-On: {}\n".format(
+            A.subject, B.url
+        )
+        B.body = "{}\n\nDepends-On: {}\n".format(
+            B.subject, A.url
+        )
+
+        self.fake_github.emitEvent(A.addLabel("approved"))
+        self.waitUntilSettled()
+
+        # Change draft status of A so it can no longer merge. Note that we
+        # don't send an event to test the "github doesn't send an event"
+        # case.
+        A.draft = True
+        self.waitUntilSettled()
+
+        self.executor_server.hold_jobs_in_build = False
+        self.executor_server.release()
+        self.waitUntilSettled()
+
+        self.assertEqual(len(A.comments), 2)
+        self.assertEqual(len(B.comments), 2)
+        self.assertFalse(A.is_merged)
+        self.assertFalse(B.is_merged)

zuul/manager/__init__.py

@@ -486,6 +486,10 @@ class PipelineManager(metaclass=ABCMeta):
             queue_config.allow_circular_dependencies
         )
 
+    def canMergeCycle(self, bundle):
+        """Check if the cycle still fulfills the pipeline's ready criteria."""
+        return True
+
     def updateBundle(self, item, change_queue, cycle):
         if not cycle:
             return
@@ -1088,6 +1092,19 @@ class PipelineManager(metaclass=ABCMeta):
             can_report = can_report and (
                 item.isBundleFailing() or item.didBundleFinish()
             )
+            # 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
+                )
             item.bundle.started_reporting = can_report
 
         if can_report:
@@ -1100,7 +1117,10 @@ class PipelineManager(metaclass=ABCMeta):
                              "item ahead, %s, failed to merge" %
                              (item_behind.change, item))
                     self.cancelJobs(item_behind)
-                if item.bundle and not item.isBundleFailing():
+                # Only re-reported items in the cycle when we encounter a merge
+                # failure for a successful bundle.
+                if (item.bundle and not (
+                        item.isBundleFailing() or item.cannotMergeBundle())):
                     item.bundle.failed_reporting = True
                     self.reportProcessedBundleItems(item)
             self.dequeueItem(item)
@@ -1379,6 +1399,10 @@ class PipelineManager(metaclass=ABCMeta):
             log.debug("No jobs for change %s", item.change)
             actions = self.pipeline.no_jobs_actions
             item.setReportedResult('NO_JOBS')
+        elif item.cannotMergeBundle():
+            log.debug("Bundle can not be merged")
+            actions = self.pipeline.failure_actions
+            item.setReportedResult("FAILURE")
         elif item.isBundleFailing():
             log.debug("Bundle is failing")
             actions = self.pipeline.failure_actions

zuul/manager/dependent.py

@@ -50,10 +50,25 @@ class DependentPipelineManager(SharedQueuePipelineManager):
         source = change.project.source
         if not source.canMerge(change, self.getSubmitAllowNeeds(),
                                event=event):
-            log.debug("Change %s can not merge, ignoring", change)
+            log.debug("Change %s can not merge", change)
             return False
         return True
 
+    def canMergeCycle(self, bundle):
+        """Check if the cycle still fulfills the pipeline's ready criteria."""
+        for item in bundle.items:
+            source = item.change.project.source
+            if not source.canMerge(
+                item.change,
+                self.getSubmitAllowNeeds(),
+                event=item.event,
+                allow_refresh=True,
+            ):
+                log = get_annotated_logger(self.log, item.event)
+                log.debug("Change %s can no longer be merged", item.change)
+                return False
+        return True
+
     def enqueueChangesBehind(self, change, event, quiet, ignore_requirements,
                              change_queue, history=None,
                              dependency_graph=None):

zuul/model.py

@@ -2432,6 +2432,11 @@ class QueueItem(object):
             return self.bundle.started_reporting
         return False
 
+    def cannotMergeBundle(self):
+        if self.bundle:
+            return self.bundle.cannot_merge
+        return False
+
     def didMergerFail(self):
         return self.current_build_set.unable_to_merge
 
@@ -3208,6 +3213,7 @@ class Bundle:
         self.items = []
         self.started_reporting = False
         self.failed_reporting = False
+        self.cannot_merge = False
 
     def __repr__(self):
         return '<Bundle 0x{:x} {}'.format(id(self), self.items)

zuul/reporter/__init__.py

@@ -175,7 +175,9 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
         return msg
 
     def _formatItemReportFailure(self, item, with_jobs=True):
-        if item.dequeued_needing_change:
+        if item.cannotMergeBundle():
+            msg = 'This change is part of a bundle that failed to merge.\n'
+        elif item.dequeued_needing_change:
             msg = 'This change depends on a change that failed to merge.\n'
         elif item.isBundleFailing():
             msg = 'This change is part of a bundle that failed.\n'