Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Tree object runner with circular dependency detector #166

Closed
wants to merge 3 commits into from

3 participants

@arkaitzj

Changed the way objects are executed, instead of preparing(resolve&manifest) everything before running the first object, we walk the dependency tree preparing and running as soon as possible and always preparing after dependencies have been run, following a Depth-First tree traversal.
Dependencies change during execution as we are not aware of autorequires till we have executed the manifest, but this has been handled as well.
Also, circular dependency detection is performed on both standard and auto requirements.
Unit tests fixed where needed and removed where did not make sense, like standalone prepare dependency checking.

@telmich telmich commented on the diff
cdist/config_install.py
((26 lines not shown))
- new_objects_created = True
+ deps = list(core.CdistObject.list_objects(self.context.local.object_path,self.context.local.type_path))
+ ready_objects = {}
+ seen_objs = set()
+ while deps:
+ obj = deps[-1]
+ if obj.name in ready_objects:
+ deps.pop()
+ continue
+ seen_objs.add(obj.name)
+ unmet_reqs = [ req for req in obj.requirements if req not in ready_objects ]
+ if unmet_reqs:
+ seen_reqs = [req for req in unmet_reqs if req in seen_objs]
+ if seen_reqs:
+ raise CircularReferenceError(obj,obj.object_from_name(seen_reqs[0]))
+ deps.extend(obj.find_requirements_by_name(unmet_reqs))
@telmich Owner
telmich added a note

Running obj.find_requirements_by_name(unmet_reqs) may actually create a list with missing objects: The objects that are going to be created later, but match a wildcard: require="__file/*" __foo will only require the current __file objects, but should actually also require all future created __file objects.

Please prove me wrong.

@telmich Owner
telmich added a note

Actually it seems that this behaviour has not been documented - starting internal discussion about how to proceed.

@arkaitzj
arkaitzj added a note

Well, I was not aware that wildcards could be used for require= parameters.
The way this is laid out I would rather think its not a good idea, this adds quite some limitations and yet is not effective as a resource collector.
I would rather be in favor of adding collector behavior so that we could somehow "collect and iterate" over all resources that match a pattern, and then do something, this would obviously happen on a post-run, since you can still create resources on manifests based on explorers and explorers could be finding stuff created by other types gencode-remotes.
My 2 cents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@asteven
Collaborator

@arkaitzj agreed. pattern pased requirements creates more problems then it solves.
@telmich let's ditch it

@telmich
Owner

Hey Arkaitz,

I'd love to merge your change, but as we have a lot of dependencies in the dependency resolver (sigh!) this is a dangerous area.

Thus to be able to merge your pull request, I am now creating a test case that tests the correct run time and dependency order.

I am currently establishing it in the dependency_tests branch.

I'd be very thankful if you could also add potential test cases you see.

The idea is to have this test case independent of the resolver/object code (as far as possible) to test the correct workflow from outside (maybe using mock types or similar approaches).

@telmich
Owner

I've just given your pull request a try, there must be at least one bug:

INFO: recomy-staging.panter.ch: Generating and executing code for __directory/home/rails/app/releases
INFO: recomy-staging.panter.ch: Generating and executing code for __panter_rails_railshome/singleton

at this point cdist uses 100% cpu and does not continue => I suspect an endless loop there.

Going into debug mode I end in:

DEBUG: recomy-staging.panter.ch: Finishing run of __directory/home/rails/app/releases
DEBUG: recomy-staging.panter.ch: Trying to run object __panter_rails_railshome/singleton
INFO: recomy-staging.panter.ch: Generating and executing code for __panter_rails_railshome/singleton
DEBUG: recomy-staging.panter.ch: Finishing run of __panter_rails_railshome/singleton

I'll probably dig into that tomorrow, but for now this is not yet ready to get merged.

@telmich
Owner
@telmich
Owner

Fixed in master

@telmich telmich closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 41 additions and 51 deletions.
  1. +38 −33 cdist/config_install.py
  2. +3 −18 cdist/test/autorequire/__init__.py
View
71 cdist/config_install.py
@@ -32,7 +32,7 @@
import cdist
from cdist import core
-from cdist import resolver
+from cdist.resolver import CircularReferenceError
class ConfigInstall(object):
@@ -65,8 +65,7 @@ def cleanup(self):
def deploy_to(self):
"""Mimic the old deploy to: Deploy to one host"""
- self.stage_prepare()
- self.stage_run()
+ self.tree_deploy()
def deploy_and_cleanup(self):
"""Do what is most often done: deploy & cleanup"""
@@ -76,25 +75,47 @@ def deploy_and_cleanup(self):
self.log.info("Finished successful run in %s seconds",
time.time() - start_time)
- def stage_prepare(self):
- """Do everything for a deploy, minus the actual code stage"""
+ def tree_deploy(self):
+ """ Walks the dependency tree executing manifests and object code in strict dependency order
+ waiting to execute manifests/resolvers till the dependencies object code has been executed
+ """
self.explorer.run_global_explorers(self.context.local.global_explorer_out_path)
self.manifest.run_initial_manifest(self.context.initial_manifest)
self.log.info("Running object manifests and type explorers")
- # Continue process until no new objects are created anymore
- new_objects_created = True
- while new_objects_created:
- new_objects_created = False
- for cdist_object in core.CdistObject.list_objects(self.context.local.object_path,
- self.context.local.type_path):
- if cdist_object.state == core.CdistObject.STATE_PREPARED:
- self.log.debug("Skipping re-prepare of object %s", cdist_object)
- continue
- else:
- self.object_prepare(cdist_object)
- new_objects_created = True
+ deps = list(core.CdistObject.list_objects(self.context.local.object_path,self.context.local.type_path))
+ ready_objects = {}
+ seen_objs = set()
+ while deps:
+ obj = deps[-1]
+ if obj.name in ready_objects:
+ deps.pop()
+ continue
+ seen_objs.add(obj.name)
+ unmet_reqs = [ req for req in obj.requirements if req not in ready_objects ]
+ if unmet_reqs:
+ seen_reqs = [req for req in unmet_reqs if req in seen_objs]
+ if seen_reqs:
+ raise CircularReferenceError(obj,obj.object_from_name(seen_reqs[0]))
+ deps.extend(obj.find_requirements_by_name(unmet_reqs))
@telmich Owner
telmich added a note

Running obj.find_requirements_by_name(unmet_reqs) may actually create a list with missing objects: The objects that are going to be created later, but match a wildcard: require="__file/*" __foo will only require the current __file objects, but should actually also require all future created __file objects.

Please prove me wrong.

@telmich Owner
telmich added a note

Actually it seems that this behaviour has not been documented - starting internal discussion about how to proceed.

@arkaitzj
arkaitzj added a note

Well, I was not aware that wildcards could be used for require= parameters.
The way this is laid out I would rather think its not a good idea, this adds quite some limitations and yet is not effective as a resource collector.
I would rather be in favor of adding collector behavior so that we could somehow "collect and iterate" over all resources that match a pattern, and then do something, this would obviously happen on a post-run, since you can still create resources on manifests based on explorers and explorers could be finding stuff created by other types gencode-remotes.
My 2 cents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ continue
+ if obj.state != core.CdistObject.STATE_PREPARED:
+ self.object_prepare(obj)
+
+ unmet_autoreqs = [ req for req in obj.autorequire if req not in ready_objects]
+ if unmet_autoreqs:
+ seen_reqs = [req for req in unmet_autoreqs if req in seen_objs]
+ if seen_reqs:
+ raise CircularReferenceError(obj,obj.object_from_name(seen_reqs[0]))
+ deps.extend(obj.find_requirements_by_name(unmet_autoreqs))
+ continue
+
+ self.object_run(obj)
+ seen_objs.remove(obj.name)
+ ready_objects[obj.name] = obj
+ deps.pop()
+
def object_prepare(self, cdist_object):
"""Prepare object: Run type explorer + manifest"""
@@ -109,7 +130,6 @@ def object_run(self, cdist_object, dry_run=False):
if cdist_object.state == core.CdistObject.STATE_DONE:
raise cdist.Error("Attempting to run an already finished object: %s", cdist_object)
- cdist_type = cdist_object.cdist_type
# Generate
self.log.info("Generating and executing code for " + cdist_object.name)
@@ -129,18 +149,3 @@ def object_run(self, cdist_object, dry_run=False):
# Mark this object as done
self.log.debug("Finishing run of " + cdist_object.name)
cdist_object.state = core.CdistObject.STATE_DONE
-
- def stage_run(self):
- """The final (and real) step of deployment"""
- self.log.info("Generating and executing code")
-
- objects = core.CdistObject.list_objects(
- self.context.local.object_path,
- self.context.local.type_path)
-
- dependency_resolver = resolver.DependencyResolver(objects)
- self.log.debug(pprint.pformat(dependency_resolver.dependencies))
-
- for cdist_object in dependency_resolver:
- self.log.debug("Run object: %s", cdist_object)
- self.object_run(cdist_object)
View
21 cdist/test/autorequire/__init__.py
@@ -64,28 +64,13 @@ 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.context.local.manifest_path, 'implicit_dependencies')
- self.config.stage_prepare()
-
- objects = core.CdistObject.list_objects(self.context.local.object_path, self.context.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.context.local.manifest_path, 'circular_dependency')
- self.config.stage_prepare()
+ self.config.tree_deploy()
# raises CircularDependecyError
- self.config.stage_run()
def test_recursive_type(self):
- self.context.initial_manifest = os.path.join(self.config.local.manifest_path, 'recursive_type')
- self.config.stage_prepare()
+ self.context.initial_manifest = os.path.join(self.config.context.local.manifest_path, 'recursive_type')
+ self.config.tree_deploy()
# raises CircularDependecyError
- self.config.stage_run()
Something went wrong with that request. Please try again.