From d49af95d3c3665ebb86465862449292bf04aa03e Mon Sep 17 00:00:00 2001 From: Darko Poljak Date: Sat, 8 Oct 2016 11:40:32 +0200 Subject: [PATCH] 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