From 122714f3639c55180c65ca36e2c9e5f6a20291bc Mon Sep 17 00:00:00 2001 From: jneo8 Date: Tue, 24 Oct 2023 15:56:07 +0800 Subject: [PATCH 1/2] feat: Check resource file size Show missing resource block status if resource size is zero --- src/hw_tools.py | 127 ++++++++++++++++++++++++------------ tests/unit/test_hw_tools.py | 81 +++++++++++++++++++---- 2 files changed, 155 insertions(+), 53 deletions(-) diff --git a/src/hw_tools.py b/src/hw_tools.py index 98738800..c6948441 100644 --- a/src/hw_tools.py +++ b/src/hw_tools.py @@ -23,6 +23,14 @@ logger = logging.getLogger(__name__) +class ResourceFileSizeZeroError(Exception): + """The expect resource size should not be zero.""" + + def __init__(self, tool: HWTool, path: Path): + """Init.""" + self.message = f"Tool: {tool} path: {path} size is zero" + + def copy_to_snap_common_bin(source: Path, filename: str) -> None: """Copy file to $SNAP_COMMON/bin folder.""" Path(f"{SNAP_COMMON}/bin").mkdir(parents=False, exist_ok=True) @@ -39,6 +47,20 @@ def symlink(src: Path, dst: Path) -> None: raise +def check_file_size(path: Path) -> bool: + """Verify if the file size > 0. + + Because charm focus us to publish the resources on charmhub, + but most of the hardware related tools have the un-republish + policy. Currently our solution is publish a empty file which + size is 0. + """ + if path.stat().st_size == 0: + logger.info("% size is 0, skip install", path) + return False + return True + + def install_deb(name: str, path: Path) -> None: """Install local deb package.""" _cmd: t.List[str] = ["dpkg", "-i", str(path)] @@ -102,20 +124,6 @@ def remove(self) -> None: class TPRStrategyABC(StrategyABC, metaclass=ABCMeta): """Third party resource strategy class.""" - @staticmethod - def check_file_size(path: Path) -> bool: - """Verify if the file size > 0. - - Because charm focus us to publish the resources on charmhub, - but most of the hardware related tools have the un-republish - policy. Currently our solution is publish a empty file which - size is 0. - """ - if path.stat().st_size == 0: - logger.info("% size is 0, skip install", path) - return False - return True - @abstractmethod def install(self, path: Path) -> None: """Installation details.""" @@ -134,8 +142,8 @@ class StorCLIStrategy(TPRStrategyABC): def install(self, path: Path) -> None: """Install storcli.""" - if not self.check_file_size(path): - return + if not check_file_size(path): + raise ResourceFileSizeZeroError(tool=self._name, path=path) install_deb(self.name, path) symlink(src=self.origin_path, dst=self.symlink_bin) @@ -155,8 +163,8 @@ class PercCLIStrategy(TPRStrategyABC): def install(self, path: Path) -> None: """Install perccli.""" - if not self.check_file_size(path): - return + if not check_file_size(path): + raise ResourceFileSizeZeroError(tool=self._name, path=path) install_deb(self.name, path) symlink(src=self.origin_path, dst=self.symlink_bin) @@ -175,8 +183,8 @@ class SAS2IRCUStrategy(TPRStrategyABC): def install(self, path: Path) -> None: """Install sas2ircu.""" - if not self.check_file_size(path): - return + if not check_file_size(path): + raise ResourceFileSizeZeroError(tool=self._name, path=path) make_executable(path) symlink(src=path, dst=self.symlink_bin) @@ -365,11 +373,18 @@ def strategies(self) -> t.List[StrategyABC]: RedFishStrategy(), ] - def fetch_tools(self, resources: Resources) -> t.Dict[str, Path]: + def fetch_tools( # pylint: disable=W0102 + self, + resources: Resources, + hw_white_list: t.List[HWTool] = [], + ) -> t.Dict[HWTool, Path]: """Fetch resource from juju if it's VENDOR_TOOLS.""" - fetch_tools: t.Dict[str, Path] = {} + fetch_tools: t.Dict[HWTool, Path] = {} # Fetch all tools from juju resources for tool, resource in TPR_RESOURCES.items(): + if tool not in hw_white_list: + logger.info("Skip fetch tool: %s", tool) + continue try: path = resources.fetch(resource) fetch_tools[tool] = path @@ -378,34 +393,64 @@ def fetch_tools(self, resources: Resources) -> t.Dict[str, Path]: return fetch_tools + def check_missing_resources( + self, hw_white_list: t.List[HWTool], fetch_tools: t.Dict[HWTool, Path] + ) -> t.Tuple[bool, str]: + """Check if required resources are not been uploaded.""" + missing_resources = [] + for tool in hw_white_list: + if tool in TPR_RESOURCES: + # Resource hasn't been uploaded + if tool not in fetch_tools: + missing_resources.append(TPR_RESOURCES[tool]) + # Uploaded but file size is zero + path = fetch_tools.get(tool) + if path and not check_file_size(path): + logger.warning("Tool: %s path: %s size is zero", tool, path) + missing_resources.append(TPR_RESOURCES[tool]) + if len(missing_resources) > 0: + return False, f"Missing resources: {missing_resources}" + return True, "" + def install(self, resources: Resources) -> t.Tuple[bool, str]: """Install tools.""" - fetch_tools = self.fetch_tools(resources) hw_white_list = get_hw_tool_white_list() logger.info("hw_tool_white_list: %s", hw_white_list) - # Check if required resources are not been uploaded. - missing_resources = [] - for tool in hw_white_list: - if tool in TPR_RESOURCES and tool not in fetch_tools: - missing_resources.append(TPR_RESOURCES[tool]) - if len(missing_resources) > 0: - return False, f"Missing resources: {missing_resources}" + fetch_tools = self.fetch_tools(resources, hw_white_list) + + ok, msg = self.check_missing_resources(hw_white_list, fetch_tools) + if not ok: + return ok, msg + fail_strategies = [] + strategy_errors = [] + + # Iterate over each strategy and execute. for strategy in self.strategies: if strategy.name not in hw_white_list: continue # TPRStrategy - if isinstance(strategy, TPRStrategyABC): - resource = TPR_RESOURCES.get(strategy.name) - if not resource: - continue - path = resources._paths.get(resource) # pylint: disable=W0212 - if path: - strategy.install(path) - # APTStrategy - elif isinstance(strategy, APTStrategyABC): - strategy.install() # pylint: disable=E1120 + try: + if isinstance(strategy, TPRStrategyABC): + path = fetch_tools.get(strategy.name) # pylint: disable=W0212 + if path: + strategy.install(path) + # APTStrategy + elif isinstance(strategy, APTStrategyABC): + strategy.install() # pylint: disable=E1120 + logger.info("Strategy %s install success", strategy) + except ( + ResourceFileSizeZeroError, + OSError, + apt.PackageError, + ) as e: + logger.warning("Strategy %s install fail: %s", strategy, e) + fail_strategies.append(strategy.name) + strategy_errors.append(e) + + if len(strategy_errors) > 0: + return False, f"Fail strategies: {fail_strategies}" return True, "" def remove(self, resources: Resources) -> None: # pylint: disable=W0613 @@ -416,4 +461,4 @@ def remove(self, resources: Resources) -> None: # pylint: disable=W0613 continue if isinstance(strategy, (TPRStrategyABC, APTStrategyABC)): strategy.remove() - logger.info("Remove resource: %s", strategy) + logger.info("Strategy %s remove success", strategy) diff --git a/tests/unit/test_hw_tools.py b/tests/unit/test_hw_tools.py index 6d8f34cd..3705f9e7 100644 --- a/tests/unit/test_hw_tools.py +++ b/tests/unit/test_hw_tools.py @@ -18,6 +18,7 @@ InvalidCredentialsError, IPMIStrategy, PercCLIStrategy, + ResourceFileSizeZeroError, RetriesExhaustedError, SAS2IRCUStrategy, SAS3IRCUStrategy, @@ -27,6 +28,7 @@ StrategyABC, TPRStrategyABC, bmc_hw_verifier, + check_file_size, copy_to_snap_common_bin, get_hw_tool_white_list, install_deb, @@ -119,7 +121,10 @@ def test_02_fetch_tools(self): for hw_tools_tool in TPR_RESOURCES.values(): mock_resources._paths[hw_tools_tool] = f"path-{hw_tools_tool}" - self.hw_tool_helper.fetch_tools(mock_resources) + self.hw_tool_helper.fetch_tools( + resources=mock_resources, + hw_white_list=[tool for tool in HWTool], + ) for tool in TPR_RESOURCES.values(): mock_resources.fetch.assert_any_call(tool) @@ -130,7 +135,10 @@ def test_03_fetch_tools_error_handling(self): mock_resources._paths = {} mock_resources.fetch.side_effect = ModelError() - fetch_tools = self.hw_tool_helper.fetch_tools(mock_resources) + fetch_tools = self.hw_tool_helper.fetch_tools( + mock_resources, + hw_white_list=[tool for tool in HWTool], + ) for tool in TPR_RESOURCES.values(): mock_resources.fetch.assert_any_call(tool) @@ -263,9 +271,54 @@ def test_09_install_required_resource_not_uploaded(self, _, mock_hw_white_list): self.assertEqual(msg, "Missing resources: ['storcli-deb', 'perccli-deb']") self.assertFalse(self.harness.charm._stored.installed) + @mock.patch( + "hw_tools.get_hw_tool_white_list", + return_value=[HWTool.STORCLI, HWTool.IPMI, HWTool.REDFISH], + ) + @mock.patch( + "hw_tools.HWToolHelper.strategies", + return_value=[ + mock.PropertyMock(spec=TPRStrategyABC), + mock.PropertyMock(spec=APTStrategyABC), + mock.PropertyMock(spec=APTStrategyABC), + ], + new_callable=mock.PropertyMock, + ) + def test_10_install_strategy_errors(self, mock_strategies, mock_hw_white_list): + """Catch excepted error when execute strategies' install method.""" + self.harness.add_resource("storcli-deb", "storcli.deb") + self.harness.begin() + mock_resources = self.harness.charm.model.resources + mock_strategies.return_value[0].name = HWTool.STORCLI + mock_strategies.return_value[1].name = HWTool.IPMI + mock_strategies.return_value[2].name = HWTool.REDFISH + + mock_strategies.return_value[0].install.side_effect = ResourceFileSizeZeroError( + HWTool.STORCLI, "fake-path" + ) + mock_strategies.return_value[1].install.side_effect = OSError("Fake os error") + mock_strategies.return_value[2].install.side_effect = apt.PackageError( + "Fake apt packge error" + ) + + ok, msg = self.hw_tool_helper.install(mock_resources) + + self.assertFalse(ok) + self.assertEqual(f"Fail strategies: {[HWTool.STORCLI, HWTool.IPMI, HWTool.REDFISH]}", msg) + + @mock.patch("hw_tools.check_file_size", return_value=False) + def test_11_check_missing_resources_zero_size_resources(self, check_file_size): + self.harness.begin() + ok, msg = self.hw_tool_helper.check_missing_resources( + hw_white_list=[HWTool.STORCLI], + fetch_tools={HWTool.STORCLI: "fake-path"}, + ) + self.assertFalse(ok) + self.assertEqual("Missing resources: ['storcli-deb']", msg) + class TestStorCLIStrategy(unittest.TestCase): - @mock.patch("hw_tools.TPRStrategyABC.check_file_size", return_value=True) + @mock.patch("hw_tools.check_file_size", return_value=True) @mock.patch("hw_tools.symlink") @mock.patch("hw_tools.install_deb") def test_install(self, mock_install_deb, mock_symlink, _): @@ -281,8 +334,9 @@ def test_install(self, mock_install_deb, mock_symlink, _): @mock.patch("hw_tools.install_deb") def test_install_empty_resource(self, mock_install_deb, mock_symlink): strategy = StorCLIStrategy() - strategy.install(get_mock_path(0)) + with pytest.raises(ResourceFileSizeZeroError): + strategy.install(get_mock_path(0)) mock_install_deb.assert_not_called() mock_symlink.assert_not_called() @@ -339,14 +393,14 @@ def test_remove_deb_error_handling(self, mock_subprocess_check_outpout): class TestTPRStrategyABC(unittest.TestCase): def test_check_file_size_not_zero(self): - self.assertTrue(TPRStrategyABC.check_file_size(get_mock_path(size=100))) + self.assertTrue(check_file_size(get_mock_path(size=100))) def test_check_file_size_zero(self): - self.assertFalse(TPRStrategyABC.check_file_size(get_mock_path(size=0))) + self.assertFalse(check_file_size(get_mock_path(size=0))) class TestSAS2IRCUStrategy(unittest.TestCase): - @mock.patch("hw_tools.TPRStrategyABC.check_file_size", return_value=True) + @mock.patch("hw_tools.check_file_size", return_value=True) @mock.patch("hw_tools.symlink") @mock.patch("hw_tools.make_executable") def test_install(self, mock_make_executable, mock_symlink, _): @@ -359,7 +413,8 @@ def test_install(self, mock_make_executable, mock_symlink, _): @mock.patch("hw_tools.make_executable") def test_install_empty_resource(self, mock_make_executable, mock_symlink): strategy = SAS2IRCUStrategy() - strategy.install(get_mock_path(0)) + with pytest.raises(ResourceFileSizeZeroError): + strategy.install(get_mock_path(0)) mock_make_executable.assert_not_called() mock_symlink.assert_not_called() @@ -372,7 +427,7 @@ def test_remove(self): class TestSAS3IRCUStrategy(unittest.TestCase): - @mock.patch("hw_tools.TPRStrategyABC.check_file_size", return_value=True) + @mock.patch("hw_tools.check_file_size", return_value=True) @mock.patch("hw_tools.symlink") @mock.patch("hw_tools.make_executable") def test_install(self, mock_make_executable, mock_symlink, _): @@ -385,7 +440,8 @@ def test_install(self, mock_make_executable, mock_symlink, _): @mock.patch("hw_tools.make_executable") def test_install_empty_resource(self, mock_make_executable, mock_symlink): strategy = SAS3IRCUStrategy() - strategy.install(get_mock_path(0)) + with pytest.raises(ResourceFileSizeZeroError): + strategy.install(get_mock_path(0)) mock_make_executable.assert_not_called() mock_symlink.assert_not_called() @@ -398,7 +454,7 @@ def test_remove(self): class TestPercCLIStrategy(unittest.TestCase): - @mock.patch("hw_tools.TPRStrategyABC.check_file_size", return_value=True) + @mock.patch("hw_tools.check_file_size", return_value=True) @mock.patch("hw_tools.symlink") @mock.patch("hw_tools.install_deb") def test_install(self, mock_install_deb, mock_symlink, _): @@ -419,7 +475,8 @@ def test_install_empty_resource(self, mock_install_deb, mock_symlink): mock_path_stat.st_size = 0 strategy = PercCLIStrategy() - strategy.install(mock_path) + with pytest.raises(ResourceFileSizeZeroError): + strategy.install(mock_path) mock_install_deb.assert_not_called() mock_symlink.assert_not_called() From b71974c870c5952b16236060b1a869d6c368e191 Mon Sep 17 00:00:00 2001 From: jneo8 Date: Wed, 25 Oct 2023 15:14:16 +0800 Subject: [PATCH 2/2] docs: Update doc-string for ResourceFileSizeZeroError --- src/hw_tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hw_tools.py b/src/hw_tools.py index c6948441..f54618f1 100644 --- a/src/hw_tools.py +++ b/src/hw_tools.py @@ -24,7 +24,7 @@ class ResourceFileSizeZeroError(Exception): - """The expect resource size should not be zero.""" + """Empty resource error.""" def __init__(self, tool: HWTool, path: Path): """Init."""