SMBFS: Fix retrieving the volume path and format

The SMBFS drivers use the image format in order to construct the
according image path, as some image formats require having the
proper extension.

The format is either retrieved using qemu-img if the image exists,
either via extra specs or config options otherwise.

The issue is that when searching for local images, the extension is
not used, falling back to extra specs, which can cause lazy load
errors or config options, which might change.

This issue occurs when vhd/x images are used. The fix consists in
simply using the possible extensions when trying to find existing
images.

Change-Id: I85ef7862489eb434a00cd897f5344bb89b7de858
Closes-bug: #1425124

diff --git a/cinder/tests/test_smbfs.py b/cinder/tests/test_smbfs.py
index 75fb43188f0aee551ab599a1619da5ffba6b460a..1d0d7d38834d520e4c5a632f1d12ebaa9e4dab6f 100644 (file)
--- a/cinder/tests/test_smbfs.py
+++ b/cinder/tests/test_smbfs.py
@@ -275,16 +275,106 @@ class SmbFsTestCase(test.TestCase):
         flags = self._smbfs_driver.parse_credentials(fake_smb_options)
         self.assertEqual(expected_flags, flags)
 
-    def test_get_volume_path(self):
-        self._smbfs_driver.get_volume_format = mock.Mock(
-            return_value='vhd')
-        self._smbfs_driver._local_volume_dir = mock.Mock(
-            return_value=self._FAKE_MNT_POINT)
+    @mock.patch.object(smbfs.SmbfsDriver, '_get_local_volume_path_template')
+    @mock.patch.object(smbfs.SmbfsDriver, '_lookup_local_volume_path')
+    @mock.patch.object(smbfs.SmbfsDriver, 'get_volume_format')
+    def _test_get_volume_path(self, mock_get_volume_format, mock_lookup_volume,
+                              mock_get_path_template, volume_exists=True,
+                              volume_format='raw'):
+        drv = self._smbfs_driver
+        mock_get_path_template.return_value = self._FAKE_VOLUME_PATH
 
-        expected = self._FAKE_VOLUME_PATH + '.vhd'
+        expected_vol_path = self._FAKE_VOLUME_PATH
+        if volume_format in (drv._DISK_FORMAT_VHD, drv._DISK_FORMAT_VHDX):
+            expected_vol_path += '.' + volume_format
 
-        ret_val = self._smbfs_driver.local_path(self._FAKE_VOLUME)
-        self.assertEqual(expected, ret_val)
+        mock_lookup_volume.return_value = (
+            expected_vol_path if volume_exists else None)
+        mock_get_volume_format.return_value = volume_format
+
+        ret_val = drv.local_path(self._FAKE_VOLUME)
+
+        if volume_exists:
+            self.assertFalse(mock_get_volume_format.called)
+        else:
+            mock_get_volume_format.assert_called_once_with(self._FAKE_VOLUME)
+        self.assertEqual(expected_vol_path, ret_val)
+
+    def test_get_existing_volume_path(self):
+        self._test_get_volume_path()
+
+    def test_get_new_raw_volume_path(self):
+        self._test_get_volume_path(volume_exists=False)
+
+    def test_get_new_vhd_volume_path(self):
+        self._test_get_volume_path(volume_exists=False, volume_format='vhd')
+
+    @mock.patch.object(smbfs.SmbfsDriver, '_local_volume_dir')
+    def test_get_local_volume_path_template(self, mock_get_local_dir):
+        mock_get_local_dir.return_value = self._FAKE_MNT_POINT
+        ret_val = self._smbfs_driver._get_local_volume_path_template(
+            self._FAKE_VOLUME)
+        self.assertEqual(self._FAKE_VOLUME_PATH, ret_val)
+
+    @mock.patch('os.path.exists')
+    def test_lookup_local_volume_path(self, mock_exists):
+        expected_path = self._FAKE_VOLUME_PATH + '.vhdx'
+        mock_exists.side_effect = lambda x: x == expected_path
+
+        ret_val = self._smbfs_driver._lookup_local_volume_path(
+            self._FAKE_VOLUME_PATH)
+
+        possible_paths = [self._FAKE_VOLUME_PATH + ext
+                          for ext in ('', '.vhd', '.vhdx')]
+        mock_exists.assert_has_calls(
+            [mock.call(path) for path in possible_paths])
+        self.assertEqual(expected_path, ret_val)
+
+    @mock.patch.object(smbfs.SmbfsDriver, '_get_local_volume_path_template')
+    @mock.patch.object(smbfs.SmbfsDriver, '_lookup_local_volume_path')
+    @mock.patch.object(smbfs.SmbfsDriver, '_qemu_img_info')
+    @mock.patch.object(smbfs.SmbfsDriver, '_get_volume_format_spec')
+    def _mock_get_volume_format(self, mock_get_format_spec, mock_qemu_img_info,
+                                mock_lookup_volume, mock_get_path_template,
+                                qemu_format=False, volume_format='raw',
+                                volume_exists=True):
+        mock_get_path_template.return_value = self._FAKE_VOLUME_PATH
+        mock_lookup_volume.return_value = (
+            self._FAKE_VOLUME_PATH if volume_exists else None)
+
+        mock_qemu_img_info.return_value.file_format = volume_format
+        mock_get_format_spec.return_value = volume_format
+
+        ret_val = self._smbfs_driver.get_volume_format(self._FAKE_VOLUME,
+                                                       qemu_format)
+
+        if volume_exists:
+            mock_qemu_img_info.assert_called_once_with(self._FAKE_VOLUME_PATH,
+                                                       self._FAKE_VOLUME_NAME)
+            self.assertFalse(mock_get_format_spec.called)
+        else:
+            mock_get_format_spec.assert_called_once_with(self._FAKE_VOLUME)
+            self.assertFalse(mock_qemu_img_info.called)
+
+        return ret_val
+
+    def test_get_existing_raw_volume_format(self):
+        fmt = self._mock_get_volume_format()
+        self.assertEqual(fmt, 'raw')
+
+    def test_get_new_vhd_volume_format(self):
+        expected_fmt = 'vhd'
+        fmt = self._mock_get_volume_format(volume_format=expected_fmt,
+                                           volume_exists=False)
+        self.assertEqual(expected_fmt, fmt)
+
+    def test_get_new_vhd_legacy_volume_format(self):
+        img_fmt = 'vhd'
+        expected_fmt = 'vpc'
+        ret_val = self._mock_get_volume_format(volume_format=img_fmt,
+                                               volume_exists=False,
+                                               qemu_format=True)
+        self.assertEqual(expected_fmt, ret_val)
 
     def test_initialize_connection(self):
         self._smbfs_driver.get_active_image_from_info = mock.Mock(

diff --git a/cinder/volume/drivers/smbfs.py b/cinder/volume/drivers/smbfs.py
index 4509fb541d5ee33a9e1949e3190663840626f52e..ab2a66894cd8779c60921664c48dcf49d2b222db 100644 (file)
--- a/cinder/volume/drivers/smbfs.py
+++ b/cinder/volume/drivers/smbfs.py
@@ -166,12 +166,31 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver):
         """Get volume path (mounted locally fs path) for given volume.
         :param volume: volume reference
         """
+        volume_path_template = self._get_local_volume_path_template(volume)
+        volume_path = self._lookup_local_volume_path(volume_path_template)
+        if volume_path:
+            return volume_path
+
+        # The image does not exist, so retrieve the volume format
+        # in order to build the path.
         fmt = self.get_volume_format(volume)
-        local_dir = self._local_volume_dir(volume)
-        local_path = os.path.join(local_dir, volume['name'])
         if fmt in (self._DISK_FORMAT_VHD, self._DISK_FORMAT_VHDX):
-            local_path += '.' + fmt
-        return local_path
+            volume_path = volume_path_template + '.' + fmt
+        else:
+            volume_path = volume_path_template
+        return volume_path
+
+    def _get_local_volume_path_template(self, volume):
+        local_dir = self._local_volume_dir(volume)
+        local_path_template = os.path.join(local_dir, volume['name'])
+        return local_path_template
+
+    def _lookup_local_volume_path(self, volume_path_template):
+        for ext in ['', self._DISK_FORMAT_VHD, self._DISK_FORMAT_VHDX]:
+            volume_path = (volume_path_template + '.' + ext
+                           if ext else volume_path_template)
+            if os.path.exists(volume_path):
+                return volume_path
 
     def _local_path_volume_info(self, volume):
         return '%s%s' % (self.local_path(volume), '.info')
@@ -183,10 +202,10 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver):
         return snap_path
 
     def get_volume_format(self, volume, qemu_format=False):
-        volume_dir = self._local_volume_dir(volume)
-        volume_path = os.path.join(volume_dir, volume['name'])
+        volume_path_template = self._get_local_volume_path_template(volume)
+        volume_path = self._lookup_local_volume_path(volume_path_template)
 
-        if os.path.exists(volume_path):
+        if volume_path:
             info = self._qemu_img_info(volume_path, volume['name'])
             volume_format = info.file_format
         else: