From 600608c845e0df932ddc2a888e5de97a682d900f Mon Sep 17 00:00:00 2001 From: Pavlo Shchelokovskyy Date: Fri, 29 May 2015 18:13:40 +0300 Subject: [PATCH] Do not pass rich objects on servers' delete Partial-Bug: #1393268 Change-Id: I4034f48f75aad748ac313c385fa4f6b991cd4128 --- heat/engine/clients/os/nova.py | 51 ++++++++----------- heat/engine/resources/aws/ec2/instance.py | 17 +++---- .../engine/resources/openstack/nova/server.py | 15 +++--- heat/tests/aws/test_instance.py | 21 ++++++-- heat/tests/db/test_sqlalchemy_api.py | 9 ++-- heat/tests/engine/tools.py | 10 ++-- heat/tests/test_engine_service.py | 11 ++-- heat/tests/test_server.py | 8 +-- 8 files changed, 65 insertions(+), 77 deletions(-) diff --git a/heat/engine/clients/os/nova.py b/heat/engine/clients/os/nova.py index c1653409c9..d3a3af7665 100644 --- a/heat/engine/clients/os/nova.py +++ b/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): - ''' - Deletes a server and waits for it to disappear from Nova. - ''' - if not server: - return + def check_delete_server_complete(self, server_id): + """Wait for server to disappear from Nova.""" try: - server.delete() + server = self.fetch_server(server_id) except Exception as exc: self.ignore_not_found(exc) - return - - while True: - yield - - try: - self.refresh_server(server) - except Exception as exc: - self.ignore_not_found(exc) - break - else: - # Some clouds append extra (STATUS) strings to the status - short_server_status = server.status.split('(')[0] - if short_server_status in ("DELETED", "SOFT_DELETED"): - break - if short_server_status == "ERROR": - fault = getattr(server, 'fault', {}) - 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}) + return True + if not server: + return False + status = self.get_status(server) + if status in ("DELETED", "SOFT_DELETED"): + return True + if status == 'ERROR': + fault = getattr(server, 'fault', {}) + message = fault.get('message', 'Unknown') + code = fault.get('code') + errmsg = _("Server %(name)s delete failed: (%(code)s) " + "%(message)s") % dict(name=server.name, + code=code, + message=message) + raise resource.ResourceInError(resource_status=status, + status_reason=errmsg) + return False @scheduler.wrappertask def resize(self, server, flavor, flavor_id): diff --git a/heat/engine/resources/aws/ec2/instance.py b/heat/engine/resources/aws/ec2/instance.py index 4da83415a6..192e1416a5 100644 --- a/heat/engine/resources/aws/ec2/instance.py +++ b/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, - server) - deleter.start() - return deleter + return self.resource_id - def check_delete_complete(self, deleter): - # if the resource was already deleted, deleters will be None - if deleter: - if not deleter.step(): - return False - return True + def check_delete_complete(self, server_id): + if not server_id: + return True + return self.client_plugin().check_delete_server_complete(server_id) def handle_suspend(self): ''' diff --git a/heat/engine/resources/openstack/nova/server.py b/heat/engine/resources/openstack/nova/server.py index cd79358aa4..0abf120c77 100644 --- a/heat/engine/resources/openstack/nova/server.py +++ b/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: - deleter = scheduler.TaskRunner(self.client_plugin().delete_server, - server) - deleter.start() - return deleter + return + return self.resource_id - def check_delete_complete(self, deleter): - if deleter is None or deleter.step(): + def check_delete_complete(self, server_id): + if not server_id: return True - return False + return self.client_plugin().check_delete_server_complete(server_id) def handle_suspend(self): ''' diff --git a/heat/tests/aws/test_instance.py b/heat/tests/aws/test_instance.py index 957595cdb4..71b752e45d 100644 --- a/heat/tests/aws/test_instance.py +++ b/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): - self._test_instance_create_delete(vm_status='ERROR') + def test_instance_create_notfound_on_delete(self): + 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): - self._test_instance_create_delete( - vm_status='ERROR', vm_delete_status='DELETED') + # this makes sure the auto increment worked on instance creation + self.assertTrue(instance.id > 0) + + 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] diff --git a/heat/tests/db/test_sqlalchemy_api.py b/heat/tests/db/test_sqlalchemy_api.py index b9ab1d5fc2..469926ab19 100644 --- a/heat/tests/db/test_sqlalchemy_api.py +++ b/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') - instances.Instance.nova().MultipleTimes().AndReturn(fc) - mocks.StubOutWithMock(fc.client, 'get_servers_9999') - get = fc.client.get_servers_9999 - get().MultipleTimes().AndRaise(fakes_nova.fake_exception()) + mocks.StubOutWithMock(instances.Instance, 'client') + instances.Instance.client().MultipleTimes().AndReturn(fc) + self.patchobject(fc.servers, 'delete', + side_effect=fakes_nova.fake_exception()) @mock.patch.object(db_api, '_paginate_query') def test_filter_and_page_query_paginates_query(self, mock_paginate_query): diff --git a/heat/tests/engine/tools.py b/heat/tests/engine/tools.py index 1571fea526..4b03aaf3e6 100644 --- a/heat/tests/engine/tools.py +++ b/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') - instances.Instance.nova().MultipleTimes().AndReturn(fc) - m.StubOutWithMock(fc.client, 'get_servers_9999') - get = fc.client.get_servers_9999 - get().AndRaise(fakes_nova.fake_exception()) + m.StubOutWithMock(instances.Instance, 'client') + instances.Instance.client().MultipleTimes().AndReturn(fc) + m.StubOutWithMock(fc.servers, 'delete') + fc.servers.delete(mox.IgnoreArg()).AndRaise( + fakes_nova.fake_exception()) m.ReplayAll() stack.delete() if delete_res: diff --git a/heat/tests/test_engine_service.py b/heat/tests/test_engine_service.py index e1e2948491..497e74fec6 100644 --- a/heat/tests/test_engine_service.py +++ b/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') - get = fc.client.get_servers_9999 - get().AndRaise(fakes_nova.fake_exception()) - mox.Replay(get) + self.patchobject(fc.servers, 'delete', + side_effect=fakes_nova.fake_exception()) 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') - get = fc.client.get_servers_9999 - get().AndRaise(fakes_nova.fake_exception()) + self.patchobject(fc.servers, 'delete', + side_effect=fakes_nova.fake_exception()) self.m.ReplayAll() stack.store() diff --git a/heat/tests/test_server.py b/heat/tests/test_server.py index 5513523f0e..df975ee69a 100644 --- a/heat/tests/test_server.py +++ b/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') - get = self.fc.client.get_servers_1234 - get().AndRaise(fakes_nova.fake_exception()) - mox.Replay(get) + 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(server.delete)() self.assertEqual((server.DELETE, server.COMPLETE), server.state)