From 7f2a94fb0ecadad3e61653b08d2b5a737ddcabcd Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Fri, 31 Mar 2017 15:57:54 -0400 Subject: Validate property values in nested stacks again In ced6f78aa065c1a7e6400c3be9ec3322e1e87416 we stopped doing validations of nested stacks at stack creation time, on the assumption that they had been validated when the parent stack was created. This assumption was incorrect; for children of the stack being created, the strict_validate global is always set during validation, so property values of resources will not be validated until it comes time to create the resource. Instead, prevent only redundant non-strict validations of stacks at nested depth 2 and greater. This means that every stack other than the root will be validated exactly twice - once without validating property values when the root is created, and again including property validation when the nested stack itself is created. Most of the performance benefits should remain; in the case of a large ResourceGroup using index substitution, we will now have to validate a lot of nearly-identical resource properties, however we still will not load into memory and validate a nested stack for each one as we originally did. Since that happens synchronously, it was likely the main contributor to RPC timeouts when dealing with large scaling groups. (During the validation at the creation of the root stack, only a single member of a ResourceGroup is validated even when index substitution is used. For scaling groups with identical members, only one member is validated since 3aebdabf2e78ac9e920b9dd8c748c4fad0d723c3.) This change reverts commit ced6f78aa065c1a7e6400c3be9ec3322e1e87416. Conflicts: heat/engine/stack.py Change-Id: I97cf789cee75931edef58b78c88f02da204d2a08 Closes-Bug: #1675589 Related-Bug: #1645336 (cherry picked from commit ea2673fb9a04588e0e294d159945d075657b9112) --- heat/engine/resources/stack_resource.py | 6 ++- heat/engine/service.py | 5 +-- heat/engine/stack.py | 62 ++++++++++++-------------- heat/tests/engine/service/test_stack_create.py | 4 +- heat/tests/engine/service/test_stack_update.py | 12 ++--- heat/tests/test_engine_service.py | 4 +- 6 files changed, 46 insertions(+), 47 deletions(-) diff --git a/heat/engine/resources/stack_resource.py b/heat/engine/resources/stack_resource.py index 9b27f1fe1..0af8ed20c 100644 --- a/heat/engine/resources/stack_resource.py +++ b/heat/engine/resources/stack_resource.py @@ -61,7 +61,11 @@ class StackResource(resource.Resource): def validate(self): super(StackResource, self).validate() - self.validate_nested_stack() + # Don't redo a non-strict validation of a nested stack during the + # creation of a child stack; only validate a child stack prior to the + # creation of the root stack. + if self.stack.nested_depth == 0 or not self.stack.strict_validate: + self.validate_nested_stack() def validate_nested_stack(self): try: diff --git a/heat/engine/service.py b/heat/engine/service.py index ab47c3777..13c426afc 100644 --- a/heat/engine/service.py +++ b/heat/engine/service.py @@ -710,7 +710,7 @@ class EngineService(service.ServiceBase): self.resource_enforcer.enforce_stack(stack) self._validate_deferred_auth_context(cnxt, stack) is_root = stack.nested_depth == 0 - stack.validate(validate_resources=is_root) + stack.validate() # For the root stack, log a summary of the TemplateResources loaded if is_root: tmpl.env.registry.log_resource_info(prefix=stack_name) @@ -940,8 +940,7 @@ class EngineService(service.ServiceBase): updated_stack.parameters.set_stack_id(current_stack.identifier()) self._validate_deferred_auth_context(cnxt, updated_stack) - is_root = current_stack.nested_depth == 0 - updated_stack.validate(validate_resources=is_root) + updated_stack.validate() return tmpl, current_stack, updated_stack diff --git a/heat/engine/stack.py b/heat/engine/stack.py index c7c4c7e8f..f9f51c250 100644 --- a/heat/engine/stack.py +++ b/heat/engine/stack.py @@ -781,8 +781,7 @@ 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, - validate_resources=True): + def validate(self, ignorable_errors=None, validate_by_deps=True): """Validates the stack.""" # TODO(sdake) Should return line number of invalid reference @@ -829,39 +828,36 @@ class Stack(collections.Mapping): else: iter_rsc = six.itervalues(resources) - # 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) + 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 - try: - if self.resource_validate: - if res.external_id is not None: - res.validate_external() - continue - result = res.validate() - elif res.external_id is None: - 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: + 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: + if res.external_id is not None: + res.validate_external() + continue + result = res.validate() + elif res.external_id is None: + 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 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 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) 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 431564c7d..a8e6b6c1e 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(validate_resources=True) + mock_validate.assert_called_once_with() def test_stack_create(self): stack_name = 'service_create_test_stack' @@ -304,7 +304,7 @@ class StackCreateTest(common.HeatTestCase): convergence=convergence_engine, parent_resource=None) - mock_validate.assert_called_once_with(validate_resources=False) + mock_validate.assert_called_once_with() 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 c54f149ff..ff63b48b8 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(validate_resources=True) + mock_validate.assert_called_once_with() def test_stack_update_with_environment_files(self): # Setup @@ -188,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(validate_resources=False) + mock_validate.assert_called_once_with() def test_stack_update_existing_parameters(self): # Use a template with existing parameters, then update the stack @@ -462,7 +462,7 @@ resources: self.assertIsInstance(result, dict) self.assertTrue(result['stack_id']) - mock_validate.assert_called_once_with(validate_resources=True) + mock_validate.assert_called_once_with() 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) @@ -610,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(validate_resources=True) + mock_validate.assert_called_once_with() def test_stack_update_stack_id_equal(self): stack_name = 'test_stack_update_stack_id_equal' @@ -726,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(validate_resources=True) + mock_validate.assert_called_once_with() def test_stack_update_nonexist(self): stack_name = 'service_update_nonexist_test_stack' @@ -1043,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(validate_resources=True) + mock_validate.assert_called_once_with() if environment_files: mock_merge.assert_called_once_with(environment_files, None, diff --git a/heat/tests/test_engine_service.py b/heat/tests/test_engine_service.py index d4789e7e7..a484e7921 100644 --- a/heat/tests/test_engine_service.py +++ b/heat/tests/test_engine_service.py @@ -246,7 +246,7 @@ class StackConvergenceServiceCreateUpdateTest(common.HeatTestCase): convergence=True).AndReturn(stack) self.m.StubOutWithMock(stack, 'validate') - stack.validate(validate_resources=True).AndReturn(None) + stack.validate().AndReturn(None) self.m.ReplayAll() api_args = {'timeout_mins': 60, 'disable_rollback': False} @@ -298,7 +298,7 @@ class StackConvergenceServiceCreateUpdateTest(common.HeatTestCase): current_deps=old_stack.current_deps).AndReturn(stack) self.m.StubOutWithMock(stack, 'validate') - stack.validate(validate_resources=True).AndReturn(None) + stack.validate().AndReturn(None) self.m.ReplayAll() -- cgit v1.2.1