From cf83449bbd786f13ffa8b531e1126d02e83f7560 Mon Sep 17 00:00:00 2001 From: Nilesh Thathagar Date: Thu, 22 Feb 2024 11:20:34 +0000 Subject: [PATCH] Dell PowerFlex: Added timeout into rest API call. Added connect and read timeout into rest API call of PowerFlex to avoid cinder hang issue. Deafult value of connect and read timeout is 30 seconds. Closes-Bug: #2052995 Change-Id: I032d76627466f74121e3dc4fb2c8e175d830fa14 --- .../dell_emc/powerflex/test_create_volume.py | 13 ++ .../powerflex/test_power_flex_client.py | 167 ++++++++++++++++++ .../dell_emc/powerflex/test_versions.py | 14 ++ .../drivers/dell_emc/powerflex/options.py | 10 ++ .../drivers/dell_emc/powerflex/rest_client.py | 84 ++++++--- .../drivers/dell_emc/powerflex/utils.py | 2 + ...lex-rest-api-timeout-3a05b6b5d5460176.yaml | 11 ++ 7 files changed, 278 insertions(+), 23 deletions(-) create mode 100644 cinder/tests/unit/volume/drivers/dell_emc/powerflex/test_power_flex_client.py create mode 100644 releasenotes/notes/bug-2052995-dell-powerflex-rest-api-timeout-3a05b6b5d5460176.yaml 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.