diff --git a/nodescraper/connection/inband/inbandmanager.py b/nodescraper/connection/inband/inbandmanager.py index bec1931d..f0410e5e 100644 --- a/nodescraper/connection/inband/inbandmanager.py +++ b/nodescraper/connection/inband/inbandmanager.py @@ -119,6 +119,8 @@ def connect( priority=EventPriority.CRITICAL, console_log=True, ) + self.connection = None # Nullify the connection since its not usable + self.result.status = ExecutionStatus.EXECUTION_FAILURE except Exception as exception: self._log_event( category=EventCategory.SSH, @@ -127,10 +129,12 @@ def connect( priority=EventPriority.CRITICAL, console_log=True, ) + self.connection = None # Nullify the connection since its not usable + self.result.status = ExecutionStatus.EXECUTION_FAILURE return self.result def disconnect(self): """Disconnect in-band connection""" - super().disconnect() if isinstance(self.connection, RemoteShell): self.connection.client.close() + super().disconnect() diff --git a/nodescraper/connection/redfish/redfish_connection.py b/nodescraper/connection/redfish/redfish_connection.py index d8cbcd2b..23a4ae86 100644 --- a/nodescraper/connection/redfish/redfish_connection.py +++ b/nodescraper/connection/redfish/redfish_connection.py @@ -327,6 +327,9 @@ def close(self) -> None: self._session.delete(self._session_uri, timeout=self.timeout) except Exception: pass + + if self._session: + self._session.close() self._session = None self._session_token = None self._session_uri = None diff --git a/nodescraper/interfaces/connectionmanager.py b/nodescraper/interfaces/connectionmanager.py index 7c649021..60212130 100644 --- a/nodescraper/interfaces/connectionmanager.py +++ b/nodescraper/interfaces/connectionmanager.py @@ -95,7 +95,7 @@ def __init__( logger: Optional[logging.Logger] = None, max_event_priority_level: Union[EventPriority, str] = EventPriority.CRITICAL, parent: Optional[str] = None, - task_result_hooks: Optional[list[TaskResultHook], None] = None, + task_result_hooks: Optional[list[TaskResultHook]] = None, connection_args: Optional[Union[TConnectArg, dict[str, Any]]] = None, event_reporter: str = DEFAULT_EVENT_REPORTER, session_id: Optional[str] = None, diff --git a/test/unit/connection/redfish/test_redfish_connection.py b/test/unit/connection/redfish/test_redfish_connection.py new file mode 100644 index 00000000..b7cb61fa --- /dev/null +++ b/test/unit/connection/redfish/test_redfish_connection.py @@ -0,0 +1,39 @@ +# Copyright (C) 2025 Advanced Micro Devices, Inc. + +from __future__ import annotations + +from unittest.mock import patch + +import pytest + +from nodescraper.connection.redfish import RedfishConnection + + +@pytest.fixture +def rf_conn() -> RedfishConnection: + return RedfishConnection( + base_url="https://bmc.example", + username="u", + password=None, + verify_ssl=False, + ) + + +@patch("nodescraper.connection.redfish.redfish_connection.requests.Session") +def test_close_clears_cached_session(mock_session_cls, rf_conn: RedfishConnection) -> None: + """Baseline: close() drops the cached session so a later call builds a new one.""" + rf_conn._ensure_session() + rf_conn.close() + + assert rf_conn._session is None + + +@patch("nodescraper.connection.redfish.redfish_connection.requests.Session") +def test_close_releases_underlying_http_session( + mock_session_cls, rf_conn: RedfishConnection +) -> None: + """close() must close the requests session so its connection pool is released.""" + session = rf_conn._ensure_session() + rf_conn.close() + + session.close.assert_called_once() # type: ignore diff --git a/test/unit/connection/test_connectionmanager.py b/test/unit/connection/test_connectionmanager.py new file mode 100644 index 00000000..6ea72ec8 --- /dev/null +++ b/test/unit/connection/test_connectionmanager.py @@ -0,0 +1,198 @@ +############################################################################### +# +# MIT License +# +# Copyright (c) 2025 Advanced Micro Devices, Inc. +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in all +# copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +# SOFTWARE. +# +############################################################################### +import unittest +from unittest.mock import MagicMock + +from nodescraper.connection.inband.inbandmanager import InBandConnectionManager +from nodescraper.connection.inband.inbandremote import RemoteShell +from nodescraper.connection.redfish.redfish_connection import RedfishConnection +from nodescraper.connection.redfish.redfish_manager import RedfishConnectionManager +from nodescraper.enums import ExecutionStatus, SystemLocation +from nodescraper.interfaces.connectionmanager import ConnectionManager +from nodescraper.models import SystemInfo, TaskResult + + +class DummyConnection: + """Simple test connection class for testing ConnectionManager base behavior""" + + pass + + +class DummyConnectionManager(ConnectionManager[DummyConnection, None]): + """Concrete implementation of ConnectionManager for testing base class behavior""" + + def connect(self) -> TaskResult: + """Minimal connect implementation for testing""" + self.connection = DummyConnection() + return self.result + + +class TestConnectionManagerBase(unittest.TestCase): + """Test suite for ConnectionManager base behavior""" + + def test_disconnect_clears_connection(self): + """Test that disconnect clears the connection attribute""" + system_info = SystemInfo( + name="test_system", + location=SystemLocation.LOCAL, + ) + manager = DummyConnectionManager(system_info=system_info) + manager.connection = DummyConnection() + + manager.disconnect() + + self.assertIsNone(manager.connection) + + def test_disconnect_resets_status(self): + """Test that disconnect resets the result status to UNSET""" + system_info = SystemInfo( + name="test_system", + location=SystemLocation.LOCAL, + ) + manager = DummyConnectionManager(system_info=system_info) + manager.connection = DummyConnection() + manager.result.status = ExecutionStatus.OK + + manager.disconnect() + + self.assertEqual(manager.result.status, ExecutionStatus.UNSET) + + def test_context_manager_calls_disconnect(self): + """Test that using connection manager as context manager calls disconnect on exit""" + system_info = SystemInfo( + name="test_system", + location=SystemLocation.LOCAL, + ) + manager = DummyConnectionManager(system_info=system_info) + + with manager: + manager.connection = DummyConnection() + self.assertIsNotNone(manager.connection) + + # After exiting context, disconnect should have been called + self.assertIsNone(manager.connection) + + def test_connect_decorator_initializes_result(self): + """Test that the connect decorator properly initializes the result""" + system_info = SystemInfo( + name="test_system", + location=SystemLocation.LOCAL, + ) + manager = DummyConnectionManager(system_info=system_info) + + result = manager.connect() + + self.assertIsNotNone(result) + self.assertIsInstance(manager.connection, DummyConnection) + + def test_context_manager_handles_disconnect_exception(self): + """BUG: __exit__ doesn't handle exceptions from disconnect()""" + system_info = SystemInfo( + name="test_system", + location=SystemLocation.LOCAL, + ) + manager = DummyConnectionManager(system_info=system_info) + + # Mock disconnect to raise an exception + def bad_disconnect(): + raise RuntimeError("Disconnect failed") + + manager.disconnect = bad_disconnect + + # If disconnect raises an exception in __exit__, it could mask original exceptions + with self.assertRaises(RuntimeError) as context: + with manager: + manager.connection = DummyConnection() + + self.assertIn("Disconnect failed", str(context.exception)) + + +class TestInBandConnectionManagerDisconnect(unittest.TestCase): + """Test suite specifically for InBandConnectionManager disconnect behavior""" + + def test_disconnect_remote_closes_ssh_client(self): + """Test that disconnect properly closes SSH client for remote connections""" + system_info = SystemInfo( + name="test_system", + location=SystemLocation.REMOTE, + ) + manager = InBandConnectionManager(system_info=system_info) + + # Mock a RemoteShell with a client + mock_client = MagicMock() + mock_remote_shell = MagicMock(spec=RemoteShell) + mock_remote_shell.client = mock_client + + manager.connection = mock_remote_shell + + manager.disconnect() + + # Verify client.close() was called + mock_client.close.assert_called_once() + # Verify connection was cleared by parent class + self.assertIsNone(manager.connection) + + +class TestRedfishConnectionManagerDisconnect(unittest.TestCase): + """Test suite for RedfishConnectionManager disconnect behavior""" + + def test_disconnect_closes_redfish_connection(self): + """Test that disconnect properly closes Redfish connection""" + system_info = SystemInfo( + name="test_system", + location=SystemLocation.REMOTE, + ) + + # Create manager without actual connection + manager = RedfishConnectionManager(system_info=system_info) + + # Mock the connection with a close method + mock_connection = MagicMock(spec=RedfishConnection) + + manager.connection = mock_connection + + manager.disconnect() + + # Verify connection.close() was called + mock_connection.close.assert_called_once() + # Verify connection was cleared by parent class + self.assertIsNone(manager.connection) + + +if __name__ == "__main__": + unittest.main() + + +class TestConnectionManagerAnnotations(unittest.TestCase): + """Type annotations on the base connection manager must be resolvable.""" + + def test_init_type_hints_are_resolvable(self): + """get_type_hints must work on ConnectionManager.__init__ for introspection tooling""" + import typing + + hints = typing.get_type_hints(ConnectionManager.__init__) + + self.assertIn("task_result_hooks", hints) diff --git a/test/unit/connection/test_inbandmanager.py b/test/unit/connection/test_inbandmanager.py new file mode 100644 index 00000000..982b9c27 --- /dev/null +++ b/test/unit/connection/test_inbandmanager.py @@ -0,0 +1,170 @@ +############################################################################### +# +# MIT License +# +# Copyright (c) 2025 Advanced Micro Devices, Inc. +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in all +# copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +# SOFTWARE. +# +############################################################################### +import unittest +from unittest.mock import MagicMock, patch + +from nodescraper.connection.inband.inbandlocal import LocalShell +from nodescraper.connection.inband.inbandmanager import InBandConnectionManager +from nodescraper.connection.inband.inbandremote import RemoteShell, SSHConnectionError +from nodescraper.connection.inband.sshparams import SSHConnectionParams +from nodescraper.enums import SystemLocation +from nodescraper.models import SystemInfo + + +class TestInBandConnectionManager(unittest.TestCase): + """Test suite for InBandConnectionManager""" + + def setUp(self): + """Set up test fixtures""" + self.system_info = SystemInfo( + name="test_system", + location=SystemLocation.REMOTE, + ) + + def test_connect_local(self): + """Test connecting to a local system""" + from nodescraper.enums import ExecutionStatus + + local_system_info = SystemInfo( + name="local_system", + location=SystemLocation.LOCAL, + ) + manager = InBandConnectionManager(system_info=local_system_info) + + result = manager.connect() + + self.assertIsInstance(manager.connection, LocalShell) + self.assertIn(result.status, [ExecutionStatus.OK, ExecutionStatus.UNSET]) + + @patch("nodescraper.connection.inband.inbandmanager.RemoteShell") + def test_connect_remote_success(self, mock_remote_shell_class): + """Test successful remote SSH connection""" + mock_remote_shell = MagicMock(spec=RemoteShell) + mock_remote_shell_class.return_value = mock_remote_shell + + ssh_params = SSHConnectionParams( + hostname="test.example.com", username="testuser", password="testpass" + ) + + manager = InBandConnectionManager(system_info=self.system_info, connection_args=ssh_params) + + _result = manager.connect() + + mock_remote_shell_class.assert_called_once_with(ssh_params) + mock_remote_shell.connect_ssh.assert_called_once() + self.assertEqual(manager.connection, mock_remote_shell) + + def test_connect_remote_no_credentials(self): + """Test remote connection without SSH credentials""" + from nodescraper.enums import ExecutionStatus + + manager = InBandConnectionManager(system_info=self.system_info) + + result = manager.connect() + + self.assertEqual(result.status, ExecutionStatus.EXECUTION_FAILURE) + + def test_disconnect_remote_calls_client_close(self): + """Test that disconnect calls client.close() for remote connections""" + mock_client = MagicMock() + mock_remote_shell = MagicMock(spec=RemoteShell) + mock_remote_shell.client = mock_client + + manager = InBandConnectionManager(system_info=self.system_info) + manager.connection = mock_remote_shell + + manager.disconnect() + + mock_client.close.assert_called_once() + + def test_disconnect_local_does_not_call_client_close(self): + """Test that disconnect does not call client.close() for local connections""" + mock_local_shell = MagicMock(spec=LocalShell) + + manager = InBandConnectionManager(system_info=self.system_info) + manager.connection = mock_local_shell + + # Should not raise an error even though LocalShell doesn't have a client attribute + manager.disconnect() + + # Verify that we didn't try to access client.close() + self.assertFalse(hasattr(mock_local_shell, "client")) + + def test_disconnect_remote_with_none_client_raises_error(self): + """BUG: disconnect fails when RemoteShell.client is None""" + mock_remote_shell = MagicMock(spec=RemoteShell) + mock_remote_shell.client = None + + manager = InBandConnectionManager(system_info=self.system_info) + manager.connection = mock_remote_shell + + # This should raise AttributeError because client is None + with self.assertRaises(AttributeError): + manager.disconnect() + + def test_disconnect_remote_when_client_close_raises_exception(self): + """BUG: if client.close() raises exception, super().disconnect() is never called""" + mock_client = MagicMock() + mock_client.close.side_effect = Exception("Connection already closed") + + mock_remote_shell = MagicMock(spec=RemoteShell) + mock_remote_shell.client = mock_client + + manager = InBandConnectionManager(system_info=self.system_info) + manager.connection = mock_remote_shell + + # This should raise the exception from close() + with self.assertRaises(Exception) as context: + manager.disconnect() + + self.assertIn("Connection already closed", str(context.exception)) + + # BUG: Connection is not cleared because super().disconnect() was never called + self.assertIsNotNone(manager.connection) + + @patch("nodescraper.connection.inband.inbandmanager.RemoteShell") + def test_connect_remote_failure_clears_connection(self, mock_remote_shell_class): + """A failed SSH connect must not leave an unusable connection object behind""" + mock_remote_shell = MagicMock(spec=RemoteShell) + mock_remote_shell.connect_ssh.side_effect = SSHConnectionError("SSH Authentication failed") + mock_remote_shell_class.return_value = mock_remote_shell + + manager = InBandConnectionManager( + system_info=self.system_info, + connection_args=SSHConnectionParams( + hostname="test.example.com", + username="testuser", + password="testpass", + ), + ) + + manager.connect() + + self.assertIsNone(manager.connection) + + +if __name__ == "__main__": + unittest.main()