From e264b87c489ff8918f5ca81db1d930933f78fd11 Mon Sep 17 00:00:00 2001 From: Ashley James Date: Fri, 29 Sep 2023 15:41:19 +0530 Subject: [PATCH 1/4] Replace redfish discover method. This is done because redfish.discover_ssdp() discovers all available redfish services on the same network. This change checks if redfish is available by trying to login and check for the type of exception being raised. If RetriesExhaustedError is raised, it means that there is no redfish service. Resolves #61. --- src/hw_tools.py | 32 ++++++++++++++++++++++++++++---- tests/unit/test_hw_tools.py | 4 ++-- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/src/hw_tools.py b/src/hw_tools.py index ae830b3c..7c98d536 100644 --- a/src/hw_tools.py +++ b/src/hw_tools.py @@ -11,12 +11,13 @@ from abc import ABCMeta, abstractmethod from pathlib import Path -import redfish from charms.operator_libs_linux.v0 import apt from ops.model import ModelError, Resources +from redfish import redfish_client +from redfish.rest.v1 import InvalidCredentialsError, RetriesExhaustedError, SessionCreationError from config import SNAP_COMMON, TOOLS_DIR, TPR_RESOURCES, HWTool, StorageVendor, SystemVendor -from hardware import SUPPORTED_STORAGES, lshw +from hardware import SUPPORTED_STORAGES, get_bmc_address, lshw from keys import HP_KEYS logger = logging.getLogger(__name__) @@ -299,6 +300,30 @@ def raid_hw_verifier() -> t.List[HWTool]: return list(tools) +def redfish_available() -> bool: + """Check if redfish service is available.""" + redfish_obj = None + bmc_address = get_bmc_address() + host = f"https://{bmc_address}" + # credentials can be empty because we're only checking if redfish service is accessible + user = "" + pwd = "" + + try: + redfish_obj = redfish_client(base_url=host, username=user, password=pwd) + redfish_obj.login(auth="session") + except RetriesExhaustedError: # redfish not available + result = False + except (SessionCreationError, InvalidCredentialsError): + # redfish available, wrong credentials or not able to create a session + result = True + else: # login succeeded with empty credentials + result = True + redfish_obj.logout() + + return result + + def bmc_hw_verifier() -> t.List[HWTool]: """Verify if the ipmi is available on the machine. @@ -314,8 +339,7 @@ def bmc_hw_verifier() -> t.List[HWTool]: logger.info("IPMI is not available") # Check RedFish available - services = redfish.discover_ssdp() - if len(services): + if redfish_available(): tools.append(HWTool.REDFISH) else: logger.info("Redfish is not available") diff --git a/tests/unit/test_hw_tools.py b/tests/unit/test_hw_tools.py index 509bf7f8..d0b4095b 100644 --- a/tests/unit/test_hw_tools.py +++ b/tests/unit/test_hw_tools.py @@ -550,7 +550,7 @@ def test_raid_hw_verifier(mock_lshw, lshw_output, lshw_storage_output, expect): class TestIPMIHWVerifier(unittest.TestCase): - @mock.patch("hw_tools.redfish.discover_ssdp", return_value=[1]) + @mock.patch("hw_tools.redfish_available", return_value=True) @mock.patch("hw_tools.subprocess") @mock.patch("hw_tools.apt") def test_bmc_hw_verifier(self, mock_apt, mock_subprocess, mock_redfish_discover_ssdp): @@ -560,7 +560,7 @@ def test_bmc_hw_verifier(self, mock_apt, mock_subprocess, mock_redfish_discover_ self.assertCountEqual(output, [HWTool.IPMI, HWTool.REDFISH]) mock_redfish_discover_ssdp.assert_called() - @mock.patch("hw_tools.redfish.discover_ssdp", return_value=[]) + @mock.patch("hw_tools.redfish_available", return_value=False) @mock.patch( "hw_tools.subprocess.check_output", side_effect=subprocess.CalledProcessError(-1, "cmd"), From 464c3fa6c266f68247c4e397f4468d3e9f20a516 Mon Sep 17 00:00:00 2001 From: Ashley James Date: Mon, 2 Oct 2023 10:24:01 +0530 Subject: [PATCH 2/4] func: Wait for cleanup for relation removal test. The "on_remove_event" test was failing because after the relation between the ubuntu and hw-observer charm was removed, the ubuntu app would be active/idle and settle down before the cleanup activities were completed for hw-observer. --- tests/functional/test_charm.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/functional/test_charm.py b/tests/functional/test_charm.py index 4086e6ce..a2fcf3da 100644 --- a/tests/functional/test_charm.py +++ b/tests/functional/test_charm.py @@ -195,6 +195,11 @@ async def test_20_on_remove_event(self, app, sync_helper, ops_test): ) principal_unit = ops_test.model.applications[PRINCIPAL_APP_NAME].units[0] + # Wait for cleanup activities to finish + await ops_test.model.block_until( + lambda: ops_test.model.applications[APP_NAME].status == "unknown" + ) + cmd = "ls /etc/hardware-exporter-config.yaml" results = await sync_helper.run_command_on_unit(ops_test, principal_unit.name, cmd) assert results.get("return-code") > 0 From d1442e5fc0ddc8a7e9e03a3e82ae397c0a4759c2 Mon Sep 17 00:00:00 2001 From: Ashley James Date: Tue, 3 Oct 2023 11:10:32 +0530 Subject: [PATCH 3/4] Add unit tests for redfish_available() --- src/hw_tools.py | 7 ++--- tests/unit/test_hw_tools.py | 54 ++++++++++++++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/src/hw_tools.py b/src/hw_tools.py index 7c98d536..2a704426 100644 --- a/src/hw_tools.py +++ b/src/hw_tools.py @@ -305,12 +305,9 @@ def redfish_available() -> bool: redfish_obj = None bmc_address = get_bmc_address() host = f"https://{bmc_address}" - # credentials can be empty because we're only checking if redfish service is accessible - user = "" - pwd = "" - try: - redfish_obj = redfish_client(base_url=host, username=user, password=pwd) + # credentials can be empty because we're only checking if redfish service is accessible + redfish_obj = redfish_client(base_url=host, username="", password="") redfish_obj.login(auth="session") except RetriesExhaustedError: # redfish not available result = False diff --git a/tests/unit/test_hw_tools.py b/tests/unit/test_hw_tools.py index d0b4095b..6d8f34cd 100644 --- a/tests/unit/test_hw_tools.py +++ b/tests/unit/test_hw_tools.py @@ -15,10 +15,13 @@ from hw_tools import ( APTStrategyABC, HWToolHelper, + InvalidCredentialsError, IPMIStrategy, PercCLIStrategy, + RetriesExhaustedError, SAS2IRCUStrategy, SAS3IRCUStrategy, + SessionCreationError, SSACLIStrategy, StorCLIStrategy, StrategyABC, @@ -29,6 +32,7 @@ install_deb, make_executable, raid_hw_verifier, + redfish_available, remove_deb, symlink, ) @@ -550,15 +554,59 @@ def test_raid_hw_verifier(mock_lshw, lshw_output, lshw_storage_output, expect): class TestIPMIHWVerifier(unittest.TestCase): + @mock.patch("hw_tools.redfish_client") + @mock.patch("hw_tools.get_bmc_address", return_value="1.2.3.4") + def test_redfish_not_available(self, mock_bmc_address, mock_redfish_client): + mock_redfish_obj = mock.Mock() + mock_redfish_client.return_value = mock_redfish_obj + mock_redfish_obj.login.side_effect = RetriesExhaustedError() + + result = redfish_available() + + self.assertEqual(result, False) + mock_bmc_address.assert_called_once() + mock_redfish_client.assert_called_once() + mock_redfish_obj.login.assert_called_once() + + @mock.patch("hw_tools.redfish_client") + @mock.patch("hw_tools.get_bmc_address", return_value="1.2.3.4") + def test_redfish_available(self, mock_bmc_address, mock_redfish_client): + mock_redfish_obj = mock.Mock() + mock_redfish_client.return_value = mock_redfish_obj + + for exc in [SessionCreationError, InvalidCredentialsError]: + mock_redfish_obj.login.side_effect = exc + result = redfish_available() + self.assertEqual(result, True) + + mock_bmc_address.assert_called() + mock_redfish_client.assert_called() + mock_redfish_obj.login.assert_called() + + @mock.patch("hw_tools.redfish_client") + @mock.patch("hw_tools.get_bmc_address", return_value="1.2.3.4") + def test_redfish_available_and_login_success(self, mock_bmc_address, mock_redfish_client): + mock_redfish_obj = mock.Mock() + mock_redfish_client.return_value = mock_redfish_obj + + result = redfish_available() + + self.assertEqual(result, True) + + mock_bmc_address.assert_called_once() + mock_redfish_client.assert_called_once() + mock_redfish_obj.login.assert_called_once() + mock_redfish_obj.logout.assert_called_once() + @mock.patch("hw_tools.redfish_available", return_value=True) @mock.patch("hw_tools.subprocess") @mock.patch("hw_tools.apt") - def test_bmc_hw_verifier(self, mock_apt, mock_subprocess, mock_redfish_discover_ssdp): + def test_bmc_hw_verifier(self, mock_apt, mock_subprocess, mock_redfish_available): output = bmc_hw_verifier() mock_apt.add_package.assert_called_with("ipmitool", update_cache=False) mock_subprocess.check_output.assert_called_with("ipmitool lan print".split()) self.assertCountEqual(output, [HWTool.IPMI, HWTool.REDFISH]) - mock_redfish_discover_ssdp.assert_called() + mock_redfish_available.assert_called() @mock.patch("hw_tools.redfish_available", return_value=False) @mock.patch( @@ -567,7 +615,7 @@ def test_bmc_hw_verifier(self, mock_apt, mock_subprocess, mock_redfish_discover_ ) @mock.patch("hw_tools.apt") def test_bmc_hw_verifier_error_handling( - self, mock_apt, mock_check_output, mock_redfish_discover_ssdp + self, mock_apt, mock_check_output, mock_redfish_available ): output = bmc_hw_verifier() mock_apt.add_package.assert_called_with("ipmitool", update_cache=False) From bf10177a6d4ee37221d4c44f01b70648f5ff29f9 Mon Sep 17 00:00:00 2001 From: Ashley James Date: Tue, 3 Oct 2023 14:07:17 +0530 Subject: [PATCH 4/4] Remove unnecessary statement. --- src/hw_tools.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/hw_tools.py b/src/hw_tools.py index 2a704426..98738800 100644 --- a/src/hw_tools.py +++ b/src/hw_tools.py @@ -302,7 +302,6 @@ def raid_hw_verifier() -> t.List[HWTool]: def redfish_available() -> bool: """Check if redfish service is available.""" - redfish_obj = None bmc_address = get_bmc_address() host = f"https://{bmc_address}" try: