Do not pass rich objects on servers' delete

Partial-Bug: #1393268
Change-Id: I4034f48f75aad748ac313c385fa4f6b991cd4128

heat/engine/clients/os/nova.py

@@ -379,40 +379,29 @@ echo -e '%s\tALL=(ALL)\tNOPASSWD: ALL' >> /etc/sudoers
 
         return mime_blob.as_string()
 
-    def delete_server(self, server):
+    def check_delete_server_complete(self, server_id):
-        '''
+        """Wait for server to disappear from Nova."""
-        Deletes a server and waits for it to disappear from Nova.
-        '''
-        if not server:
-            return
         try:
-            server.delete()
+            server = self.fetch_server(server_id)
         except Exception as exc:
             self.ignore_not_found(exc)
-            return
+            return True
-
+        if not server:
-        while True:
+            return False
-            yield
+        status = self.get_status(server)
-
+        if status in ("DELETED", "SOFT_DELETED"):
-            try:
+            return True
-                self.refresh_server(server)
+        if status == 'ERROR':
-            except Exception as exc:
+            fault = getattr(server, 'fault', {})
-                self.ignore_not_found(exc)
+            message = fault.get('message', 'Unknown')
-                break
+            code = fault.get('code')
-            else:
+            errmsg = _("Server %(name)s delete failed: (%(code)s) "
-                # Some clouds append extra (STATUS) strings to the status
+                       "%(message)s") % dict(name=server.name,
-                short_server_status = server.status.split('(')[0]
+                                             code=code,
-                if short_server_status in ("DELETED", "SOFT_DELETED"):
+                                             message=message)
-                    break
+            raise resource.ResourceInError(resource_status=status,
-                if short_server_status == "ERROR":
+                                           status_reason=errmsg)
-                    fault = getattr(server, 'fault', {})
+        return False
-                    message = fault.get('message', 'Unknown')
-                    code = fault.get('code')
-                    errmsg = (_("Server %(name)s delete failed: (%(code)s) "
-                                "%(message)s"))
-                    raise exception.Error(errmsg % {"name": server.name,
-                                                    "code": code,
-                                                    "message": message})
 
     @scheduler.wrappertask
     def resize(self, server, flavor, flavor_id):

heat/engine/resources/aws/ec2/instance.py

@@ -765,21 +765,16 @@ class Instance(resource.Resource):
         if self.resource_id is None:
             return
         try:
-            server = self.nova().servers.get(self.resource_id)
+            self.client().servers.delete(self.resource_id)
         except Exception as e:
             self.client_plugin().ignore_not_found(e)
             return
-        deleter = scheduler.TaskRunner(self.client_plugin().delete_server,
+        return self.resource_id
-                                       server)
-        deleter.start()
-        return deleter
 
-    def check_delete_complete(self, deleter):
+    def check_delete_complete(self, server_id):
-        # if the resource was already deleted, deleters will be None
+        if not server_id:
-        if deleter:
+            return True
-            if not deleter.step():
+        return self.client_plugin().check_delete_server_complete(server_id)
-                return False
-        return True
 
     def handle_suspend(self):
         '''

heat/engine/resources/openstack/nova/server.py

@@ -1341,19 +1341,16 @@ class Server(stack_user.StackUser):
             self._delete_temp_url()
 
         try:
-            server = self.nova().servers.get(self.resource_id)
+            self.client().servers.delete(self.resource_id)
         except Exception as e:
             self.client_plugin().ignore_not_found(e)
-        else:
+            return
-            deleter = scheduler.TaskRunner(self.client_plugin().delete_server,
+        return self.resource_id
-                                           server)
-            deleter.start()
-            return deleter
 
-    def check_delete_complete(self, deleter):
+    def check_delete_complete(self, server_id):
-        if deleter is None or deleter.step():
+        if not server_id:
             return True
-        return False
+        return self.client_plugin().check_delete_server_complete(server_id)
 
     def handle_suspend(self):
         '''

heat/tests/aws/test_instance.py

@@ -623,12 +623,23 @@ class InstancesTest(common.HeatTestCase):
     def test_instance_create_delete(self):
         self._test_instance_create_delete(vm_delete_status='DELETED')
 
-    def test_instance_create_error_delete_notfound(self):
+    def test_instance_create_notfound_on_delete(self):
-        self._test_instance_create_delete(vm_status='ERROR')
+        return_server = self.fc.servers.list()[1]
+        instance = self._create_test_instance(return_server,
+                                              'in_cr_del')
+        instance.resource_id = '1234'
 
-    def test_instance_create_error_delete(self):
+        # this makes sure the auto increment worked on instance creation
-        self._test_instance_create_delete(
+        self.assertTrue(instance.id > 0)
-            vm_status='ERROR', vm_delete_status='DELETED')
+
+        self.m.StubOutWithMock(self.fc.client, 'delete_servers_1234')
+        self.fc.client.delete_servers_1234().AndRaise(
+            fakes_nova.fake_exception())
+        self.m.ReplayAll()
+
+        scheduler.TaskRunner(instance.delete)()
+        self.assertEqual((instance.DELETE, instance.COMPLETE), instance.state)
+        self.m.VerifyAll()
 
     def test_instance_update_metadata(self):
         return_server = self.fc.servers.list()[1]

heat/tests/db/test_sqlalchemy_api.py

@@ -133,11 +133,10 @@ class SqlAlchemyTest(common.HeatTestCase):
 
     def _mock_delete(self, mocks):
         fc = fakes_nova.FakeClient()
-        mocks.StubOutWithMock(instances.Instance, 'nova')
+        mocks.StubOutWithMock(instances.Instance, 'client')
-        instances.Instance.nova().MultipleTimes().AndReturn(fc)
+        instances.Instance.client().MultipleTimes().AndReturn(fc)
-        mocks.StubOutWithMock(fc.client, 'get_servers_9999')
+        self.patchobject(fc.servers, 'delete',
-        get = fc.client.get_servers_9999
+                         side_effect=fakes_nova.fake_exception())
-        get().MultipleTimes().AndRaise(fakes_nova.fake_exception())
 
     @mock.patch.object(db_api, '_paginate_query')
     def test_filter_and_page_query_paginates_query(self, mock_paginate_query):

heat/tests/engine/tools.py

@@ -172,11 +172,11 @@ def clean_up_stack(stack, delete_res=True):
     if delete_res:
         m = mox.Mox()
         fc = fakes_nova.FakeClient()
-        m.StubOutWithMock(instances.Instance, 'nova')
+        m.StubOutWithMock(instances.Instance, 'client')
-        instances.Instance.nova().MultipleTimes().AndReturn(fc)
+        instances.Instance.client().MultipleTimes().AndReturn(fc)
-        m.StubOutWithMock(fc.client, 'get_servers_9999')
+        m.StubOutWithMock(fc.servers, 'delete')
-        get = fc.client.get_servers_9999
+        fc.servers.delete(mox.IgnoreArg()).AndRaise(
-        get().AndRaise(fakes_nova.fake_exception())
+            fakes_nova.fake_exception())
         m.ReplayAll()
     stack.delete()
     if delete_res:

heat/tests/test_engine_service.py

@@ -553,10 +553,8 @@ class StackCreateTest(common.HeatTestCase):
         self.assertIsNotNone(stack['WebServer'])
         self.assertTrue(stack['WebServer'].resource_id > 0)
 
-        self.m.StubOutWithMock(fc.client, 'get_servers_9999')
+        self.patchobject(fc.servers, 'delete',
-        get = fc.client.get_servers_9999
+                         side_effect=fakes_nova.fake_exception())
-        get().AndRaise(fakes_nova.fake_exception())
-        mox.Replay(get)
         stack.delete()
 
         rsrc = stack['WebServer']
@@ -1489,9 +1487,8 @@ class StackServiceAuthorizeTest(common.HeatTestCase):
         stack = tools.get_stack(stack_name, self.ctx, user_policy_template)
         self.stack = stack
         fc = tools.setup_mocks(self.m, stack)
-        self.m.StubOutWithMock(fc.client, 'get_servers_9999')
+        self.patchobject(fc.servers, 'delete',
-        get = fc.client.get_servers_9999
+                         side_effect=fakes_nova.fake_exception())
-        get().AndRaise(fakes_nova.fake_exception())
 
         self.m.ReplayAll()
         stack.store()

heat/tests/test_server.py

@@ -1254,10 +1254,10 @@ class ServersTest(common.HeatTestCase):
         # this makes sure the auto increment worked on server creation
         self.assertTrue(server.id > 0)
 
-        self.m.StubOutWithMock(self.fc.client, 'get_servers_1234')
+        self.m.StubOutWithMock(self.fc.client, 'delete_servers_1234')
-        get = self.fc.client.get_servers_1234
+        self.fc.client.delete_servers_1234().AndRaise(
-        get().AndRaise(fakes_nova.fake_exception())
+            fakes_nova.fake_exception())
-        mox.Replay(get)
+        self.m.ReplayAll()
 
         scheduler.TaskRunner(server.delete)()
         self.assertEqual((server.DELETE, server.COMPLETE), server.state)