Merge "Remove redfish cache entry upon errors" into stable/ussuri

4 files changed, 114 insertions, 1 deletions

diff --git a/ironic/drivers/modules/redfish/utils.py b/ironic/drivers/modules/redfish/utils.py
index 5c20667d0..bed05b8f1 100644
--- a/ironic/drivers/modules/redfish/utils.py
+++ b/ironic/drivers/modules/redfish/utils.py
@@ -233,6 +233,16 @@ class SessionCache(object):
         # NOTE(etingof): perhaps this session token is no good
         if isinstance(exc_val, sushy.exceptions.ConnectionError):
             self.__class__._sessions.pop(self._session_key, None)
+        # NOTE(TheJulia): A hard access error has surfaced, we
+        # likely need to eliminate the session.
+        if isinstance(exc_val, sushy.exceptions.AccessError):
+            self.__class__._sessions.pop(self._session_key, None)
+        # NOTE(TheJulia): Something very bad has happened, such
+        # as the session is out of date, and refresh of the SessionService
+        # failed resulting in an AttributeError surfacing.
+        # https://storyboard.openstack.org/#!/story/2009719
+        if isinstance(exc_val, AttributeError):
+            self.__class__._sessions.pop(self._session_key, None)
 
     @classmethod
     def _expire_oldest_session(cls):
@@ -284,6 +294,23 @@ def get_system(node):
                          'auth_type': driver_info['auth_type'],
                          'node': node.uuid, 'error': e})
             raise exception.RedfishConnectionError(node=node.uuid, error=e)
+        except sushy.exceptions.AccessError as e:
+            LOG.warning('For node %(node)s, we received an authentication '
+                        'access error from address %(address)s with auth_type '
+                        '%(auth_type)s. The client will not be re-used upon '
+                        'the next re-attempt. Please ensure your using the '
+                        'correct credentials. Error: %(error)s',
+                        {'address': driver_info['address'],
+                         'auth_type': driver_info['auth_type'],
+                         'node': node.uuid, 'error': e})
+            raise exception.RedfishError(node=node.uuid, error=e)
+        except AttributeError as e:
+            LOG.warning('For node %(node)s, we received at AttributeError '
+                        'when attempting to utilize the client. A new '
+                        'client session shall be used upon the next attempt.'
+                        'Attribute Error: %(error)s',
+                        {'node': node.uuid, 'error': e})
+            raise exception.RedfishError(node=node.uuid, error=e)
 
     try:
         return _get_system()
diff --git a/ironic/tests/unit/drivers/modules/redfish/test_utils.py b/ironic/tests/unit/drivers/modules/redfish/test_utils.py
index 7107a8ab2..38f11a5a2 100644
--- a/ironic/tests/unit/drivers/modules/redfish/test_utils.py
+++ b/ironic/tests/unit/drivers/modules/redfish/test_utils.py
@@ -16,6 +16,7 @@
 import collections
 import copy
 import os
+import time
 
 import mock
 from oslo_config import cfg
@@ -211,8 +212,9 @@ class RedfishUtilsTestCase(db_base.DbTestCase):
         # Redfish specific configurations
         self.config(connection_attempts=3, group='redfish')
 
-        fake_conn = mock_sushy.return_value
+        fake_conn = mock.Mock()
         fake_conn.get_system.side_effect = sushy.exceptions.ConnectionError()
+        mock_sushy.return_value = fake_conn
 
         self.assertRaises(exception.RedfishConnectionError,
                           redfish_utils.get_system, self.node)
@@ -226,6 +228,75 @@ class RedfishUtilsTestCase(db_base.DbTestCase):
         mock_sleep.assert_called_with(
             redfish_utils.CONF.redfish.connection_retry_interval)
 
+    @mock.patch.object(time, 'sleep', lambda seconds: None)
+    @mock.patch.object(sushy, 'Sushy', autospec=True)
+    @mock.patch('ironic.drivers.modules.redfish.utils.'
+                'SessionCache._sessions', {})
+    def test_get_system_resource_access_error_retry(self, mock_sushy):
+
+        # Sushy access errors HTTP Errors
+        class fake_response(object):
+            status_code = 401
+            body = None
+
+            def json():
+                return {}
+
+        fake_conn = mock_sushy.return_value
+        fake_system = mock.Mock()
+        fake_conn.get_system.side_effect = iter(
+            [
+                sushy.exceptions.AccessError(
+                    method='GET',
+                    url='http://path/to/url',
+                    response=fake_response),
+                fake_system,
+            ])
+
+        self.assertRaises(exception.RedfishError,
+                          redfish_utils.get_system, self.node)
+        # Retry, as in next power sync perhaps
+        client = redfish_utils.get_system(self.node)
+        client('foo')
+
+        expected_get_system_calls = [
+            mock.call(self.parsed_driver_info['system_id']),
+            mock.call(self.parsed_driver_info['system_id']),
+        ]
+        fake_conn.get_system.assert_has_calls(expected_get_system_calls)
+        fake_system.assert_called_with('foo')
+        self.assertEqual(fake_conn.get_system.call_count, 2)
+
+    @mock.patch.object(time, 'sleep', lambda seconds: None)
+    @mock.patch.object(sushy, 'Sushy', autospec=True)
+    @mock.patch('ironic.drivers.modules.redfish.utils.'
+                'SessionCache._sessions', {})
+    def test_get_system_resource_attribute_error(self, mock_sushy):
+
+        fake_conn = mock_sushy.return_value
+        fake_system = mock.Mock()
+        fake_conn.get_system.side_effect = iter(
+            [
+                AttributeError,
+                fake_system,
+            ])
+        # We need to check for AttributeError explicitly as
+        # otherwise we break existing tests if we try to catch
+        # it explicitly.
+        self.assertRaises(exception.RedfishError,
+                          redfish_utils.get_system, self.node)
+        # Retry, as in next power sync perhaps
+        client = redfish_utils.get_system(self.node)
+        client('bar')
+        expected_get_system_calls = [
+            mock.call(self.parsed_driver_info['system_id']),
+            mock.call(self.parsed_driver_info['system_id']),
+        ]
+
+        fake_conn.get_system.assert_has_calls(expected_get_system_calls)
+        fake_system.assert_called_once_with('bar')
+        self.assertEqual(fake_conn.get_system.call_count, 2)
+
     @mock.patch.object(sushy, 'Sushy', autospec=True)
     @mock.patch('ironic.drivers.modules.redfish.utils.'
                 'SessionCache._sessions', {})
diff --git a/ironic/tests/unit/drivers/third_party_driver_mocks.py b/ironic/tests/unit/drivers/third_party_driver_mocks.py
index 8a76d87f3..f27365971 100644
--- a/ironic/tests/unit/drivers/third_party_driver_mocks.py
+++ b/ironic/tests/unit/drivers/third_party_driver_mocks.py
@@ -228,6 +228,8 @@ if not sushy:
         type('SushyError', (MockKwargsException,), {}))
     sushy.exceptions.ConnectionError = (
         type('ConnectionError', (sushy.exceptions.SushyError,), {}))
+    sushy.exceptions.AccessError = (
+        type('AccessError', (sushy.exceptions.SushyError,), {}))
     sushy.exceptions.ResourceNotFoundError = (
         type('ResourceNotFoundError', (sushy.exceptions.SushyError,), {}))
     sushy.exceptions.MissingAttributeError = (
diff --git a/releasenotes/notes/redfish-connection-cache-pool-accesserror-743e39a2f017b990.yaml b/releasenotes/notes/redfish-connection-cache-pool-accesserror-743e39a2f017b990.yaml
new file mode 100644
index 000000000..5bca75335
--- /dev/null
+++ b/releasenotes/notes/redfish-connection-cache-pool-accesserror-743e39a2f017b990.yaml
@@ -0,0 +1,13 @@
+---
+fixes:
+  - |
+    Fixes connection caching issues with Redfish BMCs where AccessErrors were
+    previously not disqualifying the cached connection from being re-used.
+    Ironic will now explicitly open a new connection instead of using the
+    previous connection in the cache. Under normal circumstances, the
+    ``sushy`` redfish library would detect and refresh sessions,
+    however a prior case exists where it may not detect a failure and contain
+    cached session credential data which is ultimately invalid, blocking
+    future access to the BMC via Redfish until the cache entry expired or
+    the ``ironic-conductor`` service was restarted. For more information
+    please see `story 2009719 <https://storyboard.openstack.org/#!/story/2009719>`_.