Use stashed volume connector in _local_cleanup_bdm_volumes
When we perform a local delete in the compute API during the volumes
cleanup, Nova calls the volume_api.terminate_connection passing a fake
volume connector. That call is useless and it has no real effect on the
volume server side.
With a686185fc0 in mitaka we started
stashing the connector in the bdm.connection_info so this change
tries to use that if it's available, which it won't be for attachments
made before that change -or- for volumes attached to an instance in
shelved_offloaded state that were never unshelved (because the actual
volume attach for those instances happens on the compute manager after
you unshelve the instance).
If we can't find a stashed connector, or we find one whose host does
not match the instance.host, we punt and just don't call
terminate_connection at all since calling it with a fake connector
can actually make some cinder volume backends fail and then the
volume is orphaned.
Closes-Bug: #1609984
Co-Authored-By: Matt Riedemann <mriedem@us.ibm.com>
Change-Id: I9f9ead51238e27fa45084c8e3d3edee76a8b0218
This commit is contained in:
Andrea Rosa
Matt Riedemann
parent
d219e25452
commit
33510d4be9
|
|
@ -1728,24 +1728,53 @@ class API(base.Base):
|
|||
ram=-instance_memory_mb)
|
||||
return quotas
|
||||
|
||||
def _get_stashed_volume_connector(self, bdm, instance):
|
||||
"""Lookup a connector dict from the bdm.connection_info if set
|
||||
|
||||
Gets the stashed connector dict out of the bdm.connection_info if set
|
||||
and the connector host matches the instance host.
|
||||
|
||||
:param bdm: nova.objects.block_device.BlockDeviceMapping
|
||||
:param instance: nova.objects.instance.Instance
|
||||
:returns: volume connector dict or None
|
||||
"""
|
||||
if 'connection_info' in bdm and bdm.connection_info is not None:
|
||||
# NOTE(mriedem): We didn't start stashing the connector in the
|
||||
# bdm.connection_info until Mitaka so it might not be there on old
|
||||
# attachments. Also, if the volume was attached when the instance
|
||||
# was in shelved_offloaded state and it hasn't been unshelved yet
|
||||
# we don't have the attachment/connection information either.
|
||||
connector = jsonutils.loads(bdm.connection_info).get('connector')
|
||||
if connector:
|
||||
if connector.get('host') == instance.host:
|
||||
return connector
|
||||
LOG.debug('Found stashed volume connector for instance but '
|
||||
'connector host %(connector_host)s does not match '
|
||||
'the instance host %(instance_host)s.',
|
||||
{'connector_host': connector.get('host'),
|
||||
'instance_host': instance.host}, instance=instance)
|
||||
|
||||
def _local_cleanup_bdm_volumes(self, bdms, instance, context):
|
||||
"""The method deletes the bdm records and, if a bdm is a volume, call
|
||||
the terminate connection and the detach volume via the Volume API.
|
||||
Note that at this point we do not have the information about the
|
||||
correct connector so we pass a fake one.
|
||||
"""
|
||||
elevated = context.elevated()
|
||||
for bdm in bdms:
|
||||
if bdm.is_volume:
|
||||
# NOTE(vish): We don't have access to correct volume
|
||||
# connector info, so just pass a fake
|
||||
# connector. This can be improved when we
|
||||
# expose get_volume_connector to rpc.
|
||||
connector = {'ip': '127.0.0.1', 'initiator': 'iqn.fake'}
|
||||
try:
|
||||
self.volume_api.terminate_connection(context,
|
||||
bdm.volume_id,
|
||||
connector)
|
||||
connector = self._get_stashed_volume_connector(
|
||||
bdm, instance)
|
||||
if connector:
|
||||
self.volume_api.terminate_connection(context,
|
||||
bdm.volume_id,
|
||||
connector)
|
||||
else:
|
||||
LOG.debug('Unable to find connector for volume %s, '
|
||||
'not attempting terminate_connection.',
|
||||
bdm.volume_id, instance=instance)
|
||||
# Attempt to detach the volume. If there was no connection
|
||||
# made in the first place this is just cleaning up the
|
||||
# volume state in the Cinder database.
|
||||
self.volume_api.detach(elevated, bdm.volume_id,
|
||||
instance.uuid)
|
||||
if bdm.delete_on_termination:
|
||||
|
|
|
|||
|
|
@ -1236,7 +1236,7 @@ class _ComputeAPIUnitTestMixIn(object):
|
|||
self.mox.StubOutWithMock(compute_utils,
|
||||
'notify_about_instance_usage')
|
||||
self.mox.StubOutWithMock(self.compute_api.volume_api,
|
||||
'terminate_connection')
|
||||
'detach')
|
||||
self.mox.StubOutWithMock(objects.BlockDeviceMapping, 'destroy')
|
||||
|
||||
compute_utils.notify_about_instance_usage(
|
||||
|
|
@ -1247,9 +1247,9 @@ class _ComputeAPIUnitTestMixIn(object):
|
|||
self.compute_api.network_api.deallocate_for_instance(
|
||||
self.context, inst)
|
||||
|
||||
self.compute_api.volume_api.terminate_connection(
|
||||
mox.IgnoreArg(), mox.IgnoreArg(), mox.IgnoreArg()).\
|
||||
AndRaise(exception. VolumeNotFound('volume_id'))
|
||||
self.compute_api.volume_api.detach(
|
||||
mox.IgnoreArg(), 'volume_id', inst.uuid).\
|
||||
AndRaise(exception.VolumeNotFound('volume_id'))
|
||||
bdms[0].destroy()
|
||||
|
||||
inst.destroy()
|
||||
|
|
@ -1263,6 +1263,68 @@ class _ComputeAPIUnitTestMixIn(object):
|
|||
'delete',
|
||||
self._fake_do_delete)
|
||||
|
||||
@mock.patch.object(objects.BlockDeviceMapping, 'destroy')
|
||||
def test_local_cleanup_bdm_volumes_stashed_connector(self, mock_destroy):
|
||||
"""Tests that we call volume_api.terminate_connection when we found
|
||||
a stashed connector in the bdm.connection_info dict.
|
||||
"""
|
||||
inst = self._create_instance_obj()
|
||||
# create two fake bdms, one is a volume and one isn't, both will be
|
||||
# destroyed but we only cleanup the volume bdm in cinder
|
||||
conn_info = {'connector': {'host': inst.host}}
|
||||
vol_bdm = objects.BlockDeviceMapping(self.context, id=1,
|
||||
instance_uuid=inst.uuid,
|
||||
volume_id=uuids.volume_id,
|
||||
source_type='volume',
|
||||
destination_type='volume',
|
||||
delete_on_termination=True,
|
||||
connection_info=jsonutils.dumps(
|
||||
conn_info
|
||||
))
|
||||
loc_bdm = objects.BlockDeviceMapping(self.context, id=2,
|
||||
instance_uuid=inst.uuid,
|
||||
volume_id=uuids.volume_id2,
|
||||
source_type='blank',
|
||||
destination_type='local')
|
||||
bdms = objects.BlockDeviceMappingList(objects=[vol_bdm, loc_bdm])
|
||||
|
||||
@mock.patch.object(self.compute_api.volume_api, 'terminate_connection')
|
||||
@mock.patch.object(self.compute_api.volume_api, 'detach')
|
||||
@mock.patch.object(self.compute_api.volume_api, 'delete')
|
||||
@mock.patch.object(self.context, 'elevated', return_value=self.context)
|
||||
def do_test(self, mock_elevated, mock_delete,
|
||||
mock_detach, mock_terminate):
|
||||
self.compute_api._local_cleanup_bdm_volumes(
|
||||
bdms, inst, self.context)
|
||||
mock_terminate.assert_called_once_with(
|
||||
self.context, uuids.volume_id, conn_info['connector'])
|
||||
mock_detach.assert_called_once_with(
|
||||
self.context, uuids.volume_id, inst.uuid)
|
||||
mock_delete.assert_called_once_with(self.context, uuids.volume_id)
|
||||
self.assertEqual(2, mock_destroy.call_count)
|
||||
|
||||
do_test(self)
|
||||
|
||||
def test_get_stashed_volume_connector_none(self):
|
||||
inst = self._create_instance_obj()
|
||||
# connection_info isn't set
|
||||
bdm = objects.BlockDeviceMapping(self.context)
|
||||
self.assertIsNone(
|
||||
self.compute_api._get_stashed_volume_connector(bdm, inst))
|
||||
# connection_info is None
|
||||
bdm.connection_info = None
|
||||
self.assertIsNone(
|
||||
self.compute_api._get_stashed_volume_connector(bdm, inst))
|
||||
# connector is not set in connection_info
|
||||
bdm.connection_info = jsonutils.dumps({})
|
||||
self.assertIsNone(
|
||||
self.compute_api._get_stashed_volume_connector(bdm, inst))
|
||||
# connector is set but different host
|
||||
conn_info = {'connector': {'host': 'other_host'}}
|
||||
bdm.connection_info = jsonutils.dumps(conn_info)
|
||||
self.assertIsNone(
|
||||
self.compute_api._get_stashed_volume_connector(bdm, inst))
|
||||
|
||||
def test_local_delete_without_info_cache(self):
|
||||
inst = self._create_instance_obj()
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue