diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powerflex/test_create_volume.py b/cinder/tests/unit/volume/drivers/dell_emc/powerflex/test_create_volume.py index 1d8457faaa7..704b42a54a2 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/powerflex/test_create_volume.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/powerflex/test_create_volume.py @@ -16,6 +16,7 @@ from unittest import mock import ddt +import requests.exceptions from cinder import context from cinder import exception @@ -104,3 +105,15 @@ class TestCreateVolume(powerflex.TestPowerFlexDriver): self.driver._get_volumetype_extraspecs.return_value = extraspecs self.assertRaises(exception.VolumeBackendAPIException, self.test_create_volume) + + @mock.patch("requests.post") + def test_volume_post_connect_timeout_request(self, mock_request): + mock_request.side_effect = requests.exceptions.ConnectTimeout() + self.assertRaises(exception.VolumeBackendAPIException, + self.test_create_volume) + + @mock.patch("requests.post") + def test_volume_post_read_timeout_request(self, mock_request): + mock_request.side_effect = requests.exceptions.ReadTimeout() + self.assertRaises(exception.VolumeBackendAPIException, + self.test_create_volume) diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powerflex/test_power_flex_client.py b/cinder/tests/unit/volume/drivers/dell_emc/powerflex/test_power_flex_client.py new file mode 100644 index 00000000000..b2d4248d8b1 --- /dev/null +++ b/cinder/tests/unit/volume/drivers/dell_emc/powerflex/test_power_flex_client.py @@ -0,0 +1,167 @@ +# Copyright (c) 2021 Dell Inc. or its subsidiaries. +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +import http.client as http_client +import json +from unittest import mock + +import ddt +import requests.exceptions +from requests.models import Response + +from cinder.tests.unit.volume.drivers.dell_emc import powerflex +from cinder.volume import configuration as conf +from cinder.volume.drivers.dell_emc.powerflex import driver +from cinder.volume.drivers.dell_emc.powerflex import rest_client + + +@ddt.ddt +class TestPowerFlexClient(powerflex.TestPowerFlexDriver): + + params = {'protectionDomainId': '1', + 'storagePoolId': '1', + 'name': 'HlF355XlSg+xcORfS0afag==', + 'volumeType': 'ThinProvisioned', + 'volumeSizeInKb': '1048576', + 'compressionMethod': 'None'} + + expected_status_code = 500 + + def setUp(self): + super(TestPowerFlexClient, self).setUp() + self.configuration = conf.Configuration(driver.powerflex_opts, + conf.SHARED_CONF_GROUP) + self._set_overrides() + self.client = rest_client.RestClient(self.configuration) + self.client.do_setup() + + def _set_overrides(self): + # Override the defaults to fake values + self.override_config('san_ip', override='127.0.0.1', + group=conf.SHARED_CONF_GROUP) + self.override_config('powerflex_rest_server_port', override='8888', + group=conf.SHARED_CONF_GROUP) + self.override_config('san_login', override='test', + group=conf.SHARED_CONF_GROUP) + self.override_config('san_password', override='pass', + group=conf.SHARED_CONF_GROUP) + self.override_config('powerflex_storage_pools', + override='PD1:SP1', + group=conf.SHARED_CONF_GROUP) + self.override_config('max_over_subscription_ratio', + override=5.0, group=conf.SHARED_CONF_GROUP) + self.override_config('powerflex_server_api_version', + override='2.0.0', group=conf.SHARED_CONF_GROUP) + self.override_config('rest_api_connect_timeout', + override=120, group=conf.SHARED_CONF_GROUP) + self.override_config('rest_api_read_timeout', + override=120, group=conf.SHARED_CONF_GROUP) + + @mock.patch("requests.get") + def test_rest_get_request_connect_timeout_exception(self, mock_request): + mock_request.side_effect = (requests. + exceptions.ConnectTimeout + ('Fake Connect Timeout Exception')) + r, res = (self.client. + execute_powerflex_get_request(url="/version", **{})) + self.assertEqual(self.expected_status_code, r.status_code) + self.assertEqual(self.expected_status_code, res['errorCode']) + (self.assertEqual + ('The request to URL /version failed with timeout exception ' + 'Fake Connect Timeout Exception', res['message'])) + + @mock.patch("requests.get") + def test_rest_get_request_read_timeout_exception(self, mock_request): + mock_request.side_effect = (requests.exceptions.ReadTimeout + ('Fake Read Timeout Exception')) + r, res = (self.client. + execute_powerflex_get_request(url="/version", **{})) + self.assertEqual(self.expected_status_code, r.status_code) + self.assertEqual(self.expected_status_code, res['errorCode']) + (self.assertEqual + ('The request to URL /version failed with timeout exception ' + 'Fake Read Timeout Exception', res['message'])) + + @mock.patch("requests.post") + def test_rest_post_request_connect_timeout_exception(self, mock_request): + mock_request.side_effect = (requests.exceptions.ConnectTimeout + ('Fake Connect Timeout Exception')) + r, res = (self.client.execute_powerflex_post_request + (url="/types/Volume/instances", params=self.params, **{})) + self.assertEqual(self.expected_status_code, r.status_code) + self.assertEqual(self.expected_status_code, res['errorCode']) + (self.assertEqual + ('The request to URL /types/Volume/instances failed with ' + 'timeout exception Fake Connect Timeout Exception', res['message'])) + + @mock.patch("requests.post") + def test_rest_post_request_read_timeout_exception(self, mock_request): + mock_request.side_effect = (requests.exceptions.ReadTimeout + ('Fake Read Timeout Exception')) + r, res = (self.client.execute_powerflex_post_request + (url="/types/Volume/instances", params=self.params, **{})) + self.assertEqual(self.expected_status_code, r.status_code) + self.assertEqual(self.expected_status_code, res['errorCode']) + (self.assertEqual + ('The request to URL /types/Volume/instances failed with ' + 'timeout exception Fake Read Timeout Exception', res['message'])) + + @mock.patch("requests.get") + def test_response_check_read_timeout_exception_1(self, mock_request): + r = requests.Response + r.status_code = http_client.UNAUTHORIZED + mock_request.side_effect = [r, (requests.exceptions.ReadTimeout + ('Fake Read Timeout Exception'))] + r, res = (self.client. + execute_powerflex_get_request(url="/version", **{})) + self.assertEqual(self.expected_status_code, r.status_code) + self.assertEqual(self.expected_status_code, res['errorCode']) + (self.assertEqual + ('The request to URL /version failed with ' + 'timeout exception Fake Read Timeout Exception', res['message'])) + + @mock.patch("requests.get") + def test_response_check_read_timeout_exception_2(self, mock_request): + res1 = requests.Response + res1.status_code = http_client.UNAUTHORIZED + res2 = Response() + res2.status_code = 200 + res2._content = str.encode(json.dumps('faketoken')) + mock_request.side_effect = [res1, res2, + (requests.exceptions.ReadTimeout + ('Fake Read Timeout Exception'))] + r, res = (self.client. + execute_powerflex_get_request(url="/version", **{})) + self.assertEqual(self.expected_status_code, r.status_code) + self.assertEqual(self.expected_status_code, res['errorCode']) + (self.assertEqual + ('The request to URL /version failed with ' + 'timeout exception Fake Read Timeout Exception', res['message'])) + + @mock.patch("requests.post") + @mock.patch("requests.get") + def test_response_check_read_timeout_exception_3(self, mock_post_request, + mock_get_request): + r = requests.Response + r.status_code = http_client.UNAUTHORIZED + mock_post_request.side_effect = r + mock_get_request.side_effect = (requests.exceptions.ReadTimeout + ('Fake Read Timeout Exception')) + r, res = (self.client.execute_powerflex_post_request + (url="/types/Volume/instances", params=self.params, **{})) + self.assertEqual(self.expected_status_code, r.status_code) + self.assertEqual(self.expected_status_code, res['errorCode']) + (self.assertEqual + ('The request to URL /types/Volume/instances failed with ' + 'timeout exception Fake Read Timeout Exception', res['message'])) diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powerflex/test_versions.py b/cinder/tests/unit/volume/drivers/dell_emc/powerflex/test_versions.py index 085a6b1fb9b..c857aa66b79 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/powerflex/test_versions.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/powerflex/test_versions.py @@ -12,8 +12,10 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. +from unittest import mock import ddt +import requests.exceptions from cinder import exception from cinder.tests.unit.volume.drivers.dell_emc import powerflex @@ -91,3 +93,15 @@ class TestMultipleVersions(powerflex.TestPowerFlexDriver): self.driver.primary_client.query_rest_api_version(False), vers ) + + @mock.patch("requests.get") + def test_get_version_connect_timeout_request(self, mock_request): + mock_request.side_effect = requests.exceptions.ConnectTimeout() + self.assertRaises(exception.VolumeBackendAPIException, + self.test_version) + + @mock.patch("requests.get") + def test_get_version_read_timeout_request(self, mock_request): + mock_request.side_effect = requests.exceptions.ReadTimeout() + self.assertRaises(exception.VolumeBackendAPIException, + self.test_version) diff --git a/cinder/volume/drivers/dell_emc/powerflex/options.py b/cinder/volume/drivers/dell_emc/powerflex/options.py index 60a43e9a9a1..91d2919a7e4 100644 --- a/cinder/volume/drivers/dell_emc/powerflex/options.py +++ b/cinder/volume/drivers/dell_emc/powerflex/options.py @@ -19,6 +19,8 @@ named Dell EMC VxFlex OS). from oslo_config import cfg +from cinder.volume.drivers.dell_emc.powerflex import utils as flex_utils + # deprecated options VXFLEXOS_REST_SERVER_PORT = "vxflexos_rest_server_port" VXFLEXOS_ROUND_VOLUME_CAPACITY = "vxflexos_round_volume_capacity" @@ -147,4 +149,12 @@ actual_opts = [ default=False, help='Allow volume migration during rebuild.', deprecated_name=VXFLEXOS_ALLOW_MIGRATION_DURING_REBUILD), + cfg.IntOpt(flex_utils.POWERFLEX_REST_CONNECT_TIMEOUT, + default=30, min=1, + help='Use this value to specify connect ' + 'timeout value (in seconds) for rest call.'), + cfg.IntOpt(flex_utils.POWERFLEX_REST_READ_TIMEOUT, + default=30, min=1, + help='Use this value to specify read ' + 'timeout value (in seconds) for rest call.') ] diff --git a/cinder/volume/drivers/dell_emc/powerflex/rest_client.py b/cinder/volume/drivers/dell_emc/powerflex/rest_client.py index 804b3466999..a9280445dbe 100644 --- a/cinder/volume/drivers/dell_emc/powerflex/rest_client.py +++ b/cinder/volume/drivers/dell_emc/powerflex/rest_client.py @@ -21,6 +21,7 @@ import urllib.parse from oslo_log import log as logging from oslo_utils import units import requests +from requests.exceptions import Timeout from cinder import exception from cinder.i18n import _ @@ -60,6 +61,8 @@ class RestClient(object): self.certificate_path = None self.base_url = None self.is_configured = False + self.rest_api_connect_timeout = 30 + self.rest_api_read_timeout = 30 @staticmethod def _get_headers(): @@ -103,6 +106,12 @@ class RestClient(object): ) self.rest_username = get_config_value("san_login") self.rest_password = get_config_value("san_password") + self.rest_api_connect_timeout = ( + get_config_value(flex_utils.POWERFLEX_REST_CONNECT_TIMEOUT) + or self.rest_api_connect_timeout) + self.rest_api_read_timeout = ( + get_config_value(flex_utils.POWERFLEX_REST_READ_TIMEOUT) + or self.rest_api_read_timeout) if self.verify_certificate: self.certificate_path = ( get_config_value("sio_server_certificate_path") or @@ -125,13 +134,17 @@ class RestClient(object): "server_port": self.rest_port }) LOG.info("REST server IP: %(ip)s, port: %(port)s, " - "username: %(user)s. Verify server's certificate: " + "username: %(user)s, rest connect timeout: " + "%(rest_api_connect_timeout)s, rest read timeout: " + "%(rest_api_read_timeout)s. Verify server's certificate: " "%(verify_cert)s.", { "ip": self.rest_ip, "port": self.rest_port, "user": self.rest_username, "verify_cert": self.verify_certificate, + "rest_api_connect_timeout": self.rest_api_connect_timeout, + "rest_api_read_timeout": self.rest_api_read_timeout }) self.is_configured = True @@ -454,29 +467,50 @@ class RestClient(object): return verify_cert def execute_powerflex_get_request(self, url, **url_params): - request = self.base_url + url % url_params - r = requests.get(request, - auth=(self.rest_username, self.rest_token), - verify=self._get_verify_cert()) - r = self._check_response(r, request) - response = r.json() + r = requests.Response + try: + request = self.base_url + url % url_params + timeout = (self.rest_api_connect_timeout, + self.rest_api_read_timeout) + r = requests.get(request, + auth=(self.rest_username, self.rest_token), + verify=self._get_verify_cert(), timeout=timeout) + r = self._check_response(r, request) + response = r.json() + except Timeout as e: + r.status_code = http_client.INTERNAL_SERVER_ERROR + msg = _("The request to URL %s failed with timeout " + "exception %s" % (url, str(e))) + LOG.error(msg) + response = {'errorCode': http_client.INTERNAL_SERVER_ERROR, + 'message': msg} return r, response def execute_powerflex_post_request(self, url, params=None, **url_params): - if not params: - params = {} - request = self.base_url + url % url_params - r = requests.post(request, - data=json.dumps(params), - headers=self._get_headers(), - auth=(self.rest_username, self.rest_token), - verify=self._get_verify_cert()) - r = self._check_response(r, request, False, params) - response = None + r = requests.Response try: - response = r.json() - except ValueError: - response = None + if not params: + params = {} + request = self.base_url + url % url_params + timeout = (self.rest_api_connect_timeout, + self.rest_api_read_timeout) + r = requests.post(request, + data=json.dumps(params), + headers=self._get_headers(), + auth=(self.rest_username, self.rest_token), + verify=self._get_verify_cert(), timeout=timeout) + r = self._check_response(r, request, False, params) + try: + response = r.json() + except ValueError: + response = None + except Timeout as e: + r.status_code = http_client.INTERNAL_SERVER_ERROR + msg = _("The request to URL %s failed with timeout " + "exception %s" % (url, str(e))) + LOG.error(msg) + response = {'errorCode': http_client.INTERNAL_SERVER_ERROR, + 'message': msg} return r, response def _check_response(self, @@ -492,9 +526,11 @@ class RestClient(object): "a new one.") login_request = self.base_url + login_url verify_cert = self._get_verify_cert() + timeout = (self.rest_api_connect_timeout, + self.rest_api_read_timeout) r = requests.get(login_request, auth=(self.rest_username, self.rest_password), - verify=verify_cert) + verify=verify_cert, timeout=timeout) token = r.json() self.rest_token = token # Repeat request with valid token. @@ -506,7 +542,8 @@ class RestClient(object): self.rest_username, self.rest_token ), - verify=verify_cert) + verify=verify_cert, + timeout=timeout) else: response = requests.post(request, data=json.dumps(params), @@ -515,7 +552,8 @@ class RestClient(object): self.rest_username, self.rest_token ), - verify=verify_cert) + verify=verify_cert, + timeout=timeout) level = logging.DEBUG # for anything other than an OK from the REST API, log an error if response.status_code != http_client.OK: diff --git a/cinder/volume/drivers/dell_emc/powerflex/utils.py b/cinder/volume/drivers/dell_emc/powerflex/utils.py index 3aae71ab039..54e73884ff3 100644 --- a/cinder/volume/drivers/dell_emc/powerflex/utils.py +++ b/cinder/volume/drivers/dell_emc/powerflex/utils.py @@ -21,6 +21,8 @@ from oslo_utils import units from packaging import version LOG = logging.getLogger(__name__) +POWERFLEX_REST_CONNECT_TIMEOUT = "rest_api_connect_timeout" +POWERFLEX_REST_READ_TIMEOUT = "rest_api_read_timeout" def version_gte(ver1, ver2): diff --git a/releasenotes/notes/bug-2052995-dell-powerflex-rest-api-timeout-3a05b6b5d5460176.yaml b/releasenotes/notes/bug-2052995-dell-powerflex-rest-api-timeout-3a05b6b5d5460176.yaml new file mode 100644 index 00000000000..c9d0cf45845 --- /dev/null +++ b/releasenotes/notes/bug-2052995-dell-powerflex-rest-api-timeout-3a05b6b5d5460176.yaml @@ -0,0 +1,11 @@ +--- +fixes: + - | + PowerFlex Driver `bug #2052995 + `_: REST + API calls to the PowerFlex 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.