From 1fdb38fd4b43c2685658c1a7aa6a56c92ac31585 Mon Sep 17 00:00:00 2001 From: Vipin Balachandran Date: Wed, 20 Aug 2014 17:10:39 +0530 Subject: [PATCH] VMware: Remove redundant extend disk API call Extend virtual disk API is called during volume creation from image if the image size is less than volume size. In the case of streamOptimized and sparse vmdk images, the size of the virtual disk created from the image can be greater than image size and equal to the volume size. For example, streamOptimized image created from a new 1GB volume has size 69120 bytes and the size of the virtual disk created from this image is 1GB. Therefore, relying on image size to invoke extend API might result in VIM API fault, if the virtual disk size is same as the target size (volume size). The fix is to read the current virtual disk size to decide whether extend disk needs to be invoked or not. Closes-Bug: #1301854 Change-Id: I990ac0b9aef68b3ef8b6d67188db8d44b5c29599 --- cinder/tests/test_vmware_vmdk.py | 50 +++++++++++++++------- cinder/tests/test_vmware_volumeops.py | 25 +++++++++++ cinder/volume/drivers/vmware/error_util.py | 5 +++ cinder/volume/drivers/vmware/vmdk.py | 23 +++++----- cinder/volume/drivers/vmware/volumeops.py | 12 ++++++ 5 files changed, 89 insertions(+), 26 deletions(-) diff --git a/cinder/tests/test_vmware_vmdk.py b/cinder/tests/test_vmware_vmdk.py index 1a84e538c..d1411401b 100644 --- a/cinder/tests/test_vmware_vmdk.py +++ b/cinder/tests/test_vmware_vmdk.py @@ -931,6 +931,7 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): fake_context, fake_volume, image_service, fake_image_id) + @mock.patch.object(VMDK_DRIVER, '_extend_vmdk_virtual_disk') @mock.patch('cinder.openstack.common.uuidutils.generate_uuid') @mock.patch.object(VMDK_DRIVER, '_select_ds_for_volume') @mock.patch.object(VMDK_DRIVER, 'volumeops') @@ -944,7 +945,7 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): def test_copy_image_to_volume_non_stream_optimized( self, create_backing, get_ds_name_folder_path, get_disk_type, create_disk_from_sparse_image, create_disk_from_preallocated_image, - vops, select_ds_for_volume, generate_uuid): + vops, select_ds_for_volume, generate_uuid, extend_disk): self._test_copy_image_to_volume_non_stream_optimized( create_backing, get_ds_name_folder_path, @@ -953,12 +954,13 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): create_disk_from_preallocated_image, vops, select_ds_for_volume, - generate_uuid) + generate_uuid, + extend_disk) def _test_copy_image_to_volume_non_stream_optimized( self, create_backing, get_ds_name_folder_path, get_disk_type, create_disk_from_sparse_image, create_disk_from_preallocated_image, - vops, select_ds_for_volume, generate_uuid): + vops, select_ds_for_volume, generate_uuid, extend_disk): image_size_in_bytes = 2 * units.Gi adapter_type = 'lsiLogic' image_meta = {'disk_format': 'vmdk', @@ -1000,11 +1002,15 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): create_disk_from_sparse_image.return_value = path create_disk_from_preallocated_image.return_value = path + volume_size = 2 + vops.get_disk_size.return_value = volume_size * units.Gi + context = mock.Mock() volume = {'name': 'volume_name', 'id': 'volume_id', - 'size': image_size_in_bytes} + 'size': volume_size} image_id = mock.Mock() + self._driver.copy_image_to_volume( context, volume, image_service, image_id) @@ -1022,11 +1028,14 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): volume['name'], backing, None, volumeops.FULL_CLONE_TYPE, summary.datastore, disk_type) vops.delete_backing.assert_called_once_with(backing) + self.assertFalse(extend_disk.called) + vops.get_disk_size.return_value = 1 * units.Gi create_backing.reset_mock() vops.attach_disk_to_backing.reset_mock() vops.delete_backing.reset_mock() image_meta['properties']['vmware_disktype'] = 'preallocated' + self._driver.copy_image_to_volume( context, volume, image_service, image_id) @@ -1038,13 +1047,17 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): vops.attach_disk_to_backing.assert_called_once_with( backing, image_size_in_bytes / units.Ki, disk_type, adapter_type, path.get_descriptor_ds_file_path()) + extend_disk.assert_called_once_with(volume['name'], volume['size']) + extend_disk.reset_mock() create_disk_from_preallocated_image.side_effect = ( error_util.VimException("Error")) + self.assertRaises(error_util.VimException, self._driver.copy_image_to_volume, context, volume, image_service, image_id) vops.delete_backing.assert_called_once_with(backing) + self.assertFalse(extend_disk.called) @mock.patch( 'cinder.volume.drivers.vmware.volumeops.FlatExtentVirtualDiskPath') @@ -1207,15 +1220,19 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): image_service, fake_image_id) self.assertFalse(volumeops.get_create_spec.called) - # If the volume size is greater then than the image size, + # If the volume size is greater then than the backing's disk size, # _extend_vmdk_virtual_disk will be called. _select_ds_for_volume.side_effect = None _select_ds_for_volume.return_value = (fake_host, fake_rp, fake_folder, fake_summary) profile_id = 'profile-1' get_profile_id.return_value = profile_id + + volumeops.get_disk_size.return_value = size + self._driver.copy_image_to_volume(fake_context, fake_volume, image_service, fake_image_id) + image_service.show.assert_called_with(fake_context, fake_image_id) _select_ds_for_volume.assert_called_with(fake_volume) get_profile_id.assert_called_once_with(fake_volume) @@ -1236,31 +1253,30 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): vm_create_spec= vm_import_spec, image_size=size) - _extend_virtual_disk.assert_called_with(fake_volume['name'], - fake_volume_size) - self.assertFalse(volumeops.get_backing.called) - self.assertFalse(volumeops.delete_backing.called) + _extend_virtual_disk.assert_called_once_with(fake_volume['name'], + fake_volume_size) - # If the volume size is not greater then than the image size, + # If the volume size is not greater then than backing's disk size, # _extend_vmdk_virtual_disk will not be called. - fake_volume_size = size_gb - fake_volume['size'] = fake_volume_size + volumeops.get_disk_size.return_value = fake_volume_size * units.Gi _extend_virtual_disk.reset_mock() + self._driver.copy_image_to_volume(fake_context, fake_volume, image_service, fake_image_id) + self.assertFalse(_extend_virtual_disk.called) - self.assertFalse(volumeops.get_backing.called) - self.assertFalse(volumeops.delete_backing.called) # If fetch_stream_optimized_image raises an exception, # get_backing and delete_backing will be called. fetch_optimized_image.side_effect = exception.CinderException + self.assertRaises(exception.CinderException, self._driver.copy_image_to_volume, fake_context, fake_volume, image_service, fake_image_id) volumeops.get_backing.assert_called_with(fake_volume['name']) volumeops.delete_backing.assert_called_with(fake_backing) + self.assertFalse(_extend_virtual_disk.called) def test_copy_volume_to_image_non_vmdk(self): """Test copy_volume_to_image for a non-vmdk disk format.""" @@ -1986,6 +2002,7 @@ class VMwareVcVmdkDriverTestCase(VMwareEsxVmdkDriverTestCase): """Test vmdk._extend_vmdk_virtual_disk.""" self._test_extend_vmdk_virtual_disk(volume_ops) + @mock.patch.object(VMDK_DRIVER, '_extend_vmdk_virtual_disk') @mock.patch('cinder.openstack.common.uuidutils.generate_uuid') @mock.patch.object(VMDK_DRIVER, '_select_ds_for_volume') @mock.patch.object(VMDK_DRIVER, 'volumeops') @@ -1999,7 +2016,7 @@ class VMwareVcVmdkDriverTestCase(VMwareEsxVmdkDriverTestCase): def test_copy_image_to_volume_non_stream_optimized( self, create_backing, get_ds_name_folder_path, get_disk_type, create_disk_from_sparse_image, create_disk_from_preallocated_image, - vops, select_ds_for_volume, generate_uuid): + vops, select_ds_for_volume, generate_uuid, extend_disk): self._test_copy_image_to_volume_non_stream_optimized( create_backing, get_ds_name_folder_path, @@ -2008,7 +2025,8 @@ class VMwareVcVmdkDriverTestCase(VMwareEsxVmdkDriverTestCase): create_disk_from_preallocated_image, vops, select_ds_for_volume, - generate_uuid) + generate_uuid, + extend_disk) @mock.patch( 'cinder.volume.drivers.vmware.volumeops.FlatExtentVirtualDiskPath') diff --git a/cinder/tests/test_vmware_volumeops.py b/cinder/tests/test_vmware_volumeops.py index cb09a03ae..caaeb0fd5 100644 --- a/cinder/tests/test_vmware_volumeops.py +++ b/cinder/tests/test_vmware_volumeops.py @@ -1062,6 +1062,31 @@ class VolumeOpsTestCase(test.TestCase): backing.__class__.__name__ = ' VirtualDiskSparseVer2BackingInfo' self.assertRaises(AssertionError, self.vops.get_vmdk_path, backing) + # Test with no disk device. + invoke_api.return_value = [] + self.assertRaises(error_util.VirtualDiskNotFoundException, + self.vops.get_vmdk_path, + backing) + + def test_get_disk_size(self): + # Test with valid disk device. + device = mock.Mock() + device.__class__.__name__ = 'VirtualDisk' + disk_size_bytes = 1024 + device.capacityInKB = disk_size_bytes / units.Ki + invoke_api = self.session.invoke_api + invoke_api.return_value = [device] + + self.assertEqual(disk_size_bytes, + self.vops.get_disk_size(mock.sentinel.backing)) + + # Test with no disk device. + invoke_api.return_value = [] + + self.assertRaises(error_util.VirtualDiskNotFoundException, + self.vops.get_disk_size, + mock.sentinel.backing) + def test_create_virtual_disk(self): task = mock.Mock() invoke_api = self.session.invoke_api diff --git a/cinder/volume/drivers/vmware/error_util.py b/cinder/volume/drivers/vmware/error_util.py index 8df802a3c..ddd9a9ce1 100644 --- a/cinder/volume/drivers/vmware/error_util.py +++ b/cinder/volume/drivers/vmware/error_util.py @@ -83,3 +83,8 @@ class InvalidDiskTypeException(VMwareDriverException): class ImageTransferException(VMwareDriverException): """Thrown when there is an error during image transfer.""" message = _("Error occurred during image transfer.") + + +class VirtualDiskNotFoundException(VMwareDriverException): + """Thrown when virtual disk is not found.""" + message = _("There is no virtual disk device.") diff --git a/cinder/volume/drivers/vmware/vmdk.py b/cinder/volume/drivers/vmware/vmdk.py index b3d02cdfb..da50ae2cb 100644 --- a/cinder/volume/drivers/vmware/vmdk.py +++ b/cinder/volume/drivers/vmware/vmdk.py @@ -1276,17 +1276,20 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver): {'id': volume['id'], 'image_id': image_id}) - # If the user-specified volume size is greater than image size, we need - # to extend the virtual disk to the capacity specified by the user. - volume_size_in_bytes = volume['size'] * units.Gi - if volume_size_in_bytes > image_size_in_bytes: - LOG.debug("Extending volume: %(id)s since the user specified " - "volume size (bytes): %(size)s is greater than image" - " size (bytes): %(image_size)s.", - {'id': volume['id'], - 'size': volume_size_in_bytes, - 'image_size': image_size_in_bytes}) + # If the user-specified volume size is greater than backing's + # current disk size, we should extend the disk. + volume_size = volume['size'] * units.Gi + backing = self.volumeops.get_backing(volume['name']) + disk_size = self.volumeops.get_disk_size(backing) + if volume_size > disk_size: + LOG.debug("Extending volume: %(name)s since the user specified " + "volume size (bytes): %(vol_size)d is greater than " + "backing's current disk size (bytes): %(disk_size)d.", + {'name': volume['name'], + 'vol_size': volume_size, + 'disk_size': disk_size}) self._extend_vmdk_virtual_disk(volume['name'], volume['size']) + # TODO(vbala): handle volume_size < disk_size case. def copy_volume_to_image(self, context, volume, image_service, image_meta): """Creates glance image from volume. diff --git a/cinder/volume/drivers/vmware/volumeops.py b/cinder/volume/drivers/vmware/volumeops.py index c3a4c68f1..dc54eccb7 100644 --- a/cinder/volume/drivers/vmware/volumeops.py +++ b/cinder/volume/drivers/vmware/volumeops.py @@ -1142,6 +1142,9 @@ class VMwareVolumeOps(object): if device.__class__.__name__ == "VirtualDisk": return device + LOG.error(_("Virtual disk device of backing: %s not found."), backing) + raise error_util.VirtualDiskNotFoundException() + def get_vmdk_path(self, backing): """Get the vmdk file name of the backing. @@ -1159,6 +1162,15 @@ class VMwareVolumeOps(object): raise AssertionError(msg) return backing.fileName + def get_disk_size(self, backing): + """Get disk size of the backing. + + :param backing: backing VM reference + :return: disk size in bytes + """ + disk_device = self._get_disk_device(backing) + return disk_device.capacityInKB * units.Ki + def _get_virtual_disk_create_spec(self, size_in_kb, adapter_type, disk_type): """Return spec for file-backed virtual disk creation.""" -- 2.45.2