Add Snap Strategy #306

gabrielcocenza · 2024-09-06T20:14:36Z

DCGM and smartctl- will be installed via snap. This PR creates the SnapStrategy so we can install, remove and check as it happens with other strategies in the code base.

DCGM and smartctl- will be installed via snap. This PR creates the SnapStrategy so we can install, remove and check as it happens with other stratigies in the code base.

src/hw_tools.py

samuelallan72

Also, I'm not sure how helpful it is to add this before adding any actual snaps, because this code doesn't have any real use yet, can't be functionally tested, and we don't have any client code that uses it (ie. we don't know yet if this api will be what we want).

It's tricky with figuring out when to write things and in what order, and I think I've gone back and forth over it. For this, my opinion is that this is a new feature where we should add support for an actual snap, not just developing an api in a vaccuum. :)

gabrielcocenza · 2024-09-09T15:00:40Z

@samuelallan72

The DCGM and Smartctl snaps are not ready yet. This PR can be pending until those snaps are available. The idea here is just start having conversations about the approach and not wait until is ready for trying to figure out what to do.

src/hw_tools.py

rgildein · 2024-09-10T12:36:34Z

src/hw_tools.py

+        self._name = tool
+        self.snap_name = tool.value
+
+    def install(self, channel: str = "latest/stable") -> None:


I think that channel should be part of init function, so the install is not needed. Otherwise, I do not see any benefit of even adding this strategy instead of directly using snap lib.

I guess the channel is part of the configuration we get from the charm configuration?
Actually we need to know how to get the channel information when execute the install function. This PR need to include the changing of HWToolHelpe?

@gabrielcocenza Can you add more information about this?

I've included the channel at the init and had to change the HWToolHelper to be able to instantiate a class passing the channel

Thanks, it's good for me.

Maybe something was lost in the splitting of PRs, but this method still takes channel as an argument, and doesn't use self.channel.

don't you need def install(self, channel) but not def __init__(self, channel)? if you are following the current design? https://github.com/canonical/hardware-observer-operator/blob/main/src/hw_tools.py#L182

src/hw_tools.py

jneo8 · 2024-09-11T03:00:54Z

src/hw_tools.py

+        self._name = tool
+        self.snap_name = tool.value
+
+    def install(self, channel: str = "latest/stable") -> None:


I guess the channel is part of the configuration we get from the charm configuration?
Actually we need to know how to get the channel information when execute the install function. This PR need to include the changing of HWToolHelpe?

@gabrielcocenza Can you add more information about this?

src/service.py

aieri · 2024-09-11T22:01:21Z

src/hw_tools.py

+            snap.add(self.snap_name, channel=self.channel)
+        except snap.SnapError as err:
+            logger.error(
+                "Failed to install %s on channel: %s: %s", self.snap_name, self.channel, err


nit

Suggested change

"Failed to install %s on channel: %s: %s", self.snap_name, self.channel, err

"Failed to install %s from channel: %s: %s", self.snap_name, self.channel, err

aieri · 2024-09-11T22:01:48Z

src/hw_tools.py

+                "Failed to install %s on channel: %s: %s", self.snap_name, self.channel, err
+            )
+        else:
+            logger.info("Installed %s on channel: %s", self.snap_name, self.channel)


Suggested change

logger.info("Installed %s on channel: %s", self.snap_name, self.channel)

logger.info("Installed %s from channel: %s", self.snap_name, self.channel)

aieri · 2024-09-11T22:03:48Z

src/hw_tools.py

+
+    def enable_services(self) -> None:
+        """Enable the snap services."""
+        # breakpoint()


I imagine this is a leftover from manual tests

aieri · 2024-09-11T22:05:50Z

src/hw_tools.py

@@ -695,14 +738,15 @@ def install(self, resources: Resources, hw_available: Set[HWTool]) -> Tuple[bool
                    if path:
                        strategy.install(path)
                # APTStrategy


nit: needs updating

src/service.py

config.yaml

src/config.py

src/service.py

tests/unit/test_charm.py

Pjack · 2024-09-13T02:35:57Z

@gabrielcocenza Would you mind separating this PR into two parts? The first part is common to both the smartctl-exporter-snap and dcgm-exporter-snap, so it would be more convenient to work on them in parallel if we decouple the first and second parts. Thanks!

SnapStrategy
Integrate dcgm exporter snap

I propose to merge SnapStrategy first.

This reverts commit 1b4a250.

This reverts commit 021a62e.

gabrielcocenza · 2024-09-16T20:52:57Z

@samuelallan72 @aieri @rgildein Eric asked to split the SnapStrategy from the implementation of the dcgm exporter. I guess now there is a good idea on how it's going to be used and I'll open a PR soon on top of this one to implement the dcgm exporter considering those comments

samuelallan72 · 2024-09-17T00:25:16Z

src/hw_tools.py

+        self._name = tool
+        self.snap_name = tool.value
+
+    def install(self, channel: str = "latest/stable") -> None:


Maybe something was lost in the splitting of PRs, but this method still takes channel as an argument, and doesn't use self.channel.

samuelallan72 · 2024-09-17T00:27:48Z

src/hw_tools.py

+    def enable_services(self) -> None:
+        """Enable the snap services."""
+        self.snap_client.start(list(self.snap_client.services.keys()), enable=True)
+


I reckon this could be a private method (it's not used outside the strategy), or even better inlined because it's a single line method.

samuelallan72 · 2024-09-17T00:27:59Z

src/hw_tools.py

+    def install(self, channel: str = "latest/stable") -> None:
+        """Install the snap from a channel."""
+        try:
+            snap.add(self.snap_name, channel=channel)


Suggested change

snap.add(self.snap_name, channel=channel)

snap.add(self.snap_name, channel=self.channel)

samuelallan72 · 2024-09-17T00:28:49Z

src/hw_tools.py

+            logger.error(
+                "Failed to install %s from channel: %s: %s", self.snap_name, self.channel, err
+            )


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?

good found Samuel!

Please follow the current error handling design here.

hardware-observer-operator/src/hw_tools.py

Lines 701 to 708 in d420c63

except (

ResourceFileSizeZeroError,

OSError,

apt.PackageError,

ResourceChecksumError,

) as e:

logger.warning("Strategy %s install fail: %s", strategy, e)

fail_strategies.append(strategy.name)

Forgot to re-raise the error. Thanks

samuelallan72 · 2024-09-17T00:30:36Z

src/hw_tools.py

+        self.channel = channel
+        self.snap_client = snap.SnapCache()[tool.value]
+
+    def install(self, channel: str = "latest/stable") -> None:


Suggested change

def install(self, channel: str = "latest/stable") -> None:

def install(self) -> None:

src/hw_tools.py

jneo8 · 2024-09-18T03:27:22Z

src/hw_tools.py

+            logger.error(
+                "Failed to install %s from channel: %s: %s", self.snap_name, self.channel, err
+            )


good found Samuel!

Please follow the current error handling design here.

hardware-observer-operator/src/hw_tools.py

Lines 701 to 708 in d420c63

except (

ResourceFileSizeZeroError,

OSError,

apt.PackageError,

ResourceChecksumError,

) as e:

logger.warning("Strategy %s install fail: %s", strategy, e)

fail_strategies.append(strategy.name)

jneo8 · 2024-09-18T04:31:22Z

src/hw_tools.py

@@ -175,6 +176,44 @@ def remove(self) -> None:
        # the remove option.


+class SnapStrategy(StrategyABC):


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.

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.

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

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)

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.

- removed the enable_services and put at the install method

aieri

looks good, but the snap strategy should be aligned with the other strategies in terms of configuration being done by the client when calling install and not in the constructor (for consistency)

aieri · 2024-09-18T19:30:33Z

src/hw_tools.py

@@ -175,6 +176,44 @@ def remove(self) -> None:
        # the remove option.


+class SnapStrategy(StrategyABC):


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

gabrielcocenza · 2024-09-19T00:02:38Z

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

I agree....

@aieri @jneo8 @samuelallan72 After putting more effort on how to integrate the dcgm snap I ended up noticing that we might not need to have a SnapStrategy for exporters that are snaped.

When we have debian packages or other binaries the Strategy is installing and removing the packages while the exporter on the service module is responsible to render the systemd configs and check the health of the daemons.

When we have a snap I'm not seeing the need to have this kind of separation and it's seems to me that it's simpler just handle everything as a service and we'll be able to do pretty much everything in just one module.

Please take a look at the draft PR that I've created at #319

Unfortunately this PR is showing to be hard to have the Generic snap class without a real implementation. In any case, I think this other PR has a cleaner approach

gabrielcocenza · 2024-09-19T22:26:32Z

Closing this PR. The approach changed to not have a SnapStrategy and work directly with the exporter class itself. The new PR is at #319

Add Snap Strategy

7d9f44e

DCGM and smartctl- will be installed via snap. This PR creates the SnapStrategy so we can install, remove and check as it happens with other stratigies in the code base.

gabrielcocenza requested a review from a team as a code owner September 6, 2024 20:14

gabrielcocenza requested review from Pjack, aieri, samuelallan72, jneo8, rgildein and chanchiwai-ray September 6, 2024 20:14

samuelallan72 requested changes Sep 9, 2024

View reviewed changes

src/hw_tools.py Outdated Show resolved Hide resolved

samuelallan72 requested changes Sep 9, 2024

View reviewed changes

rgildein reviewed Sep 10, 2024

View reviewed changes

jneo8 requested changes Sep 11, 2024

View reviewed changes

Add dcgm-exporter

021a62e

gabrielcocenza changed the title ~~Add Snap Strategy~~ Add Snap Strategy and DCGM snap Sep 11, 2024

add charm config for dcgm snap channel

1b4a250

gabrielcocenza commented Sep 11, 2024

View reviewed changes

src/service.py Outdated Show resolved Hide resolved

src/service.py Outdated Show resolved Hide resolved

gabrielcocenza requested review from samuelallan72, rgildein and jneo8 September 11, 2024 21:41

aieri requested changes Sep 11, 2024

View reviewed changes

samuelallan72 reviewed Sep 12, 2024

View reviewed changes

config.yaml Outdated Show resolved Hide resolved

src/config.py Outdated Show resolved Hide resolved

src/service.py Outdated Show resolved Hide resolved

tests/unit/test_charm.py Outdated Show resolved Hide resolved

gabrielcocenza added 3 commits September 16, 2024 15:55

Revert "add charm config for dcgm snap channel"

b06e56e

This reverts commit 1b4a250.

Revert "Add dcgm-exporter"

7ce2ce1

This reverts commit 021a62e.

- remove dcgm exporter to just have the logic for snap strategy.

855a963

gabrielcocenza changed the title ~~Add Snap Strategy and DCGM snap~~ Add Snap Strategy Sep 16, 2024

gabrielcocenza requested review from samuelallan72 and aieri September 16, 2024 20:49

Merge branch 'main' into snap-strategy

a58b70c

samuelallan72 requested changes Sep 17, 2024

View reviewed changes

chanchiwai-ray reviewed Sep 17, 2024

View reviewed changes

src/hw_tools.py Show resolved Hide resolved

jneo8 requested changes Sep 18, 2024

View reviewed changes

gabrielcocenza added 3 commits September 18, 2024 11:28

Merge branch 'main' into snap-strategy

5fe7a2c

- fix install method to use the channel passed on init

a607856

- removed the enable_services and put at the install method

fix lint

88f7f09

gabrielcocenza requested review from samuelallan72, chanchiwai-ray and jneo8 September 18, 2024 19:08

aieri requested changes Sep 18, 2024

View reviewed changes

gabrielcocenza closed this Sep 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Snap Strategy #306

Add Snap Strategy #306

gabrielcocenza commented Sep 6, 2024 •

edited

Loading

samuelallan72 left a comment

gabrielcocenza commented Sep 9, 2024

rgildein Sep 10, 2024

jneo8 Sep 11, 2024

gabrielcocenza Sep 11, 2024

jneo8 Sep 13, 2024

samuelallan72 Sep 17, 2024

chanchiwai-ray Sep 17, 2024

jneo8 Sep 11, 2024

aieri Sep 11, 2024

aieri Sep 11, 2024

aieri Sep 11, 2024

aieri Sep 11, 2024

Pjack commented Sep 13, 2024

gabrielcocenza commented Sep 16, 2024 •

edited

Loading

samuelallan72 Sep 17, 2024

samuelallan72 Sep 17, 2024

samuelallan72 Sep 17, 2024

samuelallan72 Sep 17, 2024

jneo8 Sep 18, 2024

gabrielcocenza Sep 18, 2024

samuelallan72 Sep 17, 2024

jneo8 Sep 18, 2024

jneo8 Sep 18, 2024 •

edited

Loading

gabrielcocenza Sep 18, 2024

aieri Sep 18, 2024

jneo8 Sep 19, 2024 •

edited

Loading

jneo8 Sep 19, 2024

aieri left a comment

aieri Sep 18, 2024

gabrielcocenza commented Sep 19, 2024

gabrielcocenza commented Sep 19, 2024

	"Failed to install %s on channel: %s: %s", self.snap_name, self.channel, err
	"Failed to install %s from channel: %s: %s", self.snap_name, self.channel, err

	logger.info("Installed %s on channel: %s", self.snap_name, self.channel)
	logger.info("Installed %s from channel: %s", self.snap_name, self.channel)

	snap.add(self.snap_name, channel=channel)
	snap.add(self.snap_name, channel=self.channel)

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

	def install(self, channel: str = "latest/stable") -> None:
	def install(self) -> None:

		@@ -175,6 +176,44 @@ def remove(self) -> None:
		# the remove option.


		class SnapStrategy(StrategyABC):

Add Snap Strategy #306

Add Snap Strategy #306

Conversation

gabrielcocenza commented Sep 6, 2024 • edited Loading

samuelallan72 left a comment

Choose a reason for hiding this comment

gabrielcocenza commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pjack commented Sep 13, 2024

gabrielcocenza commented Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneo8 Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneo8 Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aieri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielcocenza commented Sep 19, 2024

gabrielcocenza commented Sep 19, 2024

gabrielcocenza commented Sep 6, 2024 •

edited

Loading

gabrielcocenza commented Sep 16, 2024 •

edited

Loading

jneo8 Sep 18, 2024 •

edited

Loading

jneo8 Sep 19, 2024 •

edited

Loading