Support enqueuing behind circular dependencies

Change Ic121b2d8d057a7dc4448ae70045853347f265c6c omitted support
for enqueuing changes behind a cycle in order to avoid having them
show up in the middle of the cycle.  However, we can process changes
behind a cycle if we wait until we have completely enqueued the
cycle before we embark on enqueing changes behind it.

This also removes unused pipelines which cause extra processing and
output in the unit tests.

Also add a release note about circular deps, and describe additional
caveats about using circular dependencies in the documentation.

Change-Id: I64c0d4e8c20e4638bbafb18409cd28c062369738

doc/source/reference/queue_def.rst

@@ -63,5 +63,16 @@ Here is an example ``queue`` configuration.
       items fail to report (e.g. merge failure when some items were already
       merged). In this case the target branch(es) might be in a broken state.
 
-      In general, circular dependencies are considered to be an antipattern but
-      can't be avoided in certain cases.
+      In general, circular dependencies are considered to be an
+      antipattern since they add extra constraints to continuous
+      deployment systems.  Additionally, due to the lack of atomicity
+      in merge operations in code review systems, it may be possible
+      for only part of a cycle to be merged.  In that case, manual
+      interventions (such as reverting a commit, or bypassing gating to
+      force-merge the remaining commits) may be required.
+
+      .. warning:: If the remote system is able to merge the first but
+                   unable to merge the second or later change in a
+                   dependency cycle, then the gating system for a
+                   project may be broken and may require an
+                   intervention to correct.

releasenotes/notes/circular-deps-649f0fdd4dc5599f.yaml

@@ -0,0 +1,6 @@
+---
+features:
+  - |
+    Circular dependencies are now optionally supported (but disabled
+    by default).  To permit dependency cycles to be enqueued, see the
+    :attr:`queue.allow-circular-dependencies` option.

tests/fixtures/config/circular-dependencies/git/common-config/zuul.yaml

@@ -26,23 +26,6 @@
       github:
         status: failure
 
-- pipeline:
-    name: check-unused
-    manager: independent
-    trigger:
-      gerrit:
-        - event: patchset-created
-    success:
-      gerrit:
-        Verified: 1
-      github:
-        status: success
-    failure:
-      gerrit:
-        Verified: -1
-      github:
-        status: failure
-
 - pipeline:
     name: gate
     manager: dependent
@@ -80,31 +63,6 @@
       github: {}
     precedence: high
 
-- pipeline:
-    name: gate-unused
-    manager: dependent
-    success-message: Build succeeded (gate).
-    trigger:
-      gerrit:
-        - event: comment-added
-          approval:
-            - Approved: 1
-    success:
-      gerrit:
-        Verified: 2
-        submit: true
-      github:
-        merge: true
-    failure:
-      gerrit:
-        Verified: -2
-      github: {}
-    start:
-      gerrit:
-        Verified: 0
-      github: {}
-    precedence: high
-
 - job:
     name: base
     parent: null

tests/unit/test_circular_dependencies.py

@@ -325,16 +325,24 @@ class TestGerritCircularDependencies(ZuulTestCase):
         self.fake_gerrit.addEvent(C.addApproval("Approved", 1))
         self.waitUntilSettled()
 
-        self.assertEqual(len(self.builds), 2)
+        self.assertEqual(len(self.builds), 3)
+
+        # Make sure the out-of-cycle change (A) is enqueued after the cycle.
+        tenant = self.scheds.first.sched.abide.tenants.get("tenant-one")
+        queue_change_numbers = []
+        for queue in tenant.layout.pipelines["gate"].queues:
+            for item in queue.queue:
+                queue_change_numbers.append(item.change.number)
+        self.assertEqual(queue_change_numbers, ['2', '3', '1'])
 
         self.executor_server.hold_jobs_in_build = False
         self.executor_server.release()
         self.waitUntilSettled()
 
-        self.assertEqual(A.reported, 0)
+        self.assertEqual(A.reported, 2)
         self.assertEqual(B.reported, 2)
         self.assertEqual(C.reported, 2)
-        self.assertEqual(A.data["status"], "NEW")
+        self.assertEqual(A.data["status"], "MERGED")
         self.assertEqual(B.data["status"], "MERGED")
         self.assertEqual(C.data["status"], "MERGED")
 

zuul/manager/__init__.py

@@ -386,7 +386,7 @@ class PipelineManager(metaclass=ABCMeta):
                 log.debug("Change %s is already in queue, ignoring" % change)
                 return True
 
-            cycle = None
+            cycle = []
             if hasattr(change, "needs_changes"):
                 cycle = self.cycleForChange(change, dependency_graph, event)
                 if cycle and not self.canProcessCycle(change.project):
@@ -423,15 +423,18 @@ class PipelineManager(metaclass=ABCMeta):
 
             # Items in a dependency cycle are expected to be enqueued after
             # each other. To prevent non-cycle items from being enqueued
-            # between items of the same cycle, we skip that step when a cycle
-            # was found.
-            if not cycle:
-                self.enqueueChangesBehind(change, event, quiet,
-                                          ignore_requirements, change_queue,
-                                          history, dependency_graph)
-            else:
-                self.log.debug("Skip enqueueing changes behind because of "
-                               "dependency cycle")
+            # between items of the same cycle, enqueue items behind each item
+            # in the cycle once all items in the cycle are enqueued.
+            if all([self.isChangeAlreadyInQueue(c, change_queue)
+                    for c in cycle]):
+                if cycle:
+                    self.log.debug("Cycle complete, enqueing changes behind")
+                for c in cycle or [change]:
+                    self.enqueueChangesBehind(c, event, quiet,
+                                              ignore_requirements,
+                                              change_queue, history,
+                                              dependency_graph)
+
             zuul_driver = self.sched.connections.drivers['zuul']
             tenant = self.pipeline.tenant
             zuul_driver.onChangeEnqueued(
@@ -453,6 +456,7 @@ class PipelineManager(metaclass=ABCMeta):
                           change, change.project)
                 # Change can not be part of multiple cycles, so we can return
                 return scc
+        return []
 
     def canProcessCycle(self, project):
         layout = self.pipeline.tenant.layout
@@ -509,7 +513,7 @@ class PipelineManager(metaclass=ABCMeta):
     def dequeueIncompleteCycle(self, change, dependency_graph, event,
                                change_queue):
         log = get_annotated_logger(self.log, event)
-        cycle = self.cycleForChange(change, dependency_graph, event) or []
+        cycle = self.cycleForChange(change, dependency_graph, event)
         enqueued_cycle_items = [i for i in (self.getItemForChange(c,
                                                                   change_queue)
                                             for c in cycle) if i is not None]