From 2a986d838d1f46311b31b39c5f7d841fbd5775b1 Mon Sep 17 00:00:00 2001 From: Steven Hardy Date: Fri, 17 May 2013 13:17:57 +0100 Subject: [PATCH] engine : cleanup Resource.update error paths The current update code uses a nasty pattern where returning an arbitrary string from Resource.update is interpreted in parser.Stack as an error. Instead use exceptions which is much nicer :) Change-Id: I2ddebfd6bbec3dc229012406da72dd928fcc4595 --- heat/common/exception.py | 4 --- heat/engine/parser.py | 65 ++++++++++--------------------------- heat/engine/resource.py | 11 ++++--- heat/tests/test_parser.py | 3 +- heat/tests/test_resource.py | 42 ++++++++++++++++++++++-- 5 files changed, 65 insertions(+), 60 deletions(-) diff --git a/heat/common/exception.py b/heat/common/exception.py index 97b8ea0c..419685e1 100644 --- a/heat/common/exception.py +++ b/heat/common/exception.py @@ -239,10 +239,6 @@ class ResourceNotAvailable(OpenstackException): message = _("The Resource (%(resource_name)s) is not available.") -class ResourceUpdateFailed(OpenstackException): - message = _("Resource (%(resource_name)s) update failed") - - class PhysicalResourceNotFound(OpenstackException): message = _("The Resource (%(resource_id)s) could not be found.") diff --git a/heat/engine/parser.py b/heat/engine/parser.py index 7e7089a2..492491e6 100644 --- a/heat/engine/parser.py +++ b/heat/engine/parser.py @@ -352,17 +352,11 @@ class Stack(object): if not res.name in newstack.keys(): logger.debug("resource %s not found in updated stack" % res.name + " definition, deleting") - try: - res.destroy() - except exception.ResourceFailure as ex: - logger.error("Failed to remove %s : %s" % - (res.name, str(ex))) - raise exception.ResourceUpdateFailed( - resource_name=res.name) - else: - del self.resources[res.name] - self.dependencies = self._get_dependencies( - self.resources.itervalues()) + # res.destroy raises exception.ResourceFailure on error + res.destroy() + del self.resources[res.name] + self.dependencies = self._get_dependencies( + self.resources.itervalues()) # Then create any which are defined in newstack but not self for res in newstack: @@ -373,13 +367,8 @@ class Stack(object): self[res.name] = res self.dependencies = self._get_dependencies( self.resources.itervalues()) - try: - scheduler.TaskRunner(res.create)() - except exception.ResourceFailure as ex: - logger.error("Failed to add %s : %s" % - (res.name, str(ex))) - raise exception.ResourceUpdateFailed( - resource_name=res.name) + # res.create raises exception.ResourceFailure on error + scheduler.TaskRunner(res.create)() # Now (the hard part :) update existing resources # The Resource base class allows equality-test of resources, @@ -404,9 +393,9 @@ class Stack(object): new_snippet = self.resolve_runtime_data(res.t) if old_snippet != new_snippet: - # Can fail if underlying resource class does not - # implement update logic or update requires replacement + # res.update raises exception.ResourceFailure on error retval = self[res.name].update(new_snippet) + if retval == self[res.name].UPDATE_COMPLETE: logger.info("Resource %s for stack %s updated" % (res.name, self.name)) @@ -415,30 +404,15 @@ class Stack(object): (res.name, self.name) + " update requires replacement") # Resource requires replacement for update - try: - self[res.name].destroy() - except exception.ResourceFailure as ex: - logger.error("Failed to delete %s : %s" % - (res.name, str(ex))) - raise exception.ResourceUpdateFailed( - resource_name=res.name) - else: - res.stack = self - self[res.name] = res - self.dependencies = self._get_dependencies( - self.resources.itervalues()) - try: - scheduler.TaskRunner(res.create)() - except exception.ResourceFailure as ex: - logger.error("Failed to create %s : %s" % - (res.name, str(ex))) - raise exception.ResourceUpdateFailed( - resource_name=res.name) + self[res.name].destroy() + res.stack = self + self[res.name] = res + self.dependencies = self._get_dependencies( + self.resources.itervalues()) + scheduler.TaskRunner(res.create)() else: - logger.error("Failed to %s %s" % - (action, res.name)) - raise exception.ResourceUpdateFailed( - resource_name=res.name) + raise exception.ResourceFailure( + "Unexpected update retval %s" % retval) if action == self.UPDATE: stack_status = self.UPDATE_COMPLETE @@ -454,16 +428,13 @@ class Stack(object): else: # not my timeout raise - except exception.ResourceUpdateFailed as e: + except exception.ResourceFailure as e: reason = str(e) or "Error : %s" % type(e) if action == self.UPDATE: stack_status = self.UPDATE_FAILED # If rollback is enabled, we do another update, with the # existing template, so we roll back to the original state - # Note - ensure nothing after the "flip the template..." - # section above can raise ResourceUpdateFailed or this - # will not work ;) if self.disable_rollback: stack_status = self.UPDATE_FAILED else: diff --git a/heat/engine/resource.py b/heat/engine/resource.py index b34a3c67..67f85e8a 100644 --- a/heat/engine/resource.py +++ b/heat/engine/resource.py @@ -364,10 +364,12 @@ class Resource(object): to customise update, the base-class handle_update will fail by default. ''' if self.state in (self.CREATE_IN_PROGRESS, self.UPDATE_IN_PROGRESS): - return 'Resource update already requested' + raise exception.ResourceFailure( + 'Resource update already requested') if not json_snippet: - return 'Must specify json snippet for resource update!' + raise exception.ResourceFailure( + 'Must specify json snippet for resource update!') logger.info('updating %s' % str(self)) @@ -383,8 +385,9 @@ class Resource(object): result = self.handle_update(json_snippet) except Exception as ex: logger.exception('update %s : %s' % (str(self), str(ex))) - self.state_set(self.UPDATE_FAILED, str(ex)) - return str(ex) or "Error : %s" % type(ex) + failure = exception.ResourceFailure(ex) + self.state_set(self.UPDATE_FAILED, str(failure)) + raise failure else: # If resource was updated (with or without interruption), # then we set the resource to UPDATE_COMPLETE diff --git a/heat/tests/test_parser.py b/heat/tests/test_parser.py index 08e31685..6f0df685 100644 --- a/heat/tests/test_parser.py +++ b/heat/tests/test_parser.py @@ -597,8 +597,7 @@ class StackTest(HeatTestCase): # patch in a dummy handle_update self.m.StubOutWithMock(generic_rsrc.GenericResource, 'handle_update') generic_rsrc.GenericResource.handle_update( - tmpl2['Resources']['AResource']).AndReturn( - resource.Resource.UPDATE_FAILED) + tmpl2['Resources']['AResource']).AndRaise(Exception("Foo")) self.m.ReplayAll() self.stack.update(updated_stack) diff --git a/heat/tests/test_resource.py b/heat/tests/test_resource.py index 5bb7af12..d69db9ec 100644 --- a/heat/tests/test_resource.py +++ b/heat/tests/test_resource.py @@ -275,6 +275,24 @@ class ResourceTest(HeatTestCase): self.assertEqual(res.UPDATE_COMPLETE, res.state) self.m.VerifyAll() + def test_update_replace(self): + # patch in a dummy property schema for GenericResource + dummy_schema = {'Foo': {'Type': 'String'}} + generic_rsrc.GenericResource.properties_schema = dummy_schema + + tmpl = {'Type': 'GenericResourceType', 'Properties': {'Foo': 'abc'}} + res = generic_rsrc.GenericResource('test_resource', tmpl, self.stack) + scheduler.TaskRunner(res.create)() + self.assertEqual(res.CREATE_COMPLETE, res.state) + + utmpl = {'Type': 'GenericResourceType', 'Properties': {'Foo': 'xyz'}} + self.m.StubOutWithMock(generic_rsrc.GenericResource, 'handle_update') + generic_rsrc.GenericResource.handle_update(utmpl).AndReturn( + resource.Resource.UPDATE_REPLACE) + self.m.ReplayAll() + self.assertEqual(res.UPDATE_REPLACE, res.update(utmpl)) + self.m.VerifyAll() + def test_update_fail_missing_req_prop(self): # patch in a dummy property schema for GenericResource dummy_schema = {'Foo': {'Type': 'String', 'Required': True}} @@ -287,8 +305,7 @@ class ResourceTest(HeatTestCase): utmpl = {'Type': 'GenericResourceType', 'Properties': {}} - estr = 'Property error : test_resource: Property Foo not assigned' - self.assertEqual(estr, res.update(utmpl)) + self.assertRaises(exception.ResourceFailure, res.update, utmpl) self.assertEqual(res.UPDATE_FAILED, res.state) def test_update_fail_prop_typo(self): @@ -303,9 +320,28 @@ class ResourceTest(HeatTestCase): utmpl = {'Type': 'GenericResourceType', 'Properties': {'Food': 'xyz'}} - self.assertEqual('Unknown Property Food', res.update(utmpl)) + self.assertRaises(exception.ResourceFailure, res.update, utmpl) self.assertEqual(res.UPDATE_FAILED, res.state) + def test_update_not_implemented(self): + # patch in a dummy property schema for GenericResource + dummy_schema = {'Foo': {'Type': 'String'}} + generic_rsrc.GenericResource.properties_schema = dummy_schema + + tmpl = {'Type': 'GenericResourceType', 'Properties': {'Foo': 'abc'}} + res = generic_rsrc.GenericResource('test_resource', tmpl, self.stack) + scheduler.TaskRunner(res.create)() + self.assertEqual(res.CREATE_COMPLETE, res.state) + + utmpl = {'Type': 'GenericResourceType', 'Properties': {'Foo': 'xyz'}} + self.m.StubOutWithMock(generic_rsrc.GenericResource, 'handle_update') + generic_rsrc.GenericResource.handle_update(utmpl).AndRaise( + NotImplemented) + self.m.ReplayAll() + self.assertRaises(exception.ResourceFailure, res.update, utmpl) + self.assertEqual(res.UPDATE_FAILED, res.state) + self.m.VerifyAll() + class MetadataTest(HeatTestCase): def setUp(self): -- 2.45.2