Detect dependency cycle as soon as possible

cdist/config.py

@@ -43,6 +43,33 @@ from cdist import core, inventory
 from cdist.util.remoteutil import inspect_ssh_mux_opts
 
 
+def graph_check_cycle(graph):
+    # Start from each node in the graph and check for cycle starting from it.
+    for node in graph:
+        # Cycle path.
+        path = [node]
+        has_cycle = _graph_dfs_cycle( graph, node, path)
+        if has_cycle:
+            return has_cycle, path
+    return False, None
+
+
+def _graph_dfs_cycle(graph, node, path):
+    for neighbour in graph.get(node, ()):
+        # If node is already in path then this is cycle.
+        if neighbour in path:
+            path.append(neighbour)
+            return True
+        path.append(neighbour)
+        rv = _graph_dfs_cycle(graph, neighbour, path)
+        if rv:
+            return True
+        # Remove last item from list - neighbour whose DFS path we have have
+        # just checked.
+        del path[-1]
+    return False
+
+
 class Config(object):
     """Cdist main class to hold arbitrary data"""
 
@@ -254,14 +281,14 @@ class Config(object):
                     cls.onehost(host, host_tags, host_base_path, hostdir,
                                 args, parallel=False,
                                 configuration=configuration)
-                except cdist.Error as e:
+                except cdist.Error:
                     failed_hosts.append(host)
         if args.parallel and len(process_args) == 1:
             log.debug("Only 1 host for parallel processing, doing it "
                       "sequentially")
             try:
                 cls.onehost(*process_args[0])
-            except cdist.Error as e:
+            except cdist.Error:
                 failed_hosts.append(host)
         elif args.parallel:
             log.trace("Multiprocessing start method is {}".format(
@@ -653,6 +680,28 @@ class Config(object):
         self.__dict__.update(state)
         self._open_logger()
 
+    def _validate_dependencies(self):
+        '''
+            Build dependency graph for unfinished objects and
+            check for cycles.
+        '''
+        graph = {}
+        for cdist_object in self.object_list():
+            obj_name = cdist_object.name
+            if obj_name not in graph:
+                graph[obj_name] = []
+            if cdist_object.state == cdist_object.STATE_DONE:
+                continue
+
+            for requirement in cdist_object.requirements_unfinished(
+                    cdist_object.requirements):
+                graph[obj_name].append(requirement.name)
+
+            for requirement in cdist_object.requirements_unfinished(
+                    cdist_object.autorequire):
+                graph[obj_name].append(requirement.name)
+        return graph_check_cycle(graph)
+
     def iterate_until_finished(self):
         """
             Go through all objects and solve them
@@ -662,6 +711,12 @@ class Config(object):
         objects_changed = True
 
         while objects_changed:
+            # Check for cycles as early as possible.
+            has_cycle, path = self._validate_dependencies()
+            if has_cycle:
+                raise cdist.UnresolvableRequirementsError(
+                    "Cycle detected in object dependencies:\n{}!".format(
+                        " -> ".join(path)))
             objects_changed = self.iterate_once()
 
         # Check whether all objects have been finished

cdist/test/config/__init__.py

@@ -23,7 +23,6 @@
 
 import os
 import shutil
-import tempfile
 
 from cdist import test
 from cdist import core
@@ -212,7 +211,7 @@ class ConfigRunTestCase(test.CdistTestCase):
         dryrun.run()
         # if we are here, dryrun works like expected
 
-    def test_desp_resolver(self):
+    def test_deps_resolver(self):
         """Test to show dependency resolver warning message."""
         local = cdist.exec.local.Local(
             target_host=self.target_host,
@@ -229,6 +228,71 @@ class ConfigRunTestCase(test.CdistTestCase):
         config = cdist.config.Config(local, self.remote, dry_run=True)
         config.run()
 
+    def test_graph_check_cycle_empty(self):
+        graph = {}
+        has_cycle, path = cdist.config.graph_check_cycle(graph)
+        self.assertFalse(has_cycle)
+
+    def test_graph_check_cycle_1(self):
+        #
+        # a -> b -> c
+        #      |
+        #      +--> d -> e
+        graph = {
+            'a': ['b', ],
+            'b': ['c', 'd', ],
+            'd': ['e', ],
+        }
+        has_cycle, path = cdist.config.graph_check_cycle(graph)
+        self.assertFalse(has_cycle)
+
+    def test_graph_check_cycle_2(self):
+        #
+        # a -> b -> c
+        # /\        |
+        #  \        |
+        #   +-------+
+        graph = {
+            'a': ['b', ],
+            'b': ['c', ],
+            'c': ['a', ],
+        }
+        has_cycle, path = cdist.config.graph_check_cycle(graph)
+        self.assertTrue(has_cycle)
+        self.assertGreater(path.count(path[-1]), 1)
+
+    def test_graph_check_cycle_3(self):
+        #
+        # a -> b -> c
+        #  \        \
+        #   \        +--> g
+        #    \            /\
+        #     \           /|
+        #      +-> d -> e  |
+        #           \      |
+        #            + --> f
+        #
+        # h -> i --> j
+        # |    /\    |
+        # \/    |    \/
+        # n     m <- k
+        graph = {
+            'a': ['b', 'd', ],
+            'b': ['c', ],
+            'c': ['g', ],
+            'd': ['e', 'f', ],
+            'e': ['g', ],
+            'f': ['g', ],
+            'h': ['i', 'n', ],
+            'i': ['j', ],
+            'j': ['k', ],
+            'k': ['m', ],
+            'm': ['i', ],
+        }
+        has_cycle, path = cdist.config.graph_check_cycle(graph)
+        self.assertTrue(has_cycle)
+        self.assertGreater(path.count(path[-1]), 1)
+
 
 # Currently the resolving code will simply detect that this object does
 # not exist. It should probably check if the type is a singleton as well