From da991316e4ac4813797b96c129ba35f6ddd0a043 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Fri, 21 Jul 2017 21:32:45 -0400 Subject: [PATCH] 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 e649574d4751ffd8578f63787621c3a26d383a34) --- heat/engine/resource.py | 74 ++++++++++++++++++------------------- heat/tests/test_resource.py | 4 +- 2 files changed, 37 insertions(+), 41 deletions(-) diff --git a/heat/engine/resource.py b/heat/engine/resource.py index 4275887a58..436fd3d616 100644 --- a/heat/engine/resource.py +++ b/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( - self.update, new_res_def, - update_templ_func=update_templ_id_and_requires) - try: - 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) + runner = scheduler.TaskRunner(self.update, new_res_def, + new_template_id=template_id, + new_requires=new_requires) + runner(timeout=timeout, progress_callback=progress_callback) 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: - update_templ_func(persist=True) + self._persist_update_no_change(new_template_id) 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: - 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) + self._persist_update_no_change(new_template_id) 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: - # template/requires will be persisted by _action_recorder() - update_templ_func(persist=False) + if new_requires is not None: + self.requires = list(new_requires) yield self._break_if_required( self.UPDATE, environment.HOOK_POST_UPDATE) diff --git a/heat/tests/test_resource.py b/heat/tests/test_resource.py index a02bfb9e5a..621aadd662 100644 --- a/heat/tests/test_resource.py +++ b/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({