diff --git a/cinder/tests/unit/volume/drivers/test_remotefs.py b/cinder/tests/unit/volume/drivers/test_remotefs.py index c36e8a70d67..a97bb8a5700 100644 --- a/cinder/tests/unit/volume/drivers/test_remotefs.py +++ b/cinder/tests/unit/volume/drivers/test_remotefs.py @@ -702,3 +702,94 @@ class RemoteFSPoolMixinTestCase(test.TestCase): mock.sentinel.share) self._driver.configuration.safe_get.assert_called_once_with( 'volume_backend_name') + + +@ddt.ddt +class RevertToSnapshotMixinTestCase(test.TestCase): + + _FAKE_MNT_POINT = '/mnt/fake_hash' + + def setUp(self): + super(RevertToSnapshotMixinTestCase, self).setUp() + self._driver = remotefs.RevertToSnapshotMixin() + self._driver._remotefsclient = mock.Mock() + self._driver._execute = mock.Mock() + self._driver._delete = mock.Mock() + + self.context = context.get_admin_context() + + self._fake_volume = fake_volume.fake_volume_obj( + self.context, provider_location='fake_share') + self._fake_volume_path = os.path.join(self._FAKE_MNT_POINT, + self._fake_volume.name) + self._fake_snapshot = fake_snapshot.fake_snapshot_obj(self.context) + self._fake_snapshot_path = (self._fake_volume_path + '.' + + self._fake_snapshot.id) + self._fake_snapshot_name = os.path.basename( + self._fake_snapshot_path) + self._fake_snapshot.volume = self._fake_volume + + @ddt.data(True, False) + @mock.patch.object(remotefs.RevertToSnapshotMixin, '_validate_state', + create=True) + @mock.patch.object(remotefs.RevertToSnapshotMixin, '_read_info_file', + create=True) + @mock.patch.object(remotefs.RevertToSnapshotMixin, + '_local_path_volume_info', create=True) + @mock.patch.object(remotefs.RevertToSnapshotMixin, '_qemu_img_info', + create=True) + @mock.patch.object(remotefs.RevertToSnapshotMixin, '_do_create_snapshot', + create=True) + @mock.patch.object(remotefs.RevertToSnapshotMixin, '_local_volume_dir', + create=True) + def test_revert_to_snapshot(self, + is_latest_snapshot, + mock_local_vol_dir, + mock_do_create_snapshot, + mock_qemu_img_info, + mock_local_path_vol_info, + mock_read_info_file, + mock_validate_state): + + active_file = (self._fake_snapshot_name if is_latest_snapshot + else 'fake_latest_snap') + fake_snapshot_info = { + 'active': active_file, + self._fake_snapshot.id: self._fake_snapshot_name + } + + mock_read_info_file.return_value = fake_snapshot_info + + fake_snap_img_info = mock.Mock() + fake_snap_img_info.backing_file = self._fake_volume.name + + mock_qemu_img_info.return_value = fake_snap_img_info + mock_local_vol_dir.return_value = self._FAKE_MNT_POINT + + if is_latest_snapshot: + self._driver._revert_to_snapshot(self.context, self._fake_volume, + self._fake_snapshot) + self._driver._delete.assert_called_once_with( + self._fake_snapshot_path) + mock_do_create_snapshot.assert_called_once_with( + self._fake_snapshot, + fake_snap_img_info.backing_file, + self._fake_snapshot_path) + mock_qemu_img_info.assert_called_once_with( + self._fake_snapshot_path, + self._fake_volume.name) + elif not is_latest_snapshot: + self.assertRaises(exception.InvalidSnapshot, + self._driver._revert_to_snapshot, + self.context, self._fake_volume, + self._fake_snapshot) + self._driver._delete.assert_not_called() + + exp_acceptable_states = ['available', 'reverting'] + mock_validate_state.assert_called_once_with( + self._fake_snapshot.volume.status, + exp_acceptable_states) + mock_local_path_vol_info.assert_called_once_with( + self._fake_snapshot.volume) + mock_read_info_file.assert_called_once_with( + mock_local_path_vol_info.return_value) diff --git a/cinder/tests/unit/volume/test_volume.py b/cinder/tests/unit/volume/test_volume.py index 72c46765a06..e061d925821 100644 --- a/cinder/tests/unit/volume/test_volume.py +++ b/cinder/tests/unit/volume/test_volume.py @@ -1771,8 +1771,13 @@ class VolumeTestCase(base.BaseVolumeTestCase): if driver_error: generic_revert.assert_called_once_with(self.context, {}, {}) - @ddt.data(True, False) - def test_revert_to_snapshot(self, has_snapshot): + @ddt.data({}, + {'has_snapshot': True}, + {'use_temp_snapshot': True}, + {'use_temp_snapshot': True, 'has_snapshot': True}) + @ddt.unpack + def test_revert_to_snapshot(self, has_snapshot=False, + use_temp_snapshot=False): fake_volume = tests_utils.create_volume(self.context, status='reverting', project_id='123', @@ -1786,8 +1791,13 @@ class VolumeTestCase(base.BaseVolumeTestCase): mock.patch.object(self.volume, '_create_backup_snapshot') as _create_snapshot,\ mock.patch.object(self.volume, - 'delete_snapshot') as _delete_snapshot: + 'delete_snapshot') as _delete_snapshot, \ + mock.patch.object(self.volume.driver, + 'snapshot_revert_use_temp_snapshot') as \ + _use_temp_snap: _revert.return_value = None + _use_temp_snap.return_value = use_temp_snapshot + if has_snapshot: _create_snapshot.return_value = {'id': 'fake_snapshot'} else: @@ -1796,12 +1806,19 @@ class VolumeTestCase(base.BaseVolumeTestCase): fake_snapshot) _revert.assert_called_once_with(self.context, fake_volume, fake_snapshot) - _create_snapshot.assert_called_once_with(self.context, fake_volume) - if has_snapshot: + + if not use_temp_snapshot: + _create_snapshot.assert_not_called() + else: + _create_snapshot.assert_called_once_with(self.context, + fake_volume) + + if use_temp_snapshot and has_snapshot: _delete_snapshot.assert_called_once_with( self.context, {'id': 'fake_snapshot'}, handle_quota=False) else: _delete_snapshot.assert_not_called() + fake_volume.refresh() fake_snapshot.refresh() self.assertEqual('available', fake_volume['status']) diff --git a/cinder/volume/driver.py b/cinder/volume/driver.py index 64ce03513e3..037ff597b7a 100644 --- a/cinder/volume/driver.py +++ b/cinder/volume/driver.py @@ -1102,6 +1102,13 @@ class BaseVD(object): """ return self.configuration.safe_get("backup_use_temp_snapshot") + def snapshot_revert_use_temp_snapshot(self): + # Specify whether a temporary backup snapshot should be used when + # reverting a snapshot. For some backends, this operation is not + # needed or not supported, in which case the driver should override + # this method. + return True + def snapshot_remote_attachable(self): # TODO(lixiaoy1): the method will be deleted later when remote # attach snapshot is implemented. diff --git a/cinder/volume/drivers/remotefs.py b/cinder/volume/drivers/remotefs.py index 554b2785380..81187740cae 100644 --- a/cinder/volume/drivers/remotefs.py +++ b/cinder/volume/drivers/remotefs.py @@ -717,6 +717,13 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): self._nova = compute.API() + def snapshot_revert_use_temp_snapshot(self): + # Considering that RemoteFS based drivers use COW images + # for storing snapshots, having chains of such images, + # creating a backup snapshot when reverting one is not + # actutally helpful. + return False + def _local_volume_dir(self, volume): share = volume.provider_location local_dir = self._get_mount_point_for_share(share) @@ -1607,6 +1614,9 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): def _extend_volume(self, volume, size_gb): raise NotImplementedError() + def _revert_to_snapshot(self, context, volume, snapshot): + raise NotImplementedError() + class RemoteFSSnapDriver(RemoteFSSnapDriverBase): @locked_volume_id_operation @@ -1642,6 +1652,12 @@ class RemoteFSSnapDriver(RemoteFSSnapDriverBase): def extend_volume(self, volume, size_gb): return self._extend_volume(volume, size_gb) + @locked_volume_id_operation + def revert_to_snapshot(self, context, volume, snapshot): + """Revert to specified snapshot.""" + + return self._revert_to_snapshot(context, volume, snapshot) + class RemoteFSSnapDriverDistributed(RemoteFSSnapDriverBase): @coordination.synchronized('{self.driver_prefix}-{snapshot.volume.id}') @@ -1677,6 +1693,12 @@ class RemoteFSSnapDriverDistributed(RemoteFSSnapDriverBase): def extend_volume(self, volume, size_gb): return self._extend_volume(volume, size_gb) + @coordination.synchronized('{self.driver_prefix}-{volume.id}') + def revert_to_snapshot(self, context, volume, snapshot): + """Revert to specified snapshot.""" + + return self._revert_to_snapshot(context, volume, snapshot) + class RemoteFSPoolMixin(object): """Drivers inheriting this will report each share as a pool.""" @@ -1737,3 +1759,48 @@ class RemoteFSPoolMixin(object): data['pools'] = pools self._stats = data + + +class RevertToSnapshotMixin(object): + + def _revert_to_snapshot(self, context, volume, snapshot): + """Revert a volume to specified snapshot + + The volume must not be attached. Only the latest snapshot + can be used. + """ + status = snapshot.volume.status + acceptable_states = ['available', 'reverting'] + + self._validate_state(status, acceptable_states) + + LOG.debug('Reverting volume %(vol)s to snapshot %(snap)s', + {'vol': snapshot.volume.id, 'snap': snapshot.id}) + + info_path = self._local_path_volume_info(snapshot.volume) + snap_info = self._read_info_file(info_path) + + snapshot_file = snap_info[snapshot.id] + active_file = snap_info['active'] + + if not utils.paths_normcase_equal(snapshot_file, active_file): + msg = _("Could not revert volume '%(volume_id)s' to snapshot " + "'%(snapshot_id)s' as it does not " + "appear to be the latest snapshot. Current active " + "image: %(active_file)s.") + raise exception.InvalidSnapshot( + msg % dict(snapshot_id=snapshot.id, + active_file=active_file, + volume_id=volume.id)) + + snapshot_path = os.path.join( + self._local_volume_dir(snapshot.volume), snapshot_file) + backing_filename = self._qemu_img_info( + snapshot_path, volume.name).backing_file + + # We revert the volume to the latest snapshot by recreating the top + # image from the chain. + # This workflow should work with most (if not all) drivers inheriting + # this class. + self._delete(snapshot_path) + self._do_create_snapshot(snapshot, backing_filename, snapshot_path) diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 4441634fb1f..ffcb3dc999f 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -924,7 +924,10 @@ class VolumeManager(manager.CleanableManager, LOG.info("Start to perform revert to snapshot process.") # Create a snapshot which can be used to restore the volume # data by hand if revert process failed. - backup_snapshot = self._create_backup_snapshot(context, volume) + + if self.driver.snapshot_revert_use_temp_snapshot(): + backup_snapshot = self._create_backup_snapshot(context, + volume) self._revert_to_snapshot(context, volume, snapshot) except Exception as error: with excutils.save_and_reraise_exception():