Reuse Redfish sessions

Fixes redfish hardware type to cache and reuse `Sushy` objects
when talking to BMC. Prior to this fix redfish hardware type
never reused authenticated HTTP sessions (tokens) e.g. effectively
created new HTTP session on every BMC call. That may have led
sometimes to session pool exhaustion with some BMC implementations.

To properly terminate authenticated session, sushy has to make
an explicit DELETE call against BMC Redfish Session API. As of this
commit, sushy does not do that so that ironic could still exhaust
BMC sessions. However, the likelihood of running out of BMC sessions
seems to be lower with this fix considering lesser frequency of
new sessions creation.

The proper fix would be to make sushy terminating authenticated
session with BMC on `Sushy` object deallocation.

Change-Id: I18ea013fd6e07c98b8c47d95772d3129132cdc53
Story: 2003813
Task: 26566

ironic/drivers/modules/redfish/utils.py

@@ -13,6 +13,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import collections
 import os
 
 from oslo_log import log
@@ -149,6 +150,52 @@ def parse_driver_info(node):
             'node_uuid': node.uuid}
 
 
+class SessionCache(object):
+    """Cache of HTTP sessions credentials"""
+    MAX_SESSIONS = 1000
+
+    sessions = collections.OrderedDict()
+
+    def __init__(self, driver_info):
+        self._driver_info = driver_info
+        self._session_key = tuple(
+            self._driver_info.get(key)
+            for key in ('address', 'username', 'verify_ca')
+        )
+
+    def __enter__(self):
+        try:
+            return self.sessions[self._session_key]
+
+        except KeyError:
+            conn = sushy.Sushy(
+                self._driver_info['address'],
+                username=self._driver_info['username'],
+                password=self._driver_info['password'],
+                verify=self._driver_info['verify_ca']
+            )
+
+            self._expire_oldest_session()
+
+            self.sessions[self._session_key] = conn
+
+            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)
+
+    def _expire_oldest_session(self):
+        """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)
+
+
 def get_system(node):
     """Get a Redfish System that represents a node.
 
@@ -167,13 +214,9 @@ def get_system(node):
         wait_fixed=CONF.redfish.connection_retry_interval * 1000)
     def _get_system():
         try:
-            # TODO(lucasagomes): We should look into a mechanism to
+            with SessionCache(driver_info) as conn:
-            # cache the connections (and maybe even system's instances)
-            # to avoid unnecessary requests to the Redfish controller
-            conn = sushy.Sushy(address, username=driver_info['username'],
-                               password=driver_info['password'],
-                               verify=driver_info['verify_ca'])
                 return conn.get_system(system_id)
+
         except sushy.exceptions.ResourceNotFoundError as e:
             LOG.error('The Redfish System "%(system)s" was not found for '
                       'node %(node)s. Error %(error)s',

ironic/tests/unit/drivers/modules/redfish/test_utils.py

@@ -13,6 +13,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import collections
 import copy
 import os
 
@@ -146,17 +147,22 @@ class RedfishUtilsTestCase(db_base.DbTestCase):
                                    redfish_utils.parse_driver_info, self.node)
 
     @mock.patch('ironic.drivers.modules.redfish.utils.sushy')
+    @mock.patch('ironic.drivers.modules.redfish.utils.'
+                'SessionCache.sessions', {})
     def test_get_system(self, mock_sushy):
+        mock_sushy.exceptions.ConnectionError = MockedConnectionError
         fake_conn = mock_sushy.Sushy.return_value
         fake_system = fake_conn.get_system.return_value
-
         response = redfish_utils.get_system(self.node)
         self.assertEqual(fake_system, response)
         fake_conn.get_system.assert_called_once_with(
             '/redfish/v1/Systems/FAKESYSTEM')
 
     @mock.patch('ironic.drivers.modules.redfish.utils.sushy')
+    @mock.patch('ironic.drivers.modules.redfish.utils.'
+                'SessionCache.sessions', {})
     def test_get_system_resource_not_found(self, mock_sushy):
+        mock_sushy.exceptions.ConnectionError = MockedConnectionError
         fake_conn = mock_sushy.Sushy.return_value
         mock_sushy.exceptions.ResourceNotFoundError = (
             MockedResourceNotFoundError)
@@ -169,6 +175,8 @@ class RedfishUtilsTestCase(db_base.DbTestCase):
 
     @mock.patch('time.sleep', autospec=True)
     @mock.patch('ironic.drivers.modules.redfish.utils.sushy')
+    @mock.patch('ironic.drivers.modules.redfish.utils.'
+                'SessionCache.sessions', {})
     def test_get_system_resource_connection_error_retry(self, mock_sushy,
                                                         mock_sleep):
         # Redfish specific configurations
@@ -191,3 +199,57 @@ class RedfishUtilsTestCase(db_base.DbTestCase):
         fake_conn.get_system.assert_has_calls(expected_get_system_calls)
         mock_sleep.assert_called_with(
             redfish_utils.CONF.redfish.connection_retry_interval)
+
+    @mock.patch('ironic.drivers.modules.redfish.utils.sushy')
+    @mock.patch('ironic.drivers.modules.redfish.utils.'
+                'SessionCache.sessions', {})
+    def test_auth_auto(self, mock_sushy):
+        mock_sushy.exceptions.ConnectionError = MockedConnectionError
+        redfish_utils.get_system(self.node)
+        mock_sushy.Sushy.assert_called_with(
+            self.parsed_driver_info['address'],
+            username=self.parsed_driver_info['username'],
+            password=self.parsed_driver_info['password'],
+            verify=True)
+
+    @mock.patch('ironic.drivers.modules.redfish.utils.sushy')
+    @mock.patch('ironic.drivers.modules.redfish.utils.'
+                'SessionCache.sessions', {})
+    def test_ensure_session_reuse(self, mock_sushy):
+        mock_sushy.exceptions.ConnectionError = MockedConnectionError
+        redfish_utils.get_system(self.node)
+        redfish_utils.get_system(self.node)
+        self.assertEqual(1, mock_sushy.Sushy.call_count)
+
+    @mock.patch('ironic.drivers.modules.redfish.utils.sushy')
+    def test_ensure_new_session_address(self, mock_sushy):
+        mock_sushy.exceptions.ConnectionError = MockedConnectionError
+        self.node.driver_info['redfish_address'] = 'http://bmc.foo'
+        redfish_utils.get_system(self.node)
+        self.node.driver_info['redfish_address'] = 'http://bmc.bar'
+        redfish_utils.get_system(self.node)
+        self.assertEqual(2, mock_sushy.Sushy.call_count)
+
+    @mock.patch('ironic.drivers.modules.redfish.utils.sushy')
+    def test_ensure_new_session_username(self, mock_sushy):
+        mock_sushy.exceptions.ConnectionError = MockedConnectionError
+        self.node.driver_info['redfish_username'] = 'foo'
+        redfish_utils.get_system(self.node)
+        self.node.driver_info['redfish_username'] = 'bar'
+        redfish_utils.get_system(self.node)
+        self.assertEqual(2, mock_sushy.Sushy.call_count)
+
+    @mock.patch('ironic.drivers.modules.redfish.utils.sushy')
+    @mock.patch('ironic.drivers.modules.redfish.utils.'
+                'SessionCache.MAX_SESSIONS', 10)
+    @mock.patch('ironic.drivers.modules.redfish.utils.SessionCache.sessions',
+                collections.OrderedDict())
+    def test_expire_old_sessions(self, mock_sushy):
+        mock_sushy.exceptions.ConnectionError = MockedConnectionError
+
+        for num in range(20):
+            self.node.driver_info['redfish_username'] = 'foo-%d' % num
+            redfish_utils.get_system(self.node)
+
+        self.assertEqual(mock_sushy.Sushy.call_count, 20)
+        self.assertEqual(len(redfish_utils.SessionCache.sessions), 10)

releasenotes/notes/persist-redfish-sessions-d521a0846fa45c40.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Fixes ``redfish`` hardware type to reuse HTTP session tokens when
+    talking to BMC using session authentication. Prior to this fix ``redfish``
+    hardware type never tried to reuse session token given out by BMC during
+    previous connection what may sometimes lead to session pool exhaustion
+    with some BMC implementations.