From 8a64d03dc0c9dc6671898c9287e3d6b1f2873ecc Mon Sep 17 00:00:00 2001 From: Ilya Etingof Date: Wed, 26 Sep 2018 14:01:55 +0200 Subject: [PATCH] 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 | 57 +++++++++++++++-- .../drivers/modules/redfish/test_utils.py | 64 ++++++++++++++++++- ...ist-redfish-sessions-d521a0846fa45c40.yaml | 8 +++ 3 files changed, 121 insertions(+), 8 deletions(-) create mode 100644 releasenotes/notes/persist-redfish-sessions-d521a0846fa45c40.yaml diff --git a/ironic/drivers/modules/redfish/utils.py b/ironic/drivers/modules/redfish/utils.py index 108b585e8a..eadabec2c9 100644 --- a/ironic/drivers/modules/redfish/utils.py +++ b/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', diff --git a/ironic/tests/unit/drivers/modules/redfish/test_utils.py b/ironic/tests/unit/drivers/modules/redfish/test_utils.py index c1e100f211..e87d2a1a40 100644 --- a/ironic/tests/unit/drivers/modules/redfish/test_utils.py +++ b/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) diff --git a/releasenotes/notes/persist-redfish-sessions-d521a0846fa45c40.yaml b/releasenotes/notes/persist-redfish-sessions-d521a0846fa45c40.yaml new file mode 100644 index 0000000000..a986d9e1b9 --- /dev/null +++ b/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.