Reuse Redfish sessions follow up

This commit addresses minor issues found in patch [1].

1. Change-Id: I18ea013fd6e07c98b8c47d95772d3129132cdc53

Change-Id: I749af28a5a9a9e2d95e18b53d5c535b3d2649c01

3 files changed, 47 insertions, 24 deletions

diff --git a/ironic/conf/redfish.py b/ironic/conf/redfish.py
index 7b5026bec..aa5751414 100644
--- a/ironic/conf/redfish.py
+++ b/ironic/conf/redfish.py
@@ -27,7 +27,16 @@ opts = [
                min=1,
                default=4,
                help=_('Number of seconds to wait between attempts to '
-                      'connect to Redfish'))
+                      'connect to Redfish')),
+    cfg.IntOpt('connection_cache_size',
+               min=0,
+               default=1000,
+               help=_('Maximum Redfish client connection cache size. '
+                      'Redfish driver would strive to reuse authenticated '
+                      'BMC connections (obtained through Redfish Session '
+                      'Service). This option caps the maximum number of '
+                      'connections to maintain. The value of `0` disables '
+                      'client connection caching completely.'))
 ]
 
 
diff --git a/ironic/drivers/modules/redfish/utils.py b/ironic/drivers/modules/redfish/utils.py
index eadabec2c..5cc8ff45b 100644
--- a/ironic/drivers/modules/redfish/utils.py
+++ b/ironic/drivers/modules/redfish/utils.py
@@ -152,9 +152,7 @@ def parse_driver_info(node):
 
 class SessionCache(object):
     """Cache of HTTP sessions credentials"""
-    MAX_SESSIONS = 1000
-
-    sessions = collections.OrderedDict()
+    _sessions = collections.OrderedDict()
 
     def __init__(self, driver_info):
         self._driver_info = driver_info
@@ -165,7 +163,7 @@ class SessionCache(object):
 
     def __enter__(self):
         try:
-            return self.sessions[self._session_key]
+            return self.__class__._sessions[self._session_key]
 
         except KeyError:
             conn = sushy.Sushy(
@@ -175,25 +173,29 @@ class SessionCache(object):
                 verify=self._driver_info['verify_ca']
             )
 
-            self._expire_oldest_session()
+            if CONF.redfish.connection_cache_size:
+                self.__class__._sessions[self._session_key] = conn
 
-            self.sessions[self._session_key] = conn
+                if (len(self.__class__._sessions) >
+                        CONF.redfish.connection_cache_size):
+                    self._expire_oldest_session()
 
             return conn
 
     def __exit__(self, exc_type, exc_val, exc_tb):
         # NOTE(etingof): perhaps this session token is no good
         if isinstance(exc_val, sushy.exceptions.ConnectionError):
-            self.sessions.pop(self._session_key, None)
+            self.__class__._sessions.pop(self._session_key, None)
 
-    def _expire_oldest_session(self):
+    @classmethod
+    def _expire_oldest_session(cls):
         """Expire oldest session"""
-        if len(self.sessions) >= self.MAX_SESSIONS:
-            session_keys = list(self.sessions)
-            session_key = session_keys[0]
-            # NOTE(etingof): GC should cause sushy to HTTP DELETE session
-            # at BMC. Trouble is that as of this commit sushy does not do that.
-            self.sessions.pop(session_key, None)
+        session_keys = list(cls._sessions)
+        session_key = next(iter(session_keys))
+        # NOTE(etingof): GC should cause sushy to HTTP DELETE session
+        # at BMC. Trouble is that contemporary sushy (1.6.0) does
+        # does not do that.
+        cls._sessions.pop(session_key, None)
 
 
 def get_system(node):
diff --git a/ironic/tests/unit/drivers/modules/redfish/test_utils.py b/ironic/tests/unit/drivers/modules/redfish/test_utils.py
index 50cc10afc..daa78d453 100644
--- a/ironic/tests/unit/drivers/modules/redfish/test_utils.py
+++ b/ironic/tests/unit/drivers/modules/redfish/test_utils.py
@@ -18,6 +18,7 @@ import copy
 import os
 
 import mock
+from oslo_config import cfg
 from oslo_utils import importutils
 import requests
 
@@ -141,7 +142,7 @@ class RedfishUtilsTestCase(db_base.DbTestCase):
 
     @mock.patch.object(sushy, 'Sushy', autospec=True)
     @mock.patch('ironic.drivers.modules.redfish.utils.'
-                'SessionCache.sessions', {})
+                'SessionCache._sessions', {})
     def test_get_system(self, mock_sushy):
         fake_conn = mock_sushy.return_value
         fake_system = fake_conn.get_system.return_value
@@ -152,7 +153,7 @@ class RedfishUtilsTestCase(db_base.DbTestCase):
 
     @mock.patch.object(sushy, 'Sushy', autospec=True)
     @mock.patch('ironic.drivers.modules.redfish.utils.'
-                'SessionCache.sessions', {})
+                'SessionCache._sessions', {})
     def test_get_system_resource_not_found(self, mock_sushy):
         fake_conn = mock_sushy.return_value
         fake_conn.get_system.side_effect = (
@@ -168,7 +169,7 @@ class RedfishUtilsTestCase(db_base.DbTestCase):
     @mock.patch('time.sleep', autospec=True)
     @mock.patch.object(sushy, 'Sushy', autospec=True)
     @mock.patch('ironic.drivers.modules.redfish.utils.'
-                'SessionCache.sessions', {})
+                'SessionCache._sessions', {})
     def test_get_system_resource_connection_error_retry(self, mock_sushy,
                                                         mock_sleep):
         # Redfish specific configurations
@@ -191,7 +192,7 @@ class RedfishUtilsTestCase(db_base.DbTestCase):
 
     @mock.patch.object(sushy, 'Sushy', autospec=True)
     @mock.patch('ironic.drivers.modules.redfish.utils.'
-                'SessionCache.sessions', {})
+                'SessionCache._sessions', {})
     def test_auth_auto(self, mock_sushy):
         redfish_utils.get_system(self.node)
         mock_sushy.assert_called_with(
@@ -202,7 +203,7 @@ class RedfishUtilsTestCase(db_base.DbTestCase):
 
     @mock.patch.object(sushy, 'Sushy', autospec=True)
     @mock.patch('ironic.drivers.modules.redfish.utils.'
-                'SessionCache.sessions', {})
+                'SessionCache._sessions', {})
     def test_ensure_session_reuse(self, mock_sushy):
         redfish_utils.get_system(self.node)
         redfish_utils.get_system(self.node)
@@ -225,14 +226,25 @@ class RedfishUtilsTestCase(db_base.DbTestCase):
         self.assertEqual(2, mock_sushy.call_count)
 
     @mock.patch.object(sushy, 'Sushy', autospec=True)
-    @mock.patch('ironic.drivers.modules.redfish.utils.'
-                'SessionCache.MAX_SESSIONS', 10)
-    @mock.patch('ironic.drivers.modules.redfish.utils.SessionCache.sessions',
+    @mock.patch('ironic.drivers.modules.redfish.utils.SessionCache._sessions',
                 collections.OrderedDict())
     def test_expire_old_sessions(self, mock_sushy):
+        cfg.CONF.set_override('connection_cache_size', 10, 'redfish')
         for num in range(20):
             self.node.driver_info['redfish_username'] = 'foo-%d' % num
             redfish_utils.get_system(self.node)
 
         self.assertEqual(mock_sushy.call_count, 20)
-        self.assertEqual(len(redfish_utils.SessionCache.sessions), 10)
+        self.assertEqual(len(redfish_utils.SessionCache._sessions), 10)
+
+    @mock.patch.object(sushy, 'Sushy', autospec=True)
+    @mock.patch('ironic.drivers.modules.redfish.utils.SessionCache._sessions',
+                collections.OrderedDict())
+    def test_disabled_sessions_cache(self, mock_sushy):
+        cfg.CONF.set_override('connection_cache_size', 0, 'redfish')
+        for num in range(2):
+            self.node.driver_info['redfish_username'] = 'foo-%d' % num
+            redfish_utils.get_system(self.node)
+
+        self.assertEqual(mock_sushy.call_count, 2)
+        self.assertEqual(len(redfish_utils.SessionCache._sessions), 0)