diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powerstore/test_client.py b/cinder/tests/unit/volume/drivers/dell_emc/powerstore/test_client.py index b8f6eed93f1..8a6a0add0ce 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/powerstore/test_client.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/powerstore/test_client.py @@ -17,6 +17,7 @@ from unittest import mock import uuid import ddt +import requests.exceptions from cinder import exception from cinder.tests.unit import test @@ -31,7 +32,9 @@ CLIENT_OPTIONS = { "rest_username": "fake_user", "rest_password": "fake_password", "verify_certificate": False, - "certificate_path": None + "certificate_path": None, + "rest_api_connect_timeout": 60, + "rest_api_read_timeout": 60 } ISCSI_IP_POOL_RESP = [ @@ -277,3 +280,52 @@ class TestClient(test.TestCase): self.client.update_qos_io_rule, "io-rule-6b6e5489-4b5b-4468-a1f7-32cec2ffa3bf", QOS_UPDATE_IO_RULE_PARAMS) + + @mock.patch("requests.request") + def test_get_request_timeout_exception(self, mock_request): + mock_request.return_value = MockResponse( + rc=501 + ) + error = self.assertRaises( + exception.VolumeBackendAPIException, + self.client.get_array_version) + self.assertEqual('Bad or unexpected response from the ' + 'storage volume backend API: Failed to ' + 'query PowerStore array version.', + error.msg) + + @mock.patch("requests.request") + def test_send_get_request_connect_timeout_exception(self, + mock_request): + mock_request.side_effect = requests.exceptions.ConnectTimeout() + r, resp = self.client._send_request("GET", + "/api/version") + self.assertEqual(500, r.status_code) + + @mock.patch("requests.request") + def test_send_get_request_read_timeout_exception(self, + mock_request): + mock_request.side_effect = requests.exceptions.ReadTimeout() + r, resp = self.client._send_request("GET", + "/api/version") + self.assertEqual(500, r.status_code) + + @mock.patch("requests.request") + def test_send_post_request_connect_timeout_exception(self, + mock_request): + params = {} + mock_request.side_effect = requests.exceptions.ConnectTimeout() + r, resp = self.client._send_request("POST", + "/metrics/generate", + params) + self.assertEqual(500, r.status_code) + + @mock.patch("requests.request") + def test_send_post_request_read_timeout_exception(self, + mock_request): + params = {} + mock_request.side_effect = requests.exceptions.ReadTimeout() + r, resp = self.client._send_request("POST", + "/metrics/generate", + params) + self.assertEqual(500, r.status_code) diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powerstore/test_volume_create_delete_extend.py b/cinder/tests/unit/volume/drivers/dell_emc/powerstore/test_volume_create_delete_extend.py index 5948c9f7f47..174090d31b5 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/powerstore/test_volume_create_delete_extend.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/powerstore/test_volume_create_delete_extend.py @@ -157,3 +157,17 @@ class TestVolumeCreateDeleteExtend(powerstore.TestPowerStoreDriver): self.volume, 16) self.assertIn("Failed to extend PowerStore volume", error.msg) + + @mock.patch("requests.request") + def test_post_request_timeout_exception(self, mock_request): + mock_request.return_value = powerstore.MockResponse( + rc=501 + ) + error = self.assertRaises(exception.VolumeBackendAPIException, + self.driver.extend_volume, + self.volume, + 16) + self.assertEqual('Bad or unexpected response from the ' + 'storage volume backend API: Failed to ' + 'extend PowerStore volume with id fake_id.', + error.msg) diff --git a/cinder/volume/drivers/dell_emc/powerstore/client.py b/cinder/volume/drivers/dell_emc/powerstore/client.py index 987683c8115..2d20eebd043 100644 --- a/cinder/volume/drivers/dell_emc/powerstore/client.py +++ b/cinder/volume/drivers/dell_emc/powerstore/client.py @@ -21,6 +21,7 @@ import json from oslo_log import log as logging from oslo_utils import strutils import requests +import requests.exceptions from cinder import exception from cinder.i18n import _ @@ -45,7 +46,9 @@ class PowerStoreClient(object): rest_username, rest_password, verify_certificate, - certificate_path): + certificate_path, + rest_api_connect_timeout, + rest_api_read_timeout): self.rest_ip = rest_ip self.rest_username = rest_username self.rest_password = rest_password @@ -59,6 +62,8 @@ class PowerStoreClient(object): requests.codes.no_content, requests.codes.partial_content ] + self.rest_api_connect_timeout = rest_api_connect_timeout + self.rest_api_read_timeout = rest_api_read_timeout @property def _verify_cert(self): @@ -92,36 +97,46 @@ class PowerStoreClient(object): payload=None, params=None, log_response_data=True): - if not params: - params = {} - request_params = { - "auth": (self.rest_username, self.rest_password), - "verify": self._verify_cert, - "params": params - } - if payload and method != "GET": - request_params["data"] = json.dumps(payload) - request_url = self.base_url + url - r = requests.request(method, request_url, **request_params) - - log_level = logging.DEBUG - if r.status_code not in self.ok_codes: - log_level = logging.ERROR - LOG.log(log_level, - "REST Request: %s %s with body %s", - r.request.method, - r.request.url, - strutils.mask_password(r.request.body)) - if log_response_data or log_level == logging.ERROR: - msg = "REST Response: %s with data %s" % (r.status_code, r.text) - else: - msg = "REST Response: %s" % r.status_code - LOG.log(log_level, msg) - + response = None + r = requests.Response try: - response = r.json() - except ValueError: - response = None + if not params: + params = {} + request_params = { + "auth": (self.rest_username, self.rest_password), + "verify": self._verify_cert, + "params": params + } + if payload and method != "GET": + request_params["data"] = json.dumps(payload) + request_url = self.base_url + url + timeout = (self.rest_api_connect_timeout, + self.rest_api_read_timeout) + r = requests.request(method, request_url, **request_params, + timeout=timeout) + log_level = logging.DEBUG + if r.status_code not in self.ok_codes: + log_level = logging.ERROR + LOG.log(log_level, + "REST Request: %s %s with body %s", + r.request.method, + r.request.url, + strutils.mask_password(r.request.body)) + if (log_response_data or + log_level == logging.ERROR): + msg = ("REST Response: %s with data %s" % + (r.status_code, r.text)) + else: + msg = "REST Response: %s" % r.status_code + LOG.log(log_level, msg) + try: + response = r.json() + except ValueError: + response = None + except requests.exceptions.Timeout as e: + r.status_code = requests.codes.internal_server_error + LOG.error("The request to URL %(url)s failed with timeout " + "exception %(exc)s", {"url": url, "exc": e}) return r, response _send_get_request = functools.partialmethod(_send_request, "GET") diff --git a/cinder/volume/drivers/dell_emc/powerstore/driver.py b/cinder/volume/drivers/dell_emc/powerstore/driver.py index 6cc64992bd3..ad34f2f7add 100644 --- a/cinder/volume/drivers/dell_emc/powerstore/driver.py +++ b/cinder/volume/drivers/dell_emc/powerstore/driver.py @@ -266,4 +266,8 @@ class PowerStoreDriver(driver.VolumeDriver): conf["rest_password"] = get_value("san_password") conf["verify_certificate"] = get_value("driver_ssl_cert_verify") conf["certificate_path"] = get_value("driver_ssl_cert_path") + conf["rest_api_connect_timeout"] = ( + self.configuration.safe_get(utils.POWERSTORE_REST_CONNECT_TIMEOUT)) + conf["rest_api_read_timeout"] = ( + self.configuration.safe_get(utils.POWERSTORE_REST_READ_TIMEOUT)) return conf diff --git a/cinder/volume/drivers/dell_emc/powerstore/options.py b/cinder/volume/drivers/dell_emc/powerstore/options.py index 5fb3a847dbd..87653c5d871 100644 --- a/cinder/volume/drivers/dell_emc/powerstore/options.py +++ b/cinder/volume/drivers/dell_emc/powerstore/options.py @@ -17,6 +17,8 @@ from oslo_config import cfg +from cinder.volume.drivers.dell_emc.powerstore import utils as store_utils + POWERSTORE_APPLIANCES = "powerstore_appliances" POWERSTORE_PORTS = "powerstore_ports" POWERSTORE_NVME = "powerstore_nvme" @@ -39,5 +41,16 @@ POWERSTORE_OPTS = [ ), cfg.BoolOpt(POWERSTORE_NVME, default=False, - help="Connect PowerStore volumes using NVMe-OF.") + help="Connect PowerStore volumes using NVMe-OF."), + cfg.IntOpt(store_utils.POWERSTORE_REST_CONNECT_TIMEOUT, + default=30, min=1, + help='Use this value to specify the connect ' + 'timeout value (in seconds) for REST API calls ' + 'to the PowerStore backend.'), + cfg.IntOpt(store_utils.POWERSTORE_REST_READ_TIMEOUT, + default=30, min=1, + help='Use this value to specify the read ' + 'timeout value (in seconds) for REST API calls ' + 'to the PowerStore backend.') + ] diff --git a/cinder/volume/drivers/dell_emc/powerstore/utils.py b/cinder/volume/drivers/dell_emc/powerstore/utils.py index 9370788ca4b..a9be8f86ca0 100644 --- a/cinder/volume/drivers/dell_emc/powerstore/utils.py +++ b/cinder/volume/drivers/dell_emc/powerstore/utils.py @@ -38,6 +38,8 @@ PROTOCOL_NVME = "NVMe" POWERSTORE_PP_KEY = "powerstore:protection_policy" VOLUME_ATTACH_OPERATION = 1 VOLUME_DETACH_OPERATION = 2 +POWERSTORE_REST_CONNECT_TIMEOUT = "rest_api_call_connect_timeout" +POWERSTORE_REST_READ_TIMEOUT = "rest_api_call_read_timeout" def bytes_to_gib(size_in_bytes): diff --git a/releasenotes/notes/bug-2055022-dell-powerstore-rest-api-timeout-51b3ae19266757f9.yaml b/releasenotes/notes/bug-2055022-dell-powerstore-rest-api-timeout-51b3ae19266757f9.yaml new file mode 100644 index 00000000000..95188118e69 --- /dev/null +++ b/releasenotes/notes/bug-2055022-dell-powerstore-rest-api-timeout-51b3ae19266757f9.yaml @@ -0,0 +1,12 @@ +--- +fixes: + - | + Dell PowerStore Driver `bug #2055022 + `_: REST + API calls to the PowerStore backend did not have a timeout + set, which could result in cinder waiting forever. + This fix introduces two configuration options, + ``rest_api_call_connect_timeout`` and + ``rest_api_call_read_timeout``, to control timeouts + when connecting to the backend. + The default value of each is 30 seconds.