From 57446aa13b303898ef8cc568a9a8250c66941bd4 Mon Sep 17 00:00:00 2001 From: "Jason C. Nucciarone" Date: Tue, 16 Jul 2024 17:50:10 -0400 Subject: [PATCH 1/4] tests: add `pyfakefs` as unit test dependency Signed-off-by: Jason C. Nucciarone --- tox.ini | 1 + 1 file changed, 1 insertion(+) diff --git a/tox.ini b/tox.ini index 7fefa6b..281f51d 100644 --- a/tox.ini +++ b/tox.ini @@ -56,6 +56,7 @@ description = Run unit tests. deps = pytest pytest-mock + pyfakefs coverage[toml] -r{toxinidir}/requirements.txt commands = From b5ea556517367f36234c221bd1392fa45c3a4304 Mon Sep 17 00:00:00 2001 From: "Jason C. Nucciarone" Date: Tue, 16 Jul 2024 17:51:04 -0400 Subject: [PATCH 2/4] refactor: use context manager to create config files on demand - No longer create empty slurm.conf file at installation. Instead the context manager will create an empty `SlurmConfig` object and make the new file the first time the admin sets a config value. Signed-off-by: Jason C. Nucciarone --- src/slurmhelpers/hooks.py | 8 --- src/slurmhelpers/models.py | 110 +++++++++++++++++++------------------ 2 files changed, 56 insertions(+), 62 deletions(-) diff --git a/src/slurmhelpers/hooks.py b/src/slurmhelpers/hooks.py index 2ee8f86..a47c83a 100644 --- a/src/slurmhelpers/hooks.py +++ b/src/slurmhelpers/hooks.py @@ -89,9 +89,7 @@ def install(snap: Snap) -> None: """ setup_logging(snap.paths.common / "hooks.log") munge = Munge(snap) - slurm = Slurm(snap) slurmd = Slurmd(snap) - slurmdbd = Slurmdbd(snap) slurmrestd = Slurmrestd(snap) logging.info("Executing snap `install` hook.") @@ -107,12 +105,6 @@ def install(snap: Snap) -> None: logging.info("Generating default munge.key secret.") munge.generate_key() - logging.info("Creating empty `slurm.conf` file.") - slurm.config_file.touch(0o644) - - logging.info("Creating empty `slurmdbd.conf` file.") - slurmdbd.config_file.touch(0o644) - def configure(snap: Snap) -> None: """Configure hook for the Slurm snap.""" diff --git a/src/slurmhelpers/models.py b/src/slurmhelpers/models.py index 99b7254..791e435 100644 --- a/src/slurmhelpers/models.py +++ b/src/slurmhelpers/models.py @@ -228,26 +228,27 @@ def __init__(self, *args) -> None: def update_config(self, config: Dict[str, str]) -> None: """Update configuration for the `slurmdbd` service.""" - # Load current `slurmdbd.conf` configuration file. - # Preserve old configuration so that we can determine - # if substantial changes were made to the `slurmdbd.conf` file. - sconf = slurmdbdconfig.load(self.config_file) - old = sconf.dict() + with slurmdbdconfig.edit(self.config_file) as sconf: + # Preserve old configuration so that we can determine + # if substantial changes were made to the `slurmdbd.conf` file. + old = sconf.dict() - # Assemble new `slurmdbd.conf` file. - for k, v in config.items(): - key = k.replace("-", "_") - if not hasattr(sconf, key): - raise AttributeError(f"`slurmdbd` config file does not support option {key}.") + # Assemble new `slurmdbd.conf` file. + for k, v in config.items(): + key = k.replace("-", "_") + if not hasattr(sconf, key): + raise AttributeError(f"`slurmdbd` config file does not support option {key}.") - setattr(sconf, key, _apply_callback(key, v, model=sconf)) + setattr(sconf, key, _apply_callback(key, v, model=sconf)) - if sconf.dict() == old: - logging.debug("No change in `slurmdbd` service configuration. Not updating.") - return + if sconf.dict() == old: + logging.debug("No change in `slurmdbd` service configuration. Not updating.") + return + + logging.info("Updating `slurmdbd` configuration file %s.", self.config_file) + self._needs_restart(["slurmdbd"]) - slurmdbdconfig.dump(sconf, self.config_file) - self._needs_restart(["slurmdbd"]) + self.config_file.chmod(0o600) class Slurmrestd(_BaseModel): @@ -402,41 +403,42 @@ def __init__(self, *args) -> None: def update_config(self, config: Dict[str, Any]) -> None: """Update configuration of the Slurm workload manager.""" - # Load current `slurm.conf` configuration file. - # Preserve old configuration so that we can determine - # if substantial changes were made to the `slurm.conf` file. - sconf = slurmconfig.load(self.config_file) - old = sconf.dict() - - # Assemble new `slurm.conf` file. - for k, v in config.items(): - match key := k.replace("-", "_"): - case "include": - # Multiline configuration options. Requires special handling. - sconf.include = v.split(",") - case "slurmctld_host": - # Multiline configuration options. Requires special handling. - sconf.slurmctld_host = v.split(",") - case "nodes": - sconf.nodes = _process_nodes(v) - case "frontend_nodes": - sconf.frontend_nodes = _process_frontend_nodes(v) - case "down_nodes": - sconf.down_nodes = _process_down_nodes(v) - case "node_sets": - sconf.node_sets = _process_node_sets(v) - case "partitions": - sconf.partitions = _process_partitions(v) - case _: - if not hasattr(sconf, key): - raise AttributeError(f"Slurm config file does not support option {key}.") - - setattr(sconf, key, _apply_callback(key, v, model=sconf)) - - if sconf.dict() == old: - logging.debug("No change in Slurm workload manager configuration. Not updating.") - return - - logging.info("Updating Slurm workload manager configuration file %s.", self.config_file) - slurmconfig.dump(sconf, self.config_file) - self._needs_restart(["slurmctld", "slurmd", "slurmdbd", "slurmrestd"]) + with slurmconfig.edit(self.config_file) as sconf: + # Preserve old configuration so that we can determine + # if substantial changes were made to the `slurm.conf` file. + old = sconf.dict() + # Assemble new `slurm.conf` file. + for k, v in config.items(): + match key := k.replace("-", "_"): + case "include": + # Multiline configuration options. Requires special handling. + sconf.include = v.split(",") + case "slurmctld_host": + # Multiline configuration options. Requires special handling. + sconf.slurmctld_host = v.split(",") + case "nodes": + sconf.nodes = _process_nodes(v) + case "frontend_nodes": + sconf.frontend_nodes = _process_frontend_nodes(v) + case "down_nodes": + sconf.down_nodes = _process_down_nodes(v) + case "node_sets": + sconf.node_sets = _process_node_sets(v) + case "partitions": + sconf.partitions = _process_partitions(v) + case _: + if not hasattr(sconf, key): + raise AttributeError( + f"Slurm config file does not support option {key}." + ) + + setattr(sconf, key, _apply_callback(key, v, model=sconf)) + + if sconf.dict() == old: + logging.debug("No change in Slurm workload manager configuration. Not updating.") + return + + logging.info("Updating Slurm configuration file %s.", self.config_file) + self._needs_restart(["slurmctld", "slurmd", "slurmdbd", "slurmrestd"]) + + self.config_file.chmod(0o600) From 4ae0d2b6b119b0eefe7742282d17370c2ab68fa7 Mon Sep 17 00:00:00 2001 From: "Jason C. Nucciarone" Date: Tue, 16 Jul 2024 17:53:16 -0400 Subject: [PATCH 3/4] tests: use `pyfakes` to create mock config file for editors Signed-off-by: Jason C. Nucciarone --- tests/unit/conftest.py | 5 +++++ tests/unit/test_models.py | 15 ++++++++------- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 41c4a06..349b668 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -23,6 +23,11 @@ from snaphelpers._ctl import ServiceInfo +@pytest.fixture +def fake_fs(fs): + """Mock filesystem for configuration hook unit tests.""" + yield fs + @pytest.fixture def env(): """Mock the snap runtime environment.""" diff --git a/tests/unit/test_models.py b/tests/unit/test_models.py index aa39863..3662c18 100644 --- a/tests/unit/test_models.py +++ b/tests/unit/test_models.py @@ -143,7 +143,7 @@ def test_update_config(self, mocker, munge) -> None: class TestSlurmModel: """Test the `Slurm` data model.""" - def test_update_config(self, mocker, slurm) -> None: + def test_update_config(self, mocker, fake_fs, slurm) -> None: """Test `update_config` method.""" nodes = NodeMap() frontend_nodes = FrontendNodeMap() @@ -160,9 +160,11 @@ def test_update_config(self, mocker, slurm) -> None: config.node_sets = node_sets config.partitions = partitions + # Create fake file for slurm configuration editor. + fake_fs.create_file("/var/snap/slurm/common/etc/slurm/slurm.conf") + # Test when there has been no change to slurm configuration. mocker.patch("slurmutils.editors.slurmconfig.load", return_value=config) - mocker.patch("slurmutils.editors.slurmconfig.dump") mocker.patch("slurmhelpers.models._process_nodes", return_value=nodes) mocker.patch("slurmhelpers.models._process_frontend_nodes", return_value=frontend_nodes) mocker.patch("slurmhelpers.models._process_down_nodes", return_value=down_nodes) @@ -183,7 +185,6 @@ def test_update_config(self, mocker, slurm) -> None: # Test when there has been a change made to the slurm configuration. mocker.patch("slurmutils.editors.slurmconfig.load", return_value=config) - mocker.patch("slurmutils.editors.slurmconfig.dump") mocker.patch("slurmhelpers.models._process_nodes", return_value=nodes) mocker.patch("slurmhelpers.models._process_frontend_nodes", return_value=frontend_nodes) mocker.patch("slurmhelpers.models._process_down_nodes", return_value=down_nodes) @@ -204,7 +205,6 @@ def test_update_config(self, mocker, slurm) -> None: # Test when a bad slurmdbd configuration option has been provided. mocker.patch("slurmutils.editors.slurmconfig.load", return_value=config) - mocker.patch("slurmutils.editors.slurmconfig.dump") mocker.patch("slurmhelpers.models._process_nodes", return_value=nodes) mocker.patch("slurmhelpers.models._process_frontend_nodes", return_value=frontend_nodes) mocker.patch("slurmhelpers.models._process_down_nodes", return_value=down_nodes) @@ -407,13 +407,16 @@ def test_update_config(self, mocker, slurmd) -> None: class TestSlurmdbdModel: """Test the `Slurmdbd` data model.""" - def test_update_config(self, mocker, slurmdbd) -> None: + def test_update_config(self, mocker, fake_fs, slurmdbd) -> None: """Test `update_config` method.""" config = SlurmdbdConfig() config.log_file = "/var/log/slurm/slurmdbd.log" config.private_data = ["accounts", "events", "jobs"] config.track_slurmctld_down = "no" + # Create fake file for slurmdbd configuration editor. + fake_fs.create_file("/var/snap/slurm/common/etc/slurm/slurmdbd.conf") + # Test when there has been no change to slurmdbd configuration. mocker.patch("slurmutils.editors.slurmdbdconfig.load", return_value=config) mocker.patch("slurmutils.editors.slurmdbdconfig.dump") @@ -427,7 +430,6 @@ def test_update_config(self, mocker, slurmdbd) -> None: # Test when there has been a change to the slurmdbd configuration. mocker.patch("slurmutils.editors.slurmdbdconfig.load", return_value=config) - mocker.patch("slurmutils.editors.slurmdbdconfig.dump") slurmdbd.update_config( { "log-file": "/var/log/slurm/slurmdbd.log", @@ -438,7 +440,6 @@ def test_update_config(self, mocker, slurmdbd) -> None: # Test when a bad slurmdbd configuration option has been provided. mocker.patch("slurmutils.editors.slurmdbdconfig.load", return_value=config) - mocker.patch("slurmutils.editors.slurmdbdconfig.dump") with pytest.raises(AttributeError): slurmdbd.update_config( { From a628f2094a7cde99c9870419546cd053d30ecdfe Mon Sep 17 00:00:00 2001 From: "Jason C. Nucciarone" Date: Tue, 16 Jul 2024 18:06:18 -0400 Subject: [PATCH 4/4] chore(fmt): apply formatting rules Signed-off-by: Jason C. Nucciarone --- tests/unit/conftest.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 349b668..7ba3567 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -28,6 +28,7 @@ def fake_fs(fs): """Mock filesystem for configuration hook unit tests.""" yield fs + @pytest.fixture def env(): """Mock the snap runtime environment."""