From c9f566beb67888a46b5a3bd6959bdb8de6f2205b Mon Sep 17 00:00:00 2001 From: "soleng-terraform[bot]" <168111096+soleng-terraform[bot]@users.noreply.github.com> Date: Mon, 23 Sep 2024 16:32:36 +0800 Subject: [PATCH 1/2] Update centrally managed files (#323) * update .gitignore * update .github/workflows/release.yaml * update .github/workflows/check.yaml --------- Co-authored-by: soleng-terraform[bot] <168111096+soleng-terraform[bot]@users.noreply.github.com> --- .github/workflows/check.yaml | 26 ++++++++++++---- .github/workflows/release.yaml | 2 +- .gitignore | 55 ++++++++++++++++------------------ 3 files changed, 47 insertions(+), 36 deletions(-) diff --git a/.github/workflows/check.yaml b/.github/workflows/check.yaml index 97d8b0d0..52c1354c 100644 --- a/.github/workflows/check.yaml +++ b/.github/workflows/check.yaml @@ -26,7 +26,7 @@ jobs: fail-fast: false matrix: python-version: ["3.8", "3.10"] - runs-on: ubuntu-latest + runs-on: ubuntu-22.04 steps: - uses: actions/checkout@v4 with: @@ -53,7 +53,7 @@ jobs: fail-fast: false matrix: python-version: ["3.8", "3.10"] - runs-on: ubuntu-latest + runs-on: ubuntu-22.04 steps: - uses: actions/checkout@v4 with: @@ -83,7 +83,7 @@ jobs: fail-fast: false matrix: runs-on: [[ubuntu-22.04], [Ubuntu_ARM64_4C_16G_01]] - test-command: ['tox -e func -- --series focal --keep-models', 'tox -e func -- --series jammy --keep-models'] + test-command: ['tox -e func -- -v --series focal --keep-models', 'tox -e func -- -v --series jammy --keep-models'] juju-channel: ["3.4/stable"] steps: @@ -91,11 +91,11 @@ jobs: with: submodules: true - # arm64 runners don't have make or gcc installed by default + # arm64 runners don't have gcc installed by default - name: Install dependencies run: | sudo apt update - sudo apt install -y make gcc + sudo apt install -y gcc - name: Setup Python uses: actions/setup-python@v5 @@ -116,8 +116,22 @@ jobs: echo "TEST_MODEL_CONSTRAINTS=arch=arm64" >> "$GITHUB_ENV" fi + - name: Build the charm + run: charmcraft -v pack + - name: Run tests - run: ${{ matrix.test-command }} + run: | + # These variables are for a consistent method to find the charm file(s) across all projects. + # It is designed to work both with charms that output one file per base, + # and charms that output a single file to run on all bases. + # Not all charms will use them, and for some charms the variables will resolve to the same file. + export CHARM_PATH_NOBLE="$(pwd)/$(ls | grep '.*24.04.*\.charm$')" + echo "$CHARM_PATH_NOBLE" + export CHARM_PATH_JAMMY="$(pwd)/$(ls | grep '.*22.04.*\.charm$')" + echo "$CHARM_PATH_JAMMY" + export CHARM_PATH_FOCAL="$(pwd)/$(ls | grep '.*20.04.*\.charm$')" + echo "$CHARM_PATH_FOCAL" + ${{ matrix.test-command }} env: TEST_JUJU3: "1" # https://github.com/openstack-charmers/zaza/pull/653 TEST_JUJU_CHANNEL: ${{ matrix.juju-channel }} diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 539925c2..31c428ae 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -22,7 +22,7 @@ jobs: strategy: fail-fast: false matrix: - runs-on: [[ubuntu-22.04]] + runs-on: [[ubuntu-22.04], [Ubuntu_ARM64_4C_16G_01]] steps: - name: Checkout uses: actions/checkout@v4 diff --git a/.gitignore b/.gitignore index 9d466f15..042b19d5 100644 --- a/.gitignore +++ b/.gitignore @@ -1,48 +1,45 @@ -# This is a template `.gitignore` file for ops charms -# This file is managed by bootstack-charms-spec and should not be modified -# within individual charm repos. https://launchpad.net/bootstack-charms-spec +# This file is centrally managed as a template file in https://github.com/canonical/solutions-engineering-automation +# To update the file: +# - Edit it in the canonical/solutions-engineering-automation repository. +# - Open a PR with the changes. +# - When the PR merges, the soleng-terraform bot will open a PR to the target repositories with the changes. -# Juju files -.unit-state.db - -# Byte-compiled / optimized / DLL files +# Python Byte-compiled / optimized / DLL files __pycache__/ *.py[cod] *$py.class -# Tests files and dir +# Test files and directories .pytest_cache/ .coverage .tox -.venv reports/ **/report/ htmlcov/ .mypy_cache -# Log files -*.log - -# IDEs -.idea/ -.vscode/ - -# vi -.*.swp - -# version data -repo-info -version +# python virtual environments (for local dev) +.venv +venv +env -# Python builds +# Build artefacts +output/ +.build/ +build/ +*.charm +*.snap +# python build artefacts deb_dist/ dist/ *.egg-info/ -# Snaps -*.snap +# Log files +*.log -# Builds -.build/ -build/ -*.charm +# general backup files +*~ +*.bak + +# Note: for editor-specific files, please don't add them here, as they are specific to your environment, not the project. +# Instead, consider using a global gitignore on your workstation. From 0f62edce1ea3b20fdcf9a3669fc59c839df78b6b Mon Sep 17 00:00:00 2001 From: Gabriel Cocenza Date: Tue, 24 Sep 2024 09:41:38 -0300 Subject: [PATCH 2/2] Add SnapExporter class and DCGM (#319) Add SnapExporter class and DCGM - Added the SnapExporter class that can be used for handle snap packages - Added the DCGMExporter that installs the DCGM exporter if detected NVIDIA GPUs - BaseExporter is a simple abstract class that has the required methods for an exporter - RendarableExporter is the legacy BaseExporter that contains the logic for exporters that are not snaped - DCGMExporterStrategy is not used on HWToolHelper. Avoid injecting the charm config and this class will eventually be removed --- config.yaml | 6 ++ src/charm.py | 8 +- src/hw_tools.py | 59 ++++++++++- src/service.py | 195 ++++++++++++++++++++++++++++++------ tests/unit/test_charm.py | 88 ++++++++++------ tests/unit/test_hw_tools.py | 140 ++++++++++++++++++++++++-- tests/unit/test_service.py | 146 +++++++++++++++++++++++---- 7 files changed, 541 insertions(+), 101 deletions(-) diff --git a/config.yaml b/config.yaml index fa15a288..60924863 100644 --- a/config.yaml +++ b/config.yaml @@ -15,6 +15,12 @@ options: description: | Start the prometheus smartctl exporter at "smartctl-exporter-port". By default, it will start at port 10201. + dcgm-snap-channel: + type: string + default: "latest/stable" + description: | + Channel to install the DCGM snap if the hardware has NVIDIA GPU. By default, it will install + from latest/stable exporter-log-level: type: string default: "INFO" diff --git a/src/charm.py b/src/charm.py index 7f5a4277..d720e594 100755 --- a/src/charm.py +++ b/src/charm.py @@ -13,7 +13,7 @@ from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus from hw_tools import HWTool, HWToolHelper, detect_available_tools -from service import BaseExporter, ExporterError, HardwareExporter, SmartCtlExporter +from service import BaseExporter, DCGMExporter, ExporterError, HardwareExporter, SmartCtlExporter logger = logging.getLogger(__name__) @@ -37,6 +37,7 @@ def __init__(self, *args: Any) -> None: metrics_endpoints=[ {"path": "/metrics", "port": int(self.model.config["hardware-exporter-port"])}, {"path": "/metrics", "port": int(self.model.config["smartctl-exporter-port"])}, + {"path": "/metrics", "port": 9400}, ], # Setting scrape_timeout as collect_timeout in the `duration` format specified in # https://prometheus.io/docs/prometheus/latest/configuration/configuration/#duration @@ -82,6 +83,9 @@ def exporters(self) -> List[BaseExporter]: if stored_tools & SmartCtlExporter.hw_tools(): exporters.append(SmartCtlExporter(self.charm_dir, self.model.config)) + if stored_tools & DCGMExporter.hw_tools(): + exporters.append(DCGMExporter(self.model.config)) + return exporters def get_stored_tools(self) -> Set[HWTool]: @@ -226,7 +230,7 @@ def _on_config_changed(self, event: EventBase) -> None: self.model.unit.status = BlockedStatus(message) return for exporter in self.exporters: - success = exporter.render_config() + success = exporter.configure() if success: exporter.restart() else: diff --git a/src/hw_tools.py b/src/hw_tools.py index 4febb159..76e883a8 100644 --- a/src/hw_tools.py +++ b/src/hw_tools.py @@ -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 @@ -187,6 +188,57 @@ def remove(self) -> None: """Remove details.""" +class SnapStrategy(StrategyABC): + """Snap strategy class.""" + + channel: str + + @property + def snap(self) -> str: + """Snap name.""" + return self._name.value + + def install(self) -> None: + """Install the snap from a channel.""" + try: + snap.add(self.snap, channel=self.channel) + logger.info("Installed %s from channel: %s", self.snap, 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, self.channel, err) + raise err + + def remove(self) -> None: + """Remove the snap.""" + try: + snap.remove([self.snap]) + + # 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 remove %s: %s", self.snap, err) + raise err + + def check(self) -> bool: + """Check if all services are active.""" + return all( + service.get("active", False) + for service in snap.SnapCache()[self.snap].services.values() + ) + + +class DCGMExporterStrategy(SnapStrategy): + """DCGM strategy class.""" + + _name = HWTool.DCGM + + def __init__(self, channel: str) -> None: + """Init.""" + self.channel = channel + + class StorCLIStrategy(TPRStrategyABC): """Strategy to install storcli.""" @@ -689,13 +741,12 @@ 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)): strategy.install() # pylint: disable=E1120 logger.info("Strategy %s install success", strategy) except ( @@ -717,7 +768,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) diff --git a/src/service.py b/src/service.py index a8555821..154ce865 100644 --- a/src/service.py +++ b/src/service.py @@ -8,6 +8,7 @@ from typing import Any, Dict, Optional, Set, Tuple from charms.operator_libs_linux.v1 import systemd +from charms.operator_libs_linux.v2 import snap from jinja2 import Environment, FileSystemLoader from ops.model import ConfigData from redfish import redfish_client @@ -21,7 +22,7 @@ HWTool, ) from hardware import get_bmc_address -from hw_tools import SmartCtlExporterStrategy +from hw_tools import DCGMExporterStrategy, SmartCtlExporterStrategy, SnapStrategy logger = getLogger(__name__) @@ -36,6 +37,56 @@ class ExporterError(Exception): class BaseExporter(ABC): """A class representing the exporter and the metric endpoints.""" + config: ConfigData + exporter_name: str + port: int + log_level: str + + @staticmethod + @abstractmethod + def hw_tools() -> Set[HWTool]: + """Return hardware tools to watch.""" + + @abstractmethod + def install(self) -> bool: + """Install the exporter.""" + + @abstractmethod + def uninstall(self) -> bool: + """Uninstall the exporter.""" + + @abstractmethod + def check_health(self) -> bool: + """Check if the exporter daemon is healthy or not.""" + + @abstractmethod + def restart(self) -> None: + """Restart exporter service with retry.""" + + @abstractmethod + def enable_and_start(self) -> None: + """Enable and start the exporter services.""" + + @abstractmethod + def disable_and_stop(self) -> None: + """Disable and stop the exporter services.""" + + @abstractmethod + def configure(self) -> bool: + """Set exporter config.""" + + def validate_exporter_configs(self) -> Tuple[bool, str]: + """Validate the static and runtime config options for the exporter.""" + if not 1 <= self.port <= 65535: + logger.error("Invalid exporter port: port must be in [1, 65535].") + return False, "Invalid config: exporter's port" + + return True, "Exporter config is valid." + + +class RenderableExporter(BaseExporter): + """A class representing an exporter that needs to render its configuration.""" + # pylint: disable=too-many-instance-attributes exporter_config_path: Optional[Path] = None @@ -54,26 +105,6 @@ def __init__(self, charm_dir: Path, config: ConfigData, settings: ExporterSettin self.log_level = str(config["exporter-log-level"]) - @staticmethod - @abstractmethod - def hw_tools() -> Set[HWTool]: - """Return hardware tools to watch.""" - - def validate_exporter_configs(self) -> Tuple[bool, str]: - """Validate the static and runtime config options for the exporter.""" - if not 1 <= self.port <= 65535: - logger.error("Invalid exporter port: port must be in [1, 65535].") - return False, "Invalid config: exporter's port" - - allowed_log_level_choices = {"DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"} - if self.log_level.upper() not in allowed_log_level_choices: - logger.error( - "Invalid exporter-log-level: log-level must be in %s (case-insensitive).", - allowed_log_level_choices, - ) - return False, "Invalid config: 'exporter-log-level'" - return True, "Exporter config is valid." - def resources_exist(self) -> bool: """Return true if required resources exist. @@ -135,8 +166,8 @@ def _render_service(self, params: Dict[str, str]) -> bool: content = self.service_template.render(**params) return write_to_file(self.exporter_service_path, content) - def render_config(self) -> bool: - """Render exporter config file.""" + def configure(self) -> bool: + """Configure the exporter by rendering templates.""" if self.exporter_config_path is not None: content = self._render_config_content() return write_to_file(self.exporter_config_path, content, mode=0o600) @@ -173,8 +204,8 @@ def install(self) -> bool: return False # Render config - render_config_success = self.render_config() - if not render_config_success: + configure_success = self.configure() + if not configure_success: logger.error("Failed to render config files for %s.", self.exporter_name) return False @@ -224,6 +255,21 @@ def restart(self) -> None: logger.error("Exporter %s crashed unexpectedly: %s", self.exporter_name, err) raise ExporterError() from err + def validate_exporter_configs(self) -> Tuple[bool, str]: + """Validate the static and runtime config options for the exporter.""" + valid, msg = super().validate_exporter_configs() + if not valid: + return valid, msg + + allowed_log_level_choices = {"DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"} + if self.log_level.upper() not in allowed_log_level_choices: + logger.error( + "Invalid exporter-log-level: log-level must be in %s (case-insensitive).", + allowed_log_level_choices, + ) + return False, "Invalid config: 'exporter-log-level'" + return True, "Exporter config is valid." + def write_to_file(path: Path, content: str, mode: Optional[int] = None) -> bool: """Write to file with provided content.""" @@ -263,7 +309,7 @@ def remove_file(path: Path) -> bool: return success -class SmartCtlExporter(BaseExporter): +class SmartCtlExporter(RenderableExporter): """A class representing the smartctl exporter and the metric endpoints.""" required_config: bool = False @@ -287,8 +333,8 @@ def render_service(self) -> bool: ) return service_rendered - def render_config(self) -> bool: - """Override base render_config to render the service file. + def configure(self) -> bool: + """Override base configure to render the service file. This is because smartctl_exporter doesn't support providing config file. The config options need to be provided as flags while exectuting @@ -324,7 +370,98 @@ def remove_resources(self) -> bool: return True -class HardwareExporter(BaseExporter): +class SnapExporter(BaseExporter): + """A class representing a snap exporter.""" + + exporter_name: str + channel: str + port: int + strategy: SnapStrategy + + def __init__(self, config: ConfigData): + """Init.""" + self.config = config + self.snap_client = snap.SnapCache()[self.strategy.snap] + + @staticmethod + def hw_tools() -> Set[HWTool]: + """Return hardware tools to watch.""" + return set() + + def install(self) -> bool: + """Install the snap from a channel. + + Returns true if the install is successful, false otherwise. + """ + try: + self.strategy.install() + # dcgm-exporter is disabled by default + self.enable_and_start() + return self.snap_client.present is True + except Exception: # pylint: disable=broad-except + return False + + def uninstall(self) -> bool: + """Uninstall the snap. + + Returns true if the uninstall is successful, false otherwise. + """ + try: + self.strategy.remove() + + # 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 remove %s: %s", self.strategy.snap, err) + return False + + return self.snap_client.present is False + + def enable_and_start(self) -> None: + """Enable and start the exporter services.""" + self.snap_client.start(list(self.snap_client.services.keys()), enable=True) + + def disable_and_stop(self) -> None: + """Disable and stop the services.""" + self.snap_client.stop(list(self.snap_client.services.keys()), disable=True) + + def restart(self) -> None: + """Restart the exporter daemon.""" + self.snap_client.restart(reload=True) + + def check_health(self) -> bool: + """Check if all services are active. + + Returns true if the service is healthy, false otherwise. + """ + return self.strategy.check() + + def configure(self) -> bool: + """Set the necessary exporter configurations or change snap channel. + + Returns true if the configure is successful, false otherwise. + """ + return self.install() + + +class DCGMExporter(SnapExporter): + """A class representing the DCGM exporter and the metric endpoints.""" + + exporter_name: str = "dcgm" + port: int = 9400 + + def __init__(self, config: ConfigData): + """Init.""" + self.strategy = DCGMExporterStrategy(str(config["dcgm-snap-channel"])) + super().__init__(config) + + @staticmethod + def hw_tools() -> Set[HWTool]: + """Return hardware tools to watch.""" + return {HWTool.DCGM} + + +class HardwareExporter(RenderableExporter): """A class representing the hardware exporter and the metric endpoints.""" required_config: bool = True diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 732b0327..68bc3890 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -15,7 +15,13 @@ import charm from charm import ExporterError, HardwareObserverCharm from config import HWTool -from service import HardwareExporter +from service import ( + HARDWARE_EXPORTER_SETTINGS, + SMARTCTL_EXPORTER_SETTINGS, + DCGMExporter, + HardwareExporter, + SmartCtlExporter, +) class TestCharm(unittest.TestCase): @@ -54,41 +60,57 @@ def test_harness(self) -> None: @parameterized.expand( [ + ( + "No exporters enabled", + set(), + set(), + ), + ( + "Enable one exporter", + {HWTool.IPMI_SEL}, + {"hardware-exporter"}, + ), ( "Enable two exporters", {HWTool.IPMI_SEL, HWTool.SMARTCTL}, {"hardware-exporter", "smartctl-exporter"}, - ) + ), + ( + "Enable all exporters", + {HWTool.IPMI_SEL, HWTool.SMARTCTL, HWTool.DCGM}, + {"hardware-exporter", "smartctl-exporter", "dcgm"}, + ), ] ) - @mock.patch("charm.SmartCtlExporter.__init__", return_value=None) - @mock.patch("charm.HardwareExporter.__init__", return_value=None) - def test_exporters(self, _, stored_tools, expect, mock_hw_exporter, mock_smart_exporter): + @mock.patch("charm.DCGMExporter") + @mock.patch("charm.SmartCtlExporter") + @mock.patch("charm.HardwareExporter") + def test_exporters( + self, _, stored_tools, expect, mock_hw_exporter, mock_smart_exporter, mock_dcgm_exporter + ): self.harness.begin() self.harness.charm.get_stored_tools = mock.MagicMock() self.harness.charm.get_stored_tools.return_value = stored_tools - self.harness.charm._stored.stored_tools = {tool.value for tool in stored_tools} + + hw_exporter = mock.MagicMock() + hw_exporter.exporter_name = HARDWARE_EXPORTER_SETTINGS.name + mock_hw_exporter.hw_tools.return_value = HardwareExporter.hw_tools() + mock_hw_exporter.return_value = hw_exporter + + smart_exporter = mock.MagicMock() + smart_exporter.exporter_name = SMARTCTL_EXPORTER_SETTINGS.name + mock_smart_exporter.hw_tools.return_value = SmartCtlExporter.hw_tools() + mock_smart_exporter.return_value = smart_exporter + + dcgm_exporter = mock.MagicMock() + dcgm_exporter.exporter_name = DCGMExporter.exporter_name + mock_dcgm_exporter.hw_tools.return_value = DCGMExporter.hw_tools() + mock_dcgm_exporter.return_value = dcgm_exporter exporters = self.harness.charm.exporters self.harness.charm.get_stored_tools.assert_called() - if "hardware-exporter" in expect: - self.assertTrue( - any([isinstance(exporter, HardwareExporter) for exporter in exporters]) - ) - mock_hw_exporter.assert_called_with( - self.harness.charm.charm_dir, - self.harness.charm.model.config, - self.harness.charm._stored.stored_tools, - ) - if "smartctl-exporter" in expect: - self.assertTrue( - any([isinstance(exporter, HardwareExporter) for exporter in exporters]) - ) - mock_smart_exporter.assert_called_with( - self.harness.charm.charm_dir, - self.harness.charm.model.config, - ) + self.assertEqual({exporter.exporter_name for exporter in exporters}, expect) @parameterized.expand( [ @@ -503,7 +525,7 @@ def test_detect_hardware_action( [(True, ""), (True, "")], ), ( - "Exporter render_config failed", + "Exporter configure failed", True, True, (True, ""), @@ -520,14 +542,14 @@ def test_config_changed( cos_agent_related, validate_configs_return, mock_exporters, - mock_exporters_render_config_returns, + mock_exporters_configure_returns, mock_logger, ): - for mock_exporter, render_config_return in zip( + for mock_exporter, configure_return in zip( mock_exporters, - mock_exporters_render_config_returns, + mock_exporters_configure_returns, ): - mock_exporter.render_config.return_value = render_config_return + mock_exporter.configure.return_value = configure_return with mock.patch( "charm.HardwareObserverCharm.exporters", @@ -556,14 +578,14 @@ def test_config_changed( return if not validate_configs_return[0]: self.assertEqual(self.harness.charm.unit.status, BlockedStatus("invalid msg")) - self.harness.charm.exporters[0].render_config.assert_not_called() + self.harness.charm.exporters[0].configure.assert_not_called() return - if not all(mock_exporters_render_config_returns): - for mock_exporter, render_config_return in zip( + if not all(mock_exporters_configure_returns): + for mock_exporter, configure_return in zip( mock_exporters, - mock_exporters_render_config_returns, + mock_exporters_configure_returns, ): - if render_config_return: + if configure_return: mock_exporter.restart.assert_called() else: message = ( diff --git a/tests/unit/test_hw_tools.py b/tests/unit/test_hw_tools.py index 5784f687..4e51fba8 100644 --- a/tests/unit/test_hw_tools.py +++ b/tests/unit/test_hw_tools.py @@ -36,6 +36,7 @@ SAS3IRCUStrategy, SmartCtlExporterStrategy, SmartCtlStrategy, + SnapStrategy, SSACLIStrategy, StorCLIStrategy, StrategyABC, @@ -128,6 +129,7 @@ def test_make_executable_error_handling(self, mock_os): class TestHWToolHelper(unittest.TestCase): def setUp(self): self.harness = ops.testing.Harness(HardwareObserverCharm) + self.harness.begin() self.addCleanup(self.harness.cleanup) self.hw_tool_helper = HWToolHelper() @@ -181,7 +183,6 @@ def test_03_fetch_tools_error_handling(self): def test_04_install(self, mock_strategies): """Check strategy is been called.""" 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 @@ -207,7 +208,6 @@ def test_04_install(self, mock_strategies): new_callable=mock.PropertyMock, ) def test_05_remove(self, mock_strategies): - self.harness.begin() mock_resources = self.harness.charm.model.resources mock_strategies.return_value[0].name = HWTool.STORCLI mock_hw_available = {HWTool.STORCLI} @@ -226,7 +226,6 @@ def test_05_remove(self, mock_strategies): def test_06_install_not_available(self, mock_strategies): """Check strategy is been called.""" 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 @@ -247,7 +246,6 @@ def test_06_install_not_available(self, mock_strategies): def test_07_install_no_resource(self, mock_strategies): """Check tpr strategy is not been called if resource is not defined.""" 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 @@ -269,7 +267,6 @@ def test_07_install_no_resource(self, mock_strategies): new_callable=mock.PropertyMock, ) def test_08_remove_not_available(self, mock_strategies): - self.harness.begin() mock_resources = self.harness.charm.model.resources mock_strategies.return_value[0].name = HWTool.STORCLI mock_hw_available = set() @@ -285,7 +282,6 @@ def test_08_remove_not_available(self, mock_strategies): new_callable=mock.PropertyMock, ) def test_09_install_required_resource_not_uploaded(self, _): - self.harness.begin() mock_resources = self.harness.charm.model.resources mock_hw_available = [HWTool.STORCLI, HWTool.PERCCLI] ok, msg = self.hw_tool_helper.install(mock_resources, mock_hw_available) @@ -306,7 +302,6 @@ def test_09_install_required_resource_not_uploaded(self, _): def test_10_install_strategy_errors(self, mock_strategies): """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_SENSOR @@ -335,7 +330,6 @@ def test_10_install_strategy_errors(self, mock_strategies): @mock.patch("hw_tools.file_is_empty", return_value=True) def test_11_check_missing_resources_zero_size_resources(self, file_is_empty): - self.harness.begin() ok, msg = self.hw_tool_helper.check_missing_resources( hw_available={HWTool.STORCLI}, fetch_tools={HWTool.STORCLI: "fake-path"}, @@ -351,7 +345,6 @@ def test_11_check_missing_resources_zero_size_resources(self, file_is_empty): new_callable=mock.PropertyMock, ) def test_12_check_installed_okay(self, mock_strategies): - self.harness.begin() mock_strategies.return_value[0].name = HWTool.STORCLI mock_hw_available = [HWTool.STORCLI] self.hw_tool_helper.check_installed(mock_hw_available) @@ -366,7 +359,6 @@ def test_12_check_installed_okay(self, mock_strategies): new_callable=mock.PropertyMock, ) def test_13_check_installed_okay(self, mock_strategies): - self.harness.begin() mock_strategies.return_value[0].name = HWTool.SSACLI mock_hw_available = [HWTool.STORCLI] success, msg = self.hw_tool_helper.check_installed(mock_hw_available) @@ -376,7 +368,6 @@ def test_13_check_installed_okay(self, mock_strategies): @mock.patch("hw_tools.os") @mock.patch("hw_tools.Path") def test_14_check_installed_not_okay(self, mock_os, mock_path): - self.harness.begin() mock_hw_available = [ HWTool.STORCLI, HWTool.PERCCLI, @@ -1147,3 +1138,130 @@ 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]) + + +@pytest.fixture +def snap_exporter(): + my_hw_tool = mock.MagicMock() + my_hw_tool.value = "my-snap" + + class MySnapStrategy(SnapStrategy): + channel = "my-channel/stable" + _name = my_hw_tool + + strategy = MySnapStrategy() + yield strategy + + +@pytest.fixture +def mock_snap_lib(): + with mock.patch("hw_tools.snap") as mock_snap: + yield mock_snap + mock_snap.reset_mock() + + +def test_snap_strategy_name(snap_exporter): + assert snap_exporter.snap == "my-snap" + + +def test_snap_strategy_channel(snap_exporter): + assert snap_exporter.channel == "my-channel/stable" + + +def test_snap_strategy_install_success(snap_exporter, mock_snap_lib): + snap_exporter.install() + mock_snap_lib.add.assert_called_once_with(snap_exporter.snap, channel=snap_exporter.channel) + + +def test_snap_strategy_install_fail(snap_exporter, mock_snap_lib): + mock_snap_lib.add.side_effect = ValueError + + with pytest.raises(ValueError): + snap_exporter.install() + + +def test_snap_strategy_remove_success(snap_exporter, mock_snap_lib): + snap_exporter.remove() + mock_snap_lib.remove.assert_called_once_with([snap_exporter.snap]) + + +def test_snap_strategy_remove_fail(snap_exporter, mock_snap_lib): + mock_snap_lib.remove.side_effect = ValueError + + with pytest.raises(ValueError): + snap_exporter.remove() + + +@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), + ], +) +def test_snap_strategy_check(snap_exporter, mock_snap_lib, services, expected): + mock_snap_client = mock.MagicMock() + mock_snap_client.services = services + mock_snap_lib.SnapCache.return_value = {"my-snap": mock_snap_client} + + assert snap_exporter.check() is expected diff --git a/tests/unit/test_service.py b/tests/unit/test_service.py index 08ba08d1..6efd4577 100644 --- a/tests/unit/test_service.py +++ b/tests/unit/test_service.py @@ -6,6 +6,7 @@ import unittest from unittest import mock +import pytest import yaml from parameterized import parameterized from redfish.rest.v1 import InvalidCredentialsError @@ -14,7 +15,7 @@ from config import HARDWARE_EXPORTER_SETTINGS, HWTool -class TestBaseExporter(unittest.TestCase): +class TestRenderableExporter(unittest.TestCase): """Test Hardware Exporter methods.""" def setUp(self) -> None: @@ -36,9 +37,9 @@ def setUp(self) -> None: "redfish-password": "", } self.mock_stored_hw_available = {"storcli", "ssacli"} - service.BaseExporter.__abstractmethods__ = set() + service.RenderableExporter.__abstractmethods__ = set() - self.exporter = service.BaseExporter( + self.exporter = service.RenderableExporter( search_path, self.mock_config, HARDWARE_EXPORTER_SETTINGS ) @@ -48,13 +49,13 @@ def setUp(self) -> None: { "verify_render_files_exist": True, "install_resources": True, - "render_config": True, + "configure": True, "render_service": True, }, { "verify_render_files_exist": True, "install_resources": True, - "render_config": True, + "configure": True, "render_service": True, }, True, @@ -64,13 +65,13 @@ def setUp(self) -> None: { "verify_render_files_exist": True, "install_resources": False, - "render_config": True, + "configure": True, "render_service": True, }, { "verify_render_files_exist": False, "install_resources": True, - "render_config": False, + "configure": False, "render_service": False, }, False, @@ -80,13 +81,13 @@ def setUp(self) -> None: { "verify_render_files_exist": True, "install_resources": True, - "render_config": False, + "configure": False, "render_service": True, }, { "verify_render_files_exist": False, "install_resources": True, - "render_config": True, + "configure": True, "render_service": False, }, False, @@ -96,13 +97,13 @@ def setUp(self) -> None: { "verify_render_files_exist": True, "install_resources": True, - "render_config": True, + "configure": True, "render_service": False, }, { "verify_render_files_exist": False, "install_resources": True, - "render_config": True, + "configure": True, "render_service": True, }, False, @@ -113,14 +114,14 @@ def setUp(self) -> None: "verify_render_files_exist": False, "install_resources": True, "resources_exist": True, - "render_config": True, + "configure": True, "render_service": True, }, { "verify_render_files_exist": True, "verify_render_files_exist": True, "install_resources": True, - "render_config": True, + "configure": True, "render_service": True, }, False, @@ -156,8 +157,8 @@ def test_install_failed_resources_not_exist(self): self.exporter.install_resources.return_value = True self.exporter.resources_exist = mock.MagicMock() self.exporter.resources_exist.return_value = False - self.exporter.render_config = mock.MagicMock() - self.exporter.render_config.return_value = True + self.exporter.configure = mock.MagicMock() + self.exporter.configure.return_value = True self.exporter.render_service = mock.MagicMock() self.exporter.render_service.return_value = True @@ -166,7 +167,7 @@ def test_install_failed_resources_not_exist(self): self.exporter.install_resources.assert_called() self.exporter.resources_exist.assert_called() - self.exporter.render_config.assert_not_called() + self.exporter.configure.assert_not_called() self.exporter.render_service.assert_not_called() self.mock_systemd.daemon_reload.assert_not_called() @@ -321,23 +322,23 @@ def test__render_service(self, mock_write_to_file): mock_write_to_file.assert_called_with("some-config-path", "some-content") @mock.patch("service.write_to_file") - def test_render_config_okay(self, mock_write_to_file): + def test_set_config_okay(self, mock_write_to_file): self.exporter.exporter_config_path = "some-path" self.exporter._render_config_content = mock.MagicMock() self.exporter._render_config_content.return_value = "some-config-content" mock_write_to_file.return_value = "some-result" - result = self.exporter.render_config() + result = self.exporter.configure() mock_write_to_file.assert_called_with("some-path", "some-config-content", mode=0o600) self.assertEqual("some-result", result) @mock.patch("service.write_to_file") - def test_render_config_skip(self, mock_write_to_file): + def test_set_config_skip(self, mock_write_to_file): self.exporter.exporter_config_path = None mock_write_to_file.return_value = "some-result" - result = self.exporter.render_config() + result = self.exporter.configure() mock_write_to_file.assert_not_called() self.assertEqual(True, result) @@ -751,12 +752,12 @@ def test_render_service(self): (False,), ] ) - def test_render_config(self, service_render_success): + def test_set_config(self, service_render_success): """Test render config.""" self.exporter.render_service = mock.MagicMock() self.exporter.render_service.return_value = service_render_success - result = self.exporter.render_config() + result = self.exporter.configure() self.assertEqual(result, service_render_success) def test_hw_tools(self): @@ -879,5 +880,106 @@ def test_write_to_file_not_a_directory_error(self, mock_open): self.assertFalse(result) +@pytest.fixture +def snap_exporter(): + my_strategy = mock.MagicMock(spec=service.SnapStrategy) + + class MySnapExporter(service.SnapExporter): + exporter_name = "my-exporter" + channel = "my-channel" + strategy = my_strategy + + mock_config = { + "dcgm-snap-channel": "latest/stable", + } + + with mock.patch("service.snap.SnapCache"): + exporter = MySnapExporter(mock_config) + + exporter.snap_client.services = {"service1": {}, "service2": {}} + + yield exporter + + my_strategy.reset_mock() + + +def test_snap_exporter_hw_tools(snap_exporter): + + assert snap_exporter.hw_tools() == set() + + +def test_snap_exporter_install(snap_exporter): + snap_exporter.strategy.install.return_value = True + snap_exporter.snap_client.present = True + + assert snap_exporter.install() is True + snap_exporter.strategy.install.assert_called_once() + + +def test_snap_exporter_install_fail(snap_exporter): + snap_exporter.strategy.install.side_effect = ValueError + + assert snap_exporter.install() is False + + +def test_snap_exporter_uninstall(snap_exporter): + snap_exporter.snap_client.present = False + + assert snap_exporter.uninstall() is True + snap_exporter.strategy.remove.assert_called_once() + + +def test_snap_exporter_uninstall_fail(snap_exporter): + snap_exporter.strategy.remove.side_effect = ValueError + + assert snap_exporter.uninstall() is False + + +def test_snap_exporter_uninstall_present(snap_exporter): + snap_exporter.snap_client.present = True + + assert snap_exporter.uninstall() is False + snap_exporter.strategy.remove.assert_called_once() + + +def test_snap_exporter_enable_and_start(snap_exporter): + snap_exporter.enable_and_start() + snap_exporter.snap_client.start.assert_called_once_with(["service1", "service2"], enable=True) + + +def test_snap_exporter_disable_and_stop(snap_exporter): + snap_exporter.disable_and_stop() + snap_exporter.snap_client.stop.assert_called_once_with(["service1", "service2"], disable=True) + + +def test_snap_exporter_restart(snap_exporter): + snap_exporter.restart() + snap_exporter.snap_client.restart.assert_called_once_with(reload=True) + + +def test_snap_exporter_check_health(snap_exporter): + snap_exporter.check_health() + snap_exporter.strategy.check.assert_called_once() + + +@pytest.mark.parametrize("install_result, expected_result", [(True, True), (False, False)]) +@mock.patch("service.SnapExporter.install") +def test_snap_exporter_configure(mock_install, snap_exporter, install_result, expected_result): + mock_install.return_value = install_result + + assert snap_exporter.configure() is expected_result + mock_install.assert_called_once() + + +def test_dcgm_exporter(): + mock_config = { + "dcgm-snap-channel": "latest/stable", + } + + exporter = service.DCGMExporter(mock_config) + assert exporter.exporter_name == "dcgm" + assert exporter.hw_tools() == {HWTool.DCGM} + + if __name__ == "__main__": unittest.main()