Merge "Reuse Redfish sessions"

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
-            # 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)
+            with SessionCache(driver_info) as conn:
+                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.