Improve best existing resource selection

Rank all existing versions of a resource in a convergence stack to improve
the likelihood that we find the best one to update.

This allows us to roll back to the original version of a resource (or
even attempt an in-place update of it) when replacing it has failed.
Previously this only worked during automatic rollback; on subsequent
updates we would always work on the failed replacement (which inevitably
meant attempting another replacement in almost all cases).

Change-Id: Ia231fae85d1ddb9fc7b7de4e82cec0c0e0fd06b7
Story: #2003579
Task: 24881

heat/engine/stack.py

@@ -42,6 +42,7 @@ from heat.engine import parent_rsrc
 from heat.engine import resource
 from heat.engine import resources
 from heat.engine import scheduler
+from heat.engine import status
 from heat.engine import stk_defn
 from heat.engine import sync_point
 from heat.engine import template as tmpl
@@ -1462,26 +1463,33 @@ class Stack(collections.Mapping):
         self.converge_stack(rollback_tmpl, action=self.ROLLBACK)
 
     def _get_best_existing_rsrc_db(self, rsrc_name):
-        candidate = None
         if self.ext_rsrcs_db:
-            for ext_rsrc in self.ext_rsrcs_db.values():
+            def suitability(ext_rsrc):
-                if ext_rsrc.name != rsrc_name:
+                score = 0
-                    continue
-                if ext_rsrc.current_template_id == self.t.id:
-                    # Rollback where the previous resource still exists
-                    candidate = ext_rsrc
-                    break
-                elif (ext_rsrc.current_template_id ==
-                        self.prev_raw_template_id):
-                    # Current resource is otherwise a good candidate
-                    candidate = ext_rsrc
-                elif candidate is None:
-                    # In multiple concurrent updates, if candidate is not
-                    # found in current/previous template, it could be found
-                    # in old tmpl.
-                    candidate = ext_rsrc
 
-        return candidate
+                if ext_rsrc.status == status.ResourceStatus.FAILED:
+                    score -= 30
+                if ext_rsrc.action == status.ResourceStatus.DELETE:
+                    score -= 50
+                if ext_rsrc.replaced_by:
+                    score -= 1
+                if ext_rsrc.current_template_id == self.prev_raw_template_id:
+                    # Current resource
+                    score += 5
+                if ext_rsrc.current_template_id == self.t.id:
+                    # Rolling back to previous resource
+                    score += 10
+
+                return score, ext_rsrc.updated_at
+
+            candidates = sorted((r for r in self.ext_rsrcs_db.values()
+                                 if r.name == rsrc_name),
+                                key=suitability,
+                                reverse=True)
+            if candidates:
+                return candidates[0]
+
+        return None
 
     def _update_or_store_resources(self):
         self.ext_rsrcs_db = self.db_active_resources_get()

heat/tests/test_convg_stack.py

@@ -16,7 +16,6 @@ from oslo_config import cfg
 
 from heat.common import template_format
 from heat.engine import environment
-from heat.engine import resource as res
 from heat.engine import stack as parser
 from heat.engine import template as templatem
 from heat.objects import raw_template as raw_template_object
@@ -426,38 +425,39 @@ class StackConvergenceCreateUpdateDeleteTest(common.HeatTestCase):
         stack = tools.get_stack('test_stack', utils.dummy_context(),
                                 template=tools.string_template_five,
                                 convergence=True)
-        stack.store()
         stack.prev_raw_template_id = 2
         stack.t.id = 3
-        dummy_res = stack.resources['A']
+
-        a_res_2 = res.Resource('A', dummy_res.t, stack)
+        def db_resource(current_template_id):
-        a_res_2.current_template_id = 2
+            db_res = resource_objects.Resource(stack.context)
-        a_res_2.id = 2
+            db_res['id'] = current_template_id
-        a_res_3 = res.Resource('A', dummy_res.t, stack)
+            db_res['name'] = 'A'
-        a_res_3.current_template_id = 3
+            db_res['current_template_id'] = current_template_id
-        a_res_3.id = 3
+            db_res['action'] = 'CREATE'
-        a_res_1 = res.Resource('A', dummy_res.t, stack)
+            db_res['status'] = 'COMPLETE'
-        a_res_1.current_template_id = 1
+            db_res['updated_at'] = None
-        a_res_1.id = 1
+            db_res['replaced_by'] = None
-        existing_res = {2: a_res_2,
+            return db_res
-                        3: a_res_3,
+
-                        1: a_res_1}
+        a_res_2 = db_resource(2)
+        a_res_3 = db_resource(3)
+        a_res_1 = db_resource(1)
+        existing_res = {a_res_2.id: a_res_2,
+                        a_res_3.id: a_res_3,
+                        a_res_1.id: a_res_1}
         stack.ext_rsrcs_db = existing_res
         best_res = stack._get_best_existing_rsrc_db('A')
         # should return resource with template id 3 which is current template
         self.assertEqual(a_res_3.id, best_res.id)
 
         # no resource with current template id as 3
-        existing_res = {1: a_res_1,
+        del existing_res[3]
-                        2: a_res_2}
-        stack.ext_rsrcs_db = existing_res
         best_res = stack._get_best_existing_rsrc_db('A')
         # should return resource with template id 2 which is prev template
         self.assertEqual(a_res_2.id, best_res.id)
 
         # no resource with current template id as 3 or 2
-        existing_res = {1: a_res_1}
+        del existing_res[2]
-        stack.ext_rsrcs_db = existing_res
         best_res = stack._get_best_existing_rsrc_db('A')
         # should return resource with template id 1 existing in DB
         self.assertEqual(a_res_1.id, best_res.id)

heat_integrationtests/functional/test_create_update.py

@@ -68,9 +68,8 @@ def _change_rsrc_properties(template, rsrcs, values):
         for rsrc_name in rsrcs:
             rsrc_prop = modified_template['resources'][
                 rsrc_name]['properties']
-            for prop in rsrc_prop:
+            for prop, new_val in values.items():
-                if prop in values:
+                rsrc_prop[prop] = new_val
-                    rsrc_prop[prop] = values[prop]
         return modified_template
 
 
@@ -280,6 +279,31 @@ resources:
         self.assertEqual(updated_resources,
                          self.list_resources(stack_identifier))
 
+    @test.requires_convergence
+    def test_stack_update_replace_manual_rollback(self):
+        template = _change_rsrc_properties(test_template_one_resource,
+                                           ['test1'],
+                                           {'update_replace_value': '1'})
+        stack_identifier = self.stack_create(template=template)
+        original_resource_id = self.get_physical_resource_id(stack_identifier,
+                                                             'test1')
+
+        tmpl_update = _change_rsrc_properties(test_template_one_resource,
+                                              ['test1'],
+                                              {'update_replace_value': '2',
+                                               'fail': True})
+        # Update with bad template, we should fail
+        self.update_stack(stack_identifier, tmpl_update,
+                          expected_status='UPDATE_FAILED',
+                          disable_rollback=True)
+        # Manually roll back to previous template
+        self.update_stack(stack_identifier, template)
+        final_resource_id = self.get_physical_resource_id(stack_identifier,
+                                                          'test1')
+        # Original resource was good, and replacement was never created, so it
+        # should be kept.
+        self.assertEqual(original_resource_id, final_resource_id)
+
     def test_stack_update_provider(self):
         template = _change_rsrc_properties(
             test_template_one_resource, ['test1'],

releasenotes/notes/manual-rollback-failed-replacement-08ebb9271617fe9d.yaml

@@ -0,0 +1,11 @@
+---
+fixes:
+  - |
+    Heat can now perform a stack update to roll back to a previous version of a
+    resource after a previous attempt to create a replacement for it failed
+    (provided that convergence is enabled). This allows the user to recover a
+    stack where a resource has been inadvertantly replaced with a definition
+    than can never succeed because it conflicts with the original. Previously
+    this required automatic rollback to be enabled, or the user had to update
+    the stack with a non-conflicting definition before rolling back to the
+    original.