Merge "RemoteFS: enable image volume cache"

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,16 +460,18 @@ 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(
@@ -470,6 +504,11 @@ class RemoteFsSnapDriverTestCase(test.TestCase):
 
         drv.create_cloned_volume(volume, src_vref)
 
+        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'])

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']:
+        acceptable_states = ['available', 'backing-up', 'downloading']
-            msg = _("Source volume status must be 'available', or "
+        self._validate_state(src_vref.status,
-                    "'backing-up' but is: "
+                             acceptable_states,
-                    "%(status)s.") % {'status': src_vref.status}
+                             obj_description='source volume')
-            raise exception.InvalidVolume(msg)
 
         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',
+        acceptable_states = ['available', 'in-use', 'backing-up', 'deleting',
-                                 'backing-up', 'deleting']:
+                             'downloading']
-            msg = _("Volume status must be 'available', 'in-use', "
+        self._validate_state(volume_status, acceptable_states)
-                    "'backing-up' or 'deleting' but is: "
-                    "%(status)s.") % {'status': volume_status}
-
-            raise exception.InvalidVolume(msg)
 
         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)