Ensure cycle dependencies are enqueued ahead

This change fixes a bug related to circular dependency resolution where
non-cycle changes could be enqueued between changes of the same cycle.

This violated the invariant assumption that changes of the same
dependency cycle are enqueued in sequence. This could cause the pipeline
processor to loop indefinitely under certain conditions.

The idea behind this fix is to treat all unprocessed dependencies of
other changes in the same cycle as if they were direct dependencies of
the current change. By that we will try to enqueue dependencies of any
change in the cycle ahead of the whole cycle.

Change-Id: I3eeb9fc9f6fca73982ce01d180dca9f58868bff3

tests/base.py

@@ -5941,8 +5941,8 @@ class ZuulTestCase(BaseTestCase):
                             unseen.remove(unseen_item)
                             break
                     if not found:
-                        raise Exception("No match found for element %i "
-                                        "in history" % (i,))
+                        raise Exception("No match found for element %i %s "
+                                        "in history" % (i, d))
                 if unseen:
                     raise Exception("Unexpected items in history")
         except Exception:

tests/unit/test_circular_dependencies.py

@@ -142,6 +142,78 @@ class TestGerritCircularDependencies(ZuulTestCase):
             "org/project", "org/project1", "org/project2"
         )
 
+    def test_enqueue_order(self):
+        A = self.fake_gerrit.addFakeChange("org/project", "master", "A")
+        B = self.fake_gerrit.addFakeChange("org/project1", "master", "B")
+        C = self.fake_gerrit.addFakeChange("org/project2", "master", "C")
+
+        # A <-> B and A -> C (via commit-depends)
+        A.data[
+            "commitMessage"
+        ] = "{}\n\nDepends-On: {}\nDepends-On: {}\n".format(
+            A.subject, B.data["url"], C.data["url"]
+        )
+        B.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format(
+            B.subject, A.data["url"]
+        )
+
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
+        self.fake_gerrit.addEvent(C.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")
+
+        self.assertEqual(len(C.patchsets[-1]["approvals"]), 1)
+        self.assertEqual(C.patchsets[-1]["approvals"][0]["type"], "Verified")
+        self.assertEqual(C.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
+        # enqueuing 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)
+        C.addApproval("Code-Review", 2)
+        B.addApproval("Approved", 1)
+        C.addApproval("Approved", 1)
+        self.fake_gerrit.addEvent(A.addApproval("Approved", 1))
+        self.waitUntilSettled()
+
+        self.assertEqual(A.reported, 3)
+        self.assertEqual(B.reported, 3)
+        self.assertEqual(C.reported, 3)
+        self.assertEqual(A.data["status"], "MERGED")
+        self.assertEqual(B.data["status"], "MERGED")
+        self.assertEqual(C.data["status"], "MERGED")
+
+        self.assertHistory([
+            # Change A (check + gate)
+            dict(name="project1-job", result="SUCCESS", changes="3,1 1,1 2,1"),
+            dict(name="project-vars-job", result="SUCCESS",
+                 changes="3,1 1,1 2,1"),
+            dict(name="project1-job", result="SUCCESS", changes="3,1 1,1 2,1"),
+            dict(name="project-vars-job", result="SUCCESS",
+                 changes="3,1 1,1 2,1"),
+            # Change B (check + gate)
+            dict(name="project-job", result="SUCCESS", changes="3,1 2,1 1,1"),
+            dict(name="project-job", result="SUCCESS", changes="3,1 1,1 2,1"),
+            # Change C (check + gate)
+            dict(name="project2-job", result="SUCCESS", changes="3,1"),
+            dict(name="project2-job", result="SUCCESS", changes="3,1"),
+        ], ordered=False)
+
     def test_forbidden_cycle(self):
         A = self.fake_gerrit.addFakeChange("org/project", "master", "A")
         B = self.fake_gerrit.addFakeChange("org/project3", "master", "B")

zuul/manager/__init__.py

@@ -11,6 +11,7 @@
 # under the License.
 import collections
 import contextlib
+import itertools
 import logging
 import textwrap
 import time
@@ -701,6 +702,13 @@ class PipelineManager(metaclass=ABCMeta):
                 return scc
         return []
 
+    def getCycleDependencies(self, change, dependency_graph, event):
+        cycle = self.cycleForChange(change, dependency_graph, event)
+        return set(
+            itertools.chain.from_iterable(
+                dependency_graph[c] for c in cycle if c != change)
+        ) - set(cycle)
+
     def getQueueConfig(self, project):
         layout = self.pipeline.tenant.layout
         queue_name = None

zuul/manager/dependent.py

@@ -159,6 +159,11 @@ class DependentPipelineManager(SharedQueuePipelineManager):
             warnings=warnings)
         if abort:
             return False
+
+        # Treat cycle dependencies as needed for the current change
+        needed_changes.extend(
+            self.getCycleDependencies(change, dependency_graph, event))
+
         if not needed_changes:
             return True
         log.debug("  Changes %s must be merged ahead of %s",

zuul/manager/independent.py

@@ -54,6 +54,11 @@ class IndependentPipelineManager(PipelineManager):
             dependency_graph=dependency_graph)
         if abort:
             return False
+
+        # Treat cycle dependencies as needed for the current change
+        needed_changes.extend(
+            self.getCycleDependencies(change, dependency_graph, event))
+
         if not needed_changes:
             return True
         log.debug("  Changes %s must be merged ahead of %s" % (