From 7d61b777088c5b254c1cfad6a615bc619c79eab6 Mon Sep 17 00:00:00 2001 From: Steven Armstrong Date: Thu, 3 May 2012 10:16:08 +0200 Subject: [PATCH] fix autorequire dependency handling - inherit explicit requirements that the user defined himself - but _not_ implicit requirements that cdist added for autorequire Changes: - added new .autorequire property to CdistObject to keep track of implicit autorequire dependencies - Emulator appends implicit requirements to this .autorequire property - DependencyResolver preprocess these .autorequire properties before resolving normal dependencies - refactored and documented DependencyResolver so it's clearer what happens and easier to use from tests - update test cases to match new DependencyResolver behaviour Signed-off-by: Steven Armstrong --- lib/cdist/config_install.py | 2 +- lib/cdist/core/cdist_object.py | 1 + lib/cdist/emulator.py | 6 +- lib/cdist/resolver.py | 80 +++++++++++++------ lib/cdist/test/autorequire/__init__.py | 78 ++++++++++++++++++ .../autorequire/fixtures/conf/explorer/.keep | 0 .../conf/manifest/circular_dependency | 2 + .../conf/manifest/implicit_dependencies | 3 + .../conf/type/__addifnosuchline/.keep | 0 .../fixtures/conf/type/__directory/.keep | 0 .../conf/type/__nfsroot_client/manifest | 3 + .../fixtures/conf/type/__package/manifest | 1 + .../conf/type/__package_special/.keep | 0 .../type/__root_ssh_authorized_key/manifest | 4 + .../fixtures/conf/type/__top/manifest | 2 + .../fixtures/conf/type/__user/.keep | 0 lib/cdist/test/emulator/__init__.py | 5 +- lib/cdist/test/resolver/__init__.py | 6 +- 18 files changed, 156 insertions(+), 37 deletions(-) create mode 100644 lib/cdist/test/autorequire/__init__.py create mode 100644 lib/cdist/test/autorequire/fixtures/conf/explorer/.keep create mode 100755 lib/cdist/test/autorequire/fixtures/conf/manifest/circular_dependency create mode 100755 lib/cdist/test/autorequire/fixtures/conf/manifest/implicit_dependencies create mode 100644 lib/cdist/test/autorequire/fixtures/conf/type/__addifnosuchline/.keep create mode 100644 lib/cdist/test/autorequire/fixtures/conf/type/__directory/.keep create mode 100755 lib/cdist/test/autorequire/fixtures/conf/type/__nfsroot_client/manifest create mode 100755 lib/cdist/test/autorequire/fixtures/conf/type/__package/manifest create mode 100644 lib/cdist/test/autorequire/fixtures/conf/type/__package_special/.keep create mode 100755 lib/cdist/test/autorequire/fixtures/conf/type/__root_ssh_authorized_key/manifest create mode 100755 lib/cdist/test/autorequire/fixtures/conf/type/__top/manifest create mode 100644 lib/cdist/test/autorequire/fixtures/conf/type/__user/.keep diff --git a/lib/cdist/config_install.py b/lib/cdist/config_install.py index 7cce240e..3aadca86 100644 --- a/lib/cdist/config_install.py +++ b/lib/cdist/config_install.py @@ -141,7 +141,7 @@ class ConfigInstall(object): self.local.type_path) dependency_resolver = resolver.DependencyResolver(objects) - self.log.debug(pprint.pformat(dependency_resolver.graph)) + self.log.debug(pprint.pformat(dependency_resolver.dependencies)) for cdist_object in dependency_resolver: self.log.debug("Run object: %s", cdist_object) diff --git a/lib/cdist/core/cdist_object.py b/lib/cdist/core/cdist_object.py index e12bcfbd..90a21e59 100644 --- a/lib/cdist/core/cdist_object.py +++ b/lib/cdist/core/cdist_object.py @@ -186,6 +186,7 @@ class CdistObject(object): return os.path.join(self.path, "explorer") requirements = fsproperty.FileListProperty(lambda obj: os.path.join(obj.absolute_path, 'require')) + autorequire = fsproperty.FileListProperty(lambda obj: os.path.join(obj.absolute_path, 'autorequire')) parameters = fsproperty.DirectoryDictProperty(lambda obj: os.path.join(obj.base_path, obj.parameter_path)) explorers = fsproperty.DirectoryDictProperty(lambda obj: os.path.join(obj.base_path, obj.explorer_path)) changed = fsproperty.FileBooleanProperty(lambda obj: os.path.join(obj.absolute_path, "changed")) diff --git a/lib/cdist/emulator.py b/lib/cdist/emulator.py index 99b34554..39d8ca40 100644 --- a/lib/cdist/emulator.py +++ b/lib/cdist/emulator.py @@ -167,12 +167,8 @@ class Emulator(object): parent = self.cdist_object.object_from_name(__object_name) # The object currently being defined current_object = self.cdist_object - # current_object shall have all dependencies that it's parent has - for req in parent.requirements: - if req not in current_object.requirements: - current_object.requirements.append(req) # As parent defined current_object it shall automatically depend on it. # But only if the user hasn't said otherwise. # Must prevent circular dependencies. if not parent.name in current_object.requirements: - parent.requirements.append(current_object.name) + parent.autorequire.append(current_object.name) diff --git a/lib/cdist/resolver.py b/lib/cdist/resolver.py index 368c9eb8..7e3a1a68 100644 --- a/lib/cdist/resolver.py +++ b/lib/cdist/resolver.py @@ -50,36 +50,41 @@ class DependencyResolver(object): """Cdist's dependency resolver. Usage: - resolver = DependencyResolver(list_of_objects) - from pprint import pprint - pprint(resolver.graph) - - for cdist_object in resolver: - do_something_with(cdist_object) - + >> resolver = DependencyResolver(list_of_objects) + # Easy access to the objects we are working with + >> resolver.objects['__some_type/object_id'] + + # Easy access to a specific objects dependencies + >> resolver.dependencies['__some_type/object_id'] + [, ] + # Pretty print the dependency graph + >> from pprint import pprint + >> pprint(resolver.dependencies) + # Iterate over all existing objects in the correct order + >> for cdist_object in resolver: + >> do_something_with(cdist_object) """ def __init__(self, objects, logger=None): - self.objects = list(objects) # make sure we store as list, not generator - self._object_index = dict((o.name, o) for o in self.objects) - self._graph = None + self.objects = dict((o.name, o) for o in objects) + self._dependencies = None self.log = logger or log @property - def graph(self): + def dependencies(self): """Build the dependency graph. Returns a dict where the keys are the object names and the values are lists of all dependencies including the key object itself. """ - if self._graph is None: - graph = {} - for o in self.objects: + if self._dependencies is None: + self._dependencies = d = {} + self._preprocess_requirements() + for name,cdist_object in self.objects.items(): resolved = [] unresolved = [] - self.resolve_object_dependencies(o, resolved, unresolved) - graph[o.name] = resolved - self._graph = graph - return self._graph + self._resolve_object_dependencies(cdist_object, resolved, unresolved) + d[name] = resolved + return self._dependencies def find_requirements_by_name(self, requirements): """Takes a list of requirement patterns and returns a list of matching object instances. @@ -89,21 +94,44 @@ class DependencyResolver(object): find_requirements_by_name(['__type/object_id', '__other_type/*']) -> [, , ] """ - object_names = self._object_index.keys() + object_names = self.objects.keys() for pattern in requirements: found = False for requirement in fnmatch.filter(object_names, pattern): found = True - yield self._object_index[requirement] + yield self.objects[requirement] if not found: # FIXME: get rid of the singleton object_id, it should be invisible to the code -> hide it in Object singleton = os.path.join(pattern, 'singleton') - if singleton in self._object_index: - yield self._object_index[singleton] + if singleton in self.objects: + yield self.objects[singleton] else: raise RequirementNotFoundError(pattern) - def resolve_object_dependencies(self, cdist_object, resolved, unresolved): + def _preprocess_requirements(self): + """Find all autorequire dependencies and merge them to be just requirements + for further processing. + """ + for cdist_object in self.objects.values(): + if cdist_object.autorequire: + # The objects (children) that this cdist_object (parent) defined + # in it's type manifest shall inherit all explicit requirements + # that the parent has so that user defined requirements are + # fullfilled and processed in the expected order. + for auto_requirement in self.find_requirements_by_name(cdist_object.autorequire): + for requirement in cdist_object.requirements: + if requirement not in auto_requirement.requirements: + auto_requirement.requirements.append(requirement) + # On the other hand the parent shall depend on all the children + # it created so that the user can setup dependencies on it as a + # whole without having to know anything about the parents + # internals. + cdist_object.requirements.extend(cdist_object.autorequire) + # As we changed the object on disc, we have to ensure it is not + # preprocessed again if someone would call us multiple times. + cdist_object.autorequire = [] + + def _resolve_object_dependencies(self, cdist_object, resolved, unresolved): """Resolve all dependencies for the given cdist_object and store them in the list which is passed as the 'resolved' arguments. @@ -121,16 +149,16 @@ class DependencyResolver(object): if required_object not in resolved: if required_object in unresolved: raise CircularReferenceError(cdist_object, required_object) - self.resolve_object_dependencies(required_object, resolved, unresolved) + self._resolve_object_dependencies(required_object, resolved, unresolved) resolved.append(cdist_object) unresolved.remove(cdist_object) except RequirementNotFoundError as e: raise cdist.CdistObjectError(cdist_object, "requires non-existing " + e.requirement) def __iter__(self): - """Iterate over all unique objects while resolving dependencies. + """Iterate over all unique objects and yield them in the correct order. """ - iterable = itertools.chain(*self.graph.values()) + iterable = itertools.chain(*self.dependencies.values()) # Keep record of objects that have already been seen seen = set() seen_add = seen.add diff --git a/lib/cdist/test/autorequire/__init__.py b/lib/cdist/test/autorequire/__init__.py new file mode 100644 index 00000000..2263cbf9 --- /dev/null +++ b/lib/cdist/test/autorequire/__init__.py @@ -0,0 +1,78 @@ +# -*- coding: utf-8 -*- +# +# 2010-2011 Steven Armstrong (steven-cdist at armstrong.cc) +# +# This file is part of cdist. +# +# cdist is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# cdist is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with cdist. If not, see . +# +# + +import os +import shutil + +import cdist +from cdist import test +from cdist.exec import local +from cdist import core +from cdist.core import manifest +from cdist import resolver +from cdist import config +import cdist.context + +import os.path as op +my_dir = op.abspath(op.dirname(__file__)) +fixtures = op.join(my_dir, 'fixtures') +local_base_path = fixtures + + +class AutorequireTestCase(test.CdistTestCase): + + def setUp(self): + self.orig_environ = os.environ + os.environ = os.environ.copy() + self.target_host = 'localhost' + self.temp_dir = self.mkdtemp() + os.environ['__cdist_out_dir'] = self.temp_dir + + self.context = cdist.context.Context( + target_host=self.target_host, + base_path=local_base_path, + exec_path=test.cdist_exec_path, + debug=False) + self.config = config.Config(self.context) + + def tearDown(self): + os.environ = self.orig_environ + shutil.rmtree(self.temp_dir) + + def test_implicit_dependencies(self): + self.context.initial_manifest = os.path.join(self.config.local.manifest_path, 'implicit_dependencies') + self.config.stage_prepare() + + objects = core.CdistObject.list_objects(self.config.local.object_path, self.config.local.type_path) + dependency_resolver = resolver.DependencyResolver(objects) + expected_dependencies = [ + dependency_resolver.objects['__package_special/b'], + dependency_resolver.objects['__package/b'], + dependency_resolver.objects['__package_special/a'] + ] + resolved_dependencies = dependency_resolver.dependencies['__package_special/a'] + self.assertEqual(resolved_dependencies, expected_dependencies) + + def test_circular_dependency(self): + self.context.initial_manifest = os.path.join(self.config.local.manifest_path, 'circular_dependency') + self.config.stage_prepare() + # raises CircularDependecyError + self.config.stage_run() diff --git a/lib/cdist/test/autorequire/fixtures/conf/explorer/.keep b/lib/cdist/test/autorequire/fixtures/conf/explorer/.keep new file mode 100644 index 00000000..e69de29b diff --git a/lib/cdist/test/autorequire/fixtures/conf/manifest/circular_dependency b/lib/cdist/test/autorequire/fixtures/conf/manifest/circular_dependency new file mode 100755 index 00000000..6ea308d1 --- /dev/null +++ b/lib/cdist/test/autorequire/fixtures/conf/manifest/circular_dependency @@ -0,0 +1,2 @@ +# this has triggered CircularReferenceError +__nfsroot_client test diff --git a/lib/cdist/test/autorequire/fixtures/conf/manifest/implicit_dependencies b/lib/cdist/test/autorequire/fixtures/conf/manifest/implicit_dependencies new file mode 100755 index 00000000..7d68aed2 --- /dev/null +++ b/lib/cdist/test/autorequire/fixtures/conf/manifest/implicit_dependencies @@ -0,0 +1,3 @@ +# this creates implicit dependencies through autorequire. +# this failed because autorequired dependencies where not aware of their anchestors dependencies +__top test diff --git a/lib/cdist/test/autorequire/fixtures/conf/type/__addifnosuchline/.keep b/lib/cdist/test/autorequire/fixtures/conf/type/__addifnosuchline/.keep new file mode 100644 index 00000000..e69de29b diff --git a/lib/cdist/test/autorequire/fixtures/conf/type/__directory/.keep b/lib/cdist/test/autorequire/fixtures/conf/type/__directory/.keep new file mode 100644 index 00000000..e69de29b diff --git a/lib/cdist/test/autorequire/fixtures/conf/type/__nfsroot_client/manifest b/lib/cdist/test/autorequire/fixtures/conf/type/__nfsroot_client/manifest new file mode 100755 index 00000000..f6cb7833 --- /dev/null +++ b/lib/cdist/test/autorequire/fixtures/conf/type/__nfsroot_client/manifest @@ -0,0 +1,3 @@ +__user root +__root_ssh_authorized_key john +__root_ssh_authorized_key frank diff --git a/lib/cdist/test/autorequire/fixtures/conf/type/__package/manifest b/lib/cdist/test/autorequire/fixtures/conf/type/__package/manifest new file mode 100755 index 00000000..6f70e627 --- /dev/null +++ b/lib/cdist/test/autorequire/fixtures/conf/type/__package/manifest @@ -0,0 +1 @@ +__package_special "$__object_id" diff --git a/lib/cdist/test/autorequire/fixtures/conf/type/__package_special/.keep b/lib/cdist/test/autorequire/fixtures/conf/type/__package_special/.keep new file mode 100644 index 00000000..e69de29b diff --git a/lib/cdist/test/autorequire/fixtures/conf/type/__root_ssh_authorized_key/manifest b/lib/cdist/test/autorequire/fixtures/conf/type/__root_ssh_authorized_key/manifest new file mode 100755 index 00000000..6224629f --- /dev/null +++ b/lib/cdist/test/autorequire/fixtures/conf/type/__root_ssh_authorized_key/manifest @@ -0,0 +1,4 @@ +user="$__object_id" +__directory /root/.ssh +require="__directory/root/.ssh" \ + __addifnosuchline "ssh-root-$user" diff --git a/lib/cdist/test/autorequire/fixtures/conf/type/__top/manifest b/lib/cdist/test/autorequire/fixtures/conf/type/__top/manifest new file mode 100755 index 00000000..d6968c25 --- /dev/null +++ b/lib/cdist/test/autorequire/fixtures/conf/type/__top/manifest @@ -0,0 +1,2 @@ +__package b +require="__package/b" __package a diff --git a/lib/cdist/test/autorequire/fixtures/conf/type/__user/.keep b/lib/cdist/test/autorequire/fixtures/conf/type/__user/.keep new file mode 100644 index 00000000..e69de29b diff --git a/lib/cdist/test/emulator/__init__.py b/lib/cdist/test/emulator/__init__.py index 370d3d82..077ea111 100644 --- a/lib/cdist/test/emulator/__init__.py +++ b/lib/cdist/test/emulator/__init__.py @@ -114,7 +114,8 @@ class AutoRequireEmulatorTestCase(test.CdistTestCase): self.manifest = core.Manifest(self.target_host, self.local) def tearDown(self): - shutil.rmtree(self.temp_dir) + pass + #shutil.rmtree(self.temp_dir) def test_autorequire(self): initial_manifest = os.path.join(self.local.manifest_path, "init") @@ -123,7 +124,7 @@ class AutoRequireEmulatorTestCase(test.CdistTestCase): cdist_object = core.CdistObject(cdist_type, self.local.object_path, 'singleton') self.manifest.run_type_manifest(cdist_object) expected = ['__planet/Saturn', '__moon/Prometheus'] - self.assertEqual(sorted(cdist_object.requirements), sorted(expected)) + self.assertEqual(sorted(cdist_object.autorequire), sorted(expected)) class ArgumentsTestCase(test.CdistTestCase): diff --git a/lib/cdist/test/resolver/__init__.py b/lib/cdist/test/resolver/__init__.py index ae8f6915..baae26de 100644 --- a/lib/cdist/test/resolver/__init__.py +++ b/lib/cdist/test/resolver/__init__.py @@ -69,7 +69,7 @@ class ResolverTestCase(test.CdistTestCase): first_man.requirements = [second_on_the.name] second_on_the.requirements = [third_moon.name] self.assertEqual( - self.dependency_resolver.graph['__first/man'], + self.dependency_resolver.dependencies['__first/man'], [third_moon, second_on_the, first_man] ) @@ -79,10 +79,10 @@ class ResolverTestCase(test.CdistTestCase): first_man.requirements = [first_woman.name] first_woman.requirements = [first_man.name] with self.assertRaises(resolver.CircularReferenceError): - self.dependency_resolver.graph + self.dependency_resolver.dependencies def test_requirement_not_found(self): first_man = self.object_index['__first/man'] first_man.requirements = ['__does/not/exist'] with self.assertRaises(cdist.Error): - self.dependency_resolver.graph + self.dependency_resolver.dependencies