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 4507322675 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

@@ -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)

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)