From edfaa65d2b37fcb4a7de2280b61dec30dd3a8c5d Mon Sep 17 00:00:00 2001 From: Darko Poljak Date: Wed, 1 May 2019 14:18:39 +0200 Subject: [PATCH] Detect dependency cycle as soon as possible --- cdist/config.py | 59 ++++++++++++++++++++++++++++-- cdist/test/config/__init__.py | 68 +++++++++++++++++++++++++++++++++-- 2 files changed, 123 insertions(+), 4 deletions(-) diff --git a/cdist/config.py b/cdist/config.py index fae2ff46..4e226b3a 100644 --- a/cdist/config.py +++ b/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 diff --git a/cdist/test/config/__init__.py b/cdist/test/config/__init__.py index 2b0d8b5f..499593e3 100644 --- a/cdist/test/config/__init__.py +++ b/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