Merge "Dell PowerMax: Added timeout into rest API call."

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

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)

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')

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'):

cinder/volume/drivers/dell_emc/powermax/rest.py

@@ -93,6 +93,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.
@@ -108,6 +110,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.
@@ -243,18 +253,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,
@@ -289,6 +300,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

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']

releasenotes/notes/bug-2051830-dell-powermax-rest-api-timeout-b70bd2754debf16a.yaml

@@ -0,0 +1,11 @@
+---
+fixes:
+  - |
+    PowerMax Driver `bug #2051830
+    <https://bugs.launchpad.net/cinder/+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.