From 93b4551d9a1ff9194b30a079f02db691129c69b0 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Wed, 1 Nov 2017 18:03:20 -0400 Subject: [PATCH] Fix non-destructive upgrade for deprecated res types in convergence When a user updates from a deprecated resource type to an equivalent-but-differently-named one (e.g. from OS::Heat::SoftwareDeployments to OS::Heat::SoftwareDeploymentGroup), Heat is supposed to change the type without replacing the resource as it would normally do when a resource type changes. This was broken in convergence, because since 45073226752c58d640ea5a59b7e532c022a4939b the new Resource object we create during the check_resource operation (using the new type's plugin) is no longer automatically initialised with data from the database as resources in the legacy path are. Move the substitution checking to the Resource.load() method, so that it now returns an instance of the new plugin where allowed. In the actual update_convergence() method then we need only check that the resource class is the one we'd expect from the new template, and replace the resource if not. We do have a test that is designed to check that this is working, but in it we didn't compare the physical IDs of the resource that is potentially getting replaced, but rather the physical IDs of some other resource that can't possibly get modified (surprise! it doesn't change). Change-Id: I75778abc303525a71d0a918f7192f00a43c21284 Closes-Bug: #1729439 --- heat/engine/resource.py | 45 ++++++++++++------- .../functional/test_replace_deprecated.py | 12 +++-- 2 files changed, 36 insertions(+), 21 deletions(-) diff --git a/heat/engine/resource.py b/heat/engine/resource.py index 5d253e8ebc..fe63726d15 100644 --- a/heat/engine/resource.py +++ b/heat/engine/resource.py @@ -351,10 +351,28 @@ class Resource(status.ResourceStatus): curr_stack.identifier()) curr_stack.defn = initial_stk_defn + res_defn = initial_stk_defn.resource_definition(db_res.name) + res_type = initial_stk_defn.env.registry.get_class_to_instantiate( + res_defn.resource_type, resource_name=db_res.name) + + # If the resource type has changed and the new one is a valid + # substitution, use that as the class to instantiate. + if is_update and (latest_stk_defn is not initial_stk_defn): + try: + new_res_defn = latest_stk_defn.resource_definition(db_res.name) + except KeyError: + pass + else: + new_registry = latest_stk_defn.env.registry + new_res_type = new_registry.get_class_to_instantiate( + new_res_defn.resource_type, resource_name=db_res.name) + + if res_type.check_is_substituted(new_res_type): + res_type = new_res_type + # Load only the resource in question; don't load all resources # by invoking stack.resources. Maintain light-weight stack. - res_defn = initial_stk_defn.resource_definition(db_res.name) - resource = cls(db_res.name, res_defn, curr_stack) + resource = res_type(db_res.name, res_defn, curr_stack) resource._load_data(db_res) curr_stack.defn = latest_stk_defn @@ -1355,15 +1373,17 @@ class Resource(status.ResourceStatus): self.store(lock=self.LOCK_RESPECT) self._calling_engine_id = engine_id + + # Check that the resource type matches. If the type has changed by a + # legitimate substitution, the load()ed resource will already be of + # the new type. registry = new_stack.env.registry new_res_def = new_stack.defn.resource_definition(self.name) new_res_type = registry.get_class_to_instantiate( new_res_def.resource_type, resource_name=self.name) - restricted_actions = registry.get_rsrc_restricted_actions( - self.name) - is_substituted = self.check_is_substituted(new_res_type) - if type(self) is not new_res_type and not is_substituted: - self._check_for_convergence_replace(restricted_actions) + if type(self) is not new_res_type: + restrictions = registry.get_rsrc_restricted_actions(self.name) + self._check_for_convergence_replace(restrictions) action_rollback = self.stack.action == self.stack.ROLLBACK status_in_progress = self.stack.status == self.stack.IN_PROGRESS @@ -1376,17 +1396,8 @@ class Resource(status.ResourceStatus): six.text_type(failure)) raise failure - # Use new resource as update method if existing resource - # need to be substituted. - if is_substituted: - substitute = new_res_type(self.name, self.t, self.stack) - self.stack.resources[self.name] = substitute - substitute._calling_engine_id = engine_id - updater = substitute.update - else: - updater = self.update runner = scheduler.TaskRunner( - updater, new_res_def, + self.update, new_res_def, update_templ_func=update_templ_id_and_requires) try: runner(timeout=timeout, progress_callback=progress_callback) diff --git a/heat_integrationtests/functional/test_replace_deprecated.py b/heat_integrationtests/functional/test_replace_deprecated.py index 5e7fdc67ee..bbf2e66cce 100644 --- a/heat_integrationtests/functional/test_replace_deprecated.py +++ b/heat_integrationtests/functional/test_replace_deprecated.py @@ -69,24 +69,28 @@ properties: parameters=parms, template=deployments_template, enable_cleanup=self.enable_cleanup) + expected_resources = {'config': 'OS::Heat::SoftwareConfig', 'dep': 'OS::Heat::SoftwareDeployments', 'server': 'OS::Nova::Server'} - resource = self.client.resources.get(stack_identifier, 'server') self.assertEqual(expected_resources, self.list_resources(stack_identifier)) + + resource = self.client.resources.get(stack_identifier, 'dep') initial_phy_id = resource.physical_resource_id + resources = deployments_template['resources'] resources['dep'] = yaml.safe_load(self.deployment_group_snippet) self.update_stack( stack_identifier, deployments_template, parameters=parms) - resource = self.client.resources.get(stack_identifier, 'server') - self.assertEqual(initial_phy_id, - resource.physical_resource_id) + expected_new_resources = {'config': 'OS::Heat::SoftwareConfig', 'dep': 'OS::Heat::SoftwareDeploymentGroup', 'server': 'OS::Nova::Server'} self.assertEqual(expected_new_resources, self.list_resources(stack_identifier)) + + resource = self.client.resources.get(stack_identifier, 'dep') + self.assertEqual(initial_phy_id, resource.physical_resource_id)