Avoid re-validating resources in nested stacks

Before creating a nested stack we validate the resources in the parent
stack. Therefore it's wasteful to validate them again upon creating the
stack - prior to this patch the number or validation calls for each
resource would be equal to the depth of the stack in the tree (so twice for
a child of the root, and three times for a grandchild). With this patch,
each resource in a nested stack is validated only once, by the parent
resource.

ResourceGroup and its subclasses are currently the only StackResources that
do not validate all of their members; they validates a stack containing a
single representative resource. This means that in the case of e.g. index
substitution, the nested stack could be created without having validated
some resources. However, in practice this is unlikely to lead to validity
problems not causing a failure until later than they previously would have.

Change-Id: Iaa36ae5543b3ce30ae8df5a05b48fe987bc0ffdc
Closes-Bug: #1645336

heat/engine/service.py

@@ -709,9 +709,10 @@ class EngineService(service.ServiceBase):
 
         self.resource_enforcer.enforce_stack(stack)
         self._validate_deferred_auth_context(cnxt, stack)
-        stack.validate()
-        # For the root stack print a summary of the TemplateResources loaded
-        if nested_depth == 0:
+        is_root = stack.nested_depth == 0
+        stack.validate(validate_resources=is_root)
+        # For the root stack, log a summary of the TemplateResources loaded
+        if is_root:
             tmpl.env.registry.log_resource_info(prefix=stack_name)
         return stack
 
@@ -939,7 +940,8 @@ class EngineService(service.ServiceBase):
         updated_stack.parameters.set_stack_id(current_stack.identifier())
 
         self._validate_deferred_auth_context(cnxt, updated_stack)
-        updated_stack.validate()
+        is_root = current_stack.nested_depth == 0
+        updated_stack.validate(validate_resources=is_root)
 
         return tmpl, current_stack, updated_stack
 

heat/engine/stack.py

@@ -781,7 +781,8 @@ class Stack(collections.Mapping):
         return handler and handler(resource_name)
 
     @profiler.trace('Stack.validate', hide_args=False)
-    def validate(self, ignorable_errors=None, validate_by_deps=True):
+    def validate(self, ignorable_errors=None, validate_by_deps=True,
+                 validate_resources=True):
         """Validates the stack."""
         # TODO(sdake) Should return line number of invalid reference
 
@@ -828,33 +829,37 @@ class Stack(collections.Mapping):
         else:
             iter_rsc = six.itervalues(resources)
 
-        unique_definitions = set(res.t for res in six.itervalues(resources))
-        unique_defn_names = set(defn.name for defn in unique_definitions)
+        # Validate resources only for top-level stacks. Nested stacks have
+        # already had their resources validated by their parent.
+        if validate_resources:
+            unique_defns = set(res.t for res in six.itervalues(resources))
+            unique_defn_names = set(defn.name for defn in unique_defns)
 
-        for res in iter_rsc:
-            # Don't validate identical definitions multiple times
-            if res.name not in unique_defn_names:
-                continue
+            for res in iter_rsc:
+                # Don't validate identical definitions multiple times
+                if res.name not in unique_defn_names:
+                    continue
 
-            try:
-                if self.resource_validate:
-                    result = res.validate()
-                else:
-                    result = res.validate_template()
-            except exception.HeatException as ex:
-                LOG.debug('%s', ex)
-                if ignorable_errors and ex.error_code in ignorable_errors:
-                    result = None
-                else:
+                try:
+                    if self.resource_validate:
+                        result = res.validate()
+                    else:
+                        result = res.validate_template()
+                except exception.HeatException as ex:
+                    LOG.debug('%s', ex)
+                    if ignorable_errors and ex.error_code in ignorable_errors:
+                        result = None
+                    else:
+                        raise
+                except AssertionError:
                     raise
-            except AssertionError:
-                raise
-            except Exception as ex:
-                LOG.info(_LI("Exception in stack validation"), exc_info=True)
-                raise exception.StackValidationFailed(
-                    message=encodeutils.safe_decode(six.text_type(ex)))
-            if result:
-                raise exception.StackValidationFailed(message=result)
+                except Exception as ex:
+                    LOG.info(_LI("Exception in stack validation"),
+                             exc_info=True)
+                    raise exception.StackValidationFailed(
+                        message=encodeutils.safe_decode(six.text_type(ex)))
+                if result:
+                    raise exception.StackValidationFailed(message=result)
 
         for op_name, output in six.iteritems(self.outputs):
             try:

heat/tests/engine/service/test_stack_create.py

@@ -73,7 +73,7 @@ class StackCreateTest(common.HeatTestCase):
         if environment_files:
             mock_merge.assert_called_once_with(environment_files, None,
                                                params, mock.ANY)
-        mock_validate.assert_called_once_with()
+        mock_validate.assert_called_once_with(validate_resources=True)
 
     def test_stack_create(self):
         stack_name = 'service_create_test_stack'
@@ -277,16 +277,20 @@ class StackCreateTest(common.HeatTestCase):
     def test_stack_create_nested(self, mock_validate, mock_tg):
         convergence_engine = cfg.CONF.convergence_engine
         stack_name = 'service_create_nested_test_stack'
+        parent_stack = tools.get_stack(stack_name + '_parent', self.ctx)
+        owner_id = parent_stack.store()
         mock_tg.return_value = tools.DummyThreadGroup()
 
-        stk = tools.get_stack(stack_name, self.ctx, with_params=True)
+        stk = tools.get_stack(stack_name, self.ctx, with_params=True,
+                              owner_id=owner_id, nested_depth=1)
         tmpl_id = stk.t.store(self.ctx)
 
         mock_load = self.patchobject(templatem.Template, 'load',
                                      return_value=stk.t)
         mock_stack = self.patchobject(stack, 'Stack', return_value=stk)
         result = self.man.create_stack(self.ctx, stack_name, None,
-                                       None, None, {}, nested_depth=1,
+                                       None, None, {},
+                                       owner_id=owner_id, nested_depth=1,
                                        template_id=tmpl_id)
         self.assertEqual(stk.identifier(), result)
         self.assertIsInstance(result, dict)
@@ -294,13 +298,13 @@ class StackCreateTest(common.HeatTestCase):
 
         mock_load.assert_called_once_with(self.ctx, tmpl_id)
         mock_stack.assert_called_once_with(self.ctx, stack_name, stk.t,
-                                           owner_id=None, nested_depth=1,
+                                           owner_id=owner_id, nested_depth=1,
                                            user_creds_id=None,
                                            stack_user_project_id=None,
                                            convergence=convergence_engine,
                                            parent_resource=None)
 
-        mock_validate.assert_called_once_with()
+        mock_validate.assert_called_once_with(validate_resources=False)
 
     def test_stack_validate(self):
         stack_name = 'stack_create_test_validate'

heat/tests/engine/service/test_stack_update.py

@@ -99,7 +99,7 @@ class ServiceStackUpdateTest(common.HeatTestCase):
             user_creds_id=u'1',
             username='test_username')
         mock_load.assert_called_once_with(self.ctx, stack=s)
-        mock_validate.assert_called_once_with()
+        mock_validate.assert_called_once_with(validate_resources=True)
 
     def test_stack_update_with_environment_files(self):
         # Setup
@@ -136,7 +136,11 @@ class ServiceStackUpdateTest(common.HeatTestCase):
 
     def test_stack_update_nested(self):
         stack_name = 'service_update_nested_test_stack'
-        old_stack = tools.get_stack(stack_name, self.ctx)
+        parent_stack = tools.get_stack(stack_name + '_parent', self.ctx)
+        owner_id = parent_stack.store()
+        old_stack = tools.get_stack(stack_name, self.ctx,
+                                    owner_id=owner_id, nested_depth=1,
+                                    user_creds_id=parent_stack.user_creds_id)
         sid = old_stack.store()
         old_stack.set_stack_user_project_id('1234')
         s = stack_object.Stack.get_by_id(self.ctx, sid)
@@ -174,8 +178,8 @@ class ServiceStackUpdateTest(common.HeatTestCase):
             prev_raw_template_id=None,
             current_deps=None,
             disable_rollback=True,
-            nested_depth=0,
-            owner_id=None,
+            nested_depth=1,
+            owner_id=owner_id,
             parent_resource=None,
             stack_user_project_id='1234',
             strict_validate=True,
@@ -184,7 +188,7 @@ class ServiceStackUpdateTest(common.HeatTestCase):
             user_creds_id=u'1',
             username='test_username')
         mock_load.assert_called_once_with(self.ctx, stack=s)
-        mock_validate.assert_called_once_with()
+        mock_validate.assert_called_once_with(validate_resources=False)
 
     def test_stack_update_existing_parameters(self):
         # Use a template with existing parameters, then update the stack
@@ -458,7 +462,7 @@ resources:
         self.assertIsInstance(result, dict)
         self.assertTrue(result['stack_id'])
 
-        mock_validate.assert_called_once_with()
+        mock_validate.assert_called_once_with(validate_resources=True)
         mock_tmpl.assert_called_once_with(template, files=None)
         mock_env.assert_called_once_with(params)
         mock_load.assert_called_once_with(self.ctx, stack=s)
@@ -606,7 +610,7 @@ resources:
             timeout_mins=60, user_creds_id=u'1',
             username='test_username')
         mock_load.assert_called_once_with(self.ctx, stack=s)
-        mock_validate.assert_called_once_with()
+        mock_validate.assert_called_once_with(validate_resources=True)
 
     def test_stack_update_stack_id_equal(self):
         stack_name = 'test_stack_update_stack_id_equal'
@@ -722,7 +726,7 @@ resources:
             timeout_mins=60, user_creds_id=u'1',
             username='test_username')
         mock_load.assert_called_once_with(self.ctx, stack=s)
-        mock_validate.assert_called_once_with()
+        mock_validate.assert_called_once_with(validate_resources=True)
 
     def test_stack_update_nonexist(self):
         stack_name = 'service_update_nonexist_test_stack'
@@ -1039,7 +1043,7 @@ resources:
         mock_load.assert_called_once_with(self.ctx, stack=s)
         mock_tmpl.assert_called_once_with(new_template, files=None)
         mock_env.assert_called_once_with(params)
-        mock_validate.assert_called_once_with()
+        mock_validate.assert_called_once_with(validate_resources=True)
 
         if environment_files:
             mock_merge.assert_called_once_with(environment_files, None,

heat/tests/engine/tools.py

@@ -151,7 +151,7 @@ resources:
 
 
 def get_stack(stack_name, ctx, template=None, with_params=True,
-              convergence=False):
+              convergence=False, **kwargs):
     if template is None:
         t = template_format.parse(wp_template)
         if with_params:
@@ -162,7 +162,8 @@ def get_stack(stack_name, ctx, template=None, with_params=True,
     else:
         t = template_format.parse(template)
         tmpl = templatem.Template(t)
-    stack = parser.Stack(ctx, stack_name, tmpl, convergence=convergence)
+    stack = parser.Stack(ctx, stack_name, tmpl, convergence=convergence,
+                         **kwargs)
     return stack
 
 

heat/tests/test_engine_service.py

@@ -245,7 +245,7 @@ class StackConvergenceServiceCreateUpdateTest(common.HeatTestCase):
                      convergence=True).AndReturn(stack)
 
         self.m.StubOutWithMock(stack, 'validate')
-        stack.validate().AndReturn(None)
+        stack.validate(validate_resources=True).AndReturn(None)
 
         self.m.ReplayAll()
         api_args = {'timeout_mins': 60, 'disable_rollback': False}
@@ -297,7 +297,7 @@ class StackConvergenceServiceCreateUpdateTest(common.HeatTestCase):
                      current_deps=old_stack.current_deps).AndReturn(stack)
 
         self.m.StubOutWithMock(stack, 'validate')
-        stack.validate().AndReturn(None)
+        stack.validate(validate_resources=True).AndReturn(None)
 
         self.m.ReplayAll()