Add support for OS-Brick force disconnect
OS-Brick disconnect has a force parameter since version 1.15.1 that can be used to make sure a disconnect call gives more priority to cleaning up everything than preserving data. This patch adds support for this new force parameter using it in calls where we know it's OK to lose data when we encounter an error as long as we report that we have failed. Change-Id: Id6a7f4cf264c540473269104daf20f39891b5a1b
This commit is contained in:
Gorka Eguileor
parent
7c8901e764
commit
2d965f6cfd
|
|
@ -382,7 +382,8 @@ class BackupManager(manager.ThreadPoolManager):
|
|||
finally:
|
||||
self._detach_device(context, attach_info,
|
||||
backup_device.device_obj, properties,
|
||||
backup_device.is_snapshot)
|
||||
backup_device.is_snapshot, force=True,
|
||||
ignore_errors=True)
|
||||
finally:
|
||||
backup = objects.Backup.get_by_id(context, backup.id)
|
||||
self._cleanup_temp_volumes_snapshots_when_backup_created(
|
||||
|
|
@ -487,7 +488,8 @@ class BackupManager(manager.ThreadPoolManager):
|
|||
else:
|
||||
backup_service.restore(backup, volume.id, device_path)
|
||||
finally:
|
||||
self._detach_device(context, attach_info, volume, properties)
|
||||
self._detach_device(context, attach_info, volume, properties,
|
||||
force=True)
|
||||
|
||||
def delete_backup(self, context, backup):
|
||||
"""Delete volume backup from configured backup service."""
|
||||
|
|
@ -894,11 +896,13 @@ class BackupManager(manager.ThreadPoolManager):
|
|||
return {'conn': conn, 'device': vol_handle, 'connector': connector}
|
||||
|
||||
def _detach_device(self, ctxt, attach_info, device,
|
||||
properties, is_snapshot=False, force=False):
|
||||
properties, is_snapshot=False, force=False,
|
||||
ignore_errors=False):
|
||||
"""Disconnect the volume or snapshot from the host. """
|
||||
connector = attach_info['connector']
|
||||
connector.disconnect_volume(attach_info['conn']['data'],
|
||||
attach_info['device'])
|
||||
attach_info['device'],
|
||||
force=force, ignore_errors=ignore_errors)
|
||||
rpcapi = self.volume_rpcapi
|
||||
if not is_snapshot:
|
||||
rpcapi.terminate_connection(ctxt, device, properties,
|
||||
|
|
|
|||
|
|
@ -623,7 +623,9 @@ class BackupTestCase(BaseBackupTest):
|
|||
mock_get_backup_device.assert_called_once_with(self.ctxt, backup, vol)
|
||||
mock_get_conn.assert_called_once_with()
|
||||
mock_detach_device.assert_called_once_with(self.ctxt, attach_info,
|
||||
vol, properties, False)
|
||||
vol, properties, False,
|
||||
force=True,
|
||||
ignore_errors=True)
|
||||
|
||||
vol = objects.Volume.get_by_id(self.ctxt, vol_id)
|
||||
self.assertEqual('available', vol['status'])
|
||||
|
|
@ -738,7 +740,7 @@ class BackupTestCase(BaseBackupTest):
|
|||
mock_get_backup_device.assert_called_once_with(self.ctxt, backup, vol)
|
||||
mock_get_conn.assert_called_once_with()
|
||||
mock_terminate_connection_snapshot.assert_called_once_with(
|
||||
self.ctxt, snap, properties, force=False)
|
||||
self.ctxt, snap, properties, force=True)
|
||||
vol = objects.Volume.get_by_id(self.ctxt, vol_id)
|
||||
self.assertEqual('in-use', vol['status'])
|
||||
self.assertEqual('backing-up', vol['previous_status'])
|
||||
|
|
@ -927,7 +929,7 @@ class BackupTestCase(BaseBackupTest):
|
|||
mock_attach_device.assert_called_once_with(self.ctxt, vol,
|
||||
properties)
|
||||
mock_detach_device.assert_called_once_with(self.ctxt, attach_info,
|
||||
vol, properties)
|
||||
vol, properties, force=True)
|
||||
|
||||
vol = objects.Volume.get_by_id(self.ctxt, vol_id)
|
||||
self.assertEqual('available', vol['status'])
|
||||
|
|
|
|||
|
|
@ -304,10 +304,10 @@ class GenericVolumeDriverTestCase(BaseDriverTestCase):
|
|||
|
||||
detach_expected = [
|
||||
mock.call(self.context, {'device': {'path': 'bar'}},
|
||||
dest_vol, {}, force=False, remote=False,
|
||||
dest_vol, {}, force=True, remote=False,
|
||||
attach_encryptor=encryption_changed),
|
||||
mock.call(self.context, {'device': {'path': 'foo'}},
|
||||
src_vol, {}, force=False, remote=False,
|
||||
src_vol, {}, force=True, remote=False,
|
||||
attach_encryptor=encryption_changed)]
|
||||
|
||||
attach_volume_returns = [
|
||||
|
|
@ -387,7 +387,7 @@ class GenericVolumeDriverTestCase(BaseDriverTestCase):
|
|||
mock_detach_encryptor.assert_called_once_with(
|
||||
attach_info, encryption)
|
||||
mock_detach_volume.assert_called_once_with(
|
||||
self.context, attach_info, volume, properties)
|
||||
self.context, attach_info, volume, properties, force=True)
|
||||
|
||||
@mock.patch.object(os_brick.initiator.connector,
|
||||
'get_connector_properties')
|
||||
|
|
@ -435,7 +435,7 @@ class GenericVolumeDriverTestCase(BaseDriverTestCase):
|
|||
self.assertFalse(mock_fetch_to_raw.called)
|
||||
self.assertFalse(mock_detach_encryptor.called)
|
||||
mock_detach_volume.assert_called_once_with(
|
||||
self.context, attach_info, volume, properties)
|
||||
self.context, attach_info, volume, properties, force=True)
|
||||
|
||||
@mock.patch.object(os_brick.initiator.connector,
|
||||
'get_connector_properties')
|
||||
|
|
@ -485,7 +485,42 @@ class GenericVolumeDriverTestCase(BaseDriverTestCase):
|
|||
mock_detach_encryptor.assert_called_once_with(
|
||||
attach_info, encryption)
|
||||
mock_detach_volume.assert_called_once_with(
|
||||
self.context, attach_info, volume, properties)
|
||||
self.context, attach_info, volume, properties, force=True)
|
||||
|
||||
@mock.patch('cinder.volume.driver.brick_exception')
|
||||
@mock.patch('cinder.tests.fake_driver.FakeLoggingVolumeDriver.'
|
||||
'terminate_connection', side_effect=Exception)
|
||||
@mock.patch('cinder.tests.fake_driver.FakeLoggingVolumeDriver.'
|
||||
'remove_export', side_effect=Exception)
|
||||
def test_detach_volume_force(self, remove_mock, terminate_mock, exc_mock):
|
||||
"""Test force parameter on _detach_volume.
|
||||
|
||||
On the driver if we receive the force parameter we will do everything
|
||||
even with Exceptions on disconnect, terminate, and remove export.
|
||||
"""
|
||||
connector = mock.Mock()
|
||||
connector.disconnect_volume.side_effect = Exception
|
||||
# TODO(geguileo): Remove this ExceptionChainer simulation once we
|
||||
# release OS-Brick version with it and bump min version.
|
||||
exc = exc_mock.ExceptionChainer.return_value
|
||||
exc.context.return_value.__enter__.return_value = exc
|
||||
exc.context.return_value.__exit__.return_value = True
|
||||
|
||||
volume = {'id': fake.VOLUME_ID}
|
||||
attach_info = {'device': {},
|
||||
'connector': connector,
|
||||
'conn': {'data': {}, }}
|
||||
|
||||
# TODO(geguileo): Change TypeError to ExceptionChainer once we release
|
||||
# OS-Brick version with it and bump min version.
|
||||
self.assertRaises(TypeError,
|
||||
self.volume.driver._detach_volume, self.context,
|
||||
attach_info, volume, {}, force=True)
|
||||
|
||||
self.assertTrue(connector.disconnect_volume.called)
|
||||
self.assertTrue(remove_mock.called)
|
||||
self.assertTrue(terminate_mock.called)
|
||||
self.assertEqual(3, exc.context.call_count)
|
||||
|
||||
|
||||
class FibreChannelTestCase(BaseDriverTestCase):
|
||||
|
|
|
|||
|
|
@ -18,6 +18,7 @@
|
|||
import abc
|
||||
import time
|
||||
|
||||
from os_brick import exception as brick_exception
|
||||
from oslo_concurrency import processutils
|
||||
from oslo_config import cfg
|
||||
from oslo_config import types
|
||||
|
|
@ -432,40 +433,59 @@ class BaseVD(object):
|
|||
time.sleep(tries ** 2)
|
||||
|
||||
def _detach_volume(self, context, attach_info, volume, properties,
|
||||
force=False, remote=False):
|
||||
"""Disconnect the volume from the host."""
|
||||
force=False, remote=False, ignore_errors=False):
|
||||
"""Disconnect the volume from the host.
|
||||
|
||||
With the force parameter we can indicate if we give more importance to
|
||||
cleaning up as much as possible or if data integrity has higher
|
||||
priority. This requires the latests OS-Brick code that adds this
|
||||
feature.
|
||||
|
||||
We can also force errors to be ignored using ignore_errors.
|
||||
"""
|
||||
# Use Brick's code to do attach/detach
|
||||
exc = brick_exception.ExceptionChainer()
|
||||
if attach_info:
|
||||
connector = attach_info['connector']
|
||||
connector.disconnect_volume(attach_info['conn']['data'],
|
||||
attach_info['device'])
|
||||
with exc.context(force, 'Disconnect failed'):
|
||||
connector.disconnect_volume(attach_info['conn']['data'],
|
||||
attach_info['device'], force=force,
|
||||
ignore_errors=ignore_errors)
|
||||
|
||||
if remote:
|
||||
# Call remote manager's terminate_connection which includes
|
||||
# driver's terminate_connection and remove export
|
||||
rpcapi = volume_rpcapi.VolumeAPI()
|
||||
rpcapi.terminate_connection(context, volume, properties,
|
||||
force=force)
|
||||
with exc.context(force, 'Remote terminate connection failed'):
|
||||
rpcapi.terminate_connection(context, volume, properties,
|
||||
force=force)
|
||||
else:
|
||||
# Call local driver's terminate_connection and remove export.
|
||||
# NOTE(avishay) This is copied from the manager's code - need to
|
||||
# clean this up in the future.
|
||||
try:
|
||||
self.terminate_connection(volume, properties, force=force)
|
||||
except Exception as err:
|
||||
err_msg = (_('Unable to terminate volume connection: %(err)s')
|
||||
% {'err': six.text_type(err)})
|
||||
LOG.error(err_msg)
|
||||
raise exception.VolumeBackendAPIException(data=err_msg)
|
||||
with exc.context(force,
|
||||
_('Unable to terminate volume connection')):
|
||||
try:
|
||||
self.terminate_connection(volume, properties, force=force)
|
||||
except Exception as err:
|
||||
err_msg = (
|
||||
_('Unable to terminate volume connection: %(err)s')
|
||||
% {'err': err})
|
||||
LOG.error(err_msg)
|
||||
raise exception.VolumeBackendAPIException(data=err_msg)
|
||||
|
||||
try:
|
||||
LOG.debug("volume %s: removing export", volume['id'])
|
||||
self.remove_export(context, volume)
|
||||
except Exception as ex:
|
||||
LOG.exception("Error detaching volume %(volume)s, "
|
||||
"due to remove export failure.",
|
||||
{"volume": volume['id']})
|
||||
raise exception.RemoveExportException(volume=volume['id'],
|
||||
reason=ex)
|
||||
with exc.context(force, _('Unable to remove export')):
|
||||
try:
|
||||
LOG.debug("volume %s: removing export", volume['id'])
|
||||
self.remove_export(context, volume)
|
||||
except Exception as ex:
|
||||
LOG.exception("Error detaching volume %(volume)s, "
|
||||
"due to remove export failure.",
|
||||
{"volume": volume['id']})
|
||||
raise exception.RemoveExportException(volume=volume['id'],
|
||||
reason=ex)
|
||||
if exc and not ignore_errors:
|
||||
raise exc
|
||||
|
||||
def set_initialized(self):
|
||||
self._initialized = True
|
||||
|
|
@ -816,7 +836,8 @@ class BaseVD(object):
|
|||
utils.brick_detach_volume_encryptor(attach_info,
|
||||
encryption)
|
||||
finally:
|
||||
self._detach_volume(context, attach_info, volume, properties)
|
||||
self._detach_volume(context, attach_info, volume, properties,
|
||||
force=True)
|
||||
|
||||
def copy_volume_to_image(self, context, volume, image_service, image_meta):
|
||||
"""Copy the volume to the specified image."""
|
||||
|
|
@ -834,7 +855,10 @@ class BaseVD(object):
|
|||
image_meta,
|
||||
attach_info['device']['path'])
|
||||
finally:
|
||||
self._detach_volume(context, attach_info, volume, properties)
|
||||
# Since attached volume was not used for writing we can force
|
||||
# detach it
|
||||
self._detach_volume(context, attach_info, volume, properties,
|
||||
force=True, ignore_errors=True)
|
||||
|
||||
def before_volume_copy(self, context, src_vol, dest_vol, remote=None):
|
||||
"""Driver-specific actions before copyvolume data.
|
||||
|
|
|
|||
|
|
@ -1922,7 +1922,8 @@ class VolumeManager(manager.CleanableManager,
|
|||
with excutils.save_and_reraise_exception():
|
||||
LOG.error("Failed to attach volume encryptor"
|
||||
" %(vol)s.", {'vol': volume['id']})
|
||||
self._detach_volume(ctxt, attach_info, volume, properties)
|
||||
self._detach_volume(ctxt, attach_info, volume, properties,
|
||||
force=True)
|
||||
return attach_info
|
||||
|
||||
def _detach_volume(self, ctxt, attach_info, volume, properties,
|
||||
|
|
@ -1937,7 +1938,7 @@ class VolumeManager(manager.CleanableManager,
|
|||
if encryption:
|
||||
utils.brick_detach_volume_encryptor(attach_info, encryption)
|
||||
connector.disconnect_volume(attach_info['conn']['data'],
|
||||
attach_info['device'])
|
||||
attach_info['device'], force=force)
|
||||
|
||||
if remote:
|
||||
rpcapi = volume_rpcapi.VolumeAPI()
|
||||
|
|
@ -1985,7 +1986,8 @@ class VolumeManager(manager.CleanableManager,
|
|||
LOG.error("Failed to attach source volume for copy.")
|
||||
self._detach_volume(ctxt, dest_attach_info, dest_vol,
|
||||
properties, remote=dest_remote,
|
||||
attach_encryptor=attach_encryptor)
|
||||
attach_encryptor=attach_encryptor,
|
||||
force=True)
|
||||
|
||||
# Check the backend capabilities of migration destination host.
|
||||
rpcapi = volume_rpcapi.VolumeAPI()
|
||||
|
|
@ -1996,7 +1998,6 @@ class VolumeManager(manager.CleanableManager,
|
|||
capabilities.get('sparse_copy_volume',
|
||||
False))
|
||||
|
||||
copy_error = True
|
||||
try:
|
||||
size_in_mb = int(src_vol['size']) * units.Ki # vol size is in GB
|
||||
vol_utils.copy_volume(src_attach_info['device']['path'],
|
||||
|
|
@ -2004,7 +2005,6 @@ class VolumeManager(manager.CleanableManager,
|
|||
size_in_mb,
|
||||
self.configuration.volume_dd_blocksize,
|
||||
sparse=sparse_copy_volume)
|
||||
copy_error = False
|
||||
except Exception:
|
||||
with excutils.save_and_reraise_exception():
|
||||
LOG.error("Failed to copy volume %(src)s to %(dest)s.",
|
||||
|
|
@ -2012,12 +2012,12 @@ class VolumeManager(manager.CleanableManager,
|
|||
finally:
|
||||
try:
|
||||
self._detach_volume(ctxt, dest_attach_info, dest_vol,
|
||||
properties, force=copy_error,
|
||||
properties, force=True,
|
||||
remote=dest_remote,
|
||||
attach_encryptor=attach_encryptor)
|
||||
finally:
|
||||
self._detach_volume(ctxt, src_attach_info, src_vol,
|
||||
properties, force=copy_error,
|
||||
properties, force=True,
|
||||
remote=src_remote,
|
||||
attach_encryptor=attach_encryptor)
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue