From 010fe685e50a3680d8c8b9b6602677df90e815b3 Mon Sep 17 00:00:00 2001 From: Kartik Bommepally Date: Mon, 16 Sep 2013 03:23:23 -0700 Subject: [PATCH] VMware: Usng RetrvProprtisEx & does multi ESX scan The existing code uses deprecated API RetrieveProperties for querying through property collector. The public doc asks to use RetrievePropertiesEx instead. RetrievePropertiesEx works by retrieving results in serveral batches rather than retrieval in a single API call. This works well in environments containing large number of VMs or ESX hosts. There are three steps involved 1. Get first batch 2. Continue retrieving using a token 3. Cancel retrieval when not needed Batch size is user configurable, default is 100 results per batch. The review also fixes a careless programming error leading to scanning only 1 ESX host under the vCenter server for datastores. Fixes bug: 1227078 Fixes bug: 1229654 DocImpact Change-Id: I85a6f6a7e2d764b9843efbd0dd347fba3ef19310 --- cinder/tests/test_vmware_vmdk.py | 96 ++++++++++++++++++++++- cinder/volume/drivers/vmware/vim_util.py | 59 +++++++++++++- cinder/volume/drivers/vmware/vmdk.py | 59 ++++++++------ cinder/volume/drivers/vmware/volumeops.py | 43 ++++++++-- etc/cinder/cinder.conf.sample | 8 +- 5 files changed, 225 insertions(+), 40 deletions(-) diff --git a/cinder/tests/test_vmware_vmdk.py b/cinder/tests/test_vmware_vmdk.py index 973ed75e3..221a45c8a 100644 --- a/cinder/tests/test_vmware_vmdk.py +++ b/cinder/tests/test_vmware_vmdk.py @@ -108,6 +108,17 @@ class FakeProp(object): self.val = val +class FakeRetrieveResult(object): + def __init__(self, objects, token): + self.objects = objects + self.token = token + + +class FakeObj(object): + def __init__(self, obj=None): + self.obj = obj + + class VMwareEsxVmdkDriverTestCase(test.TestCase): """Test class for VMwareEsxVmdkDriver.""" @@ -118,6 +129,7 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): API_RETRY_COUNT = 3 TASK_POLL_INTERVAL = 5.0 IMG_TX_TIMEOUT = 10 + MAX_OBJECTS = 100 def setUp(self): super(VMwareEsxVmdkDriverTestCase, self).setUp() @@ -131,6 +143,7 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): self._config.vmware_api_retry_count = self.API_RETRY_COUNT self._config.vmware_task_poll_interval = self.TASK_POLL_INTERVAL self._config.vmware_image_transfer_timeout_secs = self.IMG_TX_TIMEOUT + self._config.vmware_max_objects_retrieval = self.MAX_OBJECTS self._driver = vmdk.VMwareEsxVmdkDriver(configuration=self._config) api_retry_count = self._config.vmware_api_retry_count, task_poll_interval = self._config.vmware_task_poll_interval, @@ -138,7 +151,8 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): self.PASSWORD, api_retry_count, task_poll_interval, create_session=False) - self._volumeops = volumeops.VMwareVolumeOps(self._session) + self._volumeops = volumeops.VMwareVolumeOps(self._session, + self.MAX_OBJECTS) self._vim = FakeVim() def test_retry(self): @@ -233,6 +247,34 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): m.UnsetStubs() m.VerifyAll() + def test_continue_retrieval(self): + """Test continue_retrieval.""" + m = self.mox + m.StubOutWithMock(api.VMwareAPISession, 'vim') + self._session.vim = self._vim + m.StubOutWithMock(self._session, 'invoke_api') + self._session.invoke_api(vim_util, 'continue_retrieval', + self._vim, mox.IgnoreArg()) + + m.ReplayAll() + self._volumeops.continue_retrieval(mox.IgnoreArg()) + m.UnsetStubs() + m.VerifyAll() + + def test_cancel_retrieval(self): + """Test cancel_retrieval.""" + m = self.mox + m.StubOutWithMock(api.VMwareAPISession, 'vim') + self._session.vim = self._vim + m.StubOutWithMock(self._session, 'invoke_api') + self._session.invoke_api(vim_util, 'cancel_retrieval', + self._vim, mox.IgnoreArg()) + + m.ReplayAll() + self._volumeops.cancel_retrieval(mox.IgnoreArg()) + m.UnsetStubs() + m.VerifyAll() + def test_get_backing(self): """Test get_backing.""" m = self.mox @@ -240,7 +282,26 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): self._session.vim = self._vim m.StubOutWithMock(self._session, 'invoke_api') self._session.invoke_api(vim_util, 'get_objects', - self._vim, 'VirtualMachine').AndReturn([]) + self._vim, 'VirtualMachine', + self.MAX_OBJECTS) + + m.ReplayAll() + self._volumeops.get_backing(mox.IgnoreArg()) + m.UnsetStubs() + m.VerifyAll() + + def test_get_backing_multiple_retrieval(self): + """Test get_backing with multiple retrieval.""" + m = self.mox + m.StubOutWithMock(api.VMwareAPISession, 'vim') + self._session.vim = self._vim + m.StubOutWithMock(self._session, 'invoke_api') + retrieve_result = FakeRetrieveResult([], 'my_token') + self._session.invoke_api(vim_util, 'get_objects', + self._vim, 'VirtualMachine', + self.MAX_OBJECTS).AndReturn(retrieve_result) + m.StubOutWithMock(self._volumeops, 'cancel_retrieval') + self._volumeops.continue_retrieval(retrieve_result) m.ReplayAll() self._volumeops.get_backing(mox.IgnoreArg()) @@ -337,8 +398,8 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): m.StubOutWithMock(api.VMwareAPISession, 'vim') self._session.vim = self._vim m.StubOutWithMock(self._session, 'invoke_api') - self._session.invoke_api(vim_util, 'get_objects', - self._vim, 'HostSystem') + self._session.invoke_api(vim_util, 'get_objects', self._vim, + 'HostSystem', self.MAX_OBJECTS) m.ReplayAll() self._volumeops.get_hosts() @@ -515,6 +576,33 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): m.UnsetStubs() m.VerifyAll() + 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() + backing = FakeMor('VirtualMachine', 'my_back') + mux = self._driver._create_backing(volume, host1.obj) + mux.AndRaise(error_util.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_datastore(self): """Test get_datastore.""" m = self.mox diff --git a/cinder/volume/drivers/vmware/vim_util.py b/cinder/volume/drivers/vmware/vim_util.py index 14af6c718..fd731901c 100644 --- a/cinder/volume/drivers/vmware/vim_util.py +++ b/cinder/volume/drivers/vmware/vim_util.py @@ -176,17 +176,23 @@ def build_property_filter_spec(client_factory, property_specs, object_specs): return property_filter_spec -def get_objects(vim, type, props_to_collect=None, all_properties=False): +def get_objects(vim, type, max_objects, props_to_collect=None, + all_properties=False): """Gets all managed object references of a specified type. + It is caller's responsibility to continue or cancel retrieval. + :param vim: Vim object :param type: Type of the managed object reference + :param max_objects: Maximum number of objects that should be returned in + a single call :param props_to_collect: Properties of the managed object reference to be collected :param all_properties: Whether all properties of the managed object reference are to be collected :return: All managed object references of a specified type """ + if not props_to_collect: props_to_collect = ['name'] @@ -201,8 +207,11 @@ def get_objects(vim, type, props_to_collect=None, all_properties=False): property_filter_spec = build_property_filter_spec(client_factory, [property_spec], [object_spec]) - return vim.RetrieveProperties(vim.service_content.propertyCollector, - specSet=[property_filter_spec]) + options = client_factory.create('ns0:RetrieveOptions') + options.maxObjects = max_objects + return vim.RetrievePropertiesEx(vim.service_content.propertyCollector, + specSet=[property_filter_spec], + options=options) def get_object_properties(vim, mobj, properties): @@ -214,6 +223,7 @@ def get_object_properties(vim, mobj, properties): to be retrieved :return: Properties of the managed object specified """ + client_factory = vim.client.factory if mobj is None: return None @@ -228,7 +238,48 @@ def get_object_properties(vim, mobj, properties): object_spec.skip = False property_filter_spec.propSet = [property_spec] property_filter_spec.objectSet = [object_spec] - return vim.RetrieveProperties(collector, specSet=[property_filter_spec]) + options = client_factory.create('ns0:RetrieveOptions') + options.maxObjects = 1 + retrieve_result = vim.RetrievePropertiesEx(collector, + specSet=[property_filter_spec], + options=options) + cancel_retrieval(vim, retrieve_result) + return retrieve_result.objects + + +def _get_token(retrieve_result): + """Get token from results to obtain next set of results. + + :retrieve_result: Result from the RetrievePropertiesEx API + :return: Token to obtain next set of results. None if no more results. + """ + return getattr(retrieve_result, 'token', None) + + +def cancel_retrieval(vim, retrieve_result): + """Cancels the retrive operation if necessary. + + :param vim: Vim object + :param retrieve_result: Result from the RetrievePropertiesEx API + """ + + token = _get_token(retrieve_result) + if token: + collector = vim.service_content.propertyCollector + vim.CancelRetrievePropertiesEx(collector, token=token) + + +def continue_retrieval(vim, retrieve_result): + """Continue retrieving results, if present. + + :param vim: Vim object + :param retrieve_result: Result from the RetrievePropertiesEx API + """ + + token = _get_token(retrieve_result) + if token: + collector = vim.service_content.propertyCollector + return vim.ContinueRetrievePropertiesEx(collector, token=token) def get_object_property(vim, mobj, property_name): diff --git a/cinder/volume/drivers/vmware/vmdk.py b/cinder/volume/drivers/vmware/vmdk.py index a63d63bce..f28321d72 100644 --- a/cinder/volume/drivers/vmware/vmdk.py +++ b/cinder/volume/drivers/vmware/vmdk.py @@ -69,6 +69,12 @@ vmdk_opts = [ default=7200, help='Timeout in seconds for VMDK volume transfer between ' 'Cinder and Glance.'), + cfg.IntOpt('vmware_max_objects_retrieval', + default=100, + help='Max number of objects to be retrieved per batch. ' + 'Query results will be obtained in batches from the ' + 'server and not in one shot. Server may still limit the ' + 'count to something less than the configured value.'), ] @@ -136,7 +142,9 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver): @property def volumeops(self): if not self._volumeops: - self._volumeops = volumeops.VMwareVolumeOps(self.session) + max_objects = self.configuration.vmware_max_objects_retrieval + self._volumeops = volumeops.VMwareVolumeOps(self.session, + max_objects) return self._volumeops def do_setup(self, context): @@ -154,7 +162,8 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver): raise exception.InvalidInput(_("%s not set.") % param) # Create the session object for the first time - self._volumeops = volumeops.VMwareVolumeOps(self.session) + max_objects = self.configuration.vmware_max_objects_retrieval + self._volumeops = volumeops.VMwareVolumeOps(self.session, max_objects) LOG.info(_("Successfully setup driver: %(driver)s for " "server: %(ip)s.") % {'driver': self.__class__.__name__, @@ -316,30 +325,32 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver): :param volume: Volume object :return: Reference to the created backing """ - # Get all hosts - hosts = self.volumeops.get_hosts() - if not hosts: - msg = _("There are no hosts in the inventory.") - LOG.error(msg) - raise error_util.VimException(msg) - backing = None - for host in hosts: - try: - host = hosts[0].obj - backing = self._create_backing(volume, host) + retrv_result = self.volumeops.get_hosts() + while retrv_result: + hosts = retrv_result.objects + if not hosts: break - except error_util.VimException as excep: - LOG.warn(_("Unable to find suitable datastore for " - "volume: %(vol)s under host: %(host)s. " - "More details: %(excep)s") % - {'vol': volume['name'], 'host': host, 'excep': excep}) - if backing: - return backing - msg = _("Unable to create volume: %(vol)s on the hosts: %(hosts)s.") - LOG.error(msg % {'vol': volume['name'], 'hosts': hosts}) - raise error_util.VimException(msg % - {'vol': volume['name'], 'hosts': hosts}) + backing = None + for host in hosts: + try: + backing = self._create_backing(volume, host.obj) + if backing: + break + except error_util.VimException as excep: + LOG.warn(_("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.") + LOG.error(msg % volume['name']) + raise error_util.VimException(msg % volume['name']) def _initialize_connection(self, volume, connector): """Get information of volume's backing. diff --git a/cinder/volume/drivers/vmware/volumeops.py b/cinder/volume/drivers/vmware/volumeops.py index a407e00f0..ee05456b5 100644 --- a/cinder/volume/drivers/vmware/volumeops.py +++ b/cinder/volume/drivers/vmware/volumeops.py @@ -61,8 +61,9 @@ def split_datastore_path(datastore_path): class VMwareVolumeOps(object): """Manages volume operations.""" - def __init__(self, session): + def __init__(self, session, max_objects): self._session = session + self._max_objects = max_objects def get_backing(self, name): """Get the backing based on name. @@ -70,11 +71,20 @@ class VMwareVolumeOps(object): :param name: Name of the backing :return: Managed object reference to the backing """ - vms = self._session.invoke_api(vim_util, 'get_objects', - self._session.vim, 'VirtualMachine') - for vm in vms: - if vm.propSet[0].val == name: - return vm.obj + + retrieve_result = self._session.invoke_api(vim_util, 'get_objects', + self._session.vim, + 'VirtualMachine', + self._max_objects) + while retrieve_result: + vms = retrieve_result.objects + for vm in vms: + if vm.propSet[0].val == name: + # We got the result, so cancel further retrieval. + self.cancel_retrieval(retrieve_result) + return vm.obj + # Result not obtained, continue retrieving results. + retrieve_result = self.continue_retrieval(retrieve_result) LOG.debug(_("Did not find any backing with name: %s") % name) @@ -108,7 +118,26 @@ class VMwareVolumeOps(object): :return: All the hosts from the inventory """ return self._session.invoke_api(vim_util, 'get_objects', - self._session.vim, 'HostSystem') + self._session.vim, + 'HostSystem', self._max_objects) + + def continue_retrieval(self, retrieve_result): + """Continue retrieval of results if necessary. + + :param retrieve_result: Result from RetrievePropertiesEx + """ + + return self._session.invoke_api(vim_util, 'continue_retrieval', + self._session.vim, retrieve_result) + + def cancel_retrieval(self, retrieve_result): + """Cancel retrieval of results if necessary. + + :param retrieve_result: Result from RetrievePropertiesEx + """ + + self._session.invoke_api(vim_util, 'cancel_retrieval', + self._session.vim, retrieve_result) def _is_valid(self, datastore, host): """Check if host's datastore is accessible, mounted and writable. diff --git a/etc/cinder/cinder.conf.sample b/etc/cinder/cinder.conf.sample index a839c283f..de3410cb5 100644 --- a/etc/cinder/cinder.conf.sample +++ b/etc/cinder/cinder.conf.sample @@ -1660,6 +1660,12 @@ # and Glance. (integer value) #vmware_image_transfer_timeout_secs=7200 +# Max number of objects to be retrieved per batch. Query +# results will be obtained in batches from the server and not +# in one shot. Server may still limit the count to something +# less than the configured value. (integer value) +#vmware_max_objects_retrieval=100 + # # Options defined in cinder.volume.drivers.windows.windows @@ -1772,4 +1778,4 @@ #volume_dd_blocksize=1M -# Total option count: 380 +# Total option count: 381 -- 2.45.2