From aa0a232db4e47439091e4b161abaae8f16d49996 Mon Sep 17 00:00:00 2001 From: Helen Walsh Date: Tue, 18 Jul 2017 17:25:10 +0100 Subject: [PATCH] VMAX driver - align VMAX QOS settings with front end Currently, the QOS feature used in VMAX doesn't work with the frontend parameters for the total bandwidth and the total IOPs for guest. VMAX QOS should use the total_bytes_sec and total_iops_sec so QOS enforcement can be accomplished at both the front end (hypervisor) and the back end (VMAX), if required. Change-Id: I996543b57920309969ebd31262bc4c859a08fea0 Closes-Bug: #1705042 --- .../volume/drivers/dell_emc/vmax/test_vmax.py | 32 +++++- cinder/volume/drivers/dell_emc/vmax/common.py | 15 +-- cinder/volume/drivers/dell_emc/vmax/rest.py | 99 ++++++++++++------- 3 files changed, 101 insertions(+), 45 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 8e7f91dd04a..c356d545f98 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 @@ -2289,12 +2289,12 @@ class VMAXRestTest(test.TestCase): array = self.data.array extra_specs = self.data.extra_specs extra_specs['qos'] = { - 'maxIOPS': '4000', 'DistributionType': 'Always'} + 'total_iops_sec': '4000', 'DistributionType': 'Always'} return_value = self.rest.update_storagegroup_qos( array, "OS-QOS-SG", extra_specs) self.assertEqual(False, return_value) extra_specs['qos'] = { - 'DistributionType': 'onFailure', 'maxMBPS': '4000'} + 'DistributionType': 'onFailure', 'total_bytes_sec': '419430400'} return_value = self.rest.update_storagegroup_qos( array, "OS-QOS-SG", extra_specs) self.assertTrue(return_value) @@ -2304,7 +2304,8 @@ class VMAXRestTest(test.TestCase): storage_group = self.data.defaultstoragegroup_name extra_specs = self.data.extra_specs extra_specs['qos'] = { - 'maxIOPS': '4000', 'DistributionType': 'Wrong', 'maxMBPS': '4000'} + 'total_iops_sec': '4000', 'DistributionType': 'Wrong', + 'total_bytes_sec': '4194304000'} with mock.patch.object(self.rest, 'check_status_code_success', side_effect=[None, None, None, Exception]): self.assertRaises(exception.VolumeBackendAPIException, @@ -2315,6 +2316,31 @@ class VMAXRestTest(test.TestCase): array, "OS-QOS-SG", extra_specs) self.assertFalse(return_value) + def test_validate_qos_input_exception(self): + qos_extra_spec = { + 'total_iops_sec': 90, 'DistributionType': 'Wrong', + 'total_bytes_sec': 100} + input_key = 'total_iops_sec' + sg_value = 4000 + self.assertRaises(exception.VolumeBackendAPIException, + self.rest.validate_qos_input, input_key, sg_value, + qos_extra_spec, {}) + input_key = 'total_bytes_sec' + sg_value = 4000 + self.assertRaises(exception.VolumeBackendAPIException, + self.rest.validate_qos_input, input_key, sg_value, + qos_extra_spec, {}) + + def test_validate_qos_distribution_type(self): + qos_extra_spec = { + 'total_iops_sec': 4000, 'DistributionType': 'Always', + 'total_bytes_sec': 4194304000} + input_prop_dict = {'total_iops_sec': 4000} + sg_value = 'Always' + ret_prop_dict = self.rest.validate_qos_distribution_type( + sg_value, qos_extra_spec, input_prop_dict) + self.assertEqual(input_prop_dict, ret_prop_dict) + def test_get_rdf_group(self): with mock.patch.object(self.rest, 'get_resource') as mock_get: self.rest.get_rdf_group(self.data.array, self.data.rdf_group_no) diff --git a/cinder/volume/drivers/dell_emc/vmax/common.py b/cinder/volume/drivers/dell_emc/vmax/common.py index e61ac9a5d1d..4639e2ee3d5 100644 --- a/cinder/volume/drivers/dell_emc/vmax/common.py +++ b/cinder/volume/drivers/dell_emc/vmax/common.py @@ -33,7 +33,7 @@ from cinder.volume.drivers.dell_emc.vmax import provision from cinder.volume.drivers.dell_emc.vmax import rest from cinder.volume.drivers.dell_emc.vmax import utils from cinder.volume import utils as volume_utils - +from cinder.volume import volume_types LOG = logging.getLogger(__name__) CONF = cfg.CONF @@ -831,9 +831,11 @@ class VMAXCommon(object): qos_specs = {} extra_specs = self.utils.get_volumetype_extra_specs( volume, volume_type_id) - if hasattr(volume, "volume_type") and ( - volume.volume_type and volume.volume_type.qos_specs): - qos_specs = volume.volume_type.qos_specs + type_id = volume.volume_type_id + if type_id: + res = volume_types.get_volume_type_qos_specs(type_id) + qos_specs = res['qos_specs'] + config_group = None # If there are no extra specs then the default case is assumed. if extra_specs: @@ -1047,9 +1049,8 @@ class VMAXCommon(object): self.rest.set_rest_credentials(array_info) extra_specs = self._set_vmax_extra_specs(extra_specs, array_info) - if (qos_specs and qos_specs.specs - and qos_specs.consumer != "front-end"): - extra_specs['qos'] = qos_specs.specs + if qos_specs and qos_specs.get('consumer') != "front-end": + extra_specs['qos'] = qos_specs.get('specs') except Exception: exception_message = (_( "Unable to get configuration information necessary to " diff --git a/cinder/volume/drivers/dell_emc/vmax/rest.py b/cinder/volume/drivers/dell_emc/vmax/rest.py index fde417360b6..a69a1f8697c 100644 --- a/cinder/volume/drivers/dell_emc/vmax/rest.py +++ b/cinder/volume/drivers/dell_emc/vmax/rest.py @@ -17,6 +17,7 @@ import json from oslo_log import log as logging from oslo_service import loopingcall +from oslo_utils import units import requests import requests.auth import requests.packages.urllib3.exceptions as urllib_exp @@ -773,10 +774,7 @@ class VMAXRest(object): sg_maxiops = None sg_maxmbps = None sg_distribution_type = None - maxiops = "nolimit" - maxmbps = "nolimit" - distribution_type = "never" - propertylist = [] + property_dict = {} try: sg_qos_details = sg_details['hostIOLimit'] sg_maxiops = sg_qos_details['host_io_limit_io_sec'] @@ -784,38 +782,21 @@ class VMAXRest(object): sg_distribution_type = sg_qos_details['dynamicDistribution'] except KeyError: LOG.debug("Unable to get storage group QoS details.") - if 'maxIOPS' in extra_specs.get('qos'): - maxiops = extra_specs['qos']['maxIOPS'] - if maxiops != sg_maxiops: - propertylist.append(maxiops) - if 'maxMBPS' in extra_specs.get('qos'): - maxmbps = extra_specs['qos']['maxMBPS'] - if maxmbps != sg_maxmbps: - propertylist.append(maxmbps) - if 'DistributionType' in extra_specs.get('qos') and ( - propertylist or sg_qos_details): - dynamic_list = ['never', 'onfailure', 'always'] - if (extra_specs.get('qos').get('DistributionType').lower() not - in dynamic_list): - exception_message = (_( - "Wrong Distribution type value %(dt)s entered. " - "Please enter one of: %(dl)s") % - {'dt': extra_specs.get('qos').get('DistributionType'), - 'dl': dynamic_list - }) - LOG.error(exception_message) - raise exception.VolumeBackendAPIException( - data=exception_message) - else: - distribution_type = extra_specs['qos']['DistributionType'] - if distribution_type != sg_distribution_type: - propertylist.append(distribution_type) - if propertylist: + if 'total_iops_sec' in extra_specs.get('qos'): + property_dict = self.validate_qos_input( + 'total_iops_sec', sg_maxiops, extra_specs.get('qos'), + property_dict) + if 'total_bytes_sec' in extra_specs.get('qos'): + property_dict = self.validate_qos_input( + 'total_bytes_sec', sg_maxmbps, extra_specs.get('qos'), + property_dict) + if 'DistributionType' in extra_specs.get('qos') and property_dict: + property_dict = self.validate_qos_distribution_type( + sg_distribution_type, extra_specs.get('qos'), property_dict) + + if property_dict: payload = {"editStorageGroupActionParam": { - "setHostIOLimitsParam": { - "host_io_limit_io_sec": maxiops, - "host_io_limit_mb_sec": maxmbps, - "dynamicDistribution": distribution_type}}} + "setHostIOLimitsParam": property_dict}} status_code, message = ( self.modify_storage_group(array, storage_group_name, payload)) try: @@ -828,6 +809,54 @@ class VMAXRest(object): return_value = False return return_value + @staticmethod + def validate_qos_input(input_key, sg_value, qos_extra_spec, property_dict): + max_value = 100000 + qos_unit = "IO/Sec" + if input_key == 'total_iops_sec': + min_value = 100 + input_value = int(qos_extra_spec['total_iops_sec']) + sg_key = 'host_io_limit_io_sec' + else: + qos_unit = "MB/sec" + min_value = 1 + input_value = int(qos_extra_spec['total_bytes_sec']) / units.Mi + sg_key = 'host_io_limit_mb_sec' + if min_value <= input_value <= max_value: + if sg_value is None or input_value != int(sg_value): + property_dict[sg_key] = input_value + else: + exception_message = (_( + "Invalid %(ds)s with value %(dt)s entered. " + "Valid values range from %(du)s %(dv)s to 100,000 %(dv)s") % + {'ds': input_key, 'dt': input_value, 'du': min_value, + 'dv': qos_unit + }) + LOG.error(exception_message) + raise exception.VolumeBackendAPIException( + data=exception_message) + return property_dict + + @staticmethod + def validate_qos_distribution_type( + sg_value, qos_extra_spec, property_dict): + dynamic_list = ['never', 'onfailure', 'always'] + if qos_extra_spec.get('DistributionType').lower() in dynamic_list: + distribution_type = qos_extra_spec['DistributionType'] + if distribution_type != sg_value: + property_dict["dynamicDistribution"] = distribution_type + else: + exception_message = (_( + "Wrong Distribution type value %(dt)s entered. " + "Please enter one of: %(dl)s") % + {'dt': qos_extra_spec.get('DistributionType'), + 'dl': dynamic_list + }) + LOG.error(exception_message) + raise exception.VolumeBackendAPIException( + data=exception_message) + return property_dict + def get_vmax_default_storage_group( self, array, srp, slo, workload, do_disable_compression=False, is_re=False):