Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/checksize #73

Merged
merged 2 commits into from
Oct 27, 2023
+155 −53
Merged

Feat/checksize #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
122714f
feat: Check resource file size
jneo8 Oct 24, 2023
b71974c
docs: Update doc-string for ResourceFileSizeZeroError
jneo8 Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 86 additions & 41 deletions src/hw_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@
logger = logging.getLogger(__name__)


class ResourceFileSizeZeroError(Exception):
"""Empty resource error."""

def __init__(self, tool: HWTool, path: Path):
"""Init."""
self.message = f"Tool: {tool} path: {path} size is zero"
Comment on lines +29 to +31
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work properly.
example:

>>> class TestException(Exception):
...  def __init__(self, arg1, arg2):
...   self.message = f"test message with: {arg1}, {arg2}"
... 
>>> raise TestException("a", "b")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TestException: ('a', 'b')

We should use super().__init__(f"message").



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)
Expand All @@ -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)]
Expand Down Expand Up @@ -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."""
Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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)

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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)
81 changes: 69 additions & 12 deletions tests/unit/test_hw_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
InvalidCredentialsError,
IPMIStrategy,
PercCLIStrategy,
ResourceFileSizeZeroError,
RetriesExhaustedError,
SAS2IRCUStrategy,
SAS3IRCUStrategy,
Expand All @@ -27,6 +28,7 @@
StrategyABC,
TPRStrategyABC,
bmc_hw_verifier,
check_file_size,
copy_to_snap_common_bin,
get_hw_tool_white_list,
install_deb,
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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, _):
Expand All @@ -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()

Expand Down Expand Up @@ -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, _):
Expand All @@ -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()
Expand All @@ -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, _):
Expand All @@ -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()
Expand All @@ -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, _):
Expand All @@ -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()
Expand Down
Loading