From 413664d11ca002c49a41c9d2e2efa85afeaca791 Mon Sep 17 00:00:00 2001 From: Alexandru Muresan Date: Wed, 13 Sep 2017 14:58:02 +0300 Subject: [PATCH] RemoteFS: revert snapshot support This change implements the 'revert_to_snapshot' method at the remotefs base driver level in a generic way. This should work right away with most (if not all) remotefs based drivers. Note that we only consider the case in which the latest snapshot is being reverted (the Cinder API won't allow reverting volumes to other snapshots anyway, at least for the time being). The RemoteFS based drivers (e.g. NFS driver, SMB driver) use COW images (e.g. qcow2, differencing vhdx) in order to provide volume snapshot functionality. Reverting a volume to its latest snapshot is a trivial operation in this case and can be achived by simply cleaning up (e.g. recreating) the latest image from chain. In order to avoid breaking drivers, the 'revert_to_snapshot' method is implemented in a mixin that may be inherited by any RemoteFS based driver. Implements: blueprint remotefs-revert-snapshot Change-Id: Id66d3f0fe36cf7c32ef3875bf5fcb1c7a4678273 --- .../unit/volume/drivers/test_remotefs.py | 91 +++++++++++++++++++ cinder/tests/unit/volume/test_volume.py | 27 +++++- cinder/volume/driver.py | 7 ++ cinder/volume/drivers/remotefs.py | 67 ++++++++++++++ cinder/volume/manager.py | 5 +- 5 files changed, 191 insertions(+), 6 deletions(-) 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():