From 3becb19a2402d615a901b65a8a52b09bc0c6576a Mon Sep 17 00:00:00 2001 From: Vipin Balachandran Date: Wed, 11 Feb 2015 18:24:56 +0530 Subject: [PATCH] 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 --- cinder/tests/test_vmware_vmdk.py | 146 ++++++++++++++------- cinder/volume/drivers/vmware/exceptions.py | 5 + cinder/volume/drivers/vmware/vmdk.py | 121 +++++------------ 3 files changed, 141 insertions(+), 131 deletions(-) diff --git a/cinder/tests/test_vmware_vmdk.py b/cinder/tests/test_vmware_vmdk.py index 18db984db..d64ca6b65 100644 --- 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 48fdd8d70..6c44d90c8 100644 --- 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 ca555a67f..f3f3fbd48 100644 --- 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", -- 2.45.2