From 2896715334d373a3d4873fcc255d98a358e7af1d Mon Sep 17 00:00:00 2001 From: Michael McAleer Date: Wed, 1 Aug 2018 14:38:17 +0100 Subject: [PATCH] VMAX Driver - Fix for get-pools and returned Service Levels This code change corrects the issue with not all Service Levels being returned by 'cinder get-pools' command. Change-Id: I09724aef341b6fd75891b76bab97edd7d32af0a6 Closes-Bug: #1784856 --- .../volume/drivers/dell_emc/vmax/test_vmax.py | 74 ++++++++++++- cinder/volume/drivers/dell_emc/vmax/common.py | 103 +++++++++++------- cinder/volume/drivers/dell_emc/vmax/fc.py | 1 + cinder/volume/drivers/dell_emc/vmax/iscsi.py | 1 + .../volume/drivers/dell_emc/vmax/provision.py | 2 +- cinder/volume/drivers/dell_emc/vmax/rest.py | 32 ++++-- cinder/volume/drivers/dell_emc/vmax/utils.py | 12 +- 7 files changed, 170 insertions(+), 55 deletions(-) diff --git a/cinder/tests/unit/volume/drivers/dell_emc/vmax/test_vmax.py b/cinder/tests/unit/volume/drivers/dell_emc/vmax/test_vmax.py index 6cb4317d0ca..0ed766c5f8b 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/vmax/test_vmax.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/vmax/test_vmax.py @@ -538,6 +538,18 @@ class VMAXCommonData(object): "fba_used_capacity": 5244.7, "reserved_cap_percent": 10} + array_info_wl = {'RestServerIp': '1.1.1.1', 'RestServerPort': 3448, + 'RestUserName': 'smc', 'RestPassword': 'smc', + 'SSLVerify': False, 'SerialNumber': array, + 'srpName': 'SRP_1', 'PortGroup': port_group_name_i, + 'SLO': 'Diamond', 'Workload': 'OLTP'} + + array_info_no_wl = {'RestServerIp': '1.1.1.1', 'RestServerPort': 3448, + 'RestUserName': 'smc', 'RestPassword': 'smc', + 'SSLVerify': False, 'SerialNumber': array, + 'srpName': 'SRP_1', 'PortGroup': port_group_name_i, + 'SLO': 'Diamond'} + volume_details = [{"cap_gb": 2, "num_of_storage_groups": 1, "volumeId": device_id, @@ -593,11 +605,19 @@ class VMAXCommonData(object): "remoteDeviceID": device_id2, "remoteSymmetrixID": remote_array}]}}]}} + # Service Levels / Workloads workloadtype = {"workloadId": ["OLTP", "OLTP_REP", "DSS", "DSS_REP"]} srp_slo_details = {"serviceLevelDemand": [ {"serviceLevelId": "None"}, {"serviceLevelId": "Diamond"}, {"serviceLevelId": "Gold"}, {"serviceLevelId": "Optimized"}]} slo_details = ['None', 'Diamond', 'Gold', 'Optimized'] + powermax_slo_details = {"sloId": ["Bronze", "Diamond", "Gold", + "Optimized", "Platinum", "Silver"]} + powermax_model_details = {"symmetrixId": array, + "model": "PowerMax_2000", + "ucode": "5978.1091.1092"} + vmax_slo_details = {"sloId": ["Diamond", "Optimized"]} + vmax_model_details = {"model": "VMAX450F"} # replication volume_snap_vx = {"snapshotLnks": [], @@ -1025,6 +1045,8 @@ class FakeRequestsSession(object): return_object = self.data.workloadtype elif 'compressionCapable' in url: return_object = self.data.compression_info + elif 'slo' in url: + return_object = self.data.powermax_slo_details elif 'replication' in url: return_object = self._replication(url) @@ -1853,11 +1875,18 @@ class VMAXRestTest(test.TestCase): self.data.array, self.data.srp) self.assertEqual(ref_details, srp_details) - def test_get_slo_list(self): - ref_settings = self.data.slo_details - slo_settings = self.rest.get_slo_list(self.data.array, self.data.srp) + def test_get_slo_list_powermax(self): + ref_settings = self.data.powermax_slo_details['sloId'] + slo_settings = self.rest.get_slo_list(self.data.array) self.assertEqual(ref_settings, slo_settings) + def test_get_slo_list_vmax(self): + ref_settings = ['Diamond'] + with mock.patch.object(self.rest, 'get_resource', + return_value=self.data.vmax_slo_details): + slo_settings = self.rest.get_slo_list(self.data.array) + self.assertEqual(ref_settings, slo_settings) + def test_get_workload_settings(self): ref_settings = self.data.workloadtype['workloadId'] wl_settings = self.rest.get_workload_settings( @@ -3193,6 +3222,13 @@ class VMAXRestTest(test.TestCase): # second iterator page self.assertTrue(2 == len(result_list)) + def test_get_vmax_model(self): + reference = 'PowerMax_2000' + with mock.patch.object(self.rest, '_get_request', + return_value=self.data.powermax_model_details): + self.assertEqual(self.rest.get_vmax_model(self.data.array), + reference) + class VMAXProvisionTest(test.TestCase): def setUp(self): @@ -3739,7 +3775,17 @@ class VMAXCommonTest(test.TestCase): configuration = FakeConfiguration(None, 'config_group', None, None) fc.VMAXFCDriver(configuration=configuration) - def test_get_slo_workload_combinations_success(self): + def test_get_slo_workload_combinations_powermax(self): + array_info = self.common.get_attributes_from_cinder_config() + finalarrayinfolist = self.common._get_slo_workload_combinations( + array_info) + self.assertTrue(len(finalarrayinfolist) > 1) + + @mock.patch.object(rest.VMAXRest, 'get_vmax_model', + return_value=VMAXCommonData.vmax_model_details['model']) + @mock.patch.object(rest.VMAXRest, 'get_slo_list', + return_value=VMAXCommonData.vmax_slo_details['sloId']) + def test_get_slo_workload_combinations_vmax(self, mck_slo, mck_model): array_info = self.common.get_attributes_from_cinder_config() finalarrayinfolist = self.common._get_slo_workload_combinations( array_info) @@ -4074,6 +4120,20 @@ class VMAXCommonTest(test.TestCase): data = self.common.update_volume_stats() self.assertEqual('CommonTests', data['volume_backend_name']) + def test_update_srp_stats_with_wl(self): + with mock.patch.object(self.rest, 'get_srp_by_name', + return_value=self.data.srp_details): + location_info, __, __, __, __ = self.common._update_srp_stats( + self.data.array_info_wl) + self.assertEqual(location_info, '000197800123#SRP_1#Diamond#OLTP') + + def test_update_srp_stats_no_wl(self): + with mock.patch.object(self.rest, 'get_srp_by_name', + return_value=self.data.srp_details): + location_info, __, __, __, __ = self.common._update_srp_stats( + self.data.array_info_no_wl) + self.assertEqual(location_info, '000197800123#SRP_1#Diamond') + def test_find_device_on_array_success(self): volume = self.data.test_volume extra_specs = self.data.extra_specs @@ -4407,8 +4467,10 @@ class VMAXCommonTest(test.TestCase): def test_set_vmax_extra_specs_no_srp_name(self): srp_record = self.common.get_attributes_from_cinder_config() - extra_specs = self.common._set_vmax_extra_specs({}, srp_record) - self.assertEqual('Optimized', extra_specs['slo']) + with mock.patch.object(self.rest, 'get_slo_list', + return_value=[]): + extra_specs = self.common._set_vmax_extra_specs({}, srp_record) + self.assertIsNone(extra_specs['slo']) def test_set_vmax_extra_specs_compr_disabled(self): with mock.patch.object(self.rest, 'is_compression_capable', diff --git a/cinder/volume/drivers/dell_emc/vmax/common.py b/cinder/volume/drivers/dell_emc/vmax/common.py index 3a4a18a0a84..5f5fd319a57 100644 --- a/cinder/volume/drivers/dell_emc/vmax/common.py +++ b/cinder/volume/drivers/dell_emc/vmax/common.py @@ -215,35 +215,44 @@ class VMAXCommon(object): """ try: array = array_info['SerialNumber'] - srp = array_info['srpName'] if self.failover: array = self.active_backend_id - # Get the srp slo & workload settings - slo_settings = self.rest.get_slo_list(array, srp) - # Remove 'None' from the list (so a 'None' slo is not combined - # with a workload, which is not permitted) + + slo_settings = self.rest.get_slo_list(array) + # Remove 'None' and 'Optimized from SL list, they cannot be mixed + # with workloads so will be added later again slo_list = [x for x in slo_settings if x.lower() not in ['none', 'optimized']] workload_settings = self.rest.get_workload_settings(array) workload_settings.append("None") - slo_workload_set = set( - ['%(slo)s:%(workload)s' % {'slo': slo, 'workload': workload} - for slo in slo_list for workload in workload_settings]) - # Add back in in the only allowed 'None' slo/ workload combination - slo_workload_set.add('None:None') - for x in slo_settings: - if 'optimized' == x.lower(): - slo_workload_set.add('Optimized:None') - break + slo_workload_set = set() + + if self.rest.is_next_gen_array(array): + for slo in slo_list: + slo_workload_set.add(slo) + slo_workload_set.add('None') + slo_workload_set.add('Optimized') + else: + slo_workload_set = set( + ['%(slo)s:%(workload)s' % {'slo': slo, + 'workload': workload} + for slo in slo_list for workload in workload_settings]) + slo_workload_set.add('None:None') + + if not any(self.rest.get_vmax_model(array) in x for x in + utils.VMAX_AFA_MODELS) and not \ + self.rest.is_next_gen_array(array): + slo_workload_set.add('Optimized:None') finalarrayinfolist = [] for sloWorkload in slo_workload_set: - # Doing a shallow copy will work as we are modifying - # only strings temparray_info = array_info.copy() - slo, workload = sloWorkload.split(':') - temparray_info['SLO'] = slo - temparray_info['Workload'] = workload + try: + slo, workload = sloWorkload.split(':') + temparray_info['SLO'] = slo + temparray_info['Workload'] = workload + except ValueError: + temparray_info['SLO'] = sloWorkload finalarrayinfolist.append(temparray_info) except Exception as e: exception_message = (_( @@ -870,21 +879,35 @@ class VMAXCommon(object): provisioned_capacity_gb, array_reserve_percent]) else: already_queried = True - pool_name = ("%(slo)s+%(workload)s+%(srpName)s+%(array)s" - % {'slo': array_info['SLO'], - 'workload': array_info['Workload'], - 'srpName': array_info['srpName'], - 'array': array_info['SerialNumber']}) + try: + pool_name = ("%(slo)s+%(workload)s+%(srpName)s+%(array)s" + % {'slo': array_info['SLO'], + 'workload': array_info['Workload'], + 'srpName': array_info['srpName'], + 'array': array_info['SerialNumber']}) + except KeyError: + pool_name = ("%(slo)s+%(srpName)s+%(array)s" + % {'slo': array_info['SLO'], + 'srpName': array_info['srpName'], + 'array': array_info['SerialNumber']}) if already_queried: # The dictionary will only have one key per VMAX # Construct the location info - temp_location_info = ( - ("%(arrayName)s#%(srpName)s#%(slo)s#%(workload)s" - % {'arrayName': array_info['SerialNumber'], - 'srpName': array_info['srpName'], - 'slo': array_info['SLO'], - 'workload': array_info['Workload']})) + try: + temp_location_info = ( + ("%(arrayName)s#%(srpName)s#%(slo)s#%(workload)s" + % {'arrayName': array_info['SerialNumber'], + 'srpName': array_info['srpName'], + 'slo': array_info['SLO'], + 'workload': array_info['Workload']})) + except KeyError: + temp_location_info = ( + ("%(arrayName)s#%(srpName)s#%(slo)s" + % {'arrayName': array_info['SerialNumber'], + 'srpName': array_info['srpName'], + 'slo': array_info['SLO']})) + pool = {'pool_name': pool_name, 'total_capacity_gb': arrays[array_info['SerialNumber']][0], @@ -981,11 +1004,17 @@ class VMAXCommon(object): 'free_capacity_gb': remainingManagedSpaceGbs, 'provisioned_capacity_gb': provisionedManagedSpaceGbs}) - location_info = ("%(arrayName)s#%(srpName)s#%(slo)s#%(workload)s" - % {'arrayName': array_info['SerialNumber'], - 'srpName': array_info['srpName'], - 'slo': array_info['SLO'], - 'workload': array_info['Workload']}) + try: + location_info = ("%(arrayName)s#%(srpName)s#%(slo)s#%(workload)s" + % {'arrayName': array_info['SerialNumber'], + 'srpName': array_info['srpName'], + 'slo': array_info['SLO'], + 'workload': array_info['Workload']}) + except KeyError: + location_info = ("%(arrayName)s#%(srpName)s#%(slo)s" + % {'arrayName': array_info['SerialNumber'], + 'srpName': array_info['srpName'], + 'slo': array_info['SLO']}) return (location_info, totalManagedSpaceGbs, remainingManagedSpaceGbs, provisionedManagedSpaceGbs, @@ -1561,8 +1590,7 @@ class VMAXCommon(object): 'wl': workload_from_extra_spec}) else: - slo_list = self.rest.get_slo_list( - pool_record['SerialNumber'], extra_specs[utils.SRP]) + slo_list = self.rest.get_slo_list(pool_record['SerialNumber']) if 'Optimized' in slo_list: slo_from_extra_spec = 'Optimized' elif 'Diamond' in slo_list: @@ -1604,6 +1632,7 @@ class VMAXCommon(object): self.version_dict = ( self.volume_metadata.gather_version_info( extra_specs[utils.ARRAY])) + return extra_specs def _delete_from_srp(self, array, device_id, volume_name, diff --git a/cinder/volume/drivers/dell_emc/vmax/fc.py b/cinder/volume/drivers/dell_emc/vmax/fc.py index b0f0e426545..30c3eb3bfa8 100644 --- a/cinder/volume/drivers/dell_emc/vmax/fc.py +++ b/cinder/volume/drivers/dell_emc/vmax/fc.py @@ -97,6 +97,7 @@ class VMAXFCDriver(san.SanDriver, driver.FibreChannelDriver): (bp/vmax-list-manage-existing) - Fix for SSL verification/cert application (bug #1772924) - Log VMAX metadata of a volume (bp vmax-metadata) + - Fix for get-pools command (bug #1784856) """ VERSION = "3.2.0" diff --git a/cinder/volume/drivers/dell_emc/vmax/iscsi.py b/cinder/volume/drivers/dell_emc/vmax/iscsi.py index 494e32c04e0..d58d4343598 100644 --- a/cinder/volume/drivers/dell_emc/vmax/iscsi.py +++ b/cinder/volume/drivers/dell_emc/vmax/iscsi.py @@ -102,6 +102,7 @@ class VMAXISCSIDriver(san.SanISCSIDriver): (bp/vmax-list-manage-existing) - Fix for SSL verification/cert application (bug #1772924) - Log VMAX metadata of a volume (bp vmax-metadata) + - Fix for get-pools command (bug #1784856) """ VERSION = "3.2.0" diff --git a/cinder/volume/drivers/dell_emc/vmax/provision.py b/cinder/volume/drivers/dell_emc/vmax/provision.py index 72cbd69c92e..8127fc3d346 100644 --- a/cinder/volume/drivers/dell_emc/vmax/provision.py +++ b/cinder/volume/drivers/dell_emc/vmax/provision.py @@ -457,7 +457,7 @@ class VMAXProvision(object): if slo and slo.lower() == 'none': slo = None - valid_slos = self.rest.get_slo_list(array, srp) + valid_slos = self.rest.get_slo_list(array) valid_workloads = self.rest.get_workload_settings(array) for valid_slo in valid_slos: if slo == valid_slo: diff --git a/cinder/volume/drivers/dell_emc/vmax/rest.py b/cinder/volume/drivers/dell_emc/vmax/rest.py index 5a41e9295d0..da3dc39ff82 100644 --- a/cinder/volume/drivers/dell_emc/vmax/rest.py +++ b/cinder/volume/drivers/dell_emc/vmax/rest.py @@ -34,6 +34,7 @@ requests.packages.urllib3.disable_warnings(urllib_exp.InsecureRequestWarning) LOG = logging.getLogger(__name__) SLOPROVISIONING = 'sloprovisioning' REPLICATION = 'replication' +SYSTEM = 'system' U4V_VERSION = '84' UCODE_5978 = '5978' retry_exc_tuple = (exception.VolumeBackendAPIException,) @@ -461,20 +462,21 @@ class VMAXRest(object): resource_name=srp, params=None) return srp_details - def get_slo_list(self, array, srp): + def get_slo_list(self, array): """Retrieve the list of slo's from the array :param array: the array serial number - :param srp: return service levels associated with this srp :returns: slo_list -- list of service level names """ slo_list = [] - res_name = '%s/service_level_demand_report' % srp - slo_dict = self.get_resource(array, SLOPROVISIONING, 'srp', - resource_name=res_name, version='90') - if slo_dict and slo_dict.get('serviceLevelDemand'): - for d in slo_dict['serviceLevelDemand']: - slo = d.get('serviceLevelId') + slo_dict = self.get_resource(array, SLOPROVISIONING, 'slo', + version='90') + if slo_dict and slo_dict.get('sloId'): + if any(self.get_vmax_model(array) in x for x in + utils.VMAX_AFA_MODELS): + if 'Optimized' in slo_dict.get('sloId'): + slo_dict['sloId'].remove('Optimized') + for slo in slo_dict['sloId']: if slo and slo not in slo_list: slo_list.append(slo) return slo_list @@ -494,6 +496,20 @@ class VMAXRest(object): workload_setting = wl_details['workloadId'] return workload_setting + def get_vmax_model(self, array): + """Get the VMAX model. + + :param array: the array serial number + :return: the VMAX model + """ + vmax_version = '' + system_uri = ("/%(version)s/system/symmetrix/%(array)s" % { + 'version': U4V_VERSION, 'array': array}) + system_info = self._get_request(system_uri, SYSTEM) + if system_info and system_info.get('model'): + vmax_version = system_info.get('model') + return vmax_version + def is_compression_capable(self, array): """Check if array is compression capable. diff --git a/cinder/volume/drivers/dell_emc/vmax/utils.py b/cinder/volume/drivers/dell_emc/vmax/utils.py index e6e167f64aa..2a44c4c6dfc 100644 --- a/cinder/volume/drivers/dell_emc/vmax/utils.py +++ b/cinder/volume/drivers/dell_emc/vmax/utils.py @@ -37,6 +37,7 @@ FC = 'fc' INTERVAL = 'interval' RETRIES = 'retries' VOLUME_ELEMENT_NAME_PREFIX = 'OS-' +VMAX_AFA_MODELS = ['VMAX250F', 'VMAX450F', 'VMAX850F', 'VMAX950F'] MAX_SRP_LENGTH = 16 TRUNCATE_5 = 5 TRUNCATE_27 = 27 @@ -582,9 +583,14 @@ class VMAXUtils(object): if 'none' in pool['pool_name'].lower(): extra_pools.append(pool) for pool in extra_pools: - slo = pool['pool_name'].split('+')[0] - srp = pool['pool_name'].split('+')[2] - array = pool['pool_name'].split('+')[3] + try: + slo = pool['pool_name'].split('+')[0] + srp = pool['pool_name'].split('+')[2] + array = pool['pool_name'].split('+')[3] + except IndexError: + slo = pool['pool_name'].split('+')[0] + srp = pool['pool_name'].split('+')[1] + array = pool['pool_name'].split('+')[2] new_pool_name = ('%(slo)s+%(srp)s+%(array)s' % {'slo': slo, 'srp': srp, 'array': array}) new_pool = deepcopy(pool)