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.

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

Add Snap Strategy #306

Closed
wants to merge 10 commits into from
44 changes: 40 additions & 4 deletions src/hw_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import requests
import urllib3
from charms.operator_libs_linux.v0 import apt
from charms.operator_libs_linux.v2 import snap
from ops.model import ModelError, Resources

import apt_helpers
Expand Down Expand Up @@ -175,6 +176,41 @@ def remove(self) -> None:
# the remove option.


class SnapStrategy(StrategyABC):
Copy link
Contributor

@jneo8 jneo8 Sep 18, 2024

Choose a reason for hiding this comment

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

suggestion:

I kind of prefer code like this:

  • No init function
  • hard code HWTool
  • Pass channel on install function
class SnapStrategyABC(StrategyABC, metaclass=ABCMeta):
    """Snap strategy class."""

    def install(self, channel: str = "latest/stable") -> None:
        """Install the snap from a channel."""
        # define install details here.
        ...
        
class SmartCtlStrategy(SnapStrategyABC):
    _name = HWTool.SMARTCTL_EXPORTER
                     
class HWToolHelper:
    """Helper to install vendor's or hardware related tools."""

   def strategies(self) -> List[StrategyABC]:
        """Define strategies for every tools."""
        return [
            StorCLIStrategy(),
            RedFishStrategy(),
            ...
            # It's more easy to define strategy here
            SmartCtlStrategy(),
        ]
    # The channel information should only be given here. This make sure the object dependency is one-direction.
    def install(self, resources: Resources, channel: (some-type), hw_available: Set[HWTool]) -> Tuple[bool, str]:
        """Install tools."""
        
        ...
           
        for strategy in self.strategies:
            if strategy.name not in hw_available:
                continue
            try:
                if isinstance(strategy, TPRStrategyABC):
                    path = fetch_tools.get(strategy.name)  # pylint: disable=W0212
                    if path:
                        strategy.install(path)
                if isinstance(strategy, SnapStrategy):
                    # The channel should be an argument for install function
                    strategy.install(channel)
                elif isinstance(strategy, APTStrategyABC):
                    strategy.install()  # pylint: disable=E1120
                logger.info("Strategy %s install success", strategy)
            except (
                ResourceFileSizeZeroError,
                OSError,
                apt.PackageError,
                ResourceChecksumError,
                snap.SnapError,
            ) as e:
                logger.warning("Strategy %s install fail: %s", strategy, e)
                fail_strategies.append(strategy.name)

        if fail_strategies:
            return False, f"Fail strategies: {fail_strategies}"
        return True, ""

So the (dis)advantage are:

  • The strategy can keep in hard code, it's more easy to trace and keep the flexibility. (Imagine if you have different operation for DCGM & Smart).
  • Chance to use snap resource in the future
  • The only breaking change only happen on HWToolHelper.install function. Other design won't be broken.
    • Get the channel information in Charm class object and pass it to install function. This make sure the object dependency is one direaction.

Copy link
Member Author

Choose a reason for hiding this comment

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

Different snaps will have different channels to install from. I don't see how passing a single channel on HWToolHelper can solve this. I think it would be very strange to have one charm config for channel to rule on all snaps channels.

Copy link
Contributor

Choose a reason for hiding this comment

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

while I agree that we should remove the constructor for consistency with the other strategies, I don't quite get why it would be preferable to move complexity to the install function, as that requires the caller to have deeper knowledge about how each strategy works and adds extra conditionals to HWToolHelper.install

Copy link
Contributor

@jneo8 jneo8 Sep 19, 2024

Choose a reason for hiding this comment

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

We need a protocol to pass all the required material, resources, configuration, and hardware detect result, for the strategies.
So install function is the protocol right now. If we break this rule then the ISP(interface-segregation principles) will be broken then you have to find a tricky/hacky way to import those material into the strategies.

(I am open to any way to refactor the protocol we have now, but it's a refactor so it shouldn't happen on this PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

Different snaps will have different channels to install from. I don't see how passing a single channel on ...

It's a pseudocode code, so pass the configuration you want.

"""Snap strategy class."""

def __init__(self, tool: HWTool, channel: str):
"""Snap strategy constructor."""
self._name = tool
self.snap_name = tool.value
self.channel = channel
self.snap_client = snap.SnapCache()[tool.value]

def install(self) -> None:
"""Install the snap from a channel."""
try:
snap.add(self.snap_name, channel=self.channel)
# using the snap.SnapError will result into:
# TypeError: catching classes that do not inherit from BaseException is not allowed
except Exception as err: # pylint: disable=broad-except
logger.error(
"Failed to install %s from channel: %s: %s", self.snap_name, self.channel, err
)
Comment on lines +196 to +198
Copy link
Contributor

Choose a reason for hiding this comment

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

This logs an error to the charm log, but that's silent to the user - can we bubble this error up to the charm status? How do the other exporters handle installation failures?

Copy link
Contributor

Choose a reason for hiding this comment

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

good found Samuel!

Please follow the current error handling design here.

except (
ResourceFileSizeZeroError,
OSError,
apt.PackageError,
ResourceChecksumError,
) as e:
logger.warning("Strategy %s install fail: %s", strategy, e)
fail_strategies.append(strategy.name)

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot to re-raise the error. Thanks

raise err

logger.info("Installed %s from channel: %s", self.snap_name, self.channel)
# enable services because some might be disabled by default
self.snap_client.start(list(self.snap_client.services.keys()), enable=True)

def remove(self) -> None:
"""Remove the snap."""
snap.remove([self.snap_name])

def check(self) -> bool:
"""Check if all services are active."""
return all(service.get("active", False) for service in self.snap_client.services.values())


class TPRStrategyABC(StrategyABC, metaclass=ABCMeta):
"""Third party resource strategy class."""

Expand Down Expand Up @@ -689,20 +725,20 @@ def install(self, resources: Resources, hw_available: Set[HWTool]) -> Tuple[bool
if strategy.name not in hw_available:
continue
try:
# TPRStrategy
if isinstance(strategy, TPRStrategyABC):
path = fetch_tools.get(strategy.name) # pylint: disable=W0212
if path:
strategy.install(path)
# APTStrategy
elif isinstance(strategy, APTStrategyABC):

elif isinstance(strategy, (APTStrategyABC, SnapStrategy)):
chanchiwai-ray marked this conversation as resolved.
Show resolved Hide resolved
strategy.install() # pylint: disable=E1120
logger.info("Strategy %s install success", strategy)
except (
ResourceFileSizeZeroError,
OSError,
apt.PackageError,
ResourceChecksumError,
snap.SnapError,
) as e:
logger.warning("Strategy %s install fail: %s", strategy, e)
fail_strategies.append(strategy.name)
Expand All @@ -717,7 +753,7 @@ def remove(self, resources: Resources, hw_available: Set[HWTool]) -> None:
for strategy in self.strategies:
if strategy.name not in hw_available:
continue
if isinstance(strategy, (TPRStrategyABC, APTStrategyABC)):
if isinstance(strategy, (TPRStrategyABC, APTStrategyABC, SnapStrategy)):
strategy.remove()
logger.info("Strategy %s remove success", strategy)

Expand Down
132 changes: 132 additions & 0 deletions tests/unit/test_hw_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
SAS3IRCUStrategy,
SmartCtlExporterStrategy,
SmartCtlStrategy,
SnapStrategy,
SSACLIStrategy,
StorCLIStrategy,
StrategyABC,
Expand All @@ -57,6 +58,7 @@
symlink,
)
from keys import HP_KEYS
from lib.charms.operator_libs_linux.v2 import snap


def get_mock_path(size: int):
Expand Down Expand Up @@ -1147,3 +1149,133 @@ def mock_get_response_ipmi(ipmi_call):
output = bmc_hw_verifier()
mock_apt_helpers.add_pkg_with_candidate_version.assert_called_with("freeipmi-tools")
self.assertCountEqual(output, [HWTool.IPMI_SENSOR, HWTool.IPMI_SEL])


@mock.patch("hw_tools.snap")
def test_snap_strategy_name(_):
hwtool = mock.MagicMock()
hwtool.value = "my-snap"

strategy = SnapStrategy(hwtool, "my-channel")
assert strategy.name == hwtool


@mock.patch("hw_tools.snap.SnapCache")
@mock.patch("hw_tools.snap")
def test_snap_strategy_install(mock_snap, mock_snap_cache):
hwtool = mock.MagicMock()
hwtool.value = "my-snap"
channel = "my-channel"

mock_snap_client = mock.MagicMock()
mock_snap_client.services = {"service1": {}, "service2": {}}
mock_snap_cache.return_value = {"my-snap": mock_snap_client}

strategy = SnapStrategy(hwtool, channel)
strategy.install()
mock_snap.add.assert_called_with(strategy.snap_name, channel=channel)
mock_snap_client.start.assert_called_once_with(["service1", "service2"], enable=True)


@mock.patch("hw_tools.snap.SnapCache")
@mock.patch("hw_tools.snap")
def test_snap_strategy_install_fail(mock_snap, mock_snap_cache):
hwtool = mock.MagicMock()
hwtool.value = "my-snap"
channel = "my-channel"

mock_snap_client = mock.MagicMock()
mock_snap_cache.return_value = {"my-snap": mock_snap_client}

strategy = SnapStrategy(hwtool, channel)
mock_snap.add.side_effect = snap.SnapError
with pytest.raises(snap.SnapError):
strategy.install()
mock_snap_client.start.assert_not_called()


@mock.patch("hw_tools.snap")
def test_snap_strategy_remove(mock_snap):
hwtool = mock.MagicMock()
hwtool.value = "my-snap"
strategy = SnapStrategy(hwtool, "my-channel")
strategy.remove()
mock_snap.remove.assert_called_with([strategy.snap_name])


@pytest.mark.parametrize(
"services, expected",
[
# all services active
(
{
"service_1": {
"daemon": "simple",
"daemon_scope": "system",
"enabled": True,
"active": True,
"activators": [],
},
"service_2": {
"daemon": "simple",
"daemon_scope": "system",
"enabled": True,
"active": True,
"activators": [],
},
},
True,
),
# at least one services down
(
{
"service_1": {
"daemon": "simple",
"daemon_scope": "system",
"enabled": True,
"active": False,
"activators": [],
},
"service_2": {
"daemon": "simple",
"daemon_scope": "system",
"enabled": True,
"active": True,
"activators": [],
},
},
False,
),
# all services down
(
{
"service_1": {
"daemon": "simple",
"daemon_scope": "system",
"enabled": True,
"active": False,
"activators": [],
},
"service_2": {
"daemon": "simple",
"daemon_scope": "system",
"enabled": True,
"active": False,
"activators": [],
},
},
False,
),
# snap without service
({}, True),
],
)
@mock.patch("hw_tools.snap.SnapCache")
def test_snap_strategy_check(mock_snap_cache, services, expected):
mock_snap_info = mock.MagicMock()
mock_snap_info.services = services
mock_snap_cache.return_value.__getitem__.return_value = mock_snap_info
hwtool = mock.MagicMock()
hwtool.value = "my-snap"
strategy = SnapStrategy(hwtool, "my-channel")
assert strategy.check() is expected
Loading