]> review.fuel-infra Code Review - openstack-build/cinder-build.git/commitdiff
VMware: Usng RetrvProprtisEx & does multi ESX scan
authorKartik Bommepally <kbommepally@vmware.com>
Mon, 16 Sep 2013 10:23:23 +0000 (03:23 -0700)
committerKartik Bommepally <kbommepally@vmware.com>
Mon, 30 Sep 2013 05:05:43 +0000 (22:05 -0700)
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
cinder/volume/drivers/vmware/vim_util.py
cinder/volume/drivers/vmware/vmdk.py
cinder/volume/drivers/vmware/volumeops.py
etc/cinder/cinder.conf.sample

index 973ed75e341fe75be8aca51004374f206f4ee793..221a45c8a22482925c6392663f955ca9613dec7b 100644 (file)
@@ -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
index 14af6c718ae688ea2e8ebce5b1ade33d21ad89ef..fd731901c7145acf5707e63d1da18a572faabf0c 100644 (file)
@@ -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):
index a63d63bcebc847e94f9e6e8573b0e2a751b182d3..f28321d7277c55ee7531f9b32717c841998a8c1b 100644 (file)
@@ -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.
index a407e00f0e8ec54876a498a528372ba760ad407e..ee05456b56b22f2934a58bc067b7198c4305feef 100644 (file)
@@ -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.
index a839c283fe0a8927bdea5bb7abc6f4cb98b7f8ac..de3410cb546d6c7a794d2938443da16268697642 100644 (file)
 # 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
 #volume_dd_blocksize=1M
 
 
-# Total option count: 380
+# Total option count: 381