openstack
/
cinder
Code Issues Proposed changes

Remove duplicated code in volume manager and base driver 

'copy_volume_data' method and all migration-related code should be
in volume manager only.

'copy_volume_data' method from driver are used only in unit-tests.

Change-Id: I6a4cb2ff2c11101cf10df0d515e6bb6fa7cf75f6
Closes-Bug: #1506984
This commit is contained in:
Ivan Kolodyazhny 2016-01-22 16:42:10 +02:00
parent 04b2cdf9b5
commit be3a0b99c3
6 changed files with 46 additions and 188 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

32
cinder/tests/unit/test_hpe_xp_fc.py
View File

@ -569,38 +569,6 @@ class HPEXPFCDriverTest(test.TestCase):
rc = self.driver.get_volume_stats(True)
self.assertEqual({}, rc)
@mock.patch.object(driver.FibreChannelDriver, 'copy_volume_data')
def test_copy_volume_data(self, arg1):
"""Test copy_volume_data."""
volume = fake_volume.fake_db_volume(**self._VOLUME)
rc_vol = self.driver.create_volume(volume)
volume['provider_location'] = rc_vol['provider_location']
volume2 = fake_volume.fake_db_volume(**self._VOLUME2)
rc_vol2 = self.driver.create_volume(volume2)
volume2['provider_location'] = rc_vol2['provider_location']
self.driver.copy_volume_data(None, volume, volume2, None)
arg1.assert_called_with(None, volume, volume2, None)
@mock.patch.object(driver.FibreChannelDriver, 'copy_volume_data',
side_effect=exception.CinderException)
def test_copy_volume_data_error(self, arg1):
"""Test copy_volume_data is error."""
volume = fake_volume.fake_db_volume(**self._VOLUME)
rc_vol = self.driver.create_volume(volume)
volume['provider_location'] = rc_vol['provider_location']
volume2 = fake_volume.fake_db_volume(**self._VOLUME2)
volume2['provider_location'] = '2'
self.assertRaises(exception.CinderException,
self.driver.copy_volume_data,
None, volume, volume2, None)
arg1.assert_called_with(None, volume, volume2, None)
@mock.patch.object(driver.FibreChannelDriver, 'copy_image_to_volume')
def test_copy_image_to_volume(self, arg1):
"""Test copy_image_to_volume."""

68
cinder/tests/unit/test_volume.py
View File

@ -6534,74 +6534,6 @@ class GenericVolumeDriverTestCase(DriverTestCase):
self.context,
volume)
@mock.patch.object(utils, 'brick_get_connector_properties')
@mock.patch.object(cinder.volume.driver.VolumeDriver, '_attach_volume')
@mock.patch.object(cinder.volume.driver.VolumeDriver, '_detach_volume')
@mock.patch.object(volutils, 'copy_volume')
@mock.patch.object(volume_rpcapi.VolumeAPI, 'get_capabilities')
def test_copy_volume_data(self,
mock_get_capabilities,
mock_copy,
mock_detach,
mock_attach,
mock_get_connector):
src_vol = tests_utils.create_volume(self.context, size=1,
host=CONF.host)
dest_vol = tests_utils.create_volume(self.context, size=1,
host=CONF.host)
mock_get_connector.return_value = {}
self.volume.driver._throttle = mock.MagicMock()
attach_expected = [
mock.call(self.context, dest_vol, {}, remote=False),
mock.call(self.context, src_vol, {}, remote=False)]
detach_expected = [
mock.call(self.context, {'device': {'path': 'bar'}},
dest_vol, {}, force=False, remote=False),
mock.call(self.context, {'device': {'path': 'foo'}},
src_vol, {}, force=False, remote=False)]
attach_volume_returns = [
({'device': {'path': 'bar'}}, dest_vol),
({'device': {'path': 'foo'}}, src_vol),
]
# Test case for sparse_copy_volume = False
mock_attach.side_effect = attach_volume_returns
mock_get_capabilities.return_value = {}
self.volume.driver.copy_volume_data(self.context,
src_vol,
dest_vol)
self.assertEqual(attach_expected, mock_attach.mock_calls)
mock_copy.assert_called_with(
'foo', 'bar', 1024, '1M',
throttle=self.volume.driver._throttle,
sparse=False)
self.assertEqual(detach_expected, mock_detach.mock_calls)
# Test case for sparse_copy_volume = True
mock_attach.reset_mock()
mock_detach.reset_mock()
mock_attach.side_effect = attach_volume_returns
mock_get_capabilities.return_value = {'sparse_copy_volume': True}
self.volume.driver.copy_volume_data(self.context,
src_vol,
dest_vol)
self.assertEqual(attach_expected, mock_attach.mock_calls)
mock_copy.assert_called_with(
'foo', 'bar', 1024, '1M',
throttle=self.volume.driver._throttle,
sparse=True)
self.assertEqual(detach_expected, mock_detach.mock_calls)
# cleanup resource
db.volume_destroy(self.context, src_vol['id'])
db.volume_destroy(self.context, dest_vol['id'])
@mock.patch.object(utils, 'brick_get_connector_properties')
@mock.patch.object(cinder.volume.manager.VolumeManager, '_attach_volume')
@mock.patch.object(cinder.volume.manager.VolumeManager, '_detach_volume')

88
cinder/volume/driver.py
View File

@ -32,7 +32,6 @@ from cinder import objects
from cinder import utils
from cinder.volume import rpcapi as volume_rpcapi
from cinder.volume import throttling
from cinder.volume import utils as volume_utils
LOG = logging.getLogger(__name__)
@ -737,77 +736,6 @@ class BaseVD(object):
data["pools"].append(single_pool)
self._stats = data
def copy_volume_data(self, context, src_vol, dest_vol, remote=None):
"""Copy data from src_vol to dest_vol."""
LOG.debug('copy_data_between_volumes %(src)s -> %(dest)s.', {
'src': src_vol['name'], 'dest': dest_vol['name']})
use_multipath = self.configuration.use_multipath_for_image_xfer
enforce_multipath = self.configuration.enforce_multipath_for_image_xfer
properties = utils.brick_get_connector_properties(use_multipath,
enforce_multipath)
dest_remote = True if remote in ['dest', 'both'] else False
dest_orig_status = dest_vol['status']
try:
dest_attach_info, dest_vol = self._attach_volume(
context,
dest_vol,
properties,
remote=dest_remote)
except Exception:
with excutils.save_and_reraise_exception():
LOG.error(_LE("Failed to attach volume %(vol)s"),
{'vol': dest_vol['id']})
self.db.volume_update(context, dest_vol['id'],
{'status': dest_orig_status})
src_remote = True if remote in ['src', 'both'] else False
src_orig_status = src_vol['status']
try:
src_attach_info, src_vol = self._attach_volume(context,
src_vol,
properties,
remote=src_remote)
except Exception:
with excutils.save_and_reraise_exception():
LOG.error(_LE("Failed to attach volume %(vol)s"),
{'vol': src_vol['id']})
self.db.volume_update(context, src_vol['id'],
{'status': src_orig_status})
self._detach_volume(context, dest_attach_info, dest_vol,
properties, force=True, remote=dest_remote)
# Check the backend capabilities of migration destination host.
rpcapi = volume_rpcapi.VolumeAPI()
capabilities = rpcapi.get_capabilities(context, dest_vol['host'],
False)
sparse_copy_volume = bool(capabilities and
capabilities.get('sparse_copy_volume',
False))
copy_error = True
try:
size_in_mb = int(src_vol['size']) * 1024 # vol size is in GB
volume_utils.copy_volume(
src_attach_info['device']['path'],
dest_attach_info['device']['path'],
size_in_mb,
self.configuration.volume_dd_blocksize,
throttle=self._throttle,
sparse=sparse_copy_volume)
copy_error = False
except Exception:
with excutils.save_and_reraise_exception():
LOG.error(_LE("Failed to copy volume %(src)s to %(dest)s."),
{'src': src_vol['id'], 'dest': dest_vol['id']})
finally:
self._detach_volume(context, dest_attach_info, dest_vol,
properties, force=copy_error,
remote=dest_remote)
self._detach_volume(context, src_attach_info, src_vol,
properties, force=copy_error,
remote=src_remote)
def copy_image_to_volume(self, context, volume, image_service, image_id):
"""Fetch the image from image_service and write it to the volume."""
LOG.debug('copy_image_to_volume %s.', volume['name'])
@ -846,6 +774,22 @@ class BaseVD(object):
finally:
self._detach_volume(context, attach_info, volume, properties)
def before_volume_copy(self, context, src_vol, dest_vol, remote=None):
"""Driver-specific actions before copyvolume data.
This method will be called before _copy_volume_data during volume
migration
"""
pass
def after_volume_copy(self, context, src_vol, dest_vol, remote=None):
"""Driver-specific actions after copyvolume data.
This method will be called after _copy_volume_data during volume
migration
"""
pass
def get_filter_function(self):
"""Get filter_function string.

22
cinder/volume/drivers/hitachi/hbsd_fc.py
View File

@ -482,12 +482,6 @@ class HBSDFCDriver(cinder.volume.driver.FibreChannelDriver):
def remove_export(self, context, volume):
pass
def copy_volume_data(self, context, src_vol, dest_vol, remote=None):
self.do_setup_status.wait()
super(HBSDFCDriver, self).copy_volume_data(context, src_vol,
dest_vol, remote)
self.discard_zero_page(dest_vol)
def copy_image_to_volume(self, context, volume, image_service, image_id):
self.do_setup_status.wait()
super(HBSDFCDriver, self).copy_image_to_volume(context, volume,
@ -505,6 +499,22 @@ class HBSDFCDriver(cinder.volume.driver.FibreChannelDriver):
image_service,
image_meta)
def before_volume_copy(self, context, src_vol, dest_vol, remote=None):
"""Driver-specific actions before copyvolume data.
This method will be called before _copy_volume_data during volume
migration
"""
self.do_setup_status.wait()
def after_volume_copy(self, context, src_vol, dest_vol, remote=None):
"""Driver-specific actions after copyvolume data.
This method will be called after _copy_volume_data during volume
migration
"""
self.discard_zero_page(dest_vol)
def restore_backup(self, context, backup, volume, backup_service):
self.do_setup_status.wait()
super(HBSDFCDriver, self).restore_backup(context, backup,

18
cinder/volume/drivers/hpe/hpe_xp_fc.py
View File

@ -74,16 +74,6 @@ class HPEXPFCDriver(driver.FibreChannelDriver):
"""Get volume stats."""
return self.common.get_volume_stats(refresh)
def copy_volume_data(self, context, src_vol, dest_vol, remote=None):
"""Copy data from src_vol to dest_vol.
Call copy_volume_data() of super class and
carry out original postprocessing.
"""
super(HPEXPFCDriver, self).copy_volume_data(
context, src_vol, dest_vol, remote)
self.common.copy_volume_data(context, src_vol, dest_vol, remote)
def copy_image_to_volume(self, context, volume, image_service, image_id):
"""Fetch the image from image_service and write it to the volume.
@ -95,6 +85,14 @@ class HPEXPFCDriver(driver.FibreChannelDriver):
self.common.copy_image_to_volume(
context, volume, image_service, image_id)
def after_volume_copy(self, context, src_vol, dest_vol, remote=None):
"""Driver-specific actions after copyvolume data.
This method will be called after _copy_volume_data during volume
migration
"""
self.common.copy_volume_data(context, src_vol, dest_vol, remote)
def restore_backup(self, context, backup, volume, backup_service):
"""Restore an existing backup to a new or existing volume.

6
cinder/volume/manager.py
View File

@ -1686,7 +1686,13 @@ class VolumeManager(manager.SchedulerDependentManager):
try:
attachments = volume.volume_attachment
if not attachments:
# Pre- and post-copy driver-specific actions
self.driver.before_volume_copy(ctxt, volume, new_volume,
remote='dest')
self._copy_volume_data(ctxt, volume, new_volume, remote='dest')
self.driver.after_volume_copy(ctxt, volume, new_volume,
remote='dest')
# The above call is synchronous so we complete the migration
self.migrate_volume_completion(ctxt, volume.id,
new_volume.id,