From 52159197d895b65ff3720f5ee09f6b22a8a1528f Mon Sep 17 00:00:00 2001 From: Adriano Petrich Date: Wed, 11 Oct 2017 14:59:20 +0100 Subject: [PATCH] Validate parameters before updating When updating parameters we save the old env try updating it and test if that works and validates. If it doesn't the old env is restored. The update params also now returns the flattened params saving the ui from having to make another call after the update. Change-Id: I9aa18c4152ff9bf896729ace0d9481af50fd7802 Closes-Bug: #1638598 --- tripleo_common/actions/parameters.py | 152 +++++++---- tripleo_common/actions/templates.py | 4 +- .../tests/actions/test_parameters.py | 239 ++++++++++++++++-- tripleo_common/tests/actions/test_scale.py | 30 ++- 4 files changed, 351 insertions(+), 74 deletions(-) diff --git a/tripleo_common/actions/parameters.py b/tripleo_common/actions/parameters.py index 46ea01e14..4f454c0d6 100644 --- a/tripleo_common/actions/parameters.py +++ b/tripleo_common/actions/parameters.py @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. +import copy import json import logging import uuid @@ -121,7 +122,7 @@ class ResetParametersAction(base.TripleOAction): return env -class UpdateParametersAction(base.TripleOAction): +class UpdateParametersAction(templates.ProcessTemplatesAction): """Updates plan environment with parameters.""" def __init__(self, parameters, @@ -134,6 +135,7 @@ class UpdateParametersAction(base.TripleOAction): def run(self, context): swift = self.get_object_client(context) + heat = self.get_orchestration_client(context) try: env = plan_utils.get_env(swift, self.container) @@ -143,19 +145,71 @@ class UpdateParametersAction(base.TripleOAction): LOG.exception(err_msg) return actions.Result(error=err_msg) + saved_env = copy.deepcopy(env) try: + plan_utils.update_in_env(swift, env, self.key, self.parameters) + except swiftexceptions.ClientException as err: err_msg = ("Error updating environment for plan %s: %s" % ( self.container, err)) LOG.exception(err_msg) return actions.Result(error=err_msg) - self.cache_delete(context, - self.container, - "tripleo.parameters.get") - return env + processed_data = super(UpdateParametersAction, self).run(context) + + # If we receive a 'Result' instance it is because the parent action + # had an error. + if isinstance(processed_data, actions.Result): + return processed_data + + processed_data['show_nested'] = True + env = plan_utils.get_env(swift, self.container) + + params = env.get('parameter_defaults') + fields = { + 'template': processed_data['template'], + 'files': processed_data['files'], + 'environment': processed_data['environment'], + 'show_nested': True + } + + try: + result = { + 'heat_resource_tree': heat.stacks.validate(**fields), + 'environment_parameters': params, + } + + # Validation passes so the old cache gets replaced. + self.cache_set(context, + self.container, + "tripleo.parameters.get", + result) + + if result['heat_resource_tree']: + flattened = {'resources': {}, 'parameters': {}} + _flat_it(flattened, 'Root', + result['heat_resource_tree']) + result['heat_resource_tree'] = flattened + + except heat_exc.HTTPException as err: + LOG.debug("Validation failed rebuilding saved env") + + # There has been an error validating we must reprocess the + # templates with the saved working env + plan_utils.put_env(swift, saved_env) + env = saved_env + processed_data = super(UpdateParametersAction, self).run(context) + + err_msg = ("Error validating environment for plan %s: %s" % ( + self.container, err)) + LOG.exception(err_msg) + return actions.Result(error=err_msg) + + LOG.debug("Validation worked new env is saved") + + return result class UpdateRoleParametersAction(UpdateParametersAction): @@ -368,47 +422,6 @@ class GetFlattenedParametersAction(GetParametersAction): def __init__(self, container=constants.DEFAULT_CONTAINER_NAME): super(GetFlattenedParametersAction, self).__init__(container) - def _process_params(self, flattened, params): - for item in params: - if item not in flattened['parameters']: - param_obj = {} - for key, value in params.get(item).items(): - camel_case_key = key[0].lower() + key[1:] - param_obj[camel_case_key] = value - param_obj['name'] = item - flattened['parameters'][item] = param_obj - return list(params) - - def _process(self, flattened, name, data): - key = str(uuid.uuid4()) - value = {} - value.update({ - 'name': name, - 'id': key - }) - if 'Type' in data: - value['type'] = data['Type'] - if 'Description' in data: - value['description'] = data['Description'] - if 'Parameters' in data: - value['parameters'] = self._process_params(flattened, - data['Parameters']) - if 'ParameterGroups' in data: - value['parameter_groups'] = data['ParameterGroups'] - if 'NestedParameters' in data: - nested = data['NestedParameters'] - nested_ids = [] - for nested_key in nested.keys(): - nested_data = self._process(flattened, nested_key, - nested.get(nested_key)) - # nested_data will always have one key (and only one) - nested_ids.append(list(nested_data)[0]) - - value['resources'] = nested_ids - - flattened['resources'][key] = value - return {key: value} - def run(self, context): # process all plan files and create or update a stack processed_data = super(GetFlattenedParametersAction, self).run(context) @@ -420,13 +433,56 @@ class GetFlattenedParametersAction(GetParametersAction): if processed_data['heat_resource_tree']: flattened = {'resources': {}, 'parameters': {}} - self._process(flattened, 'Root', - processed_data['heat_resource_tree']) + _flat_it(flattened, 'Root', + processed_data['heat_resource_tree']) processed_data['heat_resource_tree'] = flattened return processed_data +def _process_params(flattened, params): + for item in params: + if item not in flattened['parameters']: + param_obj = {} + for key, value in params.get(item).items(): + camel_case_key = key[0].lower() + key[1:] + param_obj[camel_case_key] = value + param_obj['name'] = item + flattened['parameters'][item] = param_obj + return list(params) + + +def _flat_it(flattened, name, data): + key = str(uuid.uuid4()) + value = {} + value.update({ + 'name': name, + 'id': key + }) + if 'Type' in data: + value['type'] = data['Type'] + if 'Description' in data: + value['description'] = data['Description'] + if 'Parameters' in data: + value['parameters'] = _process_params(flattened, + data['Parameters']) + if 'ParameterGroups' in data: + value['parameter_groups'] = data['ParameterGroups'] + if 'NestedParameters' in data: + nested = data['NestedParameters'] + nested_ids = [] + for nested_key in nested.keys(): + nested_data = _flat_it(flattened, nested_key, + nested.get(nested_key)) + # nested_data will always have one key (and only one) + nested_ids.append(list(nested_data)[0]) + + value['resources'] = nested_ids + + flattened['resources'][key] = value + return {key: value} + + class GetProfileOfFlavorAction(base.TripleOAction): """Gets the profile name for a given flavor name. diff --git a/tripleo_common/actions/templates.py b/tripleo_common/actions/templates.py index 3475e20f0..1a95a5a20 100644 --- a/tripleo_common/actions/templates.py +++ b/tripleo_common/actions/templates.py @@ -348,8 +348,8 @@ class ProcessTemplatesAction(base.TripleOAction): LOG.exception("Error occurred while processing custom roles.") return actions.Result(error=six.text_type(err)) - template_name = plan_env.get('template') - environments = plan_env.get('environments') + template_name = plan_env.get('template', "") + environments = plan_env.get('environments', []) env_paths = [] temp_files = [] diff --git a/tripleo_common/tests/actions/test_parameters.py b/tripleo_common/tests/actions/test_parameters.py index 58f79965c..7bf9039d9 100644 --- a/tripleo_common/tests/actions/test_parameters.py +++ b/tripleo_common/tests/actions/test_parameters.py @@ -255,27 +255,100 @@ class ResetParametersActionTest(base.TestCase): class UpdateParametersActionTest(base.TestCase): + @mock.patch('tripleo_common.actions.parameters.uuid') + @mock.patch('heatclient.common.template_utils.' + 'process_multiple_environments_and_files') + @mock.patch('heatclient.common.template_utils.' + 'get_template_contents') @mock.patch('tripleo_common.actions.base.TripleOAction.' - 'cache_delete') - @mock.patch('tripleo_common.actions.base.TripleOAction.get_object_client') - def test_run(self, mock_get_object_client, mock_cache): + 'cache_set') + @mock.patch('tripleo_common.actions.base.TripleOAction.' + 'get_object_client') + @mock.patch('tripleo_common.actions.base.TripleOAction.' + 'get_orchestration_client') + def test_run(self, mock_get_orchestration_client_client, + mock_get_object_client, mock_cache, + mock_get_template_contents, mock_env_files, + mock_uuid): + + mock_env_files.return_value = ({}, {}) mock_ctx = mock.MagicMock() swift = mock.MagicMock(url="http://test.com") + mock_env = yaml.safe_dump({ 'name': constants.DEFAULT_CONTAINER_NAME, 'temp_environment': 'temp_environment', 'template': 'template', 'environments': [{u'path': u'environments/test.yaml'}], }, default_flow_style=False) - swift.get_object.return_value = ({}, mock_env) + + mock_roles = yaml.safe_dump([{"name": "foo"}]) + mock_network = yaml.safe_dump([{'enabled': False}]) + mock_exclude = yaml.safe_dump({"name": "foo"}) + + swift.get_object.side_effect = ( + ({}, mock_env), + ({}, mock_env), + ({}, mock_roles), + ({}, mock_network), + ({}, mock_exclude), + ({}, mock_env), + ({}, mock_env), + ({}, mock_env), + ({}, mock_roles), + ({}, mock_network), + ({}, mock_exclude), + ({}, mock_env), + ({}, mock_env), + swiftexceptions.ClientException('atest2') + ) + + def return_container_files(*args): + return ('headers', [{'name': 'foo.role.j2.yaml'}]) + + swift.get_container = mock.MagicMock( + side_effect=return_container_files) + mock_get_object_client.return_value = swift + mock_heat = mock.MagicMock() + mock_get_orchestration_client_client.return_value = mock_heat + + mock_heat.stacks.validate.return_value = { + "Type": "Foo", + "Description": "Le foo bar", + "Parameters": {"bar": {"foo": "bar barz"}}, + "NestedParameters": {"Type": "foobar"} + } + + mock_uuid.uuid4.return_value = "cheese" + + expected_value = { + 'environment_parameters': None, + 'heat_resource_tree': { + 'parameters': {'bar': {'foo': 'bar barz', + 'name': 'bar'}}, + 'resources': {'cheese': { + 'id': 'cheese', + 'name': 'Root', + 'description': 'Le foo bar', + 'parameters': ['bar'], + 'resources': ['cheese'], + 'type': 'Foo'} + } + } + } + + mock_get_template_contents.return_value = ({}, { + 'heat_template_version': '2016-04-30' + }) + # Test test_parameters = {'SomeTestParameter': 42} action = parameters.UpdateParametersAction(test_parameters) - action.run(mock_ctx) + return_value = action.run(mock_ctx) mock_env_updated = yaml.safe_dump({ 'name': constants.DEFAULT_CONTAINER_NAME, @@ -285,21 +358,42 @@ class UpdateParametersActionTest(base.TestCase): 'environments': [{u'path': u'environments/test.yaml'}] }, default_flow_style=False) - swift.put_object.assert_called_once_with( + swift.put_object.assert_any_call( constants.DEFAULT_CONTAINER_NAME, constants.PLAN_ENVIRONMENT, mock_env_updated ) + + mock_heat.stacks.validate.assert_called_once_with( + environment={}, + files={}, + show_nested=True, + template={'heat_template_version': '2016-04-30'}, + ) + mock_cache.assert_called_once_with( mock_ctx, "overcloud", - "tripleo.parameters.get" + "tripleo.parameters.get", + expected_value ) + self.assertEqual(return_value, expected_value) + @mock.patch('heatclient.common.template_utils.' + 'process_multiple_environments_and_files') + @mock.patch('heatclient.common.template_utils.' + 'get_template_contents') @mock.patch('tripleo_common.actions.base.TripleOAction.' - 'cache_delete') - @mock.patch('tripleo_common.actions.base.TripleOAction.get_object_client') - def test_run_new_key(self, mock_get_object_client, mock_cache): + 'cache_set') + @mock.patch('tripleo_common.actions.base.TripleOAction.' + 'get_object_client') + @mock.patch('tripleo_common.actions.base.TripleOAction.' + 'get_orchestration_client') + def test_run_new_key(self, mock_get_orchestration_client_client, + mock_get_object_client, mock_cache, + mock_get_template_contents, mock_env_files): + + mock_env_files.return_value = ({}, {}) mock_ctx = mock.MagicMock() @@ -310,9 +404,44 @@ class UpdateParametersActionTest(base.TestCase): 'template': 'template', 'environments': [{u'path': u'environments/test.yaml'}], }, default_flow_style=False) - swift.get_object.return_value = ({}, mock_env) + + mock_roles = yaml.safe_dump([{"name": "foo"}]) + mock_network = yaml.safe_dump([{'enabled': False}]) + mock_exclude = yaml.safe_dump({"name": "foo"}) + + swift.get_object.side_effect = ( + ({}, mock_env), + ({}, mock_env), + ({}, mock_roles), + ({}, mock_network), + ({}, mock_exclude), + ({}, mock_env), + ({}, mock_env), + ({}, mock_env), + ({}, mock_roles), + ({}, mock_network), + ({}, mock_exclude), + ({}, mock_env), + ({}, mock_env), + swiftexceptions.ClientException('atest2') + ) + + def return_container_files(*args): + return ('headers', [{'name': 'foo.role.j2.yaml'}]) + + swift.get_container = mock.MagicMock( + side_effect=return_container_files) + mock_get_object_client.return_value = swift + heat = mock.MagicMock() + heat.stacks.validate.return_value = {} + mock_get_orchestration_client_client.return_value = heat + + mock_get_template_contents.return_value = ({}, { + 'heat_template_version': '2016-04-30' + }) + # Test test_parameters = {'SomeTestParameter': 42} action = parameters.UpdateParametersAction(test_parameters, @@ -327,40 +456,95 @@ class UpdateParametersActionTest(base.TestCase): 'environments': [{u'path': u'environments/test.yaml'}] }, default_flow_style=False) - swift.put_object.assert_called_once_with( + swift.put_object.assert_any_call( constants.DEFAULT_CONTAINER_NAME, constants.PLAN_ENVIRONMENT, mock_env_updated ) + + heat.stacks.validate.assert_called_once_with( + environment={}, + files={}, + show_nested=True, + template={'heat_template_version': '2016-04-30'}, + ) + mock_cache.assert_called_once_with( mock_ctx, "overcloud", - "tripleo.parameters.get" + "tripleo.parameters.get", + {'environment_parameters': None, 'heat_resource_tree': {}} ) class UpdateRoleParametersActionTest(base.TestCase): + @mock.patch('heatclient.common.template_utils.' + 'process_multiple_environments_and_files') + @mock.patch('heatclient.common.template_utils.' + 'get_template_contents') @mock.patch('tripleo_common.actions.base.TripleOAction.' - 'cache_delete') + 'cache_set') @mock.patch('tripleo_common.utils.parameters.set_count_and_flavor_params') @mock.patch('tripleo_common.actions.base.TripleOAction.' 'get_baremetal_client') @mock.patch('tripleo_common.actions.base.TripleOAction.' 'get_compute_client') - @mock.patch('tripleo_common.actions.base.TripleOAction.get_object_client') - def test_run(self, mock_get_object_client, mock_get_compute_client, + @mock.patch('tripleo_common.actions.base.TripleOAction.' + 'get_object_client') + @mock.patch('tripleo_common.actions.base.TripleOAction.' + 'get_orchestration_client') + def test_run(self, mock_get_orchestration_client_client, + mock_get_object_client, mock_get_compute_client, mock_get_baremetal_client, mock_set_count_and_flavor, - mock_cache): + mock_cache, mock_get_template_contents, mock_env_files): + + mock_env_files.return_value = ({}, {}) mock_ctx = mock.MagicMock() swift = mock.MagicMock(url="http://test.com") - mock_env = yaml.safe_dump({'name': 'overcast'}, - default_flow_style=False) - swift.get_object.return_value = ({}, mock_env) + mock_env = yaml.safe_dump({ + 'name': 'overcast' + }, default_flow_style=False) + + mock_roles = yaml.safe_dump([{"name": "foo"}]) + mock_network = yaml.safe_dump([{'enabled': False}]) + mock_exclude = yaml.safe_dump({"name": "foo"}) + + swift.get_object.side_effect = ( + ({}, mock_env), + ({}, mock_env), + ({}, mock_roles), + ({}, mock_network), + ({}, mock_exclude), + ({}, mock_env), + ({}, mock_env), + ({}, mock_env), + ({}, mock_roles), + ({}, mock_network), + ({}, mock_exclude), + ({}, mock_env), + ({}, mock_env), + swiftexceptions.ClientException('atest2') + ) + + def return_container_files(*args): + return ('headers', [{'name': 'foo.yaml'}]) + + swift.get_container = mock.MagicMock( + side_effect=return_container_files) + mock_get_object_client.return_value = swift + heat = mock.MagicMock() + heat.stacks.validate.return_value = {} + mock_get_orchestration_client_client.return_value = heat + + mock_get_template_contents.return_value = ({}, { + 'heat_template_version': '2016-04-30' + }) + params = {'CephStorageCount': 1, 'OvercloudCephStorageFlavor': 'ceph-storage'} mock_set_count_and_flavor.return_value = params @@ -371,18 +555,27 @@ class UpdateRoleParametersActionTest(base.TestCase): mock_env_updated = yaml.safe_dump({ 'name': 'overcast', - 'parameter_defaults': params, + 'parameter_defaults': params }, default_flow_style=False) - swift.put_object.assert_called_once_with( + swift.put_object.assert_any_call( 'overcast', constants.PLAN_ENVIRONMENT, mock_env_updated ) + + heat.stacks.validate.assert_called_once_with( + environment={}, + files={}, + show_nested=True, + template={'heat_template_version': '2016-04-30'}, + ) + mock_cache.assert_called_once_with( mock_ctx, "overcast", - "tripleo.parameters.get" + "tripleo.parameters.get", + {'environment_parameters': None, 'heat_resource_tree': {}} ) diff --git a/tripleo_common/tests/actions/test_scale.py b/tripleo_common/tests/actions/test_scale.py index 3c8ef39c0..5ff599681 100644 --- a/tripleo_common/tests/actions/test_scale.py +++ b/tripleo_common/tests/actions/test_scale.py @@ -79,6 +79,7 @@ class ScaleDownActionTest(base.TestCase): ) ] heatclient.stacks.get.return_value = mock_stack() + heatclient.stacks.validate.return_value = {} mock_get_heat_client.return_value = heatclient mock_ctx = mock.MagicMock() @@ -89,11 +90,31 @@ class ScaleDownActionTest(base.TestCase): 'template': 'template', 'environments': [{u'path': u'environments/test.yaml'}] }, default_flow_style=False) + mock_roles = yaml.safe_dump([{"name": "foo"}]) + mock_network = yaml.safe_dump([{'enabled': False}]) + mock_exclude = yaml.safe_dump({"name": "foo"}) swift.get_object.side_effect = ( + ({}, mock_env), + ({}, mock_env), + ({}, mock_roles), + ({}, mock_network), + ({}, mock_exclude), + ({}, mock_env), + ({}, mock_env), + ({}, mock_env), + ({}, mock_roles), + ({}, mock_network), + ({}, mock_exclude), ({}, mock_env), ({}, mock_env), swiftexceptions.ClientException('atest2') ) + + def return_container_files(*args): + return ('headers', [{'name': 'foo.role.j2.yaml'}]) + + swift.get_container = mock.MagicMock( + side_effect=return_container_files) mock_get_object_client.return_value = swift env = { @@ -111,6 +132,13 @@ class ScaleDownActionTest(base.TestCase): constants.STACK_TIMEOUT_DEFAULT, ['resource_id'], 'stack') action.run(mock_ctx) + heatclient.stacks.validate.assert_called_once_with( + environment=env, + files={}, + show_nested=True, + template={'heat_template_version': '2016-04-30'} + ) + heatclient.stacks.update.assert_called_once_with( 'stack', stack_name='stack', @@ -120,7 +148,7 @@ class ScaleDownActionTest(base.TestCase): files={}, timeout_mins=240) - mock_cache.assert_called_once_with( + mock_cache.assert_called_with( mock_ctx, "stack", "tripleo.parameters.get"