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
(cherry picked from commit 38a46e8d21602e1ebed22c38ba53c0095c6be609)

4 files changed, 77 insertions, 22 deletions

diff --git a/heat/engine/resource.py b/heat/engine/resource.py
index 6b5a024d3..9d87e0935 100644
--- a/heat/engine/resource.py
+++ b/heat/engine/resource.py
@@ -547,11 +547,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' names which require the resource as dependency.
 
diff --git a/heat/engine/stack.py b/heat/engine/stack.py
index 744c99a83..6b313842b 100644
--- a/heat/engine/stack.py
+++ b/heat/engine/stack.py
@@ -303,7 +303,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):
@@ -375,14 +376,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/nova/test_server.py b/heat/tests/nova/test_server.py
index 74b20a521..a123e5a34 100644
--- a/heat/tests/nova/test_server.py
+++ b/heat/tests/nova/test_server.py
@@ -310,6 +310,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])
@@ -320,6 +321,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 67162180c..9fc24acd9 100644
--- a/heat/tests/test_resource.py
+++ b/heat/tests/test_resource.py
@@ -2205,12 +2205,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': {
@@ -2220,10 +2220,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):
@@ -2242,7 +2266,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)
@@ -2265,7 +2289,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)
@@ -2287,7 +2311,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)
@@ -2309,7 +2333,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)
@@ -2333,7 +2357,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)
@@ -2357,7 +2381,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)
@@ -2438,7 +2462,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)
@@ -2460,7 +2484,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)
@@ -2482,7 +2506,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)
@@ -2504,7 +2528,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)
@@ -2529,7 +2553,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)
@@ -2554,7 +2578,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)
@@ -2658,7 +2682,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)
@@ -2678,7 +2702,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)