From c2b0f8680a2526cd890c55ebaaa1efc06b94dfef Mon Sep 17 00:00:00 2001 From: Lucian Petrut Date: Tue, 17 Feb 2015 16:20:56 +0200 Subject: [PATCH] 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 --- cinder/tests/test_smbfs.py | 106 ++++++++++++++++++++++++++++++--- cinder/volume/drivers/smbfs.py | 33 +++++++--- 2 files changed, 124 insertions(+), 15 deletions(-) diff --git a/cinder/tests/test_smbfs.py b/cinder/tests/test_smbfs.py index 75fb43188..1d0d7d388 100644 --- 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 4509fb541..ab2a66894 100644 --- 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: -- 2.45.2