From f3ee3dbf4e4794d77cf263be3eec144328a3bedf Mon Sep 17 00:00:00 2001 From: Vipin Balachandran Date: Fri, 10 May 2019 16:48:22 -0700 Subject: [PATCH] Revert "Implement volume capacity stats for VMware" This reverts commit a742569dc96af4ceb5b6fb53300d8eafeffff306. Commit a742569dc96af4ceb5b6fb53300d8eafeffff306 introduced serious performance issues. Our scale tests revealed ~20% degradation in various rally tests after this commit. Our experiments show that the current implementation does not scale well for a vCenter installation with 30 datastores which is a smaller size deployment for OpenStack clouds. The patch introduced a config option 'vmware_storage_profile'. When it is set, the driver makes periodic expensive PBM API calls which put lot of load on vCenter and the controller VM. Change-Id: I838c32277624db3b82db65d7ecfe81e2a779e99a --- .../unit/volume/drivers/vmware/test_fcd.py | 34 +---- .../volume/drivers/vmware/test_vmware_vmdk.py | 91 ++++--------- cinder/volume/drivers/vmware/vmdk.py | 126 +++--------------- ...vert_datastore_stats-ba85b30612970d91.yaml | 6 + 4 files changed, 55 insertions(+), 202 deletions(-) create mode 100644 releasenotes/notes/vmware_revert_datastore_stats-ba85b30612970d91.yaml diff --git a/cinder/tests/unit/volume/drivers/vmware/test_fcd.py b/cinder/tests/unit/volume/drivers/vmware/test_fcd.py index 28df16d084d..c8ba7845bd8 100644 --- a/cinder/tests/unit/volume/drivers/vmware/test_fcd.py +++ b/cinder/tests/unit/volume/drivers/vmware/test_fcd.py @@ -42,7 +42,6 @@ class VMwareVStorageObjectDriverTestCase(test.TestCase): IP = 'localhost' PORT = 2321 IMG_TX_TIMEOUT = 10 - RESERVED_PERCENTAGE = 0 VMDK_DRIVER = vmdk.VMwareVcVmdkDriver FCD_DRIVER = fcd.VMwareVStorageObjectDriver VC_VERSION = "6.7.0" @@ -63,7 +62,6 @@ class VMwareVStorageObjectDriverTestCase(test.TestCase): self._config.vmware_host_ip = self.IP self._config.vmware_host_port = self.PORT self._config.vmware_image_transfer_timeout_secs = self.IMG_TX_TIMEOUT - self._config.reserved_percentage = self.RESERVED_PERCENTAGE self._driver = fcd.VMwareVStorageObjectDriver( configuration=self._config) self._driver._vc_version = self.VC_VERSION @@ -81,39 +79,15 @@ class VMwareVStorageObjectDriverTestCase(test.TestCase): self.assertTrue(self._driver._use_fcd_snapshot) self.assertTrue(self._driver._storage_policy_enabled) - @mock.patch.object(VMDK_DRIVER, 'volumeops') - @mock.patch.object(VMDK_DRIVER, '_get_datastore_summaries') - def test_get_volume_stats(self, _get_datastore_summaries, vops): - FREE_GB = 7 - TOTAL_GB = 11 - - class ObjMock(object): - def __init__(self, **kwargs): - self.__dict__.update(kwargs) - - _get_datastore_summaries.return_value = \ - ObjMock(objects= [ - ObjMock(propSet = [ - ObjMock(name = "host", - val = ObjMock(DatastoreHostMount = [])), - ObjMock(name = "summary", - val = ObjMock(freeSpace = FREE_GB * units.Gi, - capacity = TOTAL_GB * units.Gi, - accessible = True)) - ]) - ]) - - vops._in_maintenance.return_value = False - + def test_get_volume_stats(self): stats = self._driver.get_volume_stats() self.assertEqual('VMware', stats['vendor_name']) self.assertEqual(self._driver.VERSION, stats['driver_version']) self.assertEqual(self._driver.STORAGE_TYPE, stats['storage_protocol']) - self.assertEqual(self.RESERVED_PERCENTAGE, - stats['reserved_percentage']) - self.assertEqual(TOTAL_GB, stats['total_capacity_gb']) - self.assertEqual(FREE_GB, stats['free_capacity_gb']) + self.assertEqual(0, stats['reserved_percentage']) + self.assertEqual('unknown', stats['total_capacity_gb']) + self.assertEqual('unknown', stats['free_capacity_gb']) def _create_volume_dict(self, vol_id=VOL_ID, diff --git a/cinder/tests/unit/volume/drivers/vmware/test_vmware_vmdk.py b/cinder/tests/unit/volume/drivers/vmware/test_vmware_vmdk.py index f3fce1ec37b..a013b83b44f 100644 --- a/cinder/tests/unit/volume/drivers/vmware/test_vmware_vmdk.py +++ b/cinder/tests/unit/volume/drivers/vmware/test_vmware_vmdk.py @@ -34,29 +34,15 @@ from cinder import test from cinder.tests.unit import fake_constants from cinder.tests.unit import fake_snapshot from cinder.tests.unit import fake_volume +from cinder.volume import configuration 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 vmdk from cinder.volume.drivers.vmware import volumeops -class MockConfiguration(object): - def __init__(self, **kwargs): - for kw in kwargs: - setattr(self, kw, kwargs[kw]) - - def safe_get(self, name): - return getattr(self, name) if hasattr(self, name) else None - - def append_config_values(self, opts): - for opt in opts: - if not hasattr(self, opt.name): - setattr(self, opt.name, opt.default or None) - # TODO(vbala) Split test methods handling multiple cases into multiple methods, # each handling a specific case. - - @ddt.ddt class VMwareVcVmdkDriverTestCase(test.TestCase): """Unit tests for VMwareVcVmdkDriver.""" @@ -94,29 +80,27 @@ class VMwareVcVmdkDriverTestCase(test.TestCase): def setUp(self): super(VMwareVcVmdkDriverTestCase, self).setUp() - self._config = MockConfiguration( - vmware_host_ip=self.IP, - vmware_host_port=self.PORT, - vmware_host_username=self.USERNAME, - vmware_host_password=self.PASSWORD, - vmware_wsdl_location=None, - vmware_volume_folder=self.VOLUME_FOLDER, - vmware_api_retry_count=self.API_RETRY_COUNT, - vmware_task_poll_interval=self.TASK_POLL_INTERVAL, - vmware_image_transfer_timeout_secs=self.IMG_TX_TIMEOUT, - vmware_max_objects_retrieval=self.MAX_OBJECTS, - vmware_tmp_dir=self.TMP_DIR, - vmware_ca_file=self.CA_FILE, - vmware_insecure=False, - vmware_cluster_name=self.CLUSTERS, - vmware_host_version=self.DEFAULT_VC_VERSION, - vmware_connection_pool_size=self.POOL_SIZE, - vmware_adapter_type=self.ADAPTER_TYPE, - vmware_snapshot_format=self.SNAPSHOT_FORMAT, - vmware_lazy_create=True, - vmware_datastore_regex=None, - reserved_percentage=0 - ) + self._config = mock.Mock(spec=configuration.Configuration) + self._config.vmware_host_ip = self.IP + self._config.vmware_host_port = self.PORT + self._config.vmware_host_username = self.USERNAME + self._config.vmware_host_password = self.PASSWORD + self._config.vmware_wsdl_location = None + self._config.vmware_volume_folder = self.VOLUME_FOLDER + 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._config.vmware_tmp_dir = self.TMP_DIR + self._config.vmware_ca_file = self.CA_FILE + self._config.vmware_insecure = False + self._config.vmware_cluster_name = self.CLUSTERS + self._config.vmware_host_version = self.DEFAULT_VC_VERSION + self._config.vmware_connection_pool_size = self.POOL_SIZE + self._config.vmware_adapter_type = self.ADAPTER_TYPE + self._config.vmware_snapshot_format = self.SNAPSHOT_FORMAT + self._config.vmware_lazy_create = True + self._config.vmware_datastore_regex = None self._db = mock.Mock() self._driver = vmdk.VMwareVcVmdkDriver(configuration=self._config, @@ -124,38 +108,15 @@ class VMwareVcVmdkDriverTestCase(test.TestCase): self._context = context.get_admin_context() - @mock.patch.object(VMDK_DRIVER, 'volumeops') - @mock.patch.object(VMDK_DRIVER, '_get_datastore_summaries') - def test_get_volume_stats(self, _get_datastore_summaries, vops): - FREE_GB = 7 - TOTAL_GB = 11 - - class ObjMock(object): - def __init__(self, **kwargs): - self.__dict__.update(kwargs) - - _get_datastore_summaries.return_value = (ObjMock(objects= [ - ObjMock(propSet = [ - ObjMock(name = "host", - val = ObjMock(DatastoreHostMount = [])), - ObjMock(name = "summary", - val = ObjMock(freeSpace = FREE_GB * units.Gi, - capacity = TOTAL_GB * units.Gi, - accessible = True)) - ]) - ])) - - vops._in_maintenance.return_value = False - + def test_get_volume_stats(self): stats = self._driver.get_volume_stats() self.assertEqual('VMware', stats['vendor_name']) self.assertEqual(self._driver.VERSION, stats['driver_version']) self.assertEqual('vmdk', stats['storage_protocol']) - self.assertEqual(self._config.reserved_percentage, - stats['reserved_percentage']) - self.assertEqual(TOTAL_GB, stats['total_capacity_gb']) - self.assertEqual(FREE_GB, stats['free_capacity_gb']) + self.assertEqual(0, stats['reserved_percentage']) + self.assertEqual('unknown', stats['total_capacity_gb']) + self.assertEqual('unknown', stats['free_capacity_gb']) self.assertFalse(stats['shared_targets']) def _create_volume_dict(self, diff --git a/cinder/volume/drivers/vmware/vmdk.py b/cinder/volume/drivers/vmware/vmdk.py index 503d9be86e9..57b8fcb6170 100644 --- a/cinder/volume/drivers/vmware/vmdk.py +++ b/cinder/volume/drivers/vmware/vmdk.py @@ -36,7 +36,6 @@ from oslo_vmware import exceptions from oslo_vmware import image_transfer from oslo_vmware import pbm from oslo_vmware import vim_util -import six from cinder import exception from cinder.i18n import _ @@ -132,7 +131,10 @@ vmdk_opts = [ help='Name of a vCenter compute cluster where volumes ' 'should be created.'), cfg.MultiStrOpt('vmware_storage_profile', - help='Names of storage profiles to be monitored.'), + help='Names of storage profiles to be monitored.', + deprecated_for_removal=True, + deprecated_reason='Setting this option results in ' + 'significant performance degradation.'), cfg.IntOpt('vmware_connection_pool_size', default=10, help='Maximum number of connections in http connection pool.'), @@ -268,7 +270,8 @@ class VMwareVcVmdkDriver(driver.VolumeDriver): # 3.3.0 - config option to specify datastore name regex # 3.4.0 - added NFS41 as a supported datastore type # 3.4.1 - volume capacity stats implemented - VERSION = '3.4.1' + # 3.4.2 - deprecated option vmware_storage_profile + VERSION = '3.4.2' # ThirdPartySystems wiki page CI_WIKI_NAME = "VMware_CI" @@ -324,112 +327,21 @@ class VMwareVcVmdkDriver(driver.VolumeDriver): :param refresh: Whether to get refreshed information """ - if not self._stats or refresh: - self._stats = self._get_volume_stats() + if not self._stats: + backend_name = self.configuration.safe_get('volume_backend_name') + if not backend_name: + backend_name = self.__class__.__name__ + data = {'volume_backend_name': backend_name, + 'vendor_name': 'VMware', + 'driver_version': self.VERSION, + 'storage_protocol': 'vmdk', + 'reserved_percentage': 0, + 'total_capacity_gb': 'unknown', + 'free_capacity_gb': 'unknown', + 'shared_targets': False} + self._stats = data return self._stats - def _get_volume_stats(self): - backend_name = self.configuration.safe_get('volume_backend_name') - if not backend_name: - backend_name = self.__class__.__name__ - data = {'volume_backend_name': backend_name, - 'vendor_name': 'VMware', - 'driver_version': self.VERSION, - 'storage_protocol': 'vmdk', - 'reserved_percentage': self.configuration.reserved_percentage, - 'shared_targets': False} - ds_summaries = self._get_datastore_summaries() - available_hosts = self._get_hosts(self._clusters) - global_capacity = 0 - global_free = 0 - while True: - for ds in ds_summaries.objects: - ds_props = self._get_object_properties(ds) - summary = ds_props['summary'] - if self._is_datastore_accessible(summary, - ds_props['host'], - available_hosts): - global_capacity += summary.capacity - global_free += summary.freeSpace - if getattr(ds_summaries, 'token', None): - ds_summaries = self.volumeops.continue_retrieval(ds_summaries) - else: - break - data['total_capacity_gb'] = round(global_capacity / units.Gi) - data['free_capacity_gb'] = round(global_free / units.Gi) - return data - - def _get_datastore_summaries(self): - client_factory = self.session.vim.client.factory - object_specs = [] - if (self._storage_policy_enabled - and self.configuration.vmware_storage_profile): - # Get all available storage profiles on the vCenter and extract the - # IDs of those that we want to observe - profiles_ids = [] - for profile in pbm.get_all_profiles(self.session): - if profile.name in self.configuration.vmware_storage_profile: - profiles_ids.append(profile.profileId) - # Get all matching Datastores for each profile - datastores = {} - for profile_id in profiles_ids: - for pbm_hub in pbm.filter_hubs_by_profile(self.session, - None, - profile_id): - if pbm_hub.hubType != "Datastore": - # We are not interested in Datastore Clusters for now - continue - if pbm_hub.hubId not in datastores: - # Reconstruct a managed object reference to datastore - datastores[pbm_hub.hubId] = vim_util.get_moref( - pbm_hub.hubId, "Datastore") - # Build property collector object specs out of them - for datastore_ref in six.itervalues(datastores): - object_specs.append( - vim_util.build_object_spec(client_factory, - datastore_ref, - [])) - else: - # Build a catch-all object spec that would reach all datastores - object_specs.append( - vim_util.build_object_spec( - client_factory, - self.session.vim.service_content.rootFolder, - [vim_util.build_recursive_traversal_spec(client_factory)])) - prop_spec = vim_util.build_property_spec(client_factory, 'Datastore', - ['summary', 'host']) - filter_spec = vim_util.build_property_filter_spec(client_factory, - prop_spec, - object_specs) - options = client_factory.create('ns0:RetrieveOptions') - options.maxObjects = self.configuration.vmware_max_objects_retrieval - result = self.session.vim.RetrievePropertiesEx( - self.session.vim.service_content.propertyCollector, - specSet=[filter_spec], - options=options) - return result - - def _get_object_properties(self, obj_content): - props = {} - if hasattr(obj_content, 'propSet'): - prop_set = obj_content.propSet - if prop_set: - props = {prop.name: prop.val for prop in prop_set} - return props - - def _is_datastore_accessible(self, ds_summary, ds_host_mounts, - available_hosts): - # available_hosts empty => vmware_cluster_name not specified => don't - # filter by hosts - cluster_access_to_ds = not available_hosts - for host_mount in ds_host_mounts.DatastoreHostMount: - for avlbl_host in available_hosts: - if avlbl_host.value == host_mount.key.value: - cluster_access_to_ds = True - return (ds_summary.accessible - and not self.volumeops._in_maintenance(ds_summary) - and cluster_access_to_ds) - def _verify_volume_creation(self, volume): """Verify that the volume can be created. diff --git a/releasenotes/notes/vmware_revert_datastore_stats-ba85b30612970d91.yaml b/releasenotes/notes/vmware_revert_datastore_stats-ba85b30612970d91.yaml new file mode 100644 index 00000000000..bf84c6a3e7f --- /dev/null +++ b/releasenotes/notes/vmware_revert_datastore_stats-ba85b30612970d91.yaml @@ -0,0 +1,6 @@ +--- +deprecations: + - | + The config option ``vmware_storage_profile`` is now deprecated + and ignored. Setting this option results in performance degradation + of the controller and put lot of load on vCenter server.