From 63b9f0fb8e831c29fab00670c19056e436d5f5f0 Mon Sep 17 00:00:00 2001 From: Lucian Petrut Date: Fri, 21 Apr 2017 17:29:43 +0300 Subject: [PATCH] RemoteFS: enable image volume cache At the moment, the RemoteFS based drivers cannot leverage the generic image volume cache feature. The reason is that this involves cloning a newly created volume while still being in 'downloading' state, which is an unacceptable state from the driver's point of view. This change adds 'downloading' as an acceptable state for the volume clone operation as well as for creating/deleting snapshots used internally by the volume clone operation. Note that 'regular' volume clones will be rejected before reaching the driver when having a source volume in 'downloading' state. Change-Id: I84bb3f062637031f7f46b414d6cbbff0bb0292ea Closes-Bug: #1685277 --- .../unit/volume/drivers/test_remotefs.py | 119 ++++++++++++------ cinder/volume/drivers/remotefs.py | 49 +++++--- 2 files changed, 111 insertions(+), 57 deletions(-) diff --git a/cinder/tests/unit/volume/drivers/test_remotefs.py b/cinder/tests/unit/volume/drivers/test_remotefs.py index 2e1caec4500..24e7d436f90 100644 --- a/cinder/tests/unit/volume/drivers/test_remotefs.py +++ b/cinder/tests/unit/volume/drivers/test_remotefs.py @@ -52,9 +52,26 @@ class RemoteFsSnapDriverTestCase(test.TestCase): self._fake_snapshot.id) self._fake_snapshot.volume = self._fake_volume + @ddt.data({'current_state': 'in-use', + 'acceptable_states': ['available', 'in-use']}, + {'current_state': 'in-use', + 'acceptable_states': ['available'], + 'expected_exception': exception.InvalidVolume}) + @ddt.unpack + def test_validate_state(self, current_state, acceptable_states, + expected_exception=None): + if expected_exception: + self.assertRaises(expected_exception, + self._driver._validate_state, + current_state, + acceptable_states) + else: + self._driver._validate_state(current_state, acceptable_states) + def _test_delete_snapshot(self, volume_in_use=False, stale_snapshot=False, - is_active_image=True): + is_active_image=True, + is_tmp_snap=False): # If the snapshot is not the active image, it is guaranteed that # another snapshot exists having it as backing file. @@ -78,6 +95,7 @@ class RemoteFsSnapDriverTestCase(test.TestCase): self._driver._local_volume_dir = mock.Mock( return_value=self._FAKE_MNT_POINT) + self._driver._validate_state = mock.Mock() self._driver._read_info_file = mock.Mock() self._driver._write_info_file = mock.Mock() self._driver._img_commit = mock.Mock() @@ -91,12 +109,18 @@ class RemoteFsSnapDriverTestCase(test.TestCase): self._fake_snapshot.id: fake_snapshot_name } + exp_acceptable_states = ['available', 'in-use', 'backing-up', + 'deleting', 'downloading'] + if volume_in_use: self._fake_snapshot.volume.status = 'in-use' self._driver._read_info_file.return_value = fake_info self._driver._delete_snapshot(self._fake_snapshot) + self._driver._validate_state.assert_called_once_with( + self._fake_snapshot.volume.status, + exp_acceptable_states) if stale_snapshot: self._driver._delete_stale_snapshot.assert_called_once_with( self._fake_snapshot) @@ -228,7 +252,7 @@ class RemoteFsSnapDriverTestCase(test.TestCase): mock.call(*command3, run_as_root=True)] self._driver._execute.assert_has_calls(calls) - def _test_create_snapshot(self, volume_in_use=False): + def _test_create_snapshot(self, volume_in_use=False, tmp_snap=False): fake_snapshot_info = {} fake_snapshot_file_name = os.path.basename(self._fake_snapshot_path) @@ -243,11 +267,16 @@ class RemoteFsSnapDriverTestCase(test.TestCase): return_value=self._fake_volume.name) self._driver._get_new_snap_path = mock.Mock( return_value=self._fake_snapshot_path) + self._driver._validate_state = mock.Mock() expected_snapshot_info = { 'active': fake_snapshot_file_name, self._fake_snapshot.id: fake_snapshot_file_name } + exp_acceptable_states = ['available', 'in-use', 'backing-up'] + if tmp_snap: + exp_acceptable_states.append('downloading') + self._fake_snapshot.id = 'tmp-snap-%s' % self._fake_snapshot.id if volume_in_use: self._fake_snapshot.volume.status = 'in-use' @@ -258,6 +287,9 @@ class RemoteFsSnapDriverTestCase(test.TestCase): self._driver._create_snapshot(self._fake_snapshot) + self._driver._validate_state.assert_called_once_with( + self._fake_snapshot.volume.status, + exp_acceptable_states) fake_method = getattr(self._driver, expected_method_called) fake_method.assert_called_with( self._fake_snapshot, self._fake_volume.name, @@ -428,52 +460,59 @@ class RemoteFsSnapDriverTestCase(test.TestCase): basedir=basedir, valid_backing_file=False) - def test_create_cloned_volume(self): + @mock.patch.object(remotefs.RemoteFSSnapDriver, + '_validate_state') + @mock.patch.object(remotefs.RemoteFSSnapDriver, '_create_snapshot') + @mock.patch.object(remotefs.RemoteFSSnapDriver, '_delete_snapshot') + @mock.patch.object(remotefs.RemoteFSSnapDriver, + '_copy_volume_from_snapshot') + def test_create_cloned_volume(self, mock_copy_volume_from_snapshot, + mock_delete_snapshot, + mock_create_snapshot, + mock_validate_state): drv = self._driver - with mock.patch.object(drv, '_create_snapshot') as \ - mock_create_snapshot,\ - mock.patch.object(drv, '_delete_snapshot') as \ - mock_delete_snapshot,\ - mock.patch.object(drv, '_copy_volume_from_snapshot') as \ - mock_copy_volume_from_snapshot: + volume = fake_volume.fake_volume_obj(self.context) + src_vref_id = '375e32b2-804a-49f2-b282-85d1d5a5b9e1' + src_vref = fake_volume.fake_volume_obj( + self.context, + id=src_vref_id, + name='volume-%s' % src_vref_id) - volume = fake_volume.fake_volume_obj(self.context) - src_vref_id = '375e32b2-804a-49f2-b282-85d1d5a5b9e1' - src_vref = fake_volume.fake_volume_obj( - self.context, - id=src_vref_id, - name='volume-%s' % src_vref_id) + vol_attrs = ['provider_location', 'size', 'id', 'name', 'status', + 'volume_type', 'metadata'] + Volume = collections.namedtuple('Volume', vol_attrs) - vol_attrs = ['provider_location', 'size', 'id', 'name', 'status', - 'volume_type', 'metadata'] - Volume = collections.namedtuple('Volume', vol_attrs) + snap_attrs = ['volume_name', 'volume_size', 'name', + 'volume_id', 'id', 'volume'] + Snapshot = collections.namedtuple('Snapshot', snap_attrs) - snap_attrs = ['volume_name', 'volume_size', 'name', - 'volume_id', 'id', 'volume'] - Snapshot = collections.namedtuple('Snapshot', snap_attrs) + volume_ref = Volume(id=volume.id, + name=volume.name, + status=volume.status, + provider_location=volume.provider_location, + size=volume.size, + volume_type=volume.volume_type, + metadata=volume.metadata) - volume_ref = Volume(id=volume.id, - name=volume.name, - status=volume.status, - provider_location=volume.provider_location, - size=volume.size, - volume_type=volume.volume_type, - metadata=volume.metadata) + snap_ref = Snapshot(volume_name=volume.name, + name='clone-snap-%s' % src_vref.id, + volume_size=src_vref.size, + volume_id=src_vref.id, + id='tmp-snap-%s' % src_vref.id, + volume=src_vref) - snap_ref = Snapshot(volume_name=volume.name, - name='clone-snap-%s' % src_vref.id, - volume_size=src_vref.size, - volume_id=src_vref.id, - id='tmp-snap-%s' % src_vref.id, - volume=src_vref) + drv.create_cloned_volume(volume, src_vref) - drv.create_cloned_volume(volume, src_vref) - - mock_create_snapshot.assert_called_once_with(snap_ref) - mock_copy_volume_from_snapshot.assert_called_once_with( - snap_ref, volume_ref, volume['size']) - self.assertTrue(mock_delete_snapshot.called) + exp_acceptable_states = ['available', 'backing-up', 'downloading'] + mock_validate_state.assert_called_once_with( + src_vref.status, + exp_acceptable_states, + obj_description='source volume') + mock_create_snapshot.assert_called_once_with(snap_ref) + mock_copy_volume_from_snapshot.assert_called_once_with( + snap_ref, volume_ref, volume['size']) + self.assertTrue(mock_delete_snapshot.called) def test_create_regular_file(self): self._driver._create_regular_file('/path', 1) diff --git a/cinder/volume/drivers/remotefs.py b/cinder/volume/drivers/remotefs.py index 99481bd3480..4aa29398774 100644 --- a/cinder/volume/drivers/remotefs.py +++ b/cinder/volume/drivers/remotefs.py @@ -233,6 +233,23 @@ class RemoteFSDriver(driver.BaseVD): " mount_point_base.") return None + @staticmethod + def _validate_state(current_state, + acceptable_states, + obj_description='volume', + invalid_exc=exception.InvalidVolume): + if current_state not in acceptable_states: + message = _('Invalid %(obj_description)s state. ' + 'Acceptable states for this operation: ' + '%(acceptable_states)s. ' + 'Current %(obj_description)s state: ' + '%(current_state)s.') + raise invalid_exc( + message=message % + dict(obj_description=obj_description, + acceptable_states=acceptable_states, + current_state=current_state)) + @utils.trace def create_volume(self, volume): """Creates a volume. @@ -941,11 +958,10 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): {'src': src_vref.id, 'dst': volume.id}) - if src_vref.status not in ['available', 'backing-up']: - msg = _("Source volume status must be 'available', or " - "'backing-up' but is: " - "%(status)s.") % {'status': src_vref.status} - raise exception.InvalidVolume(msg) + acceptable_states = ['available', 'backing-up', 'downloading'] + self._validate_state(src_vref.status, + acceptable_states, + obj_description='source volume') volume_name = CONF.volume_name_template % volume.id @@ -1021,13 +1037,9 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): else 'offline')}) volume_status = snapshot.volume.status - if volume_status not in ['available', 'in-use', - 'backing-up', 'deleting']: - msg = _("Volume status must be 'available', 'in-use', " - "'backing-up' or 'deleting' but is: " - "%(status)s.") % {'status': volume_status} - - raise exception.InvalidVolume(msg) + acceptable_states = ['available', 'in-use', 'backing-up', 'deleting', + 'downloading'] + self._validate_state(volume_status, acceptable_states) vol_path = self._local_volume_dir(snapshot.volume) self._ensure_share_writable(vol_path) @@ -1332,12 +1344,15 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): else 'offline')}) status = snapshot.volume.status - if status not in ['available', 'in-use', 'backing-up']: - msg = _("Volume status must be 'available', 'in-use' or " - "'backing-up' but is: " - "%(status)s.") % {'status': status} - raise exception.InvalidVolume(msg) + acceptable_states = ['available', 'in-use', 'backing-up'] + if snapshot.id.startswith('tmp-snap-'): + # This is an internal volume snapshot. In order to support + # image caching, we'll allow creating/deleting such snapshots + # while having volumes in 'downloading' state. + acceptable_states.append('downloading') + + self._validate_state(status, acceptable_states) info_path = self._local_path_volume_info(snapshot.volume) snap_info = self._read_info_file(info_path, empty_if_missing=True)