VMware:Use datastore selection logic in new module

Volumes created by the VMDK driver are stored in vCenter
managed datastores. A datastore is selected based on
requirements such as size, storage policy etc. Commit
2c672d1100ad4f44517838e17156b3ea6300b1cc introduced a
new module for datastore selection. It improved the
current logic and grouped the code which were scattered
in multiple places (in vmdk.py). This patch refactors
vmdk module to remove duplicate code and use the methods
in the new module.

Change-Id: I3b7cb8bd33ced2f9deed2936a9c02feb90fb70b7

diff --git a/cinder/tests/test_vmware_vmdk.py b/cinder/tests/test_vmware_vmdk.py
index 18db984db1a4ad20adbf4fceaa5d1d9f9851bac0..d64ca6b650c8e36f8e01a53c2ac021ec7ddf2bf1 100644 (file)
--- a/cinder/tests/test_vmware_vmdk.py
+++ b/cinder/tests/test_vmware_vmdk.py
@@ -277,34 +277,6 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase):
         self._driver.terminate_connection(mox.IgnoreArg(), mox.IgnoreArg(),
                                           force=mox.IgnoreArg())
 
-    def test_create_backing_in_inventory_multi_hosts(self):
-        """Test _create_backing_in_inventory scanning multiple hosts."""
-        m = self.mox
-        m.StubOutWithMock(self._driver.__class__, 'volumeops')
-        self._driver.volumeops = self._volumeops
-        host1 = FakeObj(obj=FakeMor('HostSystem', 'my_host1'))
-        host2 = FakeObj(obj=FakeMor('HostSystem', 'my_host2'))
-        retrieve_result = FakeRetrieveResult([host1, host2], None)
-        m.StubOutWithMock(self._volumeops, 'get_hosts')
-        self._volumeops.get_hosts().AndReturn(retrieve_result)
-        m.StubOutWithMock(self._driver, '_create_backing')
-        volume = FakeObject()
-        volume['name'] = 'vol_name'
-        backing = FakeMor('VirtualMachine', 'my_back')
-        mux = self._driver._create_backing(volume, host1.obj, {})
-        mux.AndRaise(exceptions.VimException('Maintenance mode'))
-        mux = self._driver._create_backing(volume, host2.obj, {})
-        mux.AndReturn(backing)
-        m.StubOutWithMock(self._volumeops, 'cancel_retrieval')
-        self._volumeops.cancel_retrieval(retrieve_result)
-        m.StubOutWithMock(self._volumeops, 'continue_retrieval')
-
-        m.ReplayAll()
-        result = self._driver._create_backing_in_inventory(volume)
-        self.assertEqual(result, backing)
-        m.UnsetStubs()
-        m.VerifyAll()
-
     def test_get_volume_group_folder(self):
         """Test _get_volume_group_folder."""
         m = self.mox
@@ -580,7 +552,7 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase):
                                                                   fake_size)
 
     @mock.patch.object(VMDK_DRIVER, '_extend_volumeops_virtual_disk')
-    @mock.patch.object(VMDK_DRIVER, '_create_backing_in_inventory')
+    @mock.patch.object(VMDK_DRIVER, '_create_backing')
     @mock.patch.object(VMDK_DRIVER, 'volumeops')
     def test_create_backing_by_copying(self, volumeops, create_backing,
                                        _extend_virtual_disk):
@@ -806,7 +778,7 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase):
     @mock.patch(
         'cinder.volume.drivers.vmware.vmdk.VMwareEsxVmdkDriver._get_disk_type')
     @mock.patch.object(VMDK_DRIVER, '_get_ds_name_folder_path')
-    @mock.patch.object(VMDK_DRIVER, '_create_backing_in_inventory')
+    @mock.patch.object(VMDK_DRIVER, '_create_backing')
     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,
@@ -881,7 +853,8 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase):
 
         create_params = {vmdk.CREATE_PARAM_DISK_LESS: True,
                          vmdk.CREATE_PARAM_BACKING_NAME: uuid}
-        create_backing.assert_called_once_with(volume, create_params)
+        create_backing.assert_called_once_with(volume,
+                                               create_params=create_params)
         create_disk_from_sparse_image.assert_called_once_with(
             context, image_service, image_id, image_size_in_bytes,
             dc_ref, ds_name, folder_path, uuid)
@@ -905,7 +878,8 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase):
             context, volume, image_service, image_id)
 
         del create_params[vmdk.CREATE_PARAM_BACKING_NAME]
-        create_backing.assert_called_once_with(volume, create_params)
+        create_backing.assert_called_once_with(volume,
+                                               create_params=create_params)
         create_disk_from_preallocated_image.assert_called_once_with(
             context, image_service, image_id, image_size_in_bytes,
             dc_ref, ds_name, folder_path, volume['name'], adapter_type)
@@ -1459,7 +1433,7 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase):
     @mock.patch('cinder.openstack.common.fileutils.file_open')
     @mock.patch.object(VMDK_DRIVER, '_temporary_file')
     @mock.patch('oslo_utils.uuidutils.generate_uuid')
-    @mock.patch.object(VMDK_DRIVER, '_create_backing_in_inventory')
+    @mock.patch.object(VMDK_DRIVER, '_create_backing')
     @mock.patch.object(VMDK_DRIVER, 'volumeops')
     @mock.patch.object(VMDK_DRIVER, 'session')
     def test_backup_volume(self, session, vops, create_backing, generate_uuid,
@@ -1824,13 +1798,95 @@ class VMwareVcVmdkDriverTestCase(VMwareEsxVmdkDriverTestCase):
         self.assertEqual(session_obj, self._driver.ds_sel._session)
 
     @mock.patch.object(VMDK_DRIVER, '_extend_volumeops_virtual_disk')
-    @mock.patch.object(VMDK_DRIVER, '_create_backing_in_inventory')
+    @mock.patch.object(VMDK_DRIVER, '_create_backing')
     @mock.patch.object(VMDK_DRIVER, 'volumeops')
     def test_create_backing_by_copying(self, volumeops, create_backing,
                                        extend_virtual_disk):
         self._test_create_backing_by_copying(volumeops, create_backing,
                                              extend_virtual_disk)
 
+    @mock.patch.object(VMDK_DRIVER, '_get_storage_profile')
+    @mock.patch.object(VMDK_DRIVER, 'ds_sel')
+    @mock.patch.object(VMDK_DRIVER, 'volumeops')
+    @mock.patch.object(VMDK_DRIVER, '_get_volume_group_folder')
+    def test_select_ds_for_volume(self, get_volume_group_folder, vops, ds_sel,
+                                  get_storage_profile):
+
+        profile = mock.sentinel.profile
+        get_storage_profile.return_value = profile
+
+        host_ref = mock.sentinel.host_ref
+        rp = mock.sentinel.rp
+        summary = mock.sentinel.summary
+        ds_sel.select_datastore.return_value = (host_ref, rp, summary)
+
+        dc = mock.sentinel.dc
+        vops.get_dc.return_value = dc
+        folder = mock.sentinel.folder
+        get_volume_group_folder.return_value = folder
+
+        host = mock.sentinel.host
+        vol = {'id': 'c1037b23-c5e9-4446-815f-3e097cbf5bb0', 'size': 1,
+               'name': 'vol-c1037b23-c5e9-4446-815f-3e097cbf5bb0'}
+        ret = self._driver._select_ds_for_volume(vol, host)
+
+        self.assertEqual((host_ref, rp, folder, summary), ret)
+        exp_req = {hub.DatastoreSelector.SIZE_BYTES: units.Gi,
+                   hub.DatastoreSelector.PROFILE_NAME: profile}
+        ds_sel.select_datastore.assert_called_once_with(exp_req, hosts=[host])
+        vops.get_dc.assert_called_once_with(rp)
+        get_volume_group_folder.assert_called_once_with(dc)
+
+    @mock.patch.object(VMDK_DRIVER, '_get_storage_profile')
+    @mock.patch.object(VMDK_DRIVER, 'ds_sel')
+    @mock.patch.object(VMDK_DRIVER, 'volumeops')
+    @mock.patch.object(VMDK_DRIVER, '_get_volume_group_folder')
+    def test_select_ds_for_volume_with_no_host(
+            self, get_volume_group_folder, vops, ds_sel, get_storage_profile):
+
+        profile = mock.sentinel.profile
+        get_storage_profile.return_value = profile
+
+        host_ref = mock.sentinel.host_ref
+        rp = mock.sentinel.rp
+        summary = mock.sentinel.summary
+        ds_sel.select_datastore.return_value = (host_ref, rp, summary)
+
+        dc = mock.sentinel.dc
+        vops.get_dc.return_value = dc
+        folder = mock.sentinel.folder
+        get_volume_group_folder.return_value = folder
+
+        vol = {'id': 'c1037b23-c5e9-4446-815f-3e097cbf5bb0', 'size': 1,
+               'name': 'vol-c1037b23-c5e9-4446-815f-3e097cbf5bb0'}
+        ret = self._driver._select_ds_for_volume(vol)
+
+        self.assertEqual((host_ref, rp, folder, summary), ret)
+        exp_req = {hub.DatastoreSelector.SIZE_BYTES: units.Gi,
+                   hub.DatastoreSelector.PROFILE_NAME: profile}
+        ds_sel.select_datastore.assert_called_once_with(exp_req, hosts=None)
+        vops.get_dc.assert_called_once_with(rp)
+        get_volume_group_folder.assert_called_once_with(dc)
+
+    @mock.patch.object(VMDK_DRIVER, '_get_storage_profile')
+    @mock.patch.object(VMDK_DRIVER, 'ds_sel')
+    def test_select_ds_for_volume_with_no_best_candidate(
+            self, ds_sel, get_storage_profile):
+
+        profile = mock.sentinel.profile
+        get_storage_profile.return_value = profile
+
+        ds_sel.select_datastore.return_value = ()
+
+        vol = {'id': 'c1037b23-c5e9-4446-815f-3e097cbf5bb0', 'size': 1,
+               'name': 'vol-c1037b23-c5e9-4446-815f-3e097cbf5bb0'}
+        self.assertRaises(vmdk_exceptions.NoValidDatastoreException,
+                          self._driver._select_ds_for_volume, vol)
+
+        exp_req = {hub.DatastoreSelector.SIZE_BYTES: units.Gi,
+                   hub.DatastoreSelector.PROFILE_NAME: profile}
+        ds_sel.select_datastore.assert_called_once_with(exp_req, hosts=None)
+
     @mock.patch.object(VMDK_DRIVER, 'volumeops')
     @mock.patch.object(VMDK_DRIVER, '_relocate_backing')
     def test_initialize_connection_with_instance_and_backing(
@@ -1885,9 +1941,9 @@ class VMwareVcVmdkDriverTestCase(VMwareEsxVmdkDriverTestCase):
 
     @mock.patch.object(VMDK_DRIVER, 'volumeops')
     @mock.patch.object(VMDK_DRIVER, '_relocate_backing')
-    @mock.patch.object(VMDK_DRIVER, '_create_backing_in_inventory')
+    @mock.patch.object(VMDK_DRIVER, '_create_backing')
     def test_initialize_connection_with_no_instance_and_no_backing(
-            self, create_backing_in_inv, relocate_backing, vops):
+            self, create_backing, relocate_backing, vops):
 
         vops.get_backing.return_value = None
 
@@ -1895,13 +1951,13 @@ class VMwareVcVmdkDriverTestCase(VMwareEsxVmdkDriverTestCase):
         vops.get_host.return_value = host
 
         backing = mock.Mock(value=mock.sentinel.backing_value)
-        create_backing_in_inv.return_value = backing
+        create_backing.return_value = backing
 
         connector = {}
         volume = {'name': 'vol-1', 'id': 1}
         conn_info = self._driver.initialize_connection(volume, connector)
 
-        create_backing_in_inv.assert_called_once_with(volume)
+        create_backing.assert_called_once_with(volume)
         self.assertFalse(relocate_backing.called)
 
         self.assertEqual('vmdk', conn_info['driver_volume_type'])
@@ -2312,7 +2368,7 @@ class VMwareVcVmdkDriverTestCase(VMwareEsxVmdkDriverTestCase):
     @mock.patch(
         'cinder.volume.drivers.vmware.vmdk.VMwareEsxVmdkDriver._get_disk_type')
     @mock.patch.object(VMDK_DRIVER, '_get_ds_name_folder_path')
-    @mock.patch.object(VMDK_DRIVER, '_create_backing_in_inventory')
+    @mock.patch.object(VMDK_DRIVER, '_create_backing')
     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,
@@ -2399,7 +2455,7 @@ class VMwareVcVmdkDriverTestCase(VMwareEsxVmdkDriverTestCase):
     @mock.patch('cinder.openstack.common.fileutils.file_open')
     @mock.patch.object(VMDK_DRIVER, '_temporary_file')
     @mock.patch('oslo_utils.uuidutils.generate_uuid')
-    @mock.patch.object(VMDK_DRIVER, '_create_backing_in_inventory')
+    @mock.patch.object(VMDK_DRIVER, '_create_backing')
     @mock.patch.object(VMDK_DRIVER, 'volumeops')
     @mock.patch.object(VMDK_DRIVER, 'session')
     def test_backup_volume(self, session, vops, create_backing, generate_uuid,
@@ -2449,17 +2505,17 @@ class VMwareVcVmdkDriverTestCase(VMwareEsxVmdkDriverTestCase):
             select_ds, session, get_storage_profile_id, get_disk_type, vops,
             file_open, download_data, delete_temp_backing)
 
-    @mock.patch.object(VMDK_DRIVER, '_get_folder_ds_summary')
+    @mock.patch.object(VMDK_DRIVER, '_select_ds_for_volume')
     @mock.patch.object(VMDK_DRIVER, 'volumeops')
-    def test_create_backing_with_params(self, vops, get_folder_ds_summary):
+    def test_create_backing_with_params(self, vops, select_ds_for_volume):
+        host = mock.sentinel.host
         resource_pool = mock.sentinel.resource_pool
-        vops.get_dss_rp.return_value = (mock.Mock(), resource_pool)
         folder = mock.sentinel.folder
         summary = mock.sentinel.summary
-        get_folder_ds_summary.return_value = (folder, summary)
+        select_ds_for_volume.return_value = (host, resource_pool, folder,
+                                             summary)
 
         volume = {'name': 'vol-1', 'volume_type_id': None, 'size': 1}
-        host = mock.Mock()
         create_params = {vmdk.CREATE_PARAM_DISK_LESS: True}
         self._driver._create_backing(volume, host, create_params)
 

diff --git a/cinder/volume/drivers/vmware/exceptions.py b/cinder/volume/drivers/vmware/exceptions.py
index 48fdd8d70e485f21d993f7e2c606731852982d35..6c44d90c883340dfb6e270026b4fbbbaa2bfff17 100644 (file)
--- a/cinder/volume/drivers/vmware/exceptions.py
+++ b/cinder/volume/drivers/vmware/exceptions.py
@@ -40,3 +40,8 @@ class VirtualDiskNotFoundException(exceptions.VMwareDriverException):
 class ProfileNotFoundException(exceptions.VMwareDriverException):
     """Thrown when the given storage profile cannot be found."""
     msg_fmt = _("Storage profile: %(storage_profile)s not found.")
+
+
+class NoValidDatastoreException(exceptions.VMwareDriverException):
+    """Thrown when there are no valid datastores."""
+    message = _("There are no valid datastores.")

diff --git a/cinder/volume/drivers/vmware/vmdk.py b/cinder/volume/drivers/vmware/vmdk.py
index ca555a67f07b0913bf7f2ea8738824e2aa7677ea..f3f3fbd48bd6ac00061655de9c5eb15ea5c637b3 100644 (file)
--- a/cinder/volume/drivers/vmware/vmdk.py
+++ b/cinder/volume/drivers/vmware/vmdk.py
@@ -44,6 +44,7 @@ from cinder.openstack.common import fileutils
 from cinder.openstack.common import log as logging
 from cinder.volume import driver
 from cinder.volume.drivers.vmware import datastore as hub
+from cinder.volume.drivers.vmware import exceptions as vmdk_exceptions
 from cinder.volume.drivers.vmware import volumeops
 from cinder.volume import volume_types
 
@@ -511,9 +512,11 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver):
                 profile_id = profile.uniqueId
         return profile_id
 
-    def _create_backing(self, volume, host, create_params=None):
+    def _create_backing(self, volume, host=None, create_params=None):
         """Create volume backing under the given host.
 
+        If host is unspecified, any suitable host is selected.
+
         :param volume: Volume object
         :param host: Reference of the host
         :param create_params: Dictionary specifying optional parameters for
@@ -521,12 +524,8 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver):
         :return: Reference to the created backing
         """
         create_params = create_params or {}
-        # Get datastores and resource pool of the host
-        (datastores, resource_pool) = self.volumeops.get_dss_rp(host)
-        # Pick a folder and datastore to create the volume backing on
-        (folder, summary) = self._get_folder_ds_summary(volume,
-                                                        resource_pool,
-                                                        datastores)
+        (host_ref, resource_pool, folder,
+         summary) = self._select_ds_for_volume(volume, host)
 
         # check if a storage profile needs to be associated with the backing VM
         profile_id = self._get_storage_profile_id(volume)
@@ -543,7 +542,7 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver):
             return self.volumeops.create_backing_disk_less(backing_name,
                                                            folder,
                                                            resource_pool,
-                                                           host,
+                                                           host_ref,
                                                            summary.name,
                                                            profile_id)
 
@@ -557,7 +556,7 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver):
                                              disk_type,
                                              folder,
                                              resource_pool,
-                                             host,
+                                             host_ref,
                                              summary.name,
                                              profile_id,
                                              adapter_type)
@@ -565,83 +564,33 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver):
     def _relocate_backing(self, volume, backing, host):
         pass
 
-    def _select_ds_for_volume(self, volume):
-        """Select datastore that can accommodate a volume of given size.
+    def _select_ds_for_volume(self, volume, host=None):
+        """Select datastore that can accommodate the given volume's backing.
 
         Returns the selected datastore summary along with a compute host and
         its resource pool and folder where the volume can be created
-        :return: (host, rp, folder, summary)
+        :return: (host, resource_pool, folder, summary)
         """
-        retrv_result = self.volumeops.get_hosts()
-        while retrv_result:
-            hosts = retrv_result.objects
-            if not hosts:
-                break
-            (selected_host, rp, folder, summary) = (None, None, None, None)
-            for host in hosts:
-                host = host.obj
-                try:
-                    (dss, rp) = self.volumeops.get_dss_rp(host)
-                    (folder, summary) = self._get_folder_ds_summary(volume,
-                                                                    rp, dss)
-                    selected_host = host
-                    break
-                except exceptions.VimException as excep:
-                    LOG.warn(_LW("Unable to find suitable datastore for volume"
-                                 " of size: %(vol)s GB under host: %(host)s. "
-                                 "More details: %(excep)s"),
-                             {'vol': volume['size'],
-                              'host': host, 'excep': excep})
-            if selected_host:
-                self.volumeops.cancel_retrieval(retrv_result)
-                return (selected_host, rp, folder, summary)
-            retrv_result = self.volumeops.continue_retrieval(retrv_result)
-
-        msg = _("Unable to find host to accommodate a disk of size: %s "
-                "in the inventory.") % volume['size']
-        LOG.error(msg)
-        raise exceptions.VimException(msg)
-
-    def _create_backing_in_inventory(self, volume, create_params=None):
-        """Creates backing under any suitable host.
-
-        The method tries to pick datastore that can fit the volume under
-        any host in the inventory.
+        # Form requirements for datastore selection.
+        req = {}
+        req[hub.DatastoreSelector.SIZE_BYTES] = (volume['size'] * units.Gi)
+        req[hub.DatastoreSelector.PROFILE_NAME] = self._get_storage_profile(
+            volume)
+
+        # Select datastore satisfying the requirements.
+        hosts = [host] if host else None
+        best_candidate = self.ds_sel.select_datastore(req, hosts=hosts)
+        if not best_candidate:
+            LOG.error(_LE("There is no valid datastore to create backing for "
+                          "volume: %s."),
+                      volume['name'])
+            raise vmdk_exceptions.NoValidDatastoreException()
 
-        :param volume: Volume object
-        :param create_params: Dictionary specifying optional parameters for
-                              backing VM creation
-        :return: Reference to the created backing
-        """
-        create_params = create_params or {}
-        retrv_result = self.volumeops.get_hosts()
-        while retrv_result:
-            hosts = retrv_result.objects
-            if not hosts:
-                break
-            backing = None
-            for host in hosts:
-                try:
-                    backing = self._create_backing(volume,
-                                                   host.obj,
-                                                   create_params)
-                    if backing:
-                        break
-                except exceptions.VimException as excep:
-                    LOG.warn(_LW("Unable to find suitable datastore for "
-                                 "volume: %(vol)s under host: %(host)s. "
-                                 "More details: %(excep)s"),
-                             {'vol': volume['name'],
-                              'host': host.obj, 'excep': excep})
-            if backing:
-                self.volumeops.cancel_retrieval(retrv_result)
-                return backing
-            retrv_result = self.volumeops.continue_retrieval(retrv_result)
-
-        msg = _("Unable to create volume: %s in the "
-                "inventory.") % volume['name']
-        LOG.error(msg)
-        raise exceptions.VimException(msg)
+        (host_ref, resource_pool, summary) = best_candidate
+        dc = self.volumeops.get_dc(resource_pool)
+        folder = self._get_volume_group_folder(dc)
+
+        return (host_ref, resource_pool, folder, summary)
 
     def _initialize_connection(self, volume, connector):
         """Get information of volume's backing.
@@ -682,7 +631,7 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver):
                 LOG.warn(_LW("Trying to boot from an empty volume: %s."),
                          volume['name'])
                 # Create backing
-                backing = self._create_backing_in_inventory(volume)
+                backing = self._create_backing(volume)
 
         # Set volume's moref value and name
         connection_info['data'] = {'volume': backing.value,
@@ -805,7 +754,7 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver):
         """
 
         # Create a backing
-        backing = self._create_backing_in_inventory(volume)
+        backing = self._create_backing(volume)
         dest_vmdk_path = self.volumeops.get_vmdk_path(backing)
         datacenter = self.volumeops.get_dc(backing)
         # Deleting the current VMDK file
@@ -1101,7 +1050,7 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver):
                   "%(param)s.",
                   {'id': volume['id'],
                    'param': create_params})
-        backing = self._create_backing_in_inventory(volume, create_params)
+        backing = self._create_backing(volume, create_params=create_params)
 
         try:
             # Find the backing's datacenter, host, datastore and folder.
@@ -1366,7 +1315,7 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver):
         if not backing:
             LOG.info(_LI("Backing not found, creating for volume: %s"),
                      volume['name'])
-            backing = self._create_backing_in_inventory(volume)
+            backing = self._create_backing(volume)
         vmdk_file_path = self.volumeops.get_vmdk_path(backing)
 
         # Upload image from vmdk
@@ -1648,7 +1597,7 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver):
         backing = self.volumeops.get_backing(volume['name'])
         if backing is None:
             LOG.debug("Creating backing for volume: %s.", volume['name'])
-            backing = self._create_backing_in_inventory(volume)
+            backing = self._create_backing(volume)
 
         tmp_vmdk_name = uuidutils.generate_uuid()
         with self._temporary_file(suffix=".vmdk",