diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/powermax_fake_objects.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/powermax_fake_objects.py index 229f4b76780..0f9694e7b13 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/powermax_fake_objects.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/powermax_fake_objects.py @@ -54,7 +54,7 @@ class FakeRequestsSession(object): def __init__(self, *args, **kwargs): self.data = tpd.PowerMaxData() - def request(self, method, url, params=None, data=None): + def request(self, method, url, params=None, data=None, timeout=None): return_object = '' status_code = 200 if method == 'GET': @@ -69,6 +69,12 @@ class FakeRequestsSession(object): elif method == 'TIMEOUT': raise requests.Timeout + elif method == 'READTIMEOUT' and timeout is not None: + raise requests.ReadTimeout + + elif method == 'CONNECTTIMEOUT' and timeout is not None: + raise requests.ConnectTimeout + elif method == 'EXCEPTION': raise Exception @@ -326,6 +332,8 @@ class FakeConfiguration(object): self.initiator_check = False self.powermax_service_level = None self.vmax_workload = None + self.rest_api_connect_timeout = 30 + self.rest_api_read_timeout = 30 if replication_device: self.replication_device = replication_device for key, value in kwargs.items(): @@ -422,3 +430,9 @@ class FakeConfiguration(object): def append_config_values(self, values): pass + + def set_rest_api_connect_timeout(self, value): + self.rest_api_connect_timeout = value + + def set_rest_api_read_timeout(self, value): + self.rest_api_read_timeout = value diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py index 628fb357dd9..f716d5f76a4 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py @@ -3217,7 +3217,8 @@ class PowerMaxCommonTest(test.TestCase): {'RestServerIp': '1.1.1.1', 'RestServerPort': 8443, 'RestUserName': 'smc', 'RestPassword': 'smc', 'SSLVerify': False, 'SerialNumber': self.data.array, 'srpName': 'SRP_1', - 'PortGroup': [self.data.port_group_name_i]}) + 'PortGroup': [self.data.port_group_name_i], + 'RestAPIConnectTimeout': 30, 'RestAPIReadTimeout': 30}) old_conf = tpfo.FakeConfiguration(None, 'CommonTests', 1, 1) configuration = tpfo.FakeConfiguration( None, 'CommonTests', 1, 1, san_ip='1.1.1.1', san_login='smc', @@ -3236,7 +3237,8 @@ class PowerMaxCommonTest(test.TestCase): {'RestServerIp': '1.1.1.1', 'RestServerPort': 3448, 'RestUserName': 'smc', 'RestPassword': 'smc', 'SSLVerify': False, 'SerialNumber': self.data.array, 'srpName': 'SRP_1', - 'PortGroup': [self.data.port_group_name_i]}) + 'PortGroup': [self.data.port_group_name_i], + 'RestAPIConnectTimeout': 30, 'RestAPIReadTimeout': 30}) configuration = tpfo.FakeConfiguration( None, 'CommonTests', 1, 1, san_ip='1.1.1.1', san_login='smc', powermax_array=self.data.array, powermax_srp='SRP_1', @@ -3251,7 +3253,8 @@ class PowerMaxCommonTest(test.TestCase): {'RestServerIp': '1.1.1.1', 'RestServerPort': 8443, 'RestUserName': 'smc', 'RestPassword': 'smc', 'SSLVerify': False, 'SerialNumber': self.data.array, 'srpName': 'SRP_1', - 'PortGroup': [self.data.port_group_name_i]}) + 'PortGroup': [self.data.port_group_name_i], + 'RestAPIConnectTimeout': 30, 'RestAPIReadTimeout': 30}) configuration = tpfo.FakeConfiguration( None, 'CommonTests', 1, 1, san_ip='1.1.1.1', san_login='smc', powermax_array=self.data.array, powermax_srp='SRP_1', @@ -3606,7 +3609,8 @@ class PowerMaxCommonTest(test.TestCase): 'RestServerIp': '1.1.1.1', 'RestServerPort': 8443, 'RestUserName': 'smc', 'RestPassword': 'smc', 'SSLVerify': False, 'SerialNumber': '000197800123', 'srpName': 'SRP_1', - 'PortGroup': ['OS-fibre-PG']} + 'PortGroup': ['OS-fibre-PG'], + 'RestAPIConnectTimeout': 30, 'RestAPIReadTimeout': 30} self.mock_object(self.common.configuration, 'powermax_service_level', None) @@ -3662,7 +3666,9 @@ class PowerMaxCommonTest(test.TestCase): 'RestServerIp': '1.1.1.1', 'RestServerPort': 8443, 'RestUserName': 'test', 'RestPassword': 'test', 'SerialNumber': '000197800123', 'srpName': 'SRP_1', - 'PortGroup': None, 'SSLVerify': True}} + 'PortGroup': None, + 'RestAPIConnectTimeout': 30, 'RestAPIReadTimeout': 30, + 'SSLVerify': True}} self.mock_object(self.common, 'configuration', configuration) self.common._get_u4p_failover_info() self.assertIsNotNone(self.rest.u4p_failover_targets) @@ -4839,3 +4845,58 @@ class PowerMaxCommonTest(test.TestCase): self.data.remote_array, self.data.srp, 'Diamond', 'DSS', self.data.rep_extra_specs_rep_config, False, is_re=True, rep_mode='Synchronous') + + def test_get_connect_timeout_from_cinder_config(self): + kwargs_expected = ( + {'RestServerIp': '1.1.1.1', 'RestServerPort': 3448, + 'RestUserName': 'smc', 'RestPassword': 'smc', 'SSLVerify': False, + 'SerialNumber': self.data.array, 'srpName': 'SRP_1', + 'PortGroup': [self.data.port_group_name_i], + 'RestAPIConnectTimeout': 120, 'RestAPIReadTimeout': 30}) + configuration = tpfo.FakeConfiguration( + None, 'CommonTests', + 1, 1, san_ip='1.1.1.1', san_login='smc', + powermax_array=self.data.array, powermax_srp='SRP_1', + san_password='smc', san_api_port=3448, + powermax_port_groups=[self.data.port_group_name_i]) + configuration.set_rest_api_connect_timeout(120) + self.mock_object(self.common, 'configuration', configuration) + kwargs_returned = self.common.get_attributes_from_cinder_config() + self.assertEqual(kwargs_expected, kwargs_returned) + + def test_get_read_timeout_from_cinder_config(self): + kwargs_expected = ( + {'RestServerIp': '1.1.1.1', 'RestServerPort': 3448, + 'RestUserName': 'smc', 'RestPassword': 'smc', 'SSLVerify': False, + 'SerialNumber': self.data.array, 'srpName': 'SRP_1', + 'PortGroup': [self.data.port_group_name_i], + 'RestAPIConnectTimeout': 30, 'RestAPIReadTimeout': 120}) + configuration = tpfo.FakeConfiguration( + None, 'CommonTests', + 1, 1, san_ip='1.1.1.1', san_login='smc', + powermax_array=self.data.array, powermax_srp='SRP_1', + san_password='smc', san_api_port=3448, + powermax_port_groups=[self.data.port_group_name_i]) + configuration.set_rest_api_read_timeout(120) + self.mock_object(self.common, 'configuration', configuration) + kwargs_returned = self.common.get_attributes_from_cinder_config() + self.assertEqual(kwargs_expected, kwargs_returned) + + def test_get_connect_and_read_timeout_from_cinder_config(self): + kwargs_expected = ( + {'RestServerIp': '1.1.1.1', 'RestServerPort': 3448, + 'RestUserName': 'smc', 'RestPassword': 'smc', 'SSLVerify': False, + 'SerialNumber': self.data.array, 'srpName': 'SRP_1', + 'PortGroup': [self.data.port_group_name_i], + 'RestAPIConnectTimeout': 90, 'RestAPIReadTimeout': 90}) + configuration = tpfo.FakeConfiguration( + None, 'CommonTests', + 1, 1, san_ip='1.1.1.1', san_login='smc', + powermax_array=self.data.array, powermax_srp='SRP_1', + san_password='smc', san_api_port=3448, + powermax_port_groups=[self.data.port_group_name_i]) + configuration.set_rest_api_connect_timeout(90) + configuration.set_rest_api_read_timeout(90) + self.mock_object(self.common, 'configuration', configuration) + kwargs_returned = self.common.get_attributes_from_cinder_config() + self.assertEqual(kwargs_expected, kwargs_returned) diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_rest.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_rest.py index d3c1a2ae88b..e39e7374dbf 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_rest.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_rest.py @@ -63,6 +63,14 @@ class PowerMaxRestTest(test.TestCase): self.assertRaises(requests.exceptions.Timeout, self.rest.request, '', 'TIMEOUT') + def test_rest_request_read_timeout_exception(self): + self.assertRaises(requests.exceptions.ReadTimeout, + self.rest.request, '', 'READTIMEOUT', (60, 60)) + + def test_rest_request_connect_timeout_exception(self): + self.assertRaises(requests.exceptions.ConnectTimeout, + self.rest.request, '', 'CONNECTTIMEOUT', (60, 60)) + def test_rest_request_connection_exception(self): self.assertRaises(requests.exceptions.ConnectionError, self.rest.request, '', 'CONNECTION') diff --git a/cinder/volume/drivers/dell_emc/powermax/common.py b/cinder/volume/drivers/dell_emc/powermax/common.py index f2c54c74526..eae078f2737 100644 --- a/cinder/volume/drivers/dell_emc/powermax/common.py +++ b/cinder/volume/drivers/dell_emc/powermax/common.py @@ -146,7 +146,15 @@ powermax_opts = [ help='Metric used for port group load calculation.'), cfg.StrOpt(utils.PORT_LOAD_METRIC, default='PercentBusy', - help='Metric used for port load calculation.')] + help='Metric used for port load calculation.'), + cfg.IntOpt(utils.REST_API_CONNECT_TIMEOUT, + default=30, min=1, + help='Use this value to specify connect ' + 'timeout value (in seconds) for rest call.'), + cfg.IntOpt(utils.REST_API_READ_TIMEOUT, + default=30, min=1, + help='Use this value to specify read ' + 'timeout value (in seconds) for rest call.')] CONF.register_opts(powermax_opts, group=configuration.SHARED_CONF_GROUP) @@ -7122,6 +7130,10 @@ class PowerMaxCommon(object): workload = self.configuration.safe_get(utils.VMAX_WORKLOAD) port_groups = self.configuration.safe_get( utils.POWERMAX_PORT_GROUPS) + rest_api_connect_timeout = ( + self.configuration.safe_get(utils.REST_API_CONNECT_TIMEOUT)) + rest_api_read_timeout = ( + self.configuration.safe_get(utils.REST_API_READ_TIMEOUT)) kwargs = ( {'RestServerIp': self.configuration.safe_get( @@ -7131,7 +7143,9 @@ class PowerMaxCommon(object): 'RestPassword': password, 'SerialNumber': serial_number, 'srpName': srp_name, - 'PortGroup': port_groups}) + 'PortGroup': port_groups, + utils.REST_API_CONNECT_TIMEOUT_KEY: rest_api_connect_timeout, + utils.REST_API_READ_TIMEOUT_KEY: rest_api_read_timeout}) if self.configuration.safe_get('driver_ssl_cert_verify'): if self.configuration.safe_get('driver_ssl_cert_path'): diff --git a/cinder/volume/drivers/dell_emc/powermax/rest.py b/cinder/volume/drivers/dell_emc/powermax/rest.py index ed945c54e52..d7edd5861fa 100644 --- a/cinder/volume/drivers/dell_emc/powermax/rest.py +++ b/cinder/volume/drivers/dell_emc/powermax/rest.py @@ -92,6 +92,8 @@ class PowerMaxRest(object): self.ucode_minor_level = None self.is_snap_id = False self.u4p_version = None + self.rest_api_connect_timeout = 30 + self.rest_api_read_timeout = 30 def set_rest_credentials(self, array_info): """Given the array record set the rest server credentials. @@ -107,6 +109,14 @@ class PowerMaxRest(object): self.base_uri = ("https://%(ip_port)s/univmax/restapi" % { 'ip_port': ip_port}) self.session = self._establish_rest_session() + new_connect_timeout = ( + int(array_info.get(utils.REST_API_CONNECT_TIMEOUT_KEY, 0))) + if new_connect_timeout > 0: + self.rest_api_connect_timeout = new_connect_timeout + new_read_timeout = ( + int(array_info.get(utils.REST_API_READ_TIMEOUT_KEY, 0))) + if new_read_timeout > 0: + self.rest_api_read_timeout = new_read_timeout def set_residuals(self, serial_number): """Set ucode and snapid information. @@ -242,18 +252,19 @@ class PowerMaxRest(object): url = ("%(self.base_uri)s%(target_uri)s" % { 'self.base_uri': self.base_uri, 'target_uri': target_uri}) - + timeout = (self.rest_api_connect_timeout, + self.rest_api_read_timeout) if request_object: response = self.session.request( method=method, url=url, data=json.dumps(request_object, sort_keys=True, - indent=4)) + indent=4), timeout=timeout) elif params: response = self.session.request( - method=method, url=url, params=params) + method=method, url=url, params=params, timeout=timeout) else: response = self.session.request( - method=method, url=url) + method=method, url=url, timeout=timeout) status_code = response.status_code if retry and status_code and status_code in [STATUS_200, @@ -288,6 +299,10 @@ class PowerMaxRest(object): except (r_exc.Timeout, r_exc.ConnectionError, r_exc.HTTPError) as e: + if isinstance(e, r_exc.Timeout): + msg = _("The %s request to URL %s failed with timeout " + "exception %s" % (method, url, str(e))) + LOG.error(msg) if self.u4p_failover_enabled or u4p_check: if not u4p_check: # Failover process diff --git a/cinder/volume/drivers/dell_emc/powermax/utils.py b/cinder/volume/drivers/dell_emc/powermax/utils.py index 0ff9821f5d1..eb3f1f95dc4 100644 --- a/cinder/volume/drivers/dell_emc/powermax/utils.py +++ b/cinder/volume/drivers/dell_emc/powermax/utils.py @@ -136,6 +136,10 @@ POWERMAX_ARRAY_TAG_LIST = 'powermax_array_tag_list' POWERMAX_SHORT_HOST_NAME_TEMPLATE = 'powermax_short_host_name_template' POWERMAX_PORT_GROUP_NAME_TEMPLATE = 'powermax_port_group_name_template' PORT_GROUP_LABEL = 'port_group_label' +REST_API_CONNECT_TIMEOUT = 'rest_api_connect_timeout' +REST_API_READ_TIMEOUT = 'rest_api_read_timeout' +REST_API_CONNECT_TIMEOUT_KEY = 'RestAPIConnectTimeout' +REST_API_READ_TIMEOUT_KEY = 'RestAPIReadTimeout' # Array Models, Service Levels & Workloads VMAX_HYBRID_MODELS = ['VMAX100K', 'VMAX200K', 'VMAX400K'] diff --git a/releasenotes/notes/bug-2051830-dell-powermax-rest-api-timeout-b70bd2754debf16a.yaml b/releasenotes/notes/bug-2051830-dell-powermax-rest-api-timeout-b70bd2754debf16a.yaml new file mode 100644 index 00000000000..b699e11d50c --- /dev/null +++ b/releasenotes/notes/bug-2051830-dell-powermax-rest-api-timeout-b70bd2754debf16a.yaml @@ -0,0 +1,11 @@ +--- +fixes: + - | + PowerMax Driver `bug #2051830 + `_: REST + API calls to the PowerMax backend did not have a timeout + set, which could result in cinder waiting forever. + This fix introduces two configuration options, + ``rest_api_connect_timeout`` and ``rest_api_read_timeout``, + to control timeouts when connecting to the backend. + The default value of each is 30 seconds.