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)