From: Ivan Kolodyazhny Date: Fri, 22 Jan 2016 14:42:10 +0000 (+0200) Subject: Remove duplicated code in volume manager and base driver X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=be3a0b99c3e4fefb5dc85789a2ceba80b1cdd5cf;p=openstack-build%2Fcinder-build.git 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 --- diff --git a/cinder/tests/unit/test_hpe_xp_fc.py b/cinder/tests/unit/test_hpe_xp_fc.py index 97635accc..6b451834d 100644 --- a/cinder/tests/unit/test_hpe_xp_fc.py +++ b/cinder/tests/unit/test_hpe_xp_fc.py @@ -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.""" diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py index 2665a096b..b82952ea1 100644 --- a/cinder/tests/unit/test_volume.py +++ b/cinder/tests/unit/test_volume.py @@ -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') diff --git a/cinder/volume/driver.py b/cinder/volume/driver.py index 4d5c8c8c3..27e1bb5ab 100644 --- a/cinder/volume/driver.py +++ b/cinder/volume/driver.py @@ -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. diff --git a/cinder/volume/drivers/hitachi/hbsd_fc.py b/cinder/volume/drivers/hitachi/hbsd_fc.py index 00d51b25e..b9146f2d1 100644 --- a/cinder/volume/drivers/hitachi/hbsd_fc.py +++ b/cinder/volume/drivers/hitachi/hbsd_fc.py @@ -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, diff --git a/cinder/volume/drivers/hpe/hpe_xp_fc.py b/cinder/volume/drivers/hpe/hpe_xp_fc.py index c60c39eeb..1ec37b049 100644 --- a/cinder/volume/drivers/hpe/hpe_xp_fc.py +++ b/cinder/volume/drivers/hpe/hpe_xp_fc.py @@ -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. diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 1eb761856..d24aea1fc 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -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,