From d49af95d3c3665ebb86465862449292bf04aa03e Mon Sep 17 00:00:00 2001 From: Darko Poljak Date: Sat, 8 Oct 2016 11:40:32 +0200 Subject: [PATCH 1/3] Add warning message for faulty dependencies case. --- cdist/conf/type/__firewalld_start/man.rst | 2 +- cdist/emulator.py | 28 +++++++++- cdist/test/config/__init__.py | 16 ++++++ .../fixtures/manifest/init-deps-resolver | 8 +++ cdist/test/config/fixtures/type/__a/.keep | 0 cdist/test/config/fixtures/type/__b/.keep | 0 cdist/test/config/fixtures/type/__c/.keep | 0 cdist/test/config/fixtures/type/__d/.keep | 0 cdist/test/config/fixtures/type/__e/.keep | 0 cdist/test/config/fixtures/type/__f/.keep | 0 cdist/test/config/fixtures/type/__g/.keep | 0 cdist/test/config/fixtures/type/__g/manifest | 1 + cdist/test/config/fixtures/type/__h/.keep | 0 cdist/test/config/fixtures/type/__h/manifest | 3 ++ cdist/test/config/fixtures/type/__i/.keep | 0 cdist/test/config/fixtures/type/__j/.keep | 0 cdist/test/emulator/__init__.py | 54 +++++++++++++++++++ docs/src/man1/cdist.rst | 29 ++++++++++ 18 files changed, 138 insertions(+), 3 deletions(-) create mode 100644 cdist/test/config/fixtures/manifest/init-deps-resolver create mode 100644 cdist/test/config/fixtures/type/__a/.keep create mode 100644 cdist/test/config/fixtures/type/__b/.keep create mode 100644 cdist/test/config/fixtures/type/__c/.keep create mode 100644 cdist/test/config/fixtures/type/__d/.keep create mode 100644 cdist/test/config/fixtures/type/__e/.keep create mode 100644 cdist/test/config/fixtures/type/__f/.keep create mode 100644 cdist/test/config/fixtures/type/__g/.keep create mode 100644 cdist/test/config/fixtures/type/__g/manifest create mode 100644 cdist/test/config/fixtures/type/__h/.keep create mode 100644 cdist/test/config/fixtures/type/__h/manifest create mode 100644 cdist/test/config/fixtures/type/__i/.keep create mode 100644 cdist/test/config/fixtures/type/__j/.keep diff --git a/cdist/conf/type/__firewalld_start/man.rst b/cdist/conf/type/__firewalld_start/man.rst index 03232b72..74199cd6 100644 --- a/cdist/conf/type/__firewalld_start/man.rst +++ b/cdist/conf/type/__firewalld_start/man.rst @@ -1,5 +1,5 @@ cdist-type__firewalld_start(7) -============================= +============================== NAME ---- diff --git a/cdist/emulator.py b/cdist/emulator.py index b04ed130..6744de8b 100644 --- a/cdist/emulator.py +++ b/cdist/emulator.py @@ -84,6 +84,10 @@ class Emulator(object): self.type_name = os.path.basename(argv[0]) self.cdist_type = core.CdistType(self.type_base_path, self.type_name) + # If set then object alreay exists and this var holds existing + # requirements. + self._existing_reqs = None + self.__init_log() def run(self): @@ -166,6 +170,9 @@ class Emulator(object): self.parameters[key] = value if self.cdist_object.exists and 'CDIST_OVERRIDE' not in self.env: + # Make existing requirements a set so that we can compare it + # later with new requirements. + self._existing_reqs = set(self.cdist_object.requirements) if self.cdist_object.parameters != self.parameters: errmsg = ("Object %s already exists with conflicting " "parameters:\n%s: %s\n%s: %s" % ( @@ -243,7 +250,7 @@ class Emulator(object): return cdist_object.name def record_requirements(self): - """record requirements""" + """Record requirements.""" # Inject the predecessor, but not if its an override # (this would leed to an circular dependency) @@ -267,6 +274,7 @@ class Emulator(object): # so do not set a requirement pass + reqs = set() if "require" in self.env: requirements = self.env['require'] self.log.debug("reqs = " + requirements) @@ -274,7 +282,23 @@ class Emulator(object): # Ignore empty fields - probably the only field anyway if len(requirement) == 0: continue - self.record_requirement(requirement) + object_name = self.record_requirement(requirement) + reqs.add(object_name) + if self._existing_reqs is not None: + # If object exists then compare existing and new requirements. + if self._existing_reqs != reqs: + warnmsg = ("Object {} already exists with requirements:\n" + "{}: {}\n" + "{}: {}\n" + "Dependency resolver could not handle dependencies " + "as expected.".format( + self.cdist_object.name, + " ".join(self.cdist_object.source), + self._existing_reqs, + self.object_source, + reqs + )) + self.log.warning(warnmsg) def record_auto_requirements(self): """An object shall automatically depend on all objects that it diff --git a/cdist/test/config/__init__.py b/cdist/test/config/__init__.py index db753f41..af1aa38f 100644 --- a/cdist/test/config/__init__.py +++ b/cdist/test/config/__init__.py @@ -177,6 +177,22 @@ class ConfigRunTestCase(test.CdistTestCase): dryrun.run() # if we are here, dryrun works like expected + def test_desp_resolver(self): + """Test to show dependency resolver warning message.""" + local = cdist.exec.local.Local( + target_host=self.target_host, + base_root_path=self.host_base_path, + host_dir_name=self.hostdir, + exec_path=os.path.abspath(os.path.join( + my_dir, '../../../scripts/cdist')), + initial_manifest=os.path.join( + fixtures, 'manifest/init-deps-resolver'), + add_conf_dirs=[fixtures]) + + # dry_run is ok for dependency testing + config = cdist.config.Config(local, self.remote, dry_run=True) + config.run() + # 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 diff --git a/cdist/test/config/fixtures/manifest/init-deps-resolver b/cdist/test/config/fixtures/manifest/init-deps-resolver new file mode 100644 index 00000000..f67ab61c --- /dev/null +++ b/cdist/test/config/fixtures/manifest/init-deps-resolver @@ -0,0 +1,8 @@ +__a a +require="__e/e" __b b +require="__f/f" __c c +__e e +__f f +require="__c/c" __d d +__g g +__h h diff --git a/cdist/test/config/fixtures/type/__a/.keep b/cdist/test/config/fixtures/type/__a/.keep new file mode 100644 index 00000000..e69de29b diff --git a/cdist/test/config/fixtures/type/__b/.keep b/cdist/test/config/fixtures/type/__b/.keep new file mode 100644 index 00000000..e69de29b diff --git a/cdist/test/config/fixtures/type/__c/.keep b/cdist/test/config/fixtures/type/__c/.keep new file mode 100644 index 00000000..e69de29b diff --git a/cdist/test/config/fixtures/type/__d/.keep b/cdist/test/config/fixtures/type/__d/.keep new file mode 100644 index 00000000..e69de29b diff --git a/cdist/test/config/fixtures/type/__e/.keep b/cdist/test/config/fixtures/type/__e/.keep new file mode 100644 index 00000000..e69de29b diff --git a/cdist/test/config/fixtures/type/__f/.keep b/cdist/test/config/fixtures/type/__f/.keep new file mode 100644 index 00000000..e69de29b diff --git a/cdist/test/config/fixtures/type/__g/.keep b/cdist/test/config/fixtures/type/__g/.keep new file mode 100644 index 00000000..e69de29b diff --git a/cdist/test/config/fixtures/type/__g/manifest b/cdist/test/config/fixtures/type/__g/manifest new file mode 100644 index 00000000..107dbda4 --- /dev/null +++ b/cdist/test/config/fixtures/type/__g/manifest @@ -0,0 +1 @@ +require="__c/c __d/d" __a a diff --git a/cdist/test/config/fixtures/type/__h/.keep b/cdist/test/config/fixtures/type/__h/.keep new file mode 100644 index 00000000..e69de29b diff --git a/cdist/test/config/fixtures/type/__h/manifest b/cdist/test/config/fixtures/type/__h/manifest new file mode 100644 index 00000000..ce3d8fb7 --- /dev/null +++ b/cdist/test/config/fixtures/type/__h/manifest @@ -0,0 +1,3 @@ +# require="__b/b" __a a +require="__j/j" __i i +__j j diff --git a/cdist/test/config/fixtures/type/__i/.keep b/cdist/test/config/fixtures/type/__i/.keep new file mode 100644 index 00000000..e69de29b diff --git a/cdist/test/config/fixtures/type/__j/.keep b/cdist/test/config/fixtures/type/__j/.keep new file mode 100644 index 00000000..e69de29b diff --git a/cdist/test/emulator/__init__.py b/cdist/test/emulator/__init__.py index 4fd0ed40..51de3180 100644 --- a/cdist/test/emulator/__init__.py +++ b/cdist/test/emulator/__init__.py @@ -499,6 +499,60 @@ class StdinTestCase(test.CdistTestCase): self.assertEqual(random_string, stdin_saved_by_emulator) +class EmulatorAlreadyExistingRequirementsWarnTestCase(test.CdistTestCase): + + def setUp(self): + self.temp_dir = self.mkdtemp() + handle, self.script = self.mkstemp(dir=self.temp_dir) + os.close(handle) + base_path = self.temp_dir + hostdir = cdist.str_hash(self.target_host[0]) + host_base_path = os.path.join(base_path, hostdir) + + self.local = local.Local( + target_host=self.target_host, + base_root_path=host_base_path, + host_dir_name=hostdir, + exec_path=test.cdist_exec_path, + add_conf_dirs=[conf_dir]) + self.local.create_files_dirs() + + self.manifest = core.Manifest(self.target_host, self.local) + self.env = self.manifest.env_initial_manifest(self.script) + self.env['__cdist_object_marker'] = self.local.object_marker_name + + def tearDown(self): + shutil.rmtree(self.temp_dir) + + def test_object_existing_requirements_req_none(self): + """Test to show dependency resolver warning message.""" + argv = ['__directory', 'spam'] + emu = emulator.Emulator(argv, env=self.env) + emu.run() + argv = ['__file', 'eggs'] + self.env['require'] = '__directory/spam' + emu = emulator.Emulator(argv, env=self.env) + emu.run() + argv = ['__file', 'eggs'] + if 'require' in self.env: + del self.env['require'] + emu = emulator.Emulator(argv, env=self.env) + + def test_object_existing_requirements_none_req(self): + """Test to show dependency resolver warning message.""" + argv = ['__directory', 'spam'] + emu = emulator.Emulator(argv, env=self.env) + emu.run() + argv = ['__file', 'eggs'] + if 'require' in self.env: + del self.env['require'] + emu = emulator.Emulator(argv, env=self.env) + emu.run() + argv = ['__file', 'eggs'] + self.env['require'] = '__directory/spam' + emu = emulator.Emulator(argv, env=self.env) + + if __name__ == '__main__': import unittest unittest.main() diff --git a/docs/src/man1/cdist.rst b/docs/src/man1/cdist.rst index 52562e14..9cc28011 100644 --- a/docs/src/man1/cdist.rst +++ b/docs/src/man1/cdist.rst @@ -246,6 +246,35 @@ connection. In this case ssh will disable multiplexing. This limit is controlled with sshd :strong:`MaxSessions` configuration options. For more details refer to :strong:`sshd_config`\ (5). +When requirements for the same object are defined in different manifests (see +example below) in init manifest and in some other type manifest and they differs +then dependency resolver cannot detect dependencies right. This happends because +cdist cannot prepare all objects first and then run objects because some +object can depend on the result of type explorer(s) and explorers are executed +during object run. cdist will detect such case and write warning message. +Example for such a case: + +.. code-block:: sh + + init manifest: + __a a + require="__e/e" __b b + require="__f/f" __c c + __e e + __f f + require="__c/c" __d d + __g g + __h h + + type __g manifest: + require="__c/c __d/d" __a a + + Warning message: + .WARNING: cdisttesthost: Object __a/a already exists with requirements: + /usr/home/darko/ungleich/cdist/cdist/test/config/fixtures/manifest/init-deps-resolver /tmp/tmp.cdist.test.ozagkg54/local/759547ff4356de6e3d9e08522b0d0807/data/conf/type/__g/manifest: set() + /tmp/tmp.cdist.test.ozagkg54/local/759547ff4356de6e3d9e08522b0d0807/data/conf/type/__g/manifest: {'__c/c', '__d/d'} + Dependency resolver could not handle dependencies as expected. + COPYING ------- Copyright \(C) 2011-2013 Nico Schottelius. Free use of this software is From c4e89eb2457d79e0af3ae100367439be668124a3 Mon Sep 17 00:00:00 2001 From: Darko Poljak Date: Sat, 8 Oct 2016 11:44:55 +0200 Subject: [PATCH 2/3] Fix spelling. --- docs/src/man1/cdist.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/src/man1/cdist.rst b/docs/src/man1/cdist.rst index 9cc28011..5a30c321 100644 --- a/docs/src/man1/cdist.rst +++ b/docs/src/man1/cdist.rst @@ -248,7 +248,7 @@ options. For more details refer to :strong:`sshd_config`\ (5). When requirements for the same object are defined in different manifests (see example below) in init manifest and in some other type manifest and they differs -then dependency resolver cannot detect dependencies right. This happends because +then dependency resolver cannot detect dependencies right. This happens because cdist cannot prepare all objects first and then run objects because some object can depend on the result of type explorer(s) and explorers are executed during object run. cdist will detect such case and write warning message. From ddd8eab06f182819bb07bebe0d3ba24b3756f23c Mon Sep 17 00:00:00 2001 From: Darko Poljak Date: Sat, 8 Oct 2016 11:45:50 +0200 Subject: [PATCH 3/3] Remove extra dot. --- docs/src/man1/cdist.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/src/man1/cdist.rst b/docs/src/man1/cdist.rst index 5a30c321..c80b3386 100644 --- a/docs/src/man1/cdist.rst +++ b/docs/src/man1/cdist.rst @@ -270,7 +270,7 @@ Example for such a case: require="__c/c __d/d" __a a Warning message: - .WARNING: cdisttesthost: Object __a/a already exists with requirements: + WARNING: cdisttesthost: Object __a/a already exists with requirements: /usr/home/darko/ungleich/cdist/cdist/test/config/fixtures/manifest/init-deps-resolver /tmp/tmp.cdist.test.ozagkg54/local/759547ff4356de6e3d9e08522b0d0807/data/conf/type/__g/manifest: set() /tmp/tmp.cdist.test.ozagkg54/local/759547ff4356de6e3d9e08522b0d0807/data/conf/type/__g/manifest: {'__c/c', '__d/d'} Dependency resolver could not handle dependencies as expected.