Merge before/after 'requires' list on update failure

If an update of a resource fails, its 'requires' should be set to a union
of the previous and new requires lists. This is because if the resource
depends on a resource that has been replaced in this stack update, we can't
know if the current resource now depends on the new or old version of the
replaced resource if the current resource failed.

This is achieved by splitting up the setting of 'requires' and
'current_template_id', and changing them directly in the update() method
instead of via a callback.

When the resource state is changed to UPDATE_IN_PROGRESS, the new
requirements are added to the old ones. When the state is changed to
UPDATE_COMPLETE, the new requirements replace the old ones altogether. If
the update fails or handle_update() raises UpdateReplace, the union of the
requirements is kept. If _needs_update() raises UpdateReplace, the old
requirements are kept.

The current_template_id is updated when the state is changed to either
UPDATE_COMPLETE or UPDATE_FAILED, or when no update is required
(_needs_update() returns False).

This also saves an extra database write when the update fails.

Change-Id: If70d457fba5c64611173e3f9a0ae6b155ec69e06
Closes-Bug: #1663388
(cherry picked from commit e649574d47)

heat/engine/resource.py

@@ -1436,18 +1436,9 @@ class Resource(status.ResourceStatus):
         resource_data and existing resource's requires, then updates the
         resource by invoking the scheduler TaskRunner.
         """
-        def update_templ_id_and_requires(persist=True):
-            self.current_template_id = template_id
-            self.requires = list(
-                set(data.primary_key for data in resource_data.values()
-                    if data is not None)
-            )
-            if not persist:
-                return
-
-            self.store(lock=self.LOCK_RESPECT)
-
         self._calling_engine_id = engine_id
+        new_requires = set(data.primary_key for data in resource_data.values()
+                           if data is not None)
 
         # Check that the resource type matches. If the type has changed by a
         # legitimate substitution, the load()ed resource will already be of
@@ -1472,18 +1463,10 @@ class Resource(status.ResourceStatus):
                 raise failure
         self.replaced_by = None
 
-        runner = scheduler.TaskRunner(
+        runner = scheduler.TaskRunner(self.update, new_res_def,
-            self.update, new_res_def,
+                                      new_template_id=template_id,
-            update_templ_func=update_templ_id_and_requires)
+                                      new_requires=new_requires)
-        try:
+        runner(timeout=timeout, progress_callback=progress_callback)
-            runner(timeout=timeout, progress_callback=progress_callback)
-        except UpdateReplace:
-            raise
-        except exception.UpdateInProgress:
-            raise
-        except BaseException:
-            with excutils.save_and_reraise_exception():
-                update_templ_id_and_requires(persist=True)
 
     def preview_update(self, after, before, after_props, before_props,
                        prev_resource, check_init_complete=False):
@@ -1600,9 +1583,24 @@ class Resource(status.ResourceStatus):
                 return is_substituted
             return False
 
+    def _persist_update_no_change(self, new_template_id):
+        """Persist an update where the resource is unchanged."""
+        if new_template_id is not None:
+            self.current_template_id = new_template_id
+        lock = (self.LOCK_RESPECT if self.stack.convergence
+                else self.LOCK_NONE)
+        if self.status == self.FAILED:
+            status_reason = _('Update status to COMPLETE for '
+                              'FAILED resource neither update '
+                              'nor replace.')
+            self.state_set(self.action, self.COMPLETE,
+                           status_reason, lock=lock)
+        elif new_template_id is not None:
+            self.store(lock=lock)
+
     @scheduler.wrappertask
     def update(self, after, before=None, prev_resource=None,
-               update_templ_func=None):
+               new_template_id=None, new_requires=None):
         """Return a task to update the resource.
 
         Subclasses should provide a handle_update() method to customise update,
@@ -1623,8 +1621,7 @@ class Resource(status.ResourceStatus):
             raise exception.ResourceFailure(exc, self, action)
         elif after_external_id is not None:
             LOG.debug("Skip update on external resource.")
-            if update_templ_func is not None:
+            self._persist_update_no_change(new_template_id)
-                update_templ_func(persist=True)
             return
 
         after_props, before_props = self._prepare_update_props(after, before)
@@ -1656,16 +1653,7 @@ class Resource(status.ResourceStatus):
             raise failure
 
         if not needs_update:
-            if update_templ_func is not None:
+            self._persist_update_no_change(new_template_id)
-                update_templ_func(persist=True)
-            if self.status == self.FAILED:
-                status_reason = _('Update status to COMPLETE for '
-                                  'FAILED resource neither update '
-                                  'nor replace.')
-                lock = (self.LOCK_RESPECT if self.stack.convergence
-                        else self.LOCK_NONE)
-                self.state_set(self.action, self.COMPLETE,
-                               status_reason, lock=lock)
             return
 
         if not self.stack.convergence:
@@ -1680,10 +1668,14 @@ class Resource(status.ResourceStatus):
 
         self.updated_time = datetime.utcnow()
 
+        if new_requires is not None:
+            self.requires = list(set(self.requires) | new_requires)
+
         with self._action_recorder(action, UpdateReplace):
             after_props.validate()
             self.properties = before_props
             tmpl_diff = self.update_template_diff(after.freeze(), before)
+            old_template_id = self.current_template_id
 
             try:
                 if tmpl_diff and self.needs_replace_with_tmpl_diff(tmpl_diff):
@@ -1691,19 +1683,23 @@ class Resource(status.ResourceStatus):
 
                 prop_diff = self.update_template_diff_properties(after_props,
                                                                  before_props)
+
+                if new_template_id is not None:
+                    self.current_template_id = new_template_id
+
                 yield self.action_handler_task(action,
                                                args=[after, tmpl_diff,
                                                      prop_diff])
             except UpdateReplace:
                 with excutils.save_and_reraise_exception():
+                    self.current_template_id = old_template_id
                     self._prepare_update_replace(action)
 
             self.t = after
             self.reparse()
             self._update_stored_properties()
-            if update_templ_func is not None:
+            if new_requires is not None:
-                # template/requires will be persisted by _action_recorder()
+                self.requires = list(new_requires)
-                update_templ_func(persist=False)
 
         yield self._break_if_required(
             self.UPDATE, environment.HOOK_POST_UPDATE)

heat/tests/test_resource.py

@@ -2366,10 +2366,10 @@ class ResourceTest(common.HeatTestCase):
 
         self.assertEqual(new_temp.id, res.current_template_id)
         # check if requires was updated
-        self.assertItemsEqual([3, 4], res.requires)
+        self.assertItemsEqual([2, 3, 4], res.requires)
         self.assertEqual(res.action, resource.Resource.UPDATE)
         self.assertEqual(res.status, resource.Resource.FAILED)
-        self._assert_resource_lock(res.id, None, 3)
+        self._assert_resource_lock(res.id, None, 2)
 
     def test_update_resource_convergence_update_replace(self):
         tmpl = template.Template({