From 5082cb3e0771a24b51d5463166ae0c2b6e95d95b Mon Sep 17 00:00:00 2001 From: Nilesh Thathagar Date: Fri, 9 Feb 2024 09:39:49 +0000 Subject: [PATCH] Dell PowerMax: Added timeout into rest API call. Added connect and read timeout into rest API call of PowerMax to avoid cinder hang issue. Deafult value of connect and read timeout is 30 seconds. Closes-Bug: #2051830 Change-Id: I2d419b4257bae75c69577a34758910c4889e2507 --- .../powermax/powermax_fake_objects.py | 16 ++++- .../dell_emc/powermax/test_powermax_common.py | 71 +++++++++++++++++-- .../dell_emc/powermax/test_powermax_rest.py | 8 +++ .../drivers/dell_emc/powermax/common.py | 18 ++++- .../volume/drivers/dell_emc/powermax/rest.py | 23 ++++-- .../volume/drivers/dell_emc/powermax/utils.py | 4 ++ ...max-rest-api-timeout-b70bd2754debf16a.yaml | 11 +++ 7 files changed, 139 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/bug-2051830-dell-powermax-rest-api-timeout-b70bd2754debf16a.yaml 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.