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

diff --git a/heat/common/exception.py b/heat/common/exception.py
index 97b8ea0ca454fe05df117b00125b937ddea6f498..419685e137c385f910fb7023a67069c9114fab3c 100644 (file)
--- 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 7e7089a2ce9dc83904915878f52e457701aed529..492491e608250d0ad6783d631ed77baab6a46378 100644 (file)
--- 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 b34a3c67597ea3ec9deffc8b6f0fa10169b385aa..67f85e8a21312e0717a9639a930884a7c9832a7f 100644 (file)
--- 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 08e316855bc64e191c2dce26310f197e427fc389..6f0df6851705987ec39cb896c1788a3bf88d3f02 100644 (file)
--- 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 5bb7af125ef5113b57af32993043961751995759..d69db9ec00422bc3df8d1600f810ca7a09e1b936 100644 (file)
--- 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):