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 <steven@icarus.ethz.ch>

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)

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"))

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)

lib/cdist/resolver.py

@@ -50,36 +50,41 @@ class DependencyResolver(object):
     """Cdist's dependency resolver.
 
     Usage:
-    resolver = DependencyResolver(list_of_objects)
+    >> resolver = DependencyResolver(list_of_objects)
-    from pprint import pprint
+    # Easy access to the objects we are working with
-    pprint(resolver.graph)
+    >> resolver.objects['__some_type/object_id']
-
+    <CdistObject __some_type/object_id>
-    for cdist_object in resolver:
+    # Easy access to a specific objects dependencies
-        do_something_with(cdist_object)
+    >> resolver.dependencies['__some_type/object_id']
-
+    [<CdistObject __other_type/dependency>, <CdistObject __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.objects = dict((o.name, o) for o in objects)
-        self._object_index = dict((o.name, o) for o in self.objects)
+        self._dependencies = None
-        self._graph = 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:
+        if self._dependencies is None:
-            graph = {}
+            self._dependencies = d = {}
-            for o in self.objects:
+            self._preprocess_requirements()
+            for name,cdist_object in self.objects.items():
                 resolved = []
                 unresolved = []
-                self.resolve_object_dependencies(o, resolved, unresolved)
+                self._resolve_object_dependencies(cdist_object, resolved, unresolved)
-                graph[o.name] = resolved
+                d[name] = resolved
-            self._graph = graph
+        return self._dependencies
-        return self._graph
 
     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 __type/object_id>, <Object __other_type/any>, <Object __other_type/match>]
         """
-        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:
+                if singleton in self.objects:
-                    yield self._object_index[singleton]
+                    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

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 <http://www.gnu.org/licenses/>.
+#
+#
+
+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()

lib/cdist/test/autorequire/fixtures/conf/manifest/circular_dependency

@@ -0,0 +1,2 @@
+# this has triggered CircularReferenceError
+__nfsroot_client test

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

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

lib/cdist/test/autorequire/fixtures/conf/type/__package/manifest

@@ -0,0 +1 @@
+__package_special "$__object_id"

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"

lib/cdist/test/autorequire/fixtures/conf/type/__top/manifest

@@ -0,0 +1,2 @@
+__package b
+require="__package/b" __package a

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):

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