Revert "Detach volume after deleting instance with no host"

This reverts commit d1baa9fe7e

The change introduced a race on delete where there isn't a host while
prepping block devices and we hit a NoneType error in nova-compute.

There was also a recheck grind on the original change showing it wasn't
safe and needs to be reworked.

Change-Id: Id4e405e7579530ed1c1f22ccc972d45b6d185f41
Closes-Bug: #1456771
Related-Bug: #1448316
This commit is contained in:
Matt Riedemann 2015-05-20 17:36:21 +00:00
parent d1baa9fe7e
commit 1ba7208171
5 changed files with 112 additions and 201 deletions

View File

@ -1554,49 +1554,63 @@ class API(base.Base):
cb(context, instance, bdms, reservations=None)
quotas.commit()
return
shelved_offloaded = (instance.vm_state
== vm_states.SHELVED_OFFLOADED)
if not instance.host and not shelved_offloaded:
try:
compute_utils.notify_about_instance_usage(
self.notifier, context, instance,
"%s.start" % delete_type)
instance.destroy()
compute_utils.notify_about_instance_usage(
self.notifier, context, instance,
"%s.end" % delete_type,
system_metadata=instance.system_metadata)
quotas.commit()
return
except exception.ObjectActionError:
instance.refresh()
if instance.vm_state == vm_states.RESIZED:
self._confirm_resize_on_deleting(context, instance)
if (not instance.host
or instance.vm_state == vm_states.SHELVED_OFFLOADED):
is_local_delete = True
else:
try:
is_local_delete = True
try:
if not shelved_offloaded:
service = objects.Service.get_by_compute_host(
context.elevated(), instance.host)
is_local_delete = not self.servicegroup_api.service_is_up(
service)
except exception.ComputeHostNotFound:
is_local_delete = True
if not is_local_delete:
if original_task_state in (task_states.DELETING,
task_states.SOFT_DELETING):
LOG.info(_LI('Instance is already in deleting state, '
'ignoring this request'),
instance=instance)
quotas.rollback()
return
self._record_action_start(context, instance,
instance_actions.DELETE)
# NOTE(snikitin): If instance's vm_state is 'soft-delete',
# we should not count reservations here, because instance
# in soft-delete vm_state have already had quotas
# decremented. More details:
# https://bugs.launchpad.net/nova/+bug/1333145
if instance.vm_state == vm_states.SOFT_DELETED:
quotas.rollback()
cb(context, instance, bdms,
reservations=quotas.reservations)
except exception.ComputeHostNotFound:
pass
if is_local_delete:
# If instance is in shelved_offloaded state or compute node
# isn't up or is unknown i.e. instance.host is None, delete
# instance from db and clean bdms info and network info
# isn't up, delete instance from db and clean bdms info and
# network info
self._local_delete(context, instance, bdms, delete_type, cb)
quotas.commit()
else:
if original_task_state in (task_states.DELETING,
task_states.SOFT_DELETING):
LOG.info(_LI('Instance is already in deleting state, '
'ignoring this request'),
instance=instance)
quotas.rollback()
return
self._record_action_start(context, instance,
instance_actions.DELETE)
# NOTE(snikitin): If instance's vm_state is 'soft-delete',
# we should not count reservations here, because instance
# in soft-delete vm_state have already had quotas
# decremented. More details:
# https://bugs.launchpad.net/nova/+bug/1333145
if instance.vm_state == vm_states.SOFT_DELETED:
quotas.rollback()
cb(context, instance, bdms,
reservations=quotas.reservations)
except exception.InstanceNotFound:
# NOTE(comstud): Race condition. Instance already gone.

View File

@ -26,6 +26,7 @@ import mock
from mox3 import mox
from oslo_config import cfg
from oslo_serialization import jsonutils
from oslo_utils import timeutils
import six.moves.urllib.parse as urlparse
import testtools
import webob
@ -1322,10 +1323,12 @@ class ServersControllerDeleteTest(ControllerTest):
super(ServersControllerDeleteTest, self).setUp()
self.server_delete_called = False
def mock_local_delete(*args, **kwargs):
def instance_destroy_mock(*args, **kwargs):
self.server_delete_called = True
deleted_at = timeutils.utcnow()
return fake_instance.fake_db_instance(deleted_at=deleted_at)
self.stubs.Set(compute_api.API, '_local_delete', mock_local_delete)
self.stubs.Set(db, 'instance_destroy', instance_destroy_mock)
def _create_delete_request(self, uuid):
fakes.stub_out_instance_quota(self.stubs, 0, 10)
@ -1333,44 +1336,22 @@ class ServersControllerDeleteTest(ControllerTest):
req.method = 'DELETE'
return req
@mock.patch.object(objects.Instance, 'save')
@mock.patch.object(objects.Service, 'get_by_compute_host')
@mock.patch('nova.servicegroup.API.service_is_up')
@mock.patch.object(compute_api.compute_rpcapi.ComputeAPI,
'terminate_instance')
def test_delete_server_instance(self, mock_terminate_instance,
mock_up, mock_service, mock_save):
"""Test delete instance which is in active state
In this case delete api does not call _local_delete and makes
an rpc call to terminate_instance.
"""
req = self._create_delete_request(FAKE_UUID)
def _delete_server_instance(self, uuid=FAKE_UUID):
req = self._create_delete_request(uuid)
self.stubs.Set(db, 'instance_get_by_uuid',
fakes.fake_instance_get(vm_state=vm_states.ACTIVE,
host='fake-host'))
self.controller.delete(req, FAKE_UUID)
self.assertFalse(self.server_delete_called)
self.assertTrue(mock_terminate_instance.called)
fakes.fake_instance_get(vm_state=vm_states.ACTIVE))
self.controller.delete(req, uuid)
def test_delete_server_instance(self):
self._delete_server_instance()
self.assertTrue(self.server_delete_called)
def test_delete_server_instance_not_found(self):
"""Test delete instance which does not exist
self.assertRaises(webob.exc.HTTPNotFound,
self._delete_server_instance,
uuid='non-existent-uuid')
In this case delete will raise exception before calling compute
api's delete method.
"""
req = self._create_delete_request('non-existent-uuid')
self.assertRaises(webob.exc.HTTPNotFound, self.controller.delete,
req, 'non-existent-uuid')
@mock.patch.object(objects.Instance, 'save')
def test_delete_server_instance_while_building(self, mock_save):
"""Test delete instance when it is in building state
In this case instance.host will not be assigned, so delete api will
delete the server using _local_delete instead of making rpc call to
terminate_instance.
"""
def test_delete_server_instance_while_building(self):
req = self._create_delete_request(FAKE_UUID)
self.controller.delete(req, FAKE_UUID)
@ -1386,42 +1367,37 @@ class ServersControllerDeleteTest(ControllerTest):
self.assertRaises(webob.exc.HTTPConflict, self.controller.delete,
req, FAKE_UUID)
@mock.patch.object(objects.Instance, 'save')
@mock.patch.object(objects.Service, 'get_by_compute_host')
@mock.patch('nova.servicegroup.API.service_is_up')
@mock.patch.object(compute_api.compute_rpcapi.ComputeAPI,
'terminate_instance')
def test_delete_server_instance_while_resize(self, mock_terminate_instance,
mock_up, mock_service,
mock_save):
"""Test delete instance while resizing
In this case instance.host is set to either source or destination so
delete api will make an rpc call to terminate_instance.
"""
def test_delete_server_instance_while_resize(self):
req = self._create_delete_request(FAKE_UUID)
self.stubs.Set(db, 'instance_get_by_uuid',
fakes.fake_instance_get(vm_state=vm_states.ACTIVE,
task_state=task_states.RESIZE_PREP,
host='fake-host'))
task_state=task_states.RESIZE_PREP))
self.controller.delete(req, FAKE_UUID)
# Delete should be allowed in any case, even during resizing,
# Delete shoud be allowed in any case, even during resizing,
# because it may get stuck.
self.assertTrue(mock_terminate_instance.called)
self.assertTrue(self.server_delete_called)
def test_delete_server_instance_if_not_launched(self):
self.flags(reclaim_instance_interval=3600)
req = fakes.HTTPRequestV3.blank('/servers/%s' % FAKE_UUID)
req.method = 'DELETE'
self.server_delete_called = False
self.stubs.Set(db, 'instance_get_by_uuid',
fakes.fake_instance_get(launched_at=None))
def instance_destroy_mock(*args, **kwargs):
self.server_delete_called = True
deleted_at = timeutils.utcnow()
return fake_instance.fake_db_instance(deleted_at=deleted_at)
self.stubs.Set(db, 'instance_destroy', instance_destroy_mock)
self.controller.delete(req, FAKE_UUID)
# delete() should be called for instance which has never been active,
# even if reclaim_instance_interval has been set.
self.assertTrue(self.server_delete_called)
self.assertEqual(self.server_delete_called, True)
class ServersControllerRebuildInstanceTest(ControllerTest):

View File

@ -1488,10 +1488,12 @@ class ServersControllerDeleteTest(ControllerTest):
super(ServersControllerDeleteTest, self).setUp()
self.server_delete_called = False
def mock_local_delete(*args, **kwargs):
def instance_destroy_mock(*args, **kwargs):
self.server_delete_called = True
deleted_at = timeutils.utcnow()
return fake_instance.fake_db_instance(deleted_at=deleted_at)
self.stubs.Set(compute_api.API, '_local_delete', mock_local_delete)
self.stubs.Set(db, 'instance_destroy', instance_destroy_mock)
def _create_delete_request(self, uuid):
fakes.stub_out_instance_quota(self.stubs, 0, 10)
@ -1499,35 +1501,20 @@ class ServersControllerDeleteTest(ControllerTest):
req.method = 'DELETE'
return req
@mock.patch.object(objects.Instance, 'save')
@mock.patch.object(objects.Service, 'get_by_compute_host')
@mock.patch('nova.servicegroup.API.service_is_up')
@mock.patch.object(compute_api.compute_rpcapi.ComputeAPI,
'terminate_instance')
def test_delete_server_instance(self, mock_terminate_instance,
mock_up, mock_service, mock_save):
"""Test delete instance which is in active state
In this case delete api does not call _local_delete and makes
an rpc call to terminate_instance.
"""
req = self._create_delete_request(FAKE_UUID)
def _delete_server_instance(self, uuid=FAKE_UUID):
req = self._create_delete_request(uuid)
self.stubs.Set(db, 'instance_get_by_uuid',
fakes.fake_instance_get(vm_state=vm_states.ACTIVE,
host='fake-host'))
self.controller.delete(req, FAKE_UUID)
self.assertFalse(self.server_delete_called)
self.assertTrue(mock_terminate_instance.called)
fakes.fake_instance_get(vm_state=vm_states.ACTIVE))
self.controller.delete(req, uuid)
def test_delete_server_instance(self):
self._delete_server_instance()
self.assertTrue(self.server_delete_called)
def test_delete_server_instance_not_found(self):
"""Test delete instance which does not exist
In this case delete will raise exception before calling compute
api's delete method.
"""
req = self._create_delete_request('non-existent-uuid')
self.assertRaises(webob.exc.HTTPNotFound, self.controller.delete,
req, 'non-existent-uuid')
self.assertRaises(webob.exc.HTTPNotFound,
self._delete_server_instance,
uuid='non-existent-uuid')
def test_delete_locked_server(self):
req = self._create_delete_request(FAKE_UUID)
@ -1539,16 +1526,10 @@ class ServersControllerDeleteTest(ControllerTest):
self.assertRaises(webob.exc.HTTPConflict, self.controller.delete,
req, FAKE_UUID)
@mock.patch.object(objects.Instance, 'save')
def test_delete_server_instance_while_building(self, mock_save):
"""Test delete instance when it is in building state
In this case instance.host will not be assigned, so delete api will
delete the server using _local_delete instead of making rpc call to
terminate_instance.
"""
req = self._create_delete_request(FAKE_UUID)
self.controller.delete(req, FAKE_UUID)
def test_delete_server_instance_while_building(self):
fakes.stub_out_instance_quota(self.stubs, 0, 10)
request = self._create_delete_request(FAKE_UUID)
self.controller.delete(request, FAKE_UUID)
self.assertTrue(self.server_delete_called)
@ -1593,42 +1574,37 @@ class ServersControllerDeleteTest(ControllerTest):
# but since the host is down, api should remove the instance anyway.
self.assertTrue(self.server_delete_called)
@mock.patch.object(objects.Instance, 'save')
@mock.patch.object(objects.Service, 'get_by_compute_host')
@mock.patch('nova.servicegroup.API.service_is_up')
@mock.patch.object(compute_api.compute_rpcapi.ComputeAPI,
'terminate_instance')
def test_delete_server_instance_while_resize(self, mock_terminate_instance,
mock_up, mock_service,
mock_save):
"""Test delete instance while resizing
In this case instance.host is set to either source or destination so
delete api will make an rpc call to terminate_instance.
"""
def test_delete_server_instance_while_resize(self):
req = self._create_delete_request(FAKE_UUID)
self.stubs.Set(db, 'instance_get_by_uuid',
fakes.fake_instance_get(vm_state=vm_states.ACTIVE,
task_state=task_states.RESIZE_PREP,
host='fake-host'))
fakes.fake_instance_get(vm_state=vm_states.ACTIVE,
task_state=task_states.RESIZE_PREP))
self.controller.delete(req, FAKE_UUID)
# Delete should be allowed in any case, even during resizing,
# Delete shoud be allowed in any case, even during resizing,
# because it may get stuck.
self.assertTrue(mock_terminate_instance.called)
self.assertTrue(self.server_delete_called)
def test_delete_server_instance_if_not_launched(self):
self.flags(reclaim_instance_interval=3600)
req = fakes.HTTPRequest.blank('/fake/servers/%s' % FAKE_UUID)
req.method = 'DELETE'
self.server_delete_called = False
self.stubs.Set(db, 'instance_get_by_uuid',
fakes.fake_instance_get(launched_at=None))
def instance_destroy_mock(*args, **kwargs):
self.server_delete_called = True
deleted_at = timeutils.utcnow()
return fake_instance.fake_db_instance(deleted_at=deleted_at)
self.stubs.Set(db, 'instance_destroy', instance_destroy_mock)
self.controller.delete(req, FAKE_UUID)
# delete() should be called for instance which has never been active,
# even if reclaim_instance_interval has been set.
self.assertTrue(self.server_delete_called)
self.assertEqual(self.server_delete_called, True)
class ServersControllerRebuildInstanceTest(ControllerTest):

View File

@ -10585,8 +10585,7 @@ class ComputePolicyTestCase(BaseTestCase):
rules = {"compute:delete": []}
self.policy.set_rules(rules)
with mock.patch.object(self.compute_api, '_local_delete'):
self.compute_api.delete(self.context, instance)
self.compute_api.delete(self.context, instance)
def test_create_fail(self):
rules = {"compute:create": [["false:false"]]}

View File

@ -950,8 +950,7 @@ class _ComputeAPIUnitTestMixIn(object):
system_metadata=fake_sys_meta)
self._test_delete('force_delete', vm_state=vm_state)
@mock.patch.object(objects.InstanceInfoCache, 'delete')
def test_delete_fast_if_host_not_set(self, mock_delete):
def test_delete_fast_if_host_not_set(self):
inst = self._create_instance_obj()
inst.host = ''
quotas = quotas_obj.Quotas(self.context)
@ -995,7 +994,6 @@ class _ComputeAPIUnitTestMixIn(object):
tzinfo=iso8601.iso8601.Utc())
updates['deleted_at'] = delete_time
updates['deleted'] = True
inst.save()
fake_inst = fake_instance.fake_db_instance(**updates)
db.instance_destroy(self.context, inst.uuid,
constraint='constraint').AndReturn(fake_inst)
@ -1006,11 +1004,9 @@ class _ComputeAPIUnitTestMixIn(object):
self.mox.ReplayAll()
with mock.patch.object(self.compute_api.network_api,
'deallocate_for_instance'):
self.compute_api.delete(self.context, inst)
for k, v in updates.items():
self.assertEqual(inst[k], v)
self.compute_api.delete(self.context, inst)
for k, v in updates.items():
self.assertEqual(inst[k], v)
def test_local_delete_with_deleted_volume(self):
bdms = [objects.BlockDeviceMapping(
@ -2790,56 +2786,6 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase):
self.assertRaises(exception.CannotResizeToSameFlavor,
self._test_resize, same_flavor=True)
@mock.patch.object(objects.BlockDeviceMappingList, 'get_by_instance_uuid')
@mock.patch('nova.context.RequestContext.elevated')
@mock.patch.object(objects.Instance, 'save')
@mock.patch.object(quota.QUOTAS, 'reserve')
@mock.patch.object(objects.InstanceInfoCache, 'delete')
@mock.patch.object(compute_utils, 'notify_about_instance_usage')
@mock.patch.object(objects.BlockDeviceMapping, 'destroy')
@mock.patch.object(objects.Instance, 'destroy')
def test_delete_volume_backed_instance_in_error_state(
self, mock_instance_destroy, bdm_destroy,
notify_about_instance_usage, mock_delete,
mock_reserve, mock_save, mock_elevated,
bdm_get_by_instance_uuid):
volume_id = uuidutils.generate_uuid()
bdms = [objects.BlockDeviceMapping(
**fake_block_device.FakeDbBlockDeviceDict(
{'id': 42, 'volume_id': volume_id,
'source_type': 'volume', 'destination_type': 'volume',
'delete_on_termination': False}))]
reservations = ['fake-resv']
delete_time = datetime.datetime(1955, 11, 5, 9, 30,
tzinfo=iso8601.iso8601.Utc())
updates = {'deleted_at': delete_time,
'deleted': True}
fake_inst = fake_instance.fake_instance_obj(self.context, **updates)
mock_instance_destroy.return_value = fake_inst
bdm_get_by_instance_uuid.return_value = bdms
mock_reserve.return_value = reservations
mock_elevated.return_value = self.context
params = {'host': '', 'vm_state': vm_states.ERROR}
inst = self._create_instance_obj(params=params)
inst._context = self.context
connector = {'ip': '127.0.0.1', 'initiator': 'iqn.fake'}
with mock.patch.object(self.compute_api.network_api,
'deallocate_for_instance') as mock_deallocate, \
mock.patch.object(self.compute_api.volume_api,
'terminate_connection') as mock_terminate_conn, \
mock.patch.object(self.compute_api.volume_api,
'detach') as mock_detach:
self.compute_api.delete(self.context, inst)
mock_deallocate.assert_called_once_with(self.context, inst)
mock_detach.assert_called_once_with(self.context, volume_id)
mock_terminate_conn.assert_called_once_with(self.context,
volume_id, connector)
bdm_destroy.assert_called_once_with()
class ComputeAPIAPICellUnitTestCase(_ComputeAPIUnitTestMixIn,
test.NoDBTestCase):