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

diff --git a/cinder/tests/test_vmware_vmdk.py b/cinder/tests/test_vmware_vmdk.py
index 1a84e538cedbcca8c4c57964ddd607fe4f7941b4..d1411401b03f2a2024fd3d3377bfe448c645c85e 100644 (file)
--- 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 cb09a03aeca2bddfc2d0e3e3cebb67a60fc952f7..caaeb0fd5ce7cfee6cde2b6e0b6d7f760189afb9 100644 (file)
--- 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 8df802a3c70f82cc25c2c43035631ab4c0dec849..ddd9a9ce165d0db5cd6d0710bc6b8a1eb5ce1c7e 100644 (file)
--- 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 b3d02cdfbd0108e1da6a24036316612a8719363c..da50ae2cba1d9b1fefde7865339a04bb36d89c30 100644 (file)
--- 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 c3a4c68f1f7c41da1511a42843f173bc4faa0352..dc54eccb7d758938eed5fa185c70b0eada949947 100644 (file)
--- 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."""