From: Zane Bitter Date: Tue, 23 Apr 2013 11:36:47 +0000 (+0200) Subject: Propagate deletion errors with exceptions X-Git-Tag: 2014.1~693^2 X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=841ca88d1b68907e1aafd748ab6bc9be0280cb61;p=openstack-build%2Fheat-build.git Propagate deletion errors with exceptions Change-Id: Ib8380f6d985e328c56d53b326700d6c9df636f40 --- diff --git a/heat/common/exception.py b/heat/common/exception.py index 1e2f85e5..5fd56a19 100644 --- a/heat/common/exception.py +++ b/heat/common/exception.py @@ -256,7 +256,3 @@ class ResourceFailure(OpenstackException): exc_type = type(exception).__name__ super(ResourceFailure, self).__init__(exc_type=exc_type, message=str(exception)) - - -class NestedResourceFailure(OpenstackException): - message = _("%(message)s") diff --git a/heat/engine/parser.py b/heat/engine/parser.py index 257f3d11..992a74ee 100644 --- a/heat/engine/parser.py +++ b/heat/engine/parser.py @@ -346,10 +346,11 @@ class Stack(object): if not res.name in newstack.keys(): logger.debug("resource %s not found in updated stack" % res.name + " definition, deleting") - result = res.destroy() - if result: + try: + res.destroy() + except exception.ResourceFailure as ex: logger.error("Failed to remove %s : %s" % - (res.name, result)) + (res.name, str(ex))) raise exception.ResourceUpdateFailed( resource_name=res.name) else: @@ -408,10 +409,11 @@ class Stack(object): (res.name, self.name) + " update requires replacement") # Resource requires replacement for update - result = self[res.name].destroy() - if result: + try: + self[res.name].destroy() + except exception.ResourceFailure as ex: logger.error("Failed to delete %s : %s" % - (res.name, result)) + (res.name, str(ex))) raise exception.ResourceUpdateFailed( resource_name=res.name) else: @@ -497,10 +499,11 @@ class Stack(object): failures = [] for res in reversed(self): - result = res.destroy() - if result: + try: + res.destroy() + except exception.ResourceFailure as ex: logger.error('Failed to delete %s error: %s' % (str(res), - result)) + str(ex))) failures.append(str(res)) if failures: @@ -535,7 +538,7 @@ class Stack(object): for res in reversed(deps): try: res.destroy() - except Exception as ex: + except exception.ResourceFailure as ex: failed = True logger.error('delete: %s' % str(ex)) diff --git a/heat/engine/resource.py b/heat/engine/resource.py index bef28a85..f5ff8531 100644 --- a/heat/engine/resource.py +++ b/heat/engine/resource.py @@ -410,32 +410,37 @@ class Resource(object): if self.state == self.DELETE_COMPLETE: return if self.state == self.DELETE_IN_PROGRESS: - return 'Resource deletion already in progress' + raise exception.Error('Resource deletion already in progress') # No need to delete if the resource has never been created if self.state is None: return - logger.info('deleting %s (inst:%s db_id:%s)' % - (str(self), self.resource_id, str(self.id))) - self.state_set(self.DELETE_IN_PROGRESS) + logger.info('deleting %s' % str(self)) try: + self.state_set(self.DELETE_IN_PROGRESS) + if callable(getattr(self, 'handle_delete', None)): self.handle_delete() except Exception as ex: logger.exception('Delete %s', str(self)) - self.state_set(self.DELETE_FAILED, str(ex)) - return str(ex) or "Error : %s" % type(ex) - - self.state_set(self.DELETE_COMPLETE) + failure = exception.ResourceFailure(ex) + self.state_set(self.DELETE_FAILED, str(failure)) + raise failure + except: + with excutils.save_and_reraise_exception(): + try: + self.state_set(self.DELETE_FAILED, 'Deletion aborted') + except Exception: + logger.exception('Error marking resource deletion failed') + else: + self.state_set(self.DELETE_COMPLETE) def destroy(self): ''' Delete the resource and remove it from the database. ''' - result = self.delete() - if result: - return result + self.delete() if self.id is None: return @@ -446,9 +451,6 @@ class Resource(object): # Don't fail on delete if the db entry has # not been created yet. pass - except Exception as ex: - logger.exception('Delete %s from DB' % str(self)) - return str(ex) or "Error : %s" % type(ex) self.id = None diff --git a/heat/engine/resources/autoscaling.py b/heat/engine/resources/autoscaling.py index ed1ca5c5..a2233fd3 100644 --- a/heat/engine/resources/autoscaling.py +++ b/heat/engine/resources/autoscaling.py @@ -167,11 +167,7 @@ class InstanceGroup(resource.Resource): for victim in inst_list: logger.debug('handle_delete %s' % victim) inst = self._make_instance(victim) - error_str = inst.destroy() - if error_str is not None: - # try suck out the grouped resouces failure reason - # and re-raise - raise exception.NestedResourceFailure(message=error_str) + inst.destroy() def resize(self, new_capacity, raise_on_error=False): inst_list = [] diff --git a/heat/tests/test_parser.py b/heat/tests/test_parser.py index 8a349edd..3e154c22 100644 --- a/heat/tests/test_parser.py +++ b/heat/tests/test_parser.py @@ -470,9 +470,12 @@ class StackTest(unittest.TestCase): self.assertEqual(resource, self.stack.resource_by_refid('aaaa')) resource.state = resource.DELETE_IN_PROGRESS - self.assertEqual(None, self.stack.resource_by_refid('aaaa')) + try: + self.assertEqual(None, self.stack.resource_by_refid('aaaa')) - self.assertEqual(None, self.stack.resource_by_refid('bbbb')) + self.assertEqual(None, self.stack.resource_by_refid('bbbb')) + finally: + resource.state = resource.CREATE_COMPLETE @stack_delete_after def test_update_add(self): @@ -628,7 +631,8 @@ class StackTest(unittest.TestCase): # make the update fail deleting the existing resource self.m.StubOutWithMock(resource.Resource, 'destroy') - resource.Resource.destroy().AndReturn("Error") + exc = exception.ResourceFailure(Exception()) + resource.Resource.destroy().AndRaise(exc) self.m.ReplayAll() self.stack.update(updated_stack) @@ -839,16 +843,17 @@ class StackTest(unittest.TestCase): updated_stack = parser.Stack(self.ctx, 'updated_stack', template.Template(tmpl2)) - # patch in a dummy destroy making the delete fail - self.m.StubOutWithMock(resource.Resource, 'destroy') - resource.Resource.destroy().AndReturn('Error') + # patch in a dummy delete making the destroy fail + self.m.StubOutWithMock(resource.Resource, 'delete') + exc = exception.ResourceFailure(Exception()) + resource.Resource.delete().AndRaise(exc) self.m.ReplayAll() self.stack.update(updated_stack) self.assertEqual(self.stack.state, parser.Stack.ROLLBACK_COMPLETE) self.assertTrue('BResource' in self.stack) self.m.VerifyAll() - # Unset here so destroy() is not stubbed for stack.delete cleanup + # Unset here so delete() is not stubbed for stack.delete cleanup self.m.UnsetStubs() @stack_delete_after diff --git a/heat/tests/test_volume.py b/heat/tests/test_volume.py index 079d1e93..1a69a515 100644 --- a/heat/tests/test_volume.py +++ b/heat/tests/test_volume.py @@ -117,14 +117,13 @@ class VolumeTest(unittest.TestCase): self.assertEqual(resource.handle_update({}), vol.Volume.UPDATE_REPLACE) fv.status = 'in-use' - resource.state = 'CREATE_COMPLETE' - self.assertEqual(resource.delete(), 'Volume in use') + self.assertRaises(exception.ResourceFailure, resource.destroy) fv.status = 'available' - resource.state = 'CREATE_COMPLETE' - self.assertEqual(resource.delete(), None) - fv.status = 'available' - resource.state = 'CREATE_COMPLETE' - self.assertEqual(resource.delete(), None) + self.assertEqual(resource.destroy(), None) + + # Test when volume already deleted + resource.state = resource.CREATE_COMPLETE + self.assertEqual(resource.destroy(), None) self.m.VerifyAll()