From ced6f78aa065c1a7e6400c3be9ec3322e1e87416 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Mon, 28 Nov 2016 09:56:01 -0500 Subject: [PATCH] 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 | 10 ++-- heat/engine/stack.py | 55 ++++++++++--------- .../tests/engine/service/test_stack_create.py | 14 +++-- .../tests/engine/service/test_stack_update.py | 22 +++++--- heat/tests/engine/tools.py | 5 +- heat/tests/test_engine_service.py | 4 +- 6 files changed, 63 insertions(+), 47 deletions(-) diff --git a/heat/engine/service.py b/heat/engine/service.py index f0ad0c44ee..1936e3f685 100644 --- a/heat/engine/service.py +++ b/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 diff --git a/heat/engine/stack.py b/heat/engine/stack.py index 3fa35cd125..87f3f91d68 100644 --- a/heat/engine/stack.py +++ b/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: diff --git a/heat/tests/engine/service/test_stack_create.py b/heat/tests/engine/service/test_stack_create.py index 0c2ef39fc5..431564c7db 100644 --- a/heat/tests/engine/service/test_stack_create.py +++ b/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' diff --git a/heat/tests/engine/service/test_stack_update.py b/heat/tests/engine/service/test_stack_update.py index fe09ddf04f..fde8df368f 100644 --- a/heat/tests/engine/service/test_stack_update.py +++ b/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, diff --git a/heat/tests/engine/tools.py b/heat/tests/engine/tools.py index d3288f9c25..d62df271de 100644 --- a/heat/tests/engine/tools.py +++ b/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 diff --git a/heat/tests/test_engine_service.py b/heat/tests/test_engine_service.py index 1b8a9fa09c..ee00d99462 100644 --- a/heat/tests/test_engine_service.py +++ b/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()