feat: Check resource file size

Show missing resource block status if resource size is zero

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)

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()