From 38a46e8d21602e1ebed22c38ba53c0095c6be609 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Tue, 8 Mar 2016 11:35:12 -0500 Subject: [PATCH] Catch exceptions calculating implicit dependencies Uncaught exceptions in an overridden add_dependencies() method of a plugin can prevent a stack being loaded from the database. To protect against programming errors that can result in uncaught exceptions, separate the calculation of implicit dependencies out from the calculation of explicit dependencies, and ignore exceptions in the latter when dealing with an existing stack. Change-Id: I939dba57eeba5710bb77a1b30a872fca5d38ad71 Closes-Bug: #1554625 --- heat/engine/resource.py | 22 ++++++++- heat/engine/stack.py | 17 +++++-- heat/tests/openstack/nova/test_server.py | 2 + heat/tests/test_resource.py | 58 +++++++++++++++++------- 4 files changed, 77 insertions(+), 22 deletions(-) diff --git a/heat/engine/resource.py b/heat/engine/resource.py index a2a4d09cdb..7d11d6531f 100644 --- a/heat/engine/resource.py +++ b/heat/engine/resource.py @@ -560,11 +560,31 @@ class Resource(object): def dep_attrs(self, resource_name): return self.t.dep_attrs(resource_name) - def add_dependencies(self, deps): + def add_explicit_dependencies(self, deps): + """Add all dependencies explicitly specified in the template. + + The deps parameter is a Dependencies object to which dependency pairs + are added. + """ for dep in self.t.dependencies(self.stack): deps += (self, dep) deps += (self, None) + def add_dependencies(self, deps): + """Add implicit dependencies specific to the resource type. + + Some resource types may have implicit dependencies on other resources + in the same stack that are not linked by a property value (that would + be set using get_resource or get_attr for example, thus creating an + explicit dependency). Such dependencies are opaque to the user and + should be avoided wherever possible, however in some circumstances they + are required due to magic in the underlying API. + + The deps parameter is a Dependencies object to which dependency pairs + may be added. + """ + return + def required_by(self): """List of resources that require this one as a dependency. diff --git a/heat/engine/stack.py b/heat/engine/stack.py index 241e97adef..7d3cd05268 100644 --- a/heat/engine/stack.py +++ b/heat/engine/stack.py @@ -340,7 +340,8 @@ class Stack(collections.Mapping): def dependencies(self): if self._dependencies is None: self._dependencies = self._get_dependencies( - six.itervalues(self.resources)) + six.itervalues(self.resources), + ignore_errors=self.id is not None) return self._dependencies def reset_dependencies(self): @@ -412,14 +413,22 @@ class Stack(collections.Mapping): return set(itertools.chain.from_iterable(attr_lists)) @staticmethod - def _get_dependencies(resources): + def _get_dependencies(resources, ignore_errors=True): """Return the dependency graph for a list of resources.""" deps = dependencies.Dependencies() for res in resources: + res.add_explicit_dependencies(deps) + try: res.add_dependencies(deps) - except ValueError: - pass + except Exception as exc: + if not ignore_errors: + raise + else: + LOG.warning(_LW('Ignoring error adding implicit ' + 'dependencies for %(res)s: %(err)s') % + {'res': six.text_type(res), + 'err': six.text_type(exc)}) return deps diff --git a/heat/tests/openstack/nova/test_server.py b/heat/tests/openstack/nova/test_server.py index d11a8bffb4..56688e0aaf 100644 --- a/heat/tests/openstack/nova/test_server.py +++ b/heat/tests/openstack/nova/test_server.py @@ -326,6 +326,7 @@ class ServersTest(common.HeatTestCase): server_rsrc = stack['server'] subnet_rsrc = stack['subnet'] deps = [] + server_rsrc.add_explicit_dependencies(deps) server_rsrc.add_dependencies(deps) self.assertEqual(4, len(deps)) self.assertEqual(subnet_rsrc, deps[3]) @@ -339,6 +340,7 @@ class ServersTest(common.HeatTestCase): server_rsrc = stack['server'] subnet_rsrc = stack['subnet'] deps = [] + server_rsrc.add_explicit_dependencies(deps) server_rsrc.add_dependencies(deps) self.assertEqual(2, len(deps)) self.assertNotIn(subnet_rsrc, deps) diff --git a/heat/tests/test_resource.py b/heat/tests/test_resource.py index 1d03d97b7d..1793db8476 100644 --- a/heat/tests/test_resource.py +++ b/heat/tests/test_resource.py @@ -2289,12 +2289,12 @@ class ResourceDependenciesTest(common.HeatTestCase): stack = parser.Stack(utils.dummy_context(), 'test', tmpl) res = stack['foo'] - res.add_dependencies(self.deps) + res.add_explicit_dependencies(self.deps) graph = self.deps.graph() self.assertIn(res, graph) - def test_hot_add_dep_error(self): + def test_hot_add_dep_error_create(self): tmpl = template.Template({ 'heat_template_version': '2013-05-23', 'resources': { @@ -2304,10 +2304,34 @@ class ResourceDependenciesTest(common.HeatTestCase): }) stack = parser.Stack(utils.dummy_context(), 'test', tmpl) res = stack['bar'] + + class TestException(Exception): + pass + + self.patchobject(res, 'add_dependencies', + side_effect=TestException) + + def get_dependencies(): + return stack.dependencies + + self.assertRaises(TestException, get_dependencies) + + def test_hot_add_dep_error_load(self): + tmpl = template.Template({ + 'heat_template_version': '2013-05-23', + 'resources': { + 'foo': {'type': 'GenericResourceType'}, + 'bar': {'type': 'ResourceWithPropsType'} + } + }) + stack = parser.Stack(utils.dummy_context(), 'test_hot_add_dep_err', + tmpl) + stack.store() + res = stack['bar'] self.patchobject(res, 'add_dependencies', side_effect=ValueError) graph = stack.dependencies.graph() - self.assertNotIn(res, graph) + self.assertIn(res, graph) self.assertIn(stack['foo'], graph) def test_ref(self): @@ -2326,7 +2350,7 @@ class ResourceDependenciesTest(common.HeatTestCase): stack = parser.Stack(utils.dummy_context(), 'test', tmpl) res = stack['bar'] - res.add_dependencies(self.deps) + res.add_explicit_dependencies(self.deps) graph = self.deps.graph() self.assertIn(res, graph) @@ -2349,7 +2373,7 @@ class ResourceDependenciesTest(common.HeatTestCase): stack = parser.Stack(utils.dummy_context(), 'test', tmpl) res = stack['bar'] - res.add_dependencies(self.deps) + res.add_explicit_dependencies(self.deps) graph = self.deps.graph() self.assertIn(res, graph) @@ -2371,7 +2395,7 @@ class ResourceDependenciesTest(common.HeatTestCase): stack = parser.Stack(utils.dummy_context(), 'test', tmpl) res = stack['bar'] - res.add_dependencies(self.deps) + res.add_explicit_dependencies(self.deps) graph = self.deps.graph() self.assertIn(res, graph) @@ -2393,7 +2417,7 @@ class ResourceDependenciesTest(common.HeatTestCase): stack = parser.Stack(utils.dummy_context(), 'test', tmpl) res = stack['bar'] - res.add_dependencies(self.deps) + res.add_explicit_dependencies(self.deps) graph = self.deps.graph() self.assertIn(res, graph) @@ -2417,7 +2441,7 @@ class ResourceDependenciesTest(common.HeatTestCase): stack = parser.Stack(utils.dummy_context(), 'test', tmpl) res = stack['bar'] - res.add_dependencies(self.deps) + res.add_explicit_dependencies(self.deps) graph = self.deps.graph() self.assertIn(res, graph) @@ -2441,7 +2465,7 @@ class ResourceDependenciesTest(common.HeatTestCase): stack = parser.Stack(utils.dummy_context(), 'test', tmpl) res = stack['bar'] - res.add_dependencies(self.deps) + res.add_explicit_dependencies(self.deps) graph = self.deps.graph() self.assertIn(res, graph) @@ -2522,7 +2546,7 @@ class ResourceDependenciesTest(common.HeatTestCase): stack = parser.Stack(utils.dummy_context(), 'test', tmpl) res = stack['bar'] - res.add_dependencies(self.deps) + res.add_explicit_dependencies(self.deps) graph = self.deps.graph() self.assertIn(res, graph) @@ -2544,7 +2568,7 @@ class ResourceDependenciesTest(common.HeatTestCase): stack = parser.Stack(utils.dummy_context(), 'test', tmpl) res = stack['bar'] - res.add_dependencies(self.deps) + res.add_explicit_dependencies(self.deps) graph = self.deps.graph() self.assertIn(res, graph) @@ -2566,7 +2590,7 @@ class ResourceDependenciesTest(common.HeatTestCase): stack = parser.Stack(utils.dummy_context(), 'test', tmpl) res = stack['bar'] - res.add_dependencies(self.deps) + res.add_explicit_dependencies(self.deps) graph = self.deps.graph() self.assertIn(res, graph) @@ -2588,7 +2612,7 @@ class ResourceDependenciesTest(common.HeatTestCase): stack = parser.Stack(utils.dummy_context(), 'test', tmpl) res = stack['bar'] - res.add_dependencies(self.deps) + res.add_explicit_dependencies(self.deps) graph = self.deps.graph() self.assertIn(res, graph) @@ -2613,7 +2637,7 @@ class ResourceDependenciesTest(common.HeatTestCase): stack = parser.Stack(utils.dummy_context(), 'test', tmpl) res = stack['bar'] - res.add_dependencies(self.deps) + res.add_explicit_dependencies(self.deps) graph = self.deps.graph() self.assertIn(res, graph) @@ -2638,7 +2662,7 @@ class ResourceDependenciesTest(common.HeatTestCase): stack = parser.Stack(utils.dummy_context(), 'test', tmpl) res = stack['bar'] - res.add_dependencies(self.deps) + res.add_explicit_dependencies(self.deps) graph = self.deps.graph() self.assertIn(res, graph) @@ -2742,7 +2766,7 @@ class ResourceDependenciesTest(common.HeatTestCase): stack = parser.Stack(utils.dummy_context(), 'test', tmpl) res = stack['bar'] - res.add_dependencies(self.deps) + res.add_explicit_dependencies(self.deps) graph = self.deps.graph() self.assertIn(res, graph) @@ -2762,7 +2786,7 @@ class ResourceDependenciesTest(common.HeatTestCase): stack = parser.Stack(utils.dummy_context(), 'test', tmpl) res = stack['bar'] - res.add_dependencies(self.deps) + res.add_explicit_dependencies(self.deps) graph = self.deps.graph() self.assertIn(res, graph)