From 2dd6db1e2f0799f164cec5e2390696fc17f2d869 Mon Sep 17 00:00:00 2001 From: "Jason C. Nucciarone" Date: Tue, 9 Jul 2024 11:54:03 -0400 Subject: [PATCH 1/8] feat!: use composition and inheritance to build managers BREAKING CHANGES: Changes the current API to use composition to enable configuration modification and munge service management. Signed-off-by: Jason C. Nucciarone --- lib/charms/hpc_libs/v0/slurm_ops.py | 259 +++++++++++++++++----------- 1 file changed, 159 insertions(+), 100 deletions(-) diff --git a/lib/charms/hpc_libs/v0/slurm_ops.py b/lib/charms/hpc_libs/v0/slurm_ops.py index 1b83c0c..560d26a 100644 --- a/lib/charms/hpc_libs/v0/slurm_ops.py +++ b/lib/charms/hpc_libs/v0/slurm_ops.py @@ -13,22 +13,21 @@ # limitations under the License. -"""Library to manage the Slurm snap. +"""Abstractions for managing Slurm operations via snap. -This library contains the `SlurmManager` class, which offers interfaces to use and manage -the Slurm snap inside charms. +This library contains high-level interfaces for controlling the operations +of Slurm and its composing services. It also provides various utilities for +installing and managing the Slurm snap installation on the host. -### General usage +### Example usage -For starters, the `SlurmManager` constructor receives a `Service` enum as a parameter, which -helps the manager determine things like the correct service to enable, or the correct settings -key to mutate. +The `slurm_ops` charm library provides management interfaces for Slurm services, which +helps the charm determine correct service to enable, or the correct settings +key to mutate on the snap. + +```python +import charms.hpc_libs.v0.slurm_ops as slurm -``` -from charms.hpc_libs.v0.slurm_ops import ( - Service, - SlurmManager, -) class ApplicationCharm(CharmBase): # Application charm that needs to use the Slurm snap. @@ -37,26 +36,34 @@ def __init__(self, *args, **kwargs) -> None: super().__init__(*args, **kwargs) # Charm events defined in the NFSRequires class. - self._slurm_manager = SlurmManager(Service.SLURMCTLD) + self._slurmctld_manager = slurm.SlurmctldManager() self.framework.observe( self.on.install, self._on_install, ) def _on_install(self, _) -> None: - self._slurm_manager.install() - self.unit.set_workload_version(self._slurm_manager.version()) - self._slurm_manager.set_config("cluster-name", "cluster") + slurm.install() + self.unit.set_workload_version(slurm.version()) + self._slurmctld_manager.config.set({"cluster-name": "cluster"}) ``` """ -import base64 -import enum -import functools +__all__ = [ + "install", + "version", + "SlurmctldManager", + "SlurmdManager", + "SlurmdbdManager", + "SlurmrestdManager", +] + +import json import logging -import os import subprocess -import tempfile +from collections.abc import Mapping +from enum import Enum +from typing import Any, Optional import yaml @@ -76,7 +83,20 @@ def _on_install(self, _) -> None: PYDEPS = ["pyyaml>=6.0.1"] -def _call(cmd: str, *args: [str]) -> bytes: +def install() -> None: + """Install Slurm.""" + # TODO: Pin slurm to the stable channel + _snap("install", "slurm", "--channel", "latest/candidate", "--classic") + + +def version() -> str: + """Get the current version of Slurm installed on the system.""" + info = yaml.safe_load(_snap("info", "slurm")) + ver: str = info["installed"] + return ver.split(maxsplit=1)[0] + + +def _call(cmd: str, *args: str, stdin: Optional[str] = None) -> str: """Call a command with logging. Raises: @@ -85,7 +105,7 @@ def _call(cmd: str, *args: [str]) -> bytes: cmd = [cmd, *args] _logger.debug(f"Executing command {cmd}") try: - return subprocess.check_output(cmd, stderr=subprocess.PIPE, text=False) + return subprocess.check_output(cmd, input=stdin, stderr=subprocess.PIPE, text=True).strip() except subprocess.CalledProcessError as e: _logger.error(f"`{' '.join(cmd)}` failed") _logger.error(f"stderr: {e.stderr.decode()}") @@ -98,16 +118,26 @@ def _snap(*args) -> str: Raises: subprocess.CalledProcessError: Raised if snap command fails. """ - return _call("snap", *args).decode() + return _call("snap", *args) + + +def _mungectl(*args: str, stdin: Optional[str] = None) -> str: + """Control munge via `slurm.mungectl ...`. + Args: + *args: Arguments to pass to `mungectl`. + stdin: Input to pass to `mungectl` via stdin. -_get_config = functools.partial(_snap, "get", "slurm") -_set_config = functools.partial(_snap, "set", "slurm") + Raises: + subprocess.CalledProcessError: Raised if `mungectl` command fails. + """ + return _call("slurm.mungectl", *args, stdin=stdin) -class Service(enum.Enum): - """Type of Slurm service that will be managed by `SlurmManager`.""" +class ServiceType(Enum): + """Type of Slurm service to manage.""" + MUNGED = "munged" SLURMD = "slurmd" SLURMCTLD = "slurmctld" SLURMDBD = "slurmdbd" @@ -116,94 +146,123 @@ class Service(enum.Enum): @property def config_name(self) -> str: """Configuration name on the slurm snap for this service type.""" - if self is Service.SLURMCTLD: + if self is ServiceType.SLURMCTLD: return "slurm" + if self is ServiceType.MUNGED: + return "munge" + return self.value -class SlurmManager: - """Slurm snap manager. +class ServiceManager: + """Control a Slurm service.""" - This class offers methods to manage the Slurm snap for a certain service type. - The list of available services is specified by the `Service` enum. - """ + def enable(self) -> None: + """Enable service.""" + _snap("start", "--enable", f"slurm.{self._service.value}") - def __init__(self, service: Service): + def disable(self) -> None: + """Disable service.""" + _snap("stop", "--disable", f"slurm.{self._service.value}") + + def restart(self) -> None: + """Restart service.""" + _snap("restart", f"slurm.{self._service.value}") + + +class ConfigurationManager: + """Control configuration of a Slurm component.""" + + def __init__(self, service: ServiceType) -> None: self._service = service - def install(self): - """Install the slurm snap in this system.""" - # TODO: Pin slurm to the stable channel - _snap("install", "slurm", "--channel", "latest/candidate", "--classic") + def get_options(self, *keys: str) -> Mapping[str, Any]: + """Get given configurations values for Slurm component.""" + configs = {} + for key in keys: + config = self.get(key) + target = key.rsplit(".", maxsplit=1)[-1] + configs[target] = config - def enable(self): - """Start and enable the managed slurm service and the munged service.""" - _snap("start", "--enable", "slurm.munged") - _snap("start", "--enable", f"slurm.{self._service.value}") + return configs - def restart(self): - """Restart the managed slurm service.""" - _snap("restart", f"slurm.{self._service.value}") + def get(self, key: str) -> Any: + """Get specific configuration value for Slurm component.""" + key = f"{self._service.config_name}.{key}" + config = json.loads(_snap("get", "-d", "slurm", key)) + return config[key] - def restart_munged(self): - """Restart the munged service.""" - _snap("restart", "slurm.munged") + def set(self, config: Mapping[str, Any]) -> None: + """Set configuration for Slurm component.""" + args = [f"{self._service.config_name}.{k}={json.dumps(v)}" for k, v in config.items()] + _snap("set", "slurm", *args) - def disable(self): - """Disable the managed slurm service and the munged service.""" - _snap("stop", "--disable", "slurm.munged") - _snap("stop", "--disable", f"slurm.{self._service.value}") + def unset(self, *keys) -> None: + """Unset configuration for Slurm component.""" + args = [f"{self._service.config_name}.{k}!" for k in keys] + _snap("unset", "slurm", *args) - def set_config(self, key: str, value: str): - """Set a snap config for the managed slurm service. - See the configuration section from the [Slurm readme](https://github.com/charmed-hpc/slurm-snap#configuration) - for a list of all the available configurations. +class MungeManager(ServiceManager): + """Manage `munged` service operations.""" - Note that this will only allow configuring the settings that are exclusive to - the specific managed service. (the slurmctld service uses the slurm parent key) - """ - _set_config(f"{self._service.config_name}.{key}={value}") + def __init__(self) -> None: + self._service = ServiceType.MUNGED + self.config = ConfigurationManager(ServiceType.MUNGED) + + def get_key(self) -> str: + """Get the current munge key. - def get_config(self, key: str) -> str: - """Get a snap config for the managed slurm service. + Returns: + The current munge key as a base64-encoded string. + """ + return _mungectl("key", "get") - See the configuration section from the [Slurm readme](https://github.com/charmed-hpc/slurm-snap#configuration) - for a list of all the available configurations. + def set_key(self, key: str) -> None: + """Set a new munge key. - Note that this will only allow fetching the settings that are exclusive to - the specific managed service. (the slurmctld service uses the slurm parent key) + Args: + key: A new, base64-encoded munge key. """ - # Snap returns the config value with an additional newline at the end. - return _get_config(f"{self._service.config_name}.{key}").strip() - - def generate_munge_key(self) -> bytes: - """Generate a new cryptographically secure munged key.""" - handle, path = tempfile.mkstemp() - try: - _call("mungekey", "-f", "-k", path) - os.close(handle) - with open(path, "rb") as f: - return f.read() - finally: - os.remove(path) - - def set_munge_key(self, key: bytes): - """Set the current munged key.""" - # TODO: use `slurm.setmungekey` when implemented - # subprocess.run(["slurm.setmungekey"], stdin=key) - key = base64.b64encode(key).decode() - _set_config(f"munge.key={key}") - - def get_munge_key(self) -> bytes: - """Get the current munged key.""" - # TODO: use `slurm.setmungekey` when implemented - # key = subprocess.run(["slurm.getmungekey"]) - key = _get_config("munge.key") - return base64.b64decode(key) - - def version(self) -> str: - """Get the installed Slurm version of the snap.""" - info = yaml.safe_load(_snap("info", "slurm")) - version: str = info["installed"] - return version.split(maxsplit=1)[0] + _mungectl("key", "set", stdin=key) + + def generate_key(self) -> None: + """Generate a new, cryptographically secure munge key.""" + _mungectl("key", "generate") + + +class SlurmBaseManager(ServiceManager): + """Base manager for Slurm services.""" + + def __init__(self, service: ServiceType) -> None: + self._service = service + self.config = ConfigurationManager(service) + self.munge = MungeManager() + + +class SlurmctldManager(SlurmBaseManager): + """Manage `slurmctld` service operations.""" + + def __init__(self) -> None: + super().__init__(ServiceType.SLURMCTLD) + + +class SlurmdManager(SlurmBaseManager): + """Manage `slurmd` service operations.""" + + def __init__(self) -> None: + super().__init__(ServiceType.SLURMD) + + +class SlurmdbdManager(SlurmBaseManager): + """Manage `slurmdbd` service operations.""" + + def __init__(self) -> None: + super().__init__(ServiceType.SLURMDBD) + + +class SlurmrestdManager(SlurmBaseManager): + """Manage `slurmrestd` service operations.""" + + def __init__(self) -> None: + super().__init__(ServiceType.SLURMRESTD) From b95c767caa09c955d0a11e90e8e37f4a84e55c77 Mon Sep 17 00:00:00 2001 From: "Jason C. Nucciarone" Date: Tue, 9 Jul 2024 11:55:39 -0400 Subject: [PATCH 2/8] tests: update unit tests to use composition & inheritance API Signed-off-by: Jason C. Nucciarone --- tests/unit/test_slurm_ops.py | 158 ++++++++++++++++++++--------------- 1 file changed, 89 insertions(+), 69 deletions(-) diff --git a/tests/unit/test_slurm_ops.py b/tests/unit/test_slurm_ops.py index 49196a0..e238b03 100644 --- a/tests/unit/test_slurm_ops.py +++ b/tests/unit/test_slurm_ops.py @@ -8,7 +8,7 @@ import subprocess from unittest.mock import patch -from charms.hpc_libs.v0.slurm_ops import Service, SlurmManager +import charms.hpc_libs.v0.slurm_ops as slurm from pyfakefs.fake_filesystem_unittest import TestCase MUNGEKEY = b"1234567890" @@ -37,123 +37,143 @@ """ +@patch("charms.hpc_libs.v0.slurm_ops.subprocess.check_output") +class TestSlurmOps(TestCase): + + def setUp(self) -> None: + self.setUpPyfakefs() + + def test_install(self, subcmd) -> None: + """Test that `slurm_ops` calls the correct install command.""" + slurm.install() + args = subcmd.call_args[0][0] + self.assertEqual(args[:3], ["snap", "install", "slurm"]) + self.assertIn("--classic", args[3:]) # codespell:ignore + + def test_version(self, subcmd) -> None: + """Test that `slurm_ops` gets the correct version using the correct command.""" + subcmd.return_value = SLURM_INFO.encode() + version = slurm.version() + args = subcmd.call_args[0][0] + self.assertEqual(args, ["snap", "info", "slurm"]) + self.assertEqual(version, "23.11.7") + + def test_call_error(self, subcmd) -> None: + """Test that `slurm_ops` propagates errors when a command fails.""" + subcmd.side_effect = subprocess.CalledProcessError(-1, cmd=[""], stderr=b"error") + with self.assertRaises(subprocess.CalledProcessError): + slurm.install() + + @patch("charms.hpc_libs.v0.slurm_ops.subprocess.check_output") class SlurmOpsBase: - """Test slurm_ops library.""" + """Test the Slurm service operations managers.""" def setUp(self) -> None: self.setUpPyfakefs() - self.manager = SlurmManager(self.service) def test_config_name(self, *_) -> None: """Test that the config name is correctly set.""" self.assertEqual(self.manager._service.config_name, self.config_name) - def test_install(self, subcmd, *_) -> None: - """Test that the manager calls the correct install command.""" - self.manager.install() - args = subcmd.call_args[0][0] - self.assertEqual(args[:3], ["snap", "install", "slurm"]) - self.assertIn("--classic", args[3:]) # codespell:ignore - def test_enable(self, subcmd, *_) -> None: """Test that the manager calls the correct enable command.""" self.manager.enable() calls = [args[0][0] for args in subcmd.call_args_list] - self.assertEqual(calls[0], ["snap", "start", "--enable", "slurm.munged"]) - self.assertEqual(calls[1], ["snap", "start", "--enable", f"slurm.{self.service.value}"]) - - def test_restart(self, subcmd, *_) -> None: - """Test that the manager calls the correct restart command.""" - self.manager.restart() - - args = subcmd.call_args[0][0] - self.assertEqual(args, ["snap", "restart", f"slurm.{self.service.value}"]) - - def test_restart_munged(self, subcmd, *_) -> None: - """Test that the manager calls the correct enable command for munged.""" - self.manager.restart_munged() - args = subcmd.call_args[0][0] - self.assertEqual(args, ["snap", "restart", "slurm.munged"]) + self.assertEqual( + calls[0], ["snap", "start", "--enable", f"slurm.{self.manager._service.value}"] + ) def test_disable(self, subcmd, *_) -> None: """Test that the manager calls the correct disable command.""" self.manager.disable() calls = [args[0][0] for args in subcmd.call_args_list] - self.assertEqual(calls[0], ["snap", "stop", "--disable", "slurm.munged"]) - self.assertEqual(calls[1], ["snap", "stop", "--disable", f"slurm.{self.service.value}"]) + self.assertEqual( + calls[0], ["snap", "stop", "--disable", f"slurm.{self.manager._service.value}"] + ) + + def test_restart(self, subcmd, *_) -> None: + """Test that the manager calls the correct restart command.""" + self.manager.restart() - def test_set_config(self, subcmd, *_) -> None: - """Test that the manager calls the correct set_config command.""" - self.manager.set_config("key", "value") args = subcmd.call_args[0][0] - self.assertEqual(args, ["snap", "set", "slurm", f"{self.config_name}.key=value"]) + self.assertEqual(args, ["snap", "restart", f"slurm.{self.manager._service.value}"]) + + def test_get_options(self, subcmd) -> None: + """Test that the manager correctly collects all requested configuration options.""" + subcmd.return_value = '{"%(name)s.key1": "value1", "%(name)s.key2": "value2"}' % { + "name": self.config_name + } + value = self.manager.config.get_options("key1", "key2") + calls = [args[0][0] for args in subcmd.call_args_list] + self.assertEqual(calls[0], ["snap", "get", "-d", "slurm", f"{self.config_name}.key1"]) + self.assertEqual(calls[1], ["snap", "get", "-d", "slurm", f"{self.config_name}.key2"]) + self.assertEqual(value, {"key1": "value1", "key2": "value2"}) def test_get_config(self, subcmd, *_) -> None: - """Test that the manager calls the correct get_config command.""" - subcmd.return_value = b"value" - value = self.manager.get_config("key") + """Test that the manager calls the correct `snap get ...` command.""" + subcmd.return_value = '{"%s.key": "value"}' % self.config_name + value = self.manager.config.get("key") args = subcmd.call_args[0][0] - self.assertEqual(args, ["snap", "get", "slurm", f"{self.config_name}.key"]) + self.assertEqual(args, ["snap", "get", "-d", "slurm", f"{self.config_name}.key"]) self.assertEqual(value, "value") - def test_generate_munge_key(self, subcmd, *_) -> None: - """Test that the manager calls the correct mungekey command.""" - - def mock_mungekey(*args, **kwargs): - (_mk, _f, _k, path) = args[0] - self.assertEqual(_mk, "mungekey") + def test_set_config(self, subcmd, *_) -> None: + """Test that the manager calls the correct `snap set ...` command.""" + self.manager.config.set({"key": "value"}) + args = subcmd.call_args[0][0] + self.assertEqual(args, ["snap", "set", "slurm", f'{self.config_name}.key="value"']) - with open(path, "wb") as f: - f.write(MUNGEKEY) + def test_unset_config(self, subcmd) -> None: + """Test that the manager calls the correct `snap unset ...` command.""" + self.manager.config.unset("key") + args = subcmd.call_args[0][0] + self.assertEqual(args, ["snap", "unset", "slurm", f"{self.config_name}.key!"]) - subcmd.side_effect = mock_mungekey - key = self.manager.generate_munge_key() - self.assertEqual(key, MUNGEKEY) + def test_generate_munge_key(self, subcmd, *_) -> None: + """Test that the manager calls the correct `mungectl` command.""" + self.manager.munge.generate_key() + args = subcmd.call_args[0][0] + self.assertEqual(args, ["slurm.mungectl", "key", "generate"]) def test_set_munge_key(self, subcmd, *_) -> None: """Test that the manager sets the munge key with the correct command.""" - self.manager.set_munge_key(MUNGEKEY) + self.manager.munge.set_key(MUNGEKEY_BASE64) args = subcmd.call_args[0][0] - self.assertEqual(args, ["snap", "set", "slurm", f"munge.key={MUNGEKEY_BASE64.decode()}"]) + # MUNGEKEY_BASE64 is piped to `stdin` to avoid exposure. + self.assertEqual(args, ["slurm.mungectl", "key", "set"]) def test_get_munge_key(self, subcmd, *_) -> None: """Test that the manager gets the munge key with the correct command.""" subcmd.return_value = MUNGEKEY_BASE64 - key = self.manager.get_munge_key() - self.assertEqual(key, MUNGEKEY) - - def test_version(self, subcmd, *_) -> None: - """Test that the manager gets the version key with the correct command.""" - subcmd.return_value = SLURM_INFO.encode() - version = self.manager.version() + key = self.manager.munge.get_key() args = subcmd.call_args[0][0] - self.assertEqual(args, ["snap", "info", "slurm"]) - self.assertEqual(version, "23.11.7") + self.assertEqual(args, ["slurm.mungectl", "key", "get"]) + self.assertEqual(key, MUNGEKEY_BASE64) - def test_call_error(self, subcmd, *_) -> None: - """Test that the manager propagates errors when a command fails.""" - subcmd.side_effect = subprocess.CalledProcessError(-1, cmd=[""], stderr=b"error") - with self.assertRaises(subprocess.CalledProcessError): - self.manager.install() + def test_configure_munge(self, subcmd) -> None: + """Test that manager is able to correctly configure munge.""" + self.manager.munge.config.set({"max-thread-count": 24}) + args = subcmd.call_args[0][0] + self.assertEqual(args, ["snap", "set", "slurm", "munge.max-thread-count=24"]) parameters = [ - (Service.SLURMCTLD, "slurm"), - (Service.SLURMD, "slurmd"), - (Service.SLURMDBD, "slurmdbd"), - (Service.SLURMRESTD, "slurmrestd"), + (slurm.SlurmctldManager(), "slurm"), + (slurm.SlurmdManager(), "slurmd"), + (slurm.SlurmdbdManager(), "slurmdbd"), + (slurm.SlurmrestdManager(), "slurmrestd"), ] -for service, config_name in parameters: - cls_name = f"TestSlurmOps_{service.value}" +for manager, config_name in parameters: + cls_name = f"Test{manager._service.value.capitalize()}Ops" globals()[cls_name] = type( cls_name, (SlurmOpsBase, TestCase), { - "service": service, + "manager": manager, "config_name": config_name, }, ) From 05014a39ba6e5571fbbbf705024c0d040734c324 Mon Sep 17 00:00:00 2001 From: "Jason C. Nucciarone" Date: Tue, 9 Jul 2024 11:56:20 -0400 Subject: [PATCH 3/8] tests: update integration tests to use composition & inheritance API Signed-off-by: Jason C. Nucciarone --- tests/integration/slurm_ops/test_manager.py | 42 ++++++++++----------- 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/tests/integration/slurm_ops/test_manager.py b/tests/integration/slurm_ops/test_manager.py index 7dec19f..434fd96 100644 --- a/tests/integration/slurm_ops/test_manager.py +++ b/tests/integration/slurm_ops/test_manager.py @@ -2,49 +2,45 @@ # Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. +import base64 import pytest -from lib.charms.hpc_libs.v0.slurm_ops import Service, SlurmManager +import lib.charms.hpc_libs.v0.slurm_ops as slurm @pytest.fixture -def slurm_manager() -> SlurmManager: - return SlurmManager(Service.SLURMCTLD) +def slurmctld() -> slurm.SlurmctldManager: + return slurm.SlurmctldManager() @pytest.mark.order(1) -def test_install(slurm_manager: SlurmManager) -> None: +def test_install(slurmctld: slurm.SlurmctldManager) -> None: """Install Slurm using the manager.""" - slurm_manager.install() - slurm_manager.enable() - slurm_manager.set_munge_key(slurm_manager.generate_munge_key()) + slurm.install() + slurmctld.enable() + slurmctld.munge.generate_key() with open("/var/snap/slurm/common/etc/munge/munge.key", "rb") as f: - key: bytes = f.read() + key: str = base64.b64encode(f.read()).decode() - assert key == slurm_manager.get_munge_key() + assert key == slurmctld.munge.get_key() @pytest.mark.order(2) -def test_rotate_key(slurm_manager: SlurmManager) -> None: +def test_rotate_key(slurmctld: slurm.SlurmctldManager) -> None: """Test that the munge key can be rotated.""" - old_key = slurm_manager.get_munge_key() - - slurm_manager.set_munge_key(slurm_manager.generate_munge_key()) - - new_key = slurm_manager.get_munge_key() - + old_key = slurmctld.munge.get_key() + slurmctld.munge.generate_key() + new_key = slurmctld.munge.get_key() assert old_key != new_key @pytest.mark.order(3) -def test_slurm_config(slurm_manager: SlurmManager) -> None: +def test_slurm_config(slurmctld: slurm.SlurmctldManager) -> None: """Test that the slurm config can be changed.""" - slurm_manager.set_config("cluster-name", "test-cluster") - - value = slurm_manager.get_config("cluster-name") - + slurmctld.config.set({"cluster-name": "test-cluster"}) + value = slurmctld.config.get("cluster-name") assert value == "test-cluster" with open("/var/snap/slurm/common/etc/slurm/slurm.conf", "r") as f: @@ -60,9 +56,9 @@ def test_slurm_config(slurm_manager: SlurmManager) -> None: @pytest.mark.order(4) -def test_version(slurm_manager: SlurmManager) -> None: +def test_version() -> None: """Test that the Slurm manager can report its version.""" - version = slurm_manager.version() + version = slurm.version() # We are interested in knowing that this does not return a falsy value (`None`, `''`, `[]`, etc.) assert version From 571f75f6d2561d205cd7334d1e9073a761c2532b Mon Sep 17 00:00:00 2001 From: "Jason C. Nucciarone" Date: Tue, 9 Jul 2024 15:14:03 -0400 Subject: [PATCH 4/8] refactor: remove service-specific operations managers Decided that these should be defined locally within the Slurm operators themselves rather than bloat the charm library with code that will only be used by a specific operator. Signed-off-by: Jason C. Nucciarone --- lib/charms/hpc_libs/v0/slurm_ops.py | 71 +++++++-------------- tests/integration/slurm_ops/test_manager.py | 11 ++-- tests/unit/test_slurm_ops.py | 9 +-- 3 files changed, 35 insertions(+), 56 deletions(-) diff --git a/lib/charms/hpc_libs/v0/slurm_ops.py b/lib/charms/hpc_libs/v0/slurm_ops.py index 560d26a..7f184b8 100644 --- a/lib/charms/hpc_libs/v0/slurm_ops.py +++ b/lib/charms/hpc_libs/v0/slurm_ops.py @@ -15,18 +15,25 @@ """Abstractions for managing Slurm operations via snap. -This library contains high-level interfaces for controlling the operations -of Slurm and its composing services. It also provides various utilities for -installing and managing the Slurm snap installation on the host. +This library contains the `SlurmManagerBase` and `ServiceType` class +which provide high-level interfaces for managing Slurm within charmed operators. -### Example usage +### Example Usage -The `slurm_ops` charm library provides management interfaces for Slurm services, which -helps the charm determine correct service to enable, or the correct settings -key to mutate on the snap. +#### Managing a Slurm service -```python +The `SlurmManagerBase` constructor receives a `ServiceType` enum. The enum instructs +the inheriting Slurm service manager how to manage its corresponding Slurm service on the host. + +```python3 import charms.hpc_libs.v0.slurm_ops as slurm +from charms.hpc_libs.v0.slurm_ops import SlurmManagerBase, ServiceType + +class SlurmctldManager(SlurmManagerBase): + # Manage `slurmctld` service on host. + + def __init__(self) -> None: + super().__init__(ServiceType.SLURMCTLD) class ApplicationCharm(CharmBase): @@ -36,7 +43,7 @@ def __init__(self, *args, **kwargs) -> None: super().__init__(*args, **kwargs) # Charm events defined in the NFSRequires class. - self._slurmctld_manager = slurm.SlurmctldManager() + self._slurm_manager = SlurmctldManager() self.framework.observe( self.on.install, self._on_install, @@ -45,17 +52,15 @@ def __init__(self, *args, **kwargs) -> None: def _on_install(self, _) -> None: slurm.install() self.unit.set_workload_version(slurm.version()) - self._slurmctld_manager.config.set({"cluster-name": "cluster"}) + self._slurm_manager.config.set({"cluster-name": "cluster"}) ``` """ __all__ = [ "install", "version", - "SlurmctldManager", - "SlurmdManager", - "SlurmdbdManager", - "SlurmrestdManager", + "ServiceType", + "SlurmManagerBase", ] import json @@ -67,8 +72,6 @@ def _on_install(self, _) -> None: import yaml -_logger = logging.getLogger(__name__) - # The unique Charmhub library identifier, never change it LIBID = "541fd767f90b40539cf7cd6e7db8fabf" @@ -79,13 +82,15 @@ def _on_install(self, _) -> None: # to 0 if you are raising the major API version LIBPATCH = 1 - +# Charm library dependencies to fetch during `charmcraft pack`. PYDEPS = ["pyyaml>=6.0.1"] +_logger = logging.getLogger(__name__) + def install() -> None: """Install Slurm.""" - # TODO: Pin slurm to the stable channel + # FIXME: Pin slurm to the stable channel _snap("install", "slurm", "--channel", "latest/candidate", "--classic") @@ -231,38 +236,10 @@ def generate_key(self) -> None: _mungectl("key", "generate") -class SlurmBaseManager(ServiceManager): +class SlurmManagerBase(ServiceManager): """Base manager for Slurm services.""" def __init__(self, service: ServiceType) -> None: self._service = service self.config = ConfigurationManager(service) self.munge = MungeManager() - - -class SlurmctldManager(SlurmBaseManager): - """Manage `slurmctld` service operations.""" - - def __init__(self) -> None: - super().__init__(ServiceType.SLURMCTLD) - - -class SlurmdManager(SlurmBaseManager): - """Manage `slurmd` service operations.""" - - def __init__(self) -> None: - super().__init__(ServiceType.SLURMD) - - -class SlurmdbdManager(SlurmBaseManager): - """Manage `slurmdbd` service operations.""" - - def __init__(self) -> None: - super().__init__(ServiceType.SLURMDBD) - - -class SlurmrestdManager(SlurmBaseManager): - """Manage `slurmrestd` service operations.""" - - def __init__(self) -> None: - super().__init__(ServiceType.SLURMRESTD) diff --git a/tests/integration/slurm_ops/test_manager.py b/tests/integration/slurm_ops/test_manager.py index 434fd96..78ac8ca 100644 --- a/tests/integration/slurm_ops/test_manager.py +++ b/tests/integration/slurm_ops/test_manager.py @@ -7,15 +7,16 @@ import pytest import lib.charms.hpc_libs.v0.slurm_ops as slurm +from lib.charms.hpc_libs.v0.slurm_ops import ServiceType, SlurmManagerBase @pytest.fixture -def slurmctld() -> slurm.SlurmctldManager: - return slurm.SlurmctldManager() +def slurmctld() -> SlurmManagerBase: + return SlurmManagerBase(ServiceType.SLURMCTLD) @pytest.mark.order(1) -def test_install(slurmctld: slurm.SlurmctldManager) -> None: +def test_install(slurmctld: SlurmManagerBase) -> None: """Install Slurm using the manager.""" slurm.install() slurmctld.enable() @@ -28,7 +29,7 @@ def test_install(slurmctld: slurm.SlurmctldManager) -> None: @pytest.mark.order(2) -def test_rotate_key(slurmctld: slurm.SlurmctldManager) -> None: +def test_rotate_key(slurmctld: SlurmManagerBase) -> None: """Test that the munge key can be rotated.""" old_key = slurmctld.munge.get_key() slurmctld.munge.generate_key() @@ -37,7 +38,7 @@ def test_rotate_key(slurmctld: slurm.SlurmctldManager) -> None: @pytest.mark.order(3) -def test_slurm_config(slurmctld: slurm.SlurmctldManager) -> None: +def test_slurm_config(slurmctld: SlurmManagerBase) -> None: """Test that the slurm config can be changed.""" slurmctld.config.set({"cluster-name": "test-cluster"}) value = slurmctld.config.get("cluster-name") diff --git a/tests/unit/test_slurm_ops.py b/tests/unit/test_slurm_ops.py index e238b03..c02b5cc 100644 --- a/tests/unit/test_slurm_ops.py +++ b/tests/unit/test_slurm_ops.py @@ -9,6 +9,7 @@ from unittest.mock import patch import charms.hpc_libs.v0.slurm_ops as slurm +from charms.hpc_libs.v0.slurm_ops import ServiceType, SlurmManagerBase from pyfakefs.fake_filesystem_unittest import TestCase MUNGEKEY = b"1234567890" @@ -161,10 +162,10 @@ def test_configure_munge(self, subcmd) -> None: parameters = [ - (slurm.SlurmctldManager(), "slurm"), - (slurm.SlurmdManager(), "slurmd"), - (slurm.SlurmdbdManager(), "slurmdbd"), - (slurm.SlurmrestdManager(), "slurmrestd"), + (SlurmManagerBase(ServiceType.SLURMCTLD), "slurm"), + (SlurmManagerBase(ServiceType.SLURMD), "slurmd"), + (SlurmManagerBase(ServiceType.SLURMDBD), "slurmdbd"), + (SlurmManagerBase(ServiceType.SLURMRESTD), "slurmrestd"), ] for manager, config_name in parameters: From fff9c94e4fa0729ebe7012c13434bdd5f6745970 Mon Sep 17 00:00:00 2001 From: "Jason C. Nucciarone" Date: Tue, 9 Jul 2024 17:04:43 -0400 Subject: [PATCH 5/8] feat: add kebabizer for formatting slurm keys Signed-off-by: Jason C. Nucciarone --- lib/charms/hpc_libs/v0/slurm_ops.py | 25 +++++++++++++++++++++++++ tests/unit/test_slurm_ops.py | 5 +++++ 2 files changed, 30 insertions(+) diff --git a/lib/charms/hpc_libs/v0/slurm_ops.py b/lib/charms/hpc_libs/v0/slurm_ops.py index 7f184b8..7016dc1 100644 --- a/lib/charms/hpc_libs/v0/slurm_ops.py +++ b/lib/charms/hpc_libs/v0/slurm_ops.py @@ -57,6 +57,7 @@ def _on_install(self, _) -> None: """ __all__ = [ + "format_key", "install", "version", "ServiceType", @@ -65,6 +66,7 @@ def _on_install(self, _) -> None: import json import logging +import re import subprocess from collections.abc import Mapping from enum import Enum @@ -86,6 +88,29 @@ def _on_install(self, _) -> None: PYDEPS = ["pyyaml>=6.0.1"] _logger = logging.getLogger(__name__) +_acronym = re.compile(r"(?<=[A-Z])(?=[A-Z][a-z])") +_kebabize = re.compile(r"(?<=[a-z0-9])(?=[A-Z])") + + +def format_key(key: str) -> str: + """Format Slurm configuration keys from SlurmCASe into kebab case. + + Args: + key: Slurm configuration key to convert to kebab case. + + Notes: + Slurm configuration syntax does not follow proper PascalCasing + format, so we cannot put keys directly through a kebab case converter + to get the desired format. Some additional processing is needed for + certain keys before the key can properly kebabized. + + For example, without additional preprocessing, the key `CPUs` will + become `cp-us` if put through a kebabizer with being preformatted to `Cpus`. + """ + if "CPUs" in key: + key = key.replace("CPUs", "Cpus") + key = _acronym.sub(r"-", key) + return _kebabize.sub(r"-", key).lower() def install() -> None: diff --git a/tests/unit/test_slurm_ops.py b/tests/unit/test_slurm_ops.py index c02b5cc..48e6279 100644 --- a/tests/unit/test_slurm_ops.py +++ b/tests/unit/test_slurm_ops.py @@ -44,6 +44,11 @@ class TestSlurmOps(TestCase): def setUp(self) -> None: self.setUpPyfakefs() + def test_format_key(self, _) -> None: + """Test that `kebabize` properly formats slurm keys.""" + self.assertEqual(slurm.format_key("CPUs"), "cpus") + self.assertEqual(slurm.format_key("AccountingStorageHost"), "accounting-storage-host") + def test_install(self, subcmd) -> None: """Test that `slurm_ops` calls the correct install command.""" slurm.install() From 0ee8efbc26142a63e833a0584a5dcd8e7b3ab6be Mon Sep 17 00:00:00 2001 From: "Jason C. Nucciarone" Date: Tue, 9 Jul 2024 17:06:15 -0400 Subject: [PATCH 6/8] fix(docs): remove reference to NFSRequires integration Signed-off-by: Jason C. Nucciarone --- lib/charms/hpc_libs/v0/slurm_ops.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/charms/hpc_libs/v0/slurm_ops.py b/lib/charms/hpc_libs/v0/slurm_ops.py index 7016dc1..043be63 100644 --- a/lib/charms/hpc_libs/v0/slurm_ops.py +++ b/lib/charms/hpc_libs/v0/slurm_ops.py @@ -42,7 +42,6 @@ class ApplicationCharm(CharmBase): def __init__(self, *args, **kwargs) -> None: super().__init__(*args, **kwargs) - # Charm events defined in the NFSRequires class. self._slurm_manager = SlurmctldManager() self.framework.observe( self.on.install, From 1aabe0acafc8d6b267212dde343afef59604602e Mon Sep 17 00:00:00 2001 From: "Jason C. Nucciarone" Date: Wed, 10 Jul 2024 09:44:12 -0400 Subject: [PATCH 7/8] tests: remove `pyfakefs` as unit test dependency Signed-off-by: Jason C. Nucciarone --- tests/unit/test_slurm_ops.py | 8 +------- tox.ini | 1 - 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/tests/unit/test_slurm_ops.py b/tests/unit/test_slurm_ops.py index 48e6279..652f02c 100644 --- a/tests/unit/test_slurm_ops.py +++ b/tests/unit/test_slurm_ops.py @@ -6,11 +6,11 @@ import base64 import subprocess +from unittest import TestCase from unittest.mock import patch import charms.hpc_libs.v0.slurm_ops as slurm from charms.hpc_libs.v0.slurm_ops import ServiceType, SlurmManagerBase -from pyfakefs.fake_filesystem_unittest import TestCase MUNGEKEY = b"1234567890" MUNGEKEY_BASE64 = base64.b64encode(MUNGEKEY) @@ -41,9 +41,6 @@ @patch("charms.hpc_libs.v0.slurm_ops.subprocess.check_output") class TestSlurmOps(TestCase): - def setUp(self) -> None: - self.setUpPyfakefs() - def test_format_key(self, _) -> None: """Test that `kebabize` properly formats slurm keys.""" self.assertEqual(slurm.format_key("CPUs"), "cpus") @@ -75,9 +72,6 @@ def test_call_error(self, subcmd) -> None: class SlurmOpsBase: """Test the Slurm service operations managers.""" - def setUp(self) -> None: - self.setUpPyfakefs() - def test_config_name(self, *_) -> None: """Test that the config name is correctly set.""" self.assertEqual(self.manager._service.config_name, self.config_name) diff --git a/tox.ini b/tox.ini index 3bac066..801d23e 100644 --- a/tox.ini +++ b/tox.ini @@ -50,7 +50,6 @@ commands = description = Run unit tests deps = pytest - pyfakefs coverage[toml] -r {tox_root}/requirements.txt commands = From f7086d933ac7b1b550f3d7d75a71bb9f9e5048c2 Mon Sep 17 00:00:00 2001 From: "Jason C. Nucciarone" Date: Wed, 10 Jul 2024 11:17:38 -0400 Subject: [PATCH 8/8] chore: bump LIBPATCH version Signed-off-by: Jason C. Nucciarone --- lib/charms/hpc_libs/v0/slurm_ops.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/charms/hpc_libs/v0/slurm_ops.py b/lib/charms/hpc_libs/v0/slurm_ops.py index 043be63..d72c145 100644 --- a/lib/charms/hpc_libs/v0/slurm_ops.py +++ b/lib/charms/hpc_libs/v0/slurm_ops.py @@ -81,7 +81,7 @@ def _on_install(self, _) -> None: # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 1 +LIBPATCH = 2 # Charm library dependencies to fetch during `charmcraft pack`. PYDEPS = ["pyyaml>=6.0.1"]