From da274e5ef3e8c1eb47bb9fde003248711439fc8d Mon Sep 17 00:00:00 2001 From: Darko Poljak Date: Sat, 23 Nov 2019 00:25:51 +0100 Subject: [PATCH 1/2] Redefine/reimplement CDIST_ORDER_DEPENDENCY CDIST_ORDER_DEPENDENCY now defines type order dependency context. cdist (emulator) maintains global state variables, as files, order_dep_state and typeorder_dep, and per object state variable, as file, typeorder_dep. If order_dep_state exists then this defines that order dependency is turned on. If order_dep_state does not exist then order dependency is turned off. If order dependency is on then objects created after it is turned on are recorded into: * global typeorder_dep, in case of init manifest * object's typeorder_dep, in case of type's manifest. If order dependency is on then requirement is injected, where object created before current, is read from: * global typeorder_dep, in case of init manifest * object's typeorder_dep, in case of type's manifest. Every time order dependency is turned off, typeorder_dep files are removed, which means that type order list is cleared, context is cleaned. In the end cdist cleans after itself, i.e. mentioned files are removed. When running type manifest is finished typeorder_dep file is removed. When running config finishes global typeorder_dep and order_dep_state files are removed. Global type order recording is untouched. Furthermore, for completeness, type order is now recorded for each object too. --- cdist/config.py | 4 ++ cdist/core/cdist_object.py | 10 +++ cdist/core/manifest.py | 14 ++++ cdist/emulator.py | 124 +++++++++++++++++++++++--------- cdist/test/emulator/__init__.py | 43 +++++++++-- 5 files changed, 157 insertions(+), 38 deletions(-) diff --git a/cdist/config.py b/cdist/config.py index 6c30db2e..97cc1da6 100644 --- a/cdist/config.py +++ b/cdist/config.py @@ -124,6 +124,7 @@ class Config(object): """Remove files and directories for the run""" if self.remove_remote_files_dirs: self._remove_remote_files_dirs() + self.manifest.cleanup() @staticmethod def hosts(source): @@ -787,6 +788,9 @@ class Config(object): self.explorer.run_type_explorers(cdist_object, transfer_type_explorers) try: self.manifest.run_type_manifest(cdist_object) + self.log.trace("[ORDER_DEP] Removing order dep files for %s", + cdist_object) + cdist_object.cleanup() cdist_object.state = core.CdistObject.STATE_PREPARED except cdist.Error as e: raise cdist.CdistObjectError(cdist_object, e) diff --git a/cdist/core/cdist_object.py b/cdist/core/cdist_object.py index 237f0ddd..114a47e0 100644 --- a/cdist/core/cdist_object.py +++ b/cdist/core/cdist_object.py @@ -243,6 +243,16 @@ class CdistObject(object): lambda obj: os.path.join(obj.base_path, obj.code_local_path)) code_remote = fsproperty.FileStringProperty( lambda obj: os.path.join(obj.base_path, obj.code_remote_path)) + typeorder = fsproperty.FileListProperty( + lambda obj: os.path.join(obj.absolute_path, 'typeorder')) + typeorder_dep = fsproperty.FileListProperty( + lambda obj: os.path.join(obj.absolute_path, 'typeorder_dep')) + + def cleanup(self): + try: + os.remove(os.path.join(self.absolute_path, 'typeorder_dep')) + except FileNotFoundError: + pass @property def exists(self): diff --git a/cdist/core/manifest.py b/cdist/core/manifest.py index 07af0ef8..8aeaf860 100644 --- a/cdist/core/manifest.py +++ b/cdist/core/manifest.py @@ -96,6 +96,10 @@ class Manifest(object): """Executes cdist manifests. """ + + ORDER_DEP_STATE_NAME = 'order_dep_state' + TYPEORDER_DEP_NAME = 'typeorder_dep' + def __init__(self, target_host, local, dry_run=False): self.target_host = target_host self.local = local @@ -212,3 +216,13 @@ class Manifest(object): type_manifest, env=self.env_type_manifest(cdist_object), message_prefix=message_prefix) + + def cleanup(self): + def _rm_file(fname): + try: + self.log.trace("[ORDER_DEP] Removing %s", fname) + os.remove(os.path.join(self.local.base_path, fname)) + except FileNotFoundError: + pass + _rm_file(Manifest.ORDER_DEP_STATE_NAME) + _rm_file(Manifest.TYPEORDER_DEP_NAME) diff --git a/cdist/emulator.py b/cdist/emulator.py index 417f2cdd..4800e2a3 100644 --- a/cdist/emulator.py +++ b/cdist/emulator.py @@ -29,6 +29,7 @@ import sys import cdist from cdist import core from cdist import flock +from cdist.core.manifest import Manifest class MissingRequiredEnvironmentVariableError(cdist.Error): @@ -82,6 +83,11 @@ class Emulator(object): self.object_base_path = os.path.join(self.global_path, "object") self.typeorder_path = os.path.join(self.global_path, "typeorder") + self.typeorder_dep_path = os.path.join(self.global_path, + Manifest.TYPEORDER_DEP_NAME) + self.order_dep_state_path = os.path.join(self.global_path, + Manifest.ORDER_DEP_STATE_NAME) + self.type_name = os.path.basename(argv[0]) self.cdist_type = core.CdistType(self.type_base_path, self.type_name) @@ -206,6 +212,14 @@ class Emulator(object): return params def setup_object(self): + # CDIST_ORDER_DEPENDENCY state + order_dep_on = self._order_dep_on() + order_dep_defined = "CDIST_ORDER_DEPENDENCY" in self.env + if not order_dep_defined and order_dep_on: + self._set_order_dep_state_off() + if order_dep_defined and not order_dep_on: + self._set_order_dep_state_on() + # Create object with given parameters self.parameters = {} for key, value in vars(self.args).items(): @@ -237,6 +251,20 @@ class Emulator(object): # record the created object in typeorder file with open(self.typeorder_path, 'a') as typeorderfile: print(self.cdist_object.name, file=typeorderfile) + # record the created object in parent object typeorder file + __object_name = self.env.get('__object_name', None) + depname = self.cdist_object.name + if __object_name: + parent = self.cdist_object.object_from_name(__object_name) + parent.typeorder.append(self.cdist_object.name) + if self._order_dep_on(): + self.log.trace(('[ORDER_DEP] Adding %s to typeorder dep' + ' for %s'), depname, parent.name) + parent.typeorder_dep.append(depname) + elif self._order_dep_on(): + self.log.trace('[ORDER_DEP] Adding %s to global typeorder dep', + depname) + self._add_typeorder_dep(depname) # Record / Append source self.cdist_object.source.append(self.object_source) @@ -293,45 +321,73 @@ class Emulator(object): return cdist_object.name + def _order_dep_on(self): + return os.path.exists(self.order_dep_state_path) + + def _set_order_dep_state_on(self): + self.log.trace('[ORDER_DEP] Setting order dep state on') + with open(self.order_dep_state_path, 'w'): + pass + + def _set_order_dep_state_off(self): + self.log.trace('[ORDER_DEP] Setting order dep state off') + # remove order dep state file + try: + os.remove(self.order_dep_state_path) + except FileNotFoundError: + pass + # remove typeorder dep file + try: + os.remove(self.typeorder_dep_path) + except FileNotFoundError: + pass + + def _add_typeorder_dep(self, name): + with open(self.typeorder_dep_path, 'a') as f: + print(name, file=f) + + def _read_typeorder_dep(self): + try: + with open(self.typeorder_dep_path, 'r') as f: + return f.readlines() + except FileNotFoundError: + return [] + def record_requirements(self): """Record requirements.""" + order_dep_on = self._order_dep_on() + # Inject the predecessor, but not if its an override # (this would leed to an circular dependency) - if ("CDIST_ORDER_DEPENDENCY" in self.env and - 'CDIST_OVERRIDE' not in self.env): - # load object name created befor this one from typeorder file ... - with open(self.typeorder_path, 'r') as typecreationfile: - typecreationorder = typecreationfile.readlines() - # get the type created before this one ... - try: - lastcreatedtype = typecreationorder[-2].strip() - # __object_name is the name of the object whose type - # manifest is currently executed - __object_name = self.env.get('__object_name', None) - if lastcreatedtype == __object_name: - self.log.debug(("Not injecting require for " - "CDIST_ORDER_DEPENDENCY: %s for %s," - " %s's type manifest is currently" - " being executed"), - lastcreatedtype, - self.cdist_object.name, - lastcreatedtype) - else: - if 'require' in self.env: - appendix = " " + lastcreatedtype - if appendix not in self.env['require']: - self.env['require'] += appendix - else: - self.env['require'] = lastcreatedtype - self.log.debug(("Injecting require for " - "CDIST_ORDER_DEPENDENCY: %s for %s"), - lastcreatedtype, - self.cdist_object.name) - except IndexError: - # if no second last line, we are on the first type, - # so do not set a requirement - pass + if (order_dep_on and 'CDIST_OVERRIDE' not in self.env): + try: + # __object_name is the name of the object whose type + # manifest is currently executed + __object_name = self.env.get('__object_name', None) + # load object name created befor this one from typeorder + # dep file + if __object_name: + parent = self.cdist_object.object_from_name( + __object_name) + typeorder = parent.typeorder_dep + else: + typeorder = self._read_typeorder_dep() + # get the type created before this one + lastcreatedtype = typeorder[-2].strip() + if 'require' in self.env: + if lastcreatedtype not in self.env['require']: + self.env['require'] += " " + lastcreatedtype + else: + self.env['require'] = lastcreatedtype + self.log.debug(("Injecting require for " + "CDIST_ORDER_DEPENDENCY: %s for %s"), + lastcreatedtype, + self.cdist_object.name) + except IndexError: + # if no second last line, we are on the first type, + # so do not set a requirement + pass reqs = set() if "require" in self.env: diff --git a/cdist/test/emulator/__init__.py b/cdist/test/emulator/__init__.py index 5691093c..e375676c 100644 --- a/cdist/test/emulator/__init__.py +++ b/cdist/test/emulator/__init__.py @@ -24,8 +24,6 @@ import io import os import shutil -import string -import filecmp import random import logging @@ -34,7 +32,6 @@ from cdist import test from cdist.exec import local from cdist import emulator from cdist import core -from cdist import config import os.path as op my_dir = op.abspath(op.dirname(__file__)) @@ -115,7 +112,7 @@ class EmulatorTestCase(test.CdistTestCase): def test_requirement_pattern(self): argv = ['__file', '/tmp/foobar'] self.env['require'] = '__file/etc/*' - emu = emulator.Emulator(argv, env=self.env) + emulator.Emulator(argv, env=self.env) # if we get here all is fine def test_loglevel(self): @@ -172,6 +169,44 @@ class EmulatorTestCase(test.CdistTestCase): self.assertEqual(list(file_object.requirements), ['__planet/mars']) # if we get here all is fine + def test_order_dependency_context(self): + test_seq = ('A', True, 'B', 'C', 'D', False, 'E', 'F', True, 'G', + 'H', False, 'I', ) + expected_requirements = { + 'C': set(('__planet/B', )), + 'D': set(('__planet/C', )), + 'H': set(('__planet/G', )), + } + # Ensure env var is not in env + if 'CDIST_ORDER_DEPENDENCY' in self.env: + del self.env['CDIST_ORDER_DEPENDENCY'] + + for x in test_seq: + if isinstance(x, str): + # Clear because of order dep injection + # In real world, this is not shared over instances + if 'require' in self.env: + del self.env['require'] + argv = ['__planet', x] + emu = emulator.Emulator(argv, env=self.env) + emu.run() + elif isinstance(x, bool): + if x: + self.env['CDIST_ORDER_DEPENDENCY'] = 'on' + elif 'CDIST_ORDER_DEPENDENCY' in self.env: + del self.env['CDIST_ORDER_DEPENDENCY'] + cdist_type = core.CdistType(self.local.type_path, '__planet') + for x in test_seq: + if isinstance(x, str): + obj = core.CdistObject(cdist_type, self.local.object_path, + self.local.object_marker_name, x) + reqs = set(obj.requirements) + if x in expected_requirements: + self.assertEqual(reqs, expected_requirements[x]) + else: + self.assertTrue(len(reqs) == 0) + # if we get here all is fine + class EmulatorConflictingRequirementsTestCase(test.CdistTestCase): From 332f5dcff9b27e110c60bc94aa9c2f3c40f44121 Mon Sep 17 00:00:00 2001 From: Darko Poljak Date: Sat, 23 Nov 2019 00:25:51 +0100 Subject: [PATCH 2/2] Redefine/reimplement CDIST_ORDER_DEPENDENCY Update documentation. --- docs/src/cdist-best-practice.rst | 91 +---------------------- docs/src/cdist-manifest.rst | 121 ++++++++++++++++++++++++++++++- docs/src/cdist-reference.rst.sh | 2 +- docs/src/man1/cdist.rst | 2 + 4 files changed, 126 insertions(+), 90 deletions(-) diff --git a/docs/src/cdist-best-practice.rst b/docs/src/cdist-best-practice.rst index a91f2cc0..39ec453e 100644 --- a/docs/src/cdist-best-practice.rst +++ b/docs/src/cdist-best-practice.rst @@ -226,8 +226,8 @@ and also to store all important files in one repository. -Perils of CDIST_ORDER_DEPENDENCY --------------------------------- +Notes on CDIST_ORDER_DEPENDENCY +------------------------------- With CDIST_ORDER_DEPENDENCY all types are executed in the order in which they are created in the manifest. The current created object automatically depends on the previously created object. @@ -235,96 +235,11 @@ on the previously created object. It essentially helps you to build up blocks of code that build upon each other (like first creating the directory xyz than the file below the directory). -This can be helpful, but it can also be the source of *evil*. - - -CDIST_ORDER_DEPENDENCY easily causes unobvious dependency cycles -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -Let's see an example. Suppose you have special init manifest where among other -things you are assuring that remote host has packages `sudo` and `curl` -installed. - -**init1** - -.. code-block:: sh - - CDIST_ORDER_DEPENDENCY=1 - export CDIST_ORDER_DEPENDENCY - - for p in sudo curl - do - __package "${p}" - done - -Then you have some other special init manifest where among other things you are -assuring `sudo` package is installed. - -**init2** - -.. code-block:: sh - - CDIST_ORDER_DEPENDENCY=1 - export CDIST_ORDER_DEPENDENCY - - __package sudo - -Then you have third init manifest where you combine those two init manifests, -by including them: - -**init** - -.. code-block:: sh - - sh -e "$__manifest/init1" - sh -e "$__manifest/init2" - -The resulting init manifest is then equal to: - -.. code-block:: sh - - CDIST_ORDER_DEPENDENCY=1 - export CDIST_ORDER_DEPENDENCY - - for p in sudo curl - do - __package "${p}" - done - - CDIST_ORDER_DEPENDENCY=1 - export CDIST_ORDER_DEPENDENCY - - __package sudo - -In the end you get the following dependencies: - -* `__package/curl` depends on `__package/sudo` -* `__package/sudo` depends on `__package/curl` - -And here you have a circular dependency! - -In the real world manifest can be quite complex, dependencies can become -complicated and circual dependencies are not so obvious. Resolving it can -become cumbersome. - -**Practical solution?** - -Instead of managing complex init manifests you can write custom types. -Each custom type can do one thing, it has well defined dependencies that will -not leak into init manifest. In custom type you can also add special explorers -and gencode. - -Then, in init manifest you combine your complex types. It is: - -* cleaner -* easier to follow -* easier to maintain -* easier to debug. +This can be helpful, but one must be aware of its side effects. CDIST_ORDER_DEPENDENCY kills parallelization ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - Suppose you have defined CDIST_ORDER_DEPENDENCY and then, among other things, you specify creation of three, by nature independent, files. diff --git a/docs/src/cdist-manifest.rst b/docs/src/cdist-manifest.rst index 4dd3e74b..5dbca479 100644 --- a/docs/src/cdist-manifest.rst +++ b/docs/src/cdist-manifest.rst @@ -163,7 +163,126 @@ automatically depends on the previously created object. It essentially helps you to build up blocks of code that build upon each other (like first creating the directory xyz than the file below the directory). -Read also about `perils of CDIST_ORDER_DEPENDENCY `_. +Read also about `notes on CDIST_ORDER_DEPENDENCY `_. + +In version 6.2.0 semantic CDIST_ORDER_DEPENDENCY is finally fixed and well defined. + +CDIST_ORDER_DEPENDENCY defines type order dependency context. Order dependency context +starts when CDIST_ORDER_DEPENDENCY is set, and ends when it is unset. After each +manifest execution finishes, any existing order dependency context is automatically +unset. This ensures that CDIST_ORDER_DEPENDENCY is valid within the manifest where it +is used. When order dependency context is defined then cdist executes types in the +order in which they are created in the manifest inside order dependency context. + +Sometimes the best way to see how something works is to see examples. + +Suppose you have defined **initial manifest**: + +.. code-block:: sh + + __cycle1 cycle1 + export CDIST_ORDER_DEPENDENCY=1 + __cycle2 cycle2 + __cycle3 cycle3 + +with types **__cycle1**: + +.. code-block:: sh + + export CDIST_ORDER_DEPENDENCY=1 + __file /tmp/cycle11 + __file /tmp/cycle12 + __file /tmp/cycle13 + +**__cycle2**: + +.. code-block:: sh + + __file /tmp/cycle21 + export CDIST_ORDER_DEPENDENCY=1 + __file /tmp/cycle22 + __file /tmp/cycle23 + unset CDIST_ORDER_DEPENDENCY + __file /tmp/cycle24 + +**__cycle3**: + +.. code-block:: sh + + __file /tmp/cycle31 + __file /tmp/cycle32 + export CDIST_ORDER_DEPENDENCY=1 + __file /tmp/cycle33 + __file /tmp/cycle34 + +For the above config, cdist results in the following expected *dependency graph* +(type *__cycleX* is shown as *cX*, *__file/tmp/cycleXY* is shown as *fcXY*): + +:: + + c1---->fc11 + | /\ + | | + +----->fc12 + | /\ + | | + +----->fc13 + + c2--+--->fc21 + /\ | + | | + | +----->fc22 + | | /\ + | | | + | +----->fc23 + | | + | | + | +----->fc24 + | + | + c3---->fc31 + | + | + +----->fc32 + | + | + +----->fc33 + | /\ + | | + +----->fc34 + +Before version 6.2.0 the above configuration would result in cycle: + +:: + + ERROR: 185.203.112.26: Cycle detected in object dependencies: + __file/tmp/cycle11 -> __cycle3/cycle3 -> __cycle2/cycle2 -> __cycle1/cycle1 -> __file/tmp/cycle11! + +The following manifest shows an example for order dependency contexts: + +.. code-block:: sh + + __file /tmp/fileA + export CDIST_ORDER_DEPENDENCY=1 + __file /tmp/fileB + __file /tmp/fileC + __file /tmp/fileD + unset CDIST_ORDER_DEPENDENCY + __file /tmp/fileE + __file /tmp/fileF + export CDIST_ORDER_DEPENDENCY=1 + __file /tmp/fileG + __file /tmp/fileH + unset CDIST_ORDER_DEPENDENCY + __file /tmp/fileI + +This means: + +* C depends on B +* D depends on C +* H depends on G + +and there are no other dependencies from this manifest. Overrides diff --git a/docs/src/cdist-reference.rst.sh b/docs/src/cdist-reference.rst.sh index 3ab12fe2..e77d98f6 100755 --- a/docs/src/cdist-reference.rst.sh +++ b/docs/src/cdist-reference.rst.sh @@ -330,7 +330,7 @@ CDIST_OVERRIDE CDIST_ORDER_DEPENDENCY Create dependencies based on the execution order (see \`cdist manifest \`_). - Read also about \`perils of CDIST_ORDER_DEPENDENCY \`_. + Note that in version 6.2.0 semantic of this processing mode is finally fixed and well defined. CDIST_REMOTE_EXEC Use this command for remote execution (should behave like ssh). diff --git a/docs/src/man1/cdist.rst b/docs/src/man1/cdist.rst index 7f368e68..f40491e9 100644 --- a/docs/src/man1/cdist.rst +++ b/docs/src/man1/cdist.rst @@ -827,6 +827,8 @@ CDIST_OVERRIDE CDIST_ORDER_DEPENDENCY Create dependencies based on the execution order. + Note that in version 6.2.0 semantic of this processing mode is + finally fixed and well defined. CDIST_REMOTE_EXEC Use this command for remote execution (should behave like ssh).