From 9c7f6175e7d6a24796227e293ab0071cd5cb97af Mon Sep 17 00:00:00 2001 From: Jason Nucciarone <40342202+NucciTheBoss@users.noreply.github.com> Date: Sun, 21 Jul 2024 16:54:59 -0400 Subject: [PATCH] feat: enable juju-systemd-notices to observe snap services (#128) * fix: `ruff` -> `ruff check` Original `ruff $@` is no longer supported by latest versions of ruff Signed-off-by: Jason C. Nucciarone * fix!: add support for observing snap services Adds the `watch.yaml` file for configuring how the notices daemon observes services. The daemon will get the service alias from this file and use the alias to determine the proper hook to fire when the observed service is started or stopped. BREAKING CHANGES: __init__ for SystemdNotices now takes the Service data class instead of a generic list. The Service dataclass was added so that it would be easier to specify an alias for an observed service. Signed-off-by: Jason C. Nucciarone * tests: update unit tests to reflect new notices API Signed-off-by: Jason C. Nucciarone * tests: update integration tests * Consolidates metadata.yaml and actions.yaml into charmcraft.yaml for test charm. Less files to worry about. Signed-off-by: Jason C. Nucciarone * docs: update lib documentation Signed-off-by: Jason C. Nucciarone * chore(fmt): apply formatting rules Signed-off-by: Jason C. Nucciarone * chore(lint): update copyright year Signed-off-by: Jason C. Nucciarone * refactor: `open` -> `write_text` `yaml.dump()` returns a string if steam is none, so it is easier to just pass the content off to `write_text` instead of opening the file with a context manager. Signed-off-by: Jason C. Nucciarone * fix(tests): correctly patch filesystem on python 3.8 Signed-off-by: Jason C. Nucciarone * reactor: remove watch.yaml mechanism - Switch to passing the service names and aliases via argparse rather than using the pyyaml external dependency to create the watch.yaml file. - Use global map to track the observed service rather than caching the output of a watch.yaml read. Signed-off-by: Jason C. Nucciarone * tests: update tests to not use watch.yaml - Reverted SystemdNotices in test charm back to the original constructor API. Signed-off-by: Jason C. Nucciarone * fix(tests): remove pyfakefs as test dependency - Fixes unit tests failing on Python 3.10 due to incorrect patching. Signed-off-by: Jason C. Nucciarone * refactor: remove explicit global references Signed-off-by: Jason C. Nucciarone * docs: update example to use correct constructor arguments Signed-off-by: Jason C. Nucciarone * chore: bump libpatch version Signed-off-by: Jason C. Nucciarone --------- Signed-off-by: Jason C. Nucciarone --- .../v0/juju_systemd_notices.py | 123 +++++++++++------- .../notices-charm/actions.yaml | 5 - .../notices-charm/charmcraft.yaml | 13 +- .../notices-charm/metadata.yaml | 8 -- .../notices-charm/src/charm.py | 2 +- tests/unit/test_juju_systemd_notices.py | 102 +++++++++------ tox.ini | 4 +- 7 files changed, 152 insertions(+), 105 deletions(-) delete mode 100644 tests/integration/juju_systemd_notices/notices-charm/actions.yaml delete mode 100644 tests/integration/juju_systemd_notices/notices-charm/metadata.yaml diff --git a/lib/charms/operator_libs_linux/v0/juju_systemd_notices.py b/lib/charms/operator_libs_linux/v0/juju_systemd_notices.py index 08157c9..6a12463 100644 --- a/lib/charms/operator_libs_linux/v0/juju_systemd_notices.py +++ b/lib/charms/operator_libs_linux/v0/juju_systemd_notices.py @@ -1,5 +1,5 @@ #!/usr/bin/python3 -# Copyright 2023 Canonical Ltd. +# Copyright 2023-2024 Canonical Ltd. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -28,6 +28,7 @@ ```python from charms.operator_libs_linux.v0.juju_systemd_notices import ( + Service, ServiceStartedEvent, ServiceStoppedEvent, SystemdNotices, @@ -41,7 +42,7 @@ def __init__(self, *args, **kwargs) -> None: super().__init__(*args, **kwargs) # Register services with charm. This adds the events to observe. - self._systemd_notices = SystemdNotices(self, ["slurmd"]) + self._systemd_notices = SystemdNotices(self, [Service("snap.slurm.slurmd", alias="slurmd")]) self.framework.observe(self.on.install, self._on_install) self.framework.observe(self.on.stop, self._on_stop) self.framework.observe(self.on.service_slurmd_started, self._on_slurmd_started) @@ -58,7 +59,7 @@ def _on_install(self, _: InstallEvent) -> None: def _on_start(self, _: StartEvent) -> None: # This will trigger the juju-systemd-notices daemon to # emit a `service-slurmd-started` event. - systemd.service_start("slurmd") + snap.slurmd.enable() def _on_stop(self, _: StopEvent) -> None: # To stop the juju-systemd-notices service running in the background. @@ -72,25 +73,25 @@ def _on_slurmd_started(self, _: ServiceStartedEvent) -> None: # This will trigger the juju-systemd-notices daemon to # emit a `service-slurmd-stopped` event. - systemd.service_stop("slurmd") + snap.slurmd.stop() def _on_slurmd_stopped(self, _: ServiceStoppedEvent) -> None: self.unit.status = BlockedStatus("slurmd not running") ``` """ -__all__ = ["ServiceStartedEvent", "ServiceStoppedEvent", "SystemdNotices"] +__all__ = ["Service", "ServiceStartedEvent", "ServiceStoppedEvent", "SystemdNotices"] import argparse import asyncio import functools import logging -import re import signal import subprocess import textwrap +from dataclasses import dataclass from pathlib import Path -from typing import List +from typing import List, Optional, Union from dbus_fast.aio import MessageBus from dbus_fast.constants import BusType, MessageType @@ -107,7 +108,7 @@ def _on_slurmd_stopped(self, _: ServiceStoppedEvent) -> 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 # juju-systemd-notices charm library dependencies. # Charm library dependencies are installed when the consuming charm is packed. @@ -115,8 +116,8 @@ def _on_slurmd_stopped(self, _: ServiceStoppedEvent) -> None: _logger = logging.getLogger(__name__) _juju_unit = None +_observed_services = {} _service_states = {} -_service_hook_regex_filter = re.compile(r"service-(?P[\w\\:-]*)-(?:started|stopped)") _DBUS_CHAR_MAPPINGS = { "_5f": "_", # _ must be first since char mappings contain _. "_40": "@", @@ -148,6 +149,19 @@ def _systemctl(*args) -> None: _disable_service = functools.partial(_systemctl, "disable") +@dataclass +class Service: + """Systemd service to observe. + + Args: + name: Name of systemd service to observe on dbus. + alias: Event name alias for service. + """ + + name: str + alias: Optional[str] = None + + class ServiceStartedEvent(EventBase): """Event emitted when service has started.""" @@ -159,34 +173,52 @@ class ServiceStoppedEvent(EventBase): class SystemdNotices: """Observe systemd services on your machine base.""" - def __init__(self, charm: CharmBase, services: List[str]) -> None: + def __init__(self, charm: CharmBase, services: List[Union[str, Service]]) -> None: """Instantiate systemd notices service.""" self._charm = charm - self._services = services + self._services = [Service(s) if isinstance(s, str) else s for s in services] unit_name = self._charm.unit.name.replace("/", "-") self._service_file = Path(f"/etc/systemd/system/juju-{unit_name}-systemd-notices.service") _logger.debug( "Attaching systemd notice events to charm %s", self._charm.__class__.__name__ ) - for service in self._services: - self._charm.on.define_event(f"service_{service}_started", ServiceStartedEvent) - self._charm.on.define_event(f"service_{service}_stopped", ServiceStoppedEvent) + for s in self._services: + event = s.alias or s.name + self._charm.on.define_event(f"service_{event}_started", ServiceStartedEvent) + self._charm.on.define_event(f"service_{event}_stopped", ServiceStoppedEvent) def subscribe(self) -> None: """Subscribe charmed operator to observe status of systemd services.""" + self._generate_hooks() + self._generate_service() + self._start() + + def stop(self) -> None: + """Stop charmed operator from observing the status of subscribed services.""" + _stop_service(self._service_file.name) + # Notices daemon is disabled so that the service will not restart after machine reboot. + _disable_service(self._service_file.name) + + def _generate_hooks(self) -> None: + """Generate legacy event hooks for observed systemd services.""" _logger.debug("Generating systemd notice hooks for %s", self._services) - start_hooks = [Path(f"hooks/service-{service}-started") for service in self._services] - stop_hooks = [Path(f"hooks/service-{service}-stopped") for service in self._services] + events = [s.alias or s.name for s in self._services] + start_hooks = [Path(f"hooks/service-{e}-started") for e in events] + stop_hooks = [Path(f"hooks/service-{e}-stopped") for e in events] for hook in start_hooks + stop_hooks: if hook.exists(): _logger.debug("Hook %s already exists. Skipping...", hook.name) else: hook.symlink_to(self._charm.framework.charm_dir / "dispatch") - _logger.debug("Starting %s daemon", self._service_file.name) + def _generate_service(self) -> None: + """Generate systemd service file for notices daemon.""" + _logger.debug("Generating service file %s", self._service_file.name) if self._service_file.exists(): _logger.debug("Overwriting existing service file %s", self._service_file.name) + + services = [f"{s.name}={s.alias or s.name}" for s in self._services] self._service_file.write_text( textwrap.dedent( f""" @@ -199,27 +231,31 @@ def subscribe(self) -> None: Restart=always WorkingDirectory={self._charm.framework.charm_dir} Environment="PYTHONPATH={self._charm.framework.charm_dir / "venv"}" - ExecStart=/usr/bin/python3 {__file__} {self._charm.unit.name} + ExecStart=/usr/bin/python3 {__file__} --unit {self._charm.unit.name} {' '.join(services)} [Install] WantedBy=multi-user.target """ ).strip() ) - _logger.debug("Service file %s written. Reloading systemd", self._service_file.name) + + _logger.debug( + "Service file %s written. Reloading systemd manager configuration", + self._service_file.name, + ) + + def _start(self) -> None: + """Start systemd notices daemon to observe subscribed services.""" + _logger.debug("Starting %s daemon", self._service_file.name) + + # Reload systemd manager configuration so that it will pick up notices daemon. _daemon_reload() - # Notices daemon is enabled so that the service will start even after machine reboot. - # This functionality is needed in the event that a charm is rebooted to apply updates. + + # Enable notices daemon to start after machine reboots. _enable_service(self._service_file.name) _start_service(self._service_file.name) _logger.debug("Started %s daemon", self._service_file.name) - def stop(self) -> None: - """Stop charmed operator from observing the status of subscribed services.""" - _stop_service(self._service_file.name) - # Notices daemon is disabled so that the service will not restart after machine reboot. - _disable_service(self._service_file.name) - def _name_to_dbus_path(name: str) -> str: """Convert the specified name into an org.freedesktop.systemd1.Unit path handle. @@ -280,7 +316,6 @@ def _systemd_unit_changed(msg: Message) -> bool: if "ActiveState" not in properties: return False - global _service_states if service not in _service_states: _logger.debug("Dropping event for unwatched service: %s", service) return False @@ -310,8 +345,9 @@ async def _send_juju_notification(service: str, state: str) -> None: if service.endswith(".service"): service = service[0:-len(".service")] # fmt: skip + alias = _observed_services[service] event_name = "started" if state == "active" else "stopped" - hook = f"service-{service}-{event_name}" + hook = f"service-{alias}-{event_name}" cmd = ["/usr/bin/juju-exec", _juju_unit, f"hooks/{hook}"] _logger.debug("Invoking hook %s with command: %s", hook, " ".join(cmd)) @@ -363,30 +399,12 @@ async def _async_load_services() -> None: that should be watched. Upon finding a service hook it's current ActiveState will be queried from systemd to determine it's initial state. """ - global _juju_unit - hooks_dir = Path.cwd() / "hooks" - _logger.info("Loading services from hooks in %s", hooks_dir) - - if not hooks_dir.exists(): - _logger.warning("Hooks dir %s does not exist.", hooks_dir) - return - - watched_services = [] - # Get service-{service}-(started|stopped) hooks defined by the charm. - for hook in hooks_dir.iterdir(): - match = _service_hook_regex_filter.match(hook.name) - if match: - watched_services.append(match.group("service")) - - _logger.info("Services from hooks are %s", watched_services) - if not watched_services: - return - bus = await MessageBus(bus_type=BusType.SYSTEM).connect() # Loop through all the services and be sure that a new watcher is # started for new ones. - for service in watched_services: + _logger.info("Services to observe are %s", _observed_services) + for service in _observed_services: # The .service suffix is necessary and will cause lookup failures of the # service unit when readying the watcher if absent from the service name. service = f"{service}.service" @@ -462,13 +480,18 @@ def _main(): """ parser = argparse.ArgumentParser() parser.add_argument("-d", "--debug", action="store_true") - parser.add_argument("unit", type=str) + parser.add_argument("--unit", type=str) + parser.add_argument("services", nargs="*") args = parser.parse_args() # Intentionally set as global. global _juju_unit _juju_unit = args.unit + for s in args.services: + service, alias = s.split("=") + _observed_services[service] = alias + console_handler = logging.StreamHandler() if args.debug: _logger.setLevel(logging.DEBUG) diff --git a/tests/integration/juju_systemd_notices/notices-charm/actions.yaml b/tests/integration/juju_systemd_notices/notices-charm/actions.yaml deleted file mode 100644 index 243f52a..0000000 --- a/tests/integration/juju_systemd_notices/notices-charm/actions.yaml +++ /dev/null @@ -1,5 +0,0 @@ -# Copyright 2023 Canonical Ltd. -# See LICENSE file for licensing details. - -stop-service: - description: Stop internal test service inside charm diff --git a/tests/integration/juju_systemd_notices/notices-charm/charmcraft.yaml b/tests/integration/juju_systemd_notices/notices-charm/charmcraft.yaml index e543430..4fc0954 100644 --- a/tests/integration/juju_systemd_notices/notices-charm/charmcraft.yaml +++ b/tests/integration/juju_systemd_notices/notices-charm/charmcraft.yaml @@ -1,6 +1,12 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2023-2024 Canonical Ltd. # See LICENSE file for licensing details. +name: juju-systemd-notices +description: | + Test charm used for the juju_systemd_notices charm library integration tests. +summary: | + A charm with a minimal daemon for testing the juju-systemd-notices charm library. + type: charm bases: - build-on: @@ -9,3 +15,8 @@ bases: run-on: - name: ubuntu channel: "22.04" + +actions: + stop-service: + description: Stop internal test service inside charm + diff --git a/tests/integration/juju_systemd_notices/notices-charm/metadata.yaml b/tests/integration/juju_systemd_notices/notices-charm/metadata.yaml deleted file mode 100644 index c5a8850..0000000 --- a/tests/integration/juju_systemd_notices/notices-charm/metadata.yaml +++ /dev/null @@ -1,8 +0,0 @@ -# Copyright 2023 Canonical Ltd. -# See LICENSE file for licensing details. - -name: juju-systemd-notices -description: | - Test charm used for the juju_systemd_notices charm library integration tests. -summary: | - A charm with a minimal daemon for testing the juju-systemd-notices charm library. diff --git a/tests/integration/juju_systemd_notices/notices-charm/src/charm.py b/tests/integration/juju_systemd_notices/notices-charm/src/charm.py index ff6cf87..56426d2 100755 --- a/tests/integration/juju_systemd_notices/notices-charm/src/charm.py +++ b/tests/integration/juju_systemd_notices/notices-charm/src/charm.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -# Copyright 2023 Canonical Ltd. +# Copyright 2023-2024 Canonical Ltd. # See LICENSE file for licensing details. """Minimal charm for testing the juju_systemd_notices charm library. diff --git a/tests/unit/test_juju_systemd_notices.py b/tests/unit/test_juju_systemd_notices.py index 59f7987..2e252c9 100644 --- a/tests/unit/test_juju_systemd_notices.py +++ b/tests/unit/test_juju_systemd_notices.py @@ -7,10 +7,10 @@ import argparse import subprocess import unittest -from pathlib import Path from unittest.mock import AsyncMock, patch from charms.operator_libs_linux.v0.juju_systemd_notices import ( + Service, ServiceStartedEvent, ServiceStoppedEvent, SystemdNotices, @@ -37,7 +37,9 @@ class MockNoticesCharm(CharmBase): def __init__(self, *args, **kwargs) -> None: super().__init__(*args, **kwargs) - self._systemd_notices = SystemdNotices(self, ["foobar"]) + self.notices = SystemdNotices( + self, ["foobar", Service("snap.test.service", alias="service")] + ) event_handler_bindings = { self.on.install: self._on_install, self.on.stop: self._on_stop, @@ -49,11 +51,11 @@ def __init__(self, *args, **kwargs) -> None: def _on_install(self, _: InstallEvent) -> None: """Subscribe to foobar service to watch for events.""" - self._systemd_notices.subscribe() + self.notices.subscribe() def _on_stop(self, _: StopEvent) -> None: """Stop watching foobar service as machine is removed.""" - self._systemd_notices.stop() + self.notices.stop() def _on_foobar_started(self, _: ServiceStartedEvent) -> None: """Set status to active after systemctl marks service as active.""" @@ -72,23 +74,47 @@ def setUp(self) -> None: self.addCleanup(self.harness.cleanup) self.harness.begin() - @patch("pathlib.Path.write_text") @patch("pathlib.Path.symlink_to") - @patch("subprocess.check_output") @patch("pathlib.Path.exists") - def test_subscribe(self, mock_exists, mock_subp, *_) -> None: - # Scenario 1 - Subscribe success but no pre-existing service file. + def test_generate_hooks(self, mock_exists, _) -> None: + """Test that legacy event hooks for observed services are created correctly.""" + # Scenario 1 - Generate success but no pre-existing hooks. mock_exists.return_value = False - self.harness.charm.on.install.emit() + self.harness.charm.notices._generate_hooks() - # Scenario 2 - Subscribe success and pre-existing service file. + # Scenario 2 - Generate success but pre-existing hooks. mock_exists.return_value = True - self.harness.charm.on.install.emit() + self.harness.charm.notices._generate_hooks() + + @patch("pathlib.Path.write_text") + @patch("pathlib.Path.exists") + def test_generate_service(self, mock_exists, _) -> None: + """Test that watch configuration file is generated correctly.""" + # Scenario 1 - Generate success but no pre-existing watch configuration. + mock_exists.return_value = False + self.harness.charm.notices._generate_service() + + # Scenario 2 - Generate success but pre-existing watch configuration. + mock_exists.return_value = True + self.harness.charm.notices._generate_service() + + @patch("subprocess.check_output") + def test_start(self, mock_subp) -> None: + """Test that start method correctly starts notices daemon.""" + # Scenario 1 - systemctl successfully starts notices daemon. + self.harness.charm.notices._start() - # Scenario 3 - Subscribe success but systemctl fails to start notices daemon. + # Scenario 2 - systemctl fails to start notices daemon. mock_subp.side_effect = subprocess.CalledProcessError(1, "systemctl start foobar") with self.assertRaises(subprocess.CalledProcessError): - self.harness.charm.on.install.emit() + self.harness.charm.notices._start() + + @patch("charms.operator_libs_linux.v0.juju_systemd_notices.SystemdNotices._generate_hooks") + @patch("charms.operator_libs_linux.v0.juju_systemd_notices.SystemdNotices._generate_service") + @patch("charms.operator_libs_linux.v0.juju_systemd_notices.SystemdNotices._start") + def test_subscribe(self, *_) -> None: + """Test `subscribe` method.""" + self.harness.charm.on.install.emit() @patch("subprocess.check_output") def test_stop(self, *_) -> None: @@ -170,21 +196,25 @@ async def test_systemd_unit_changed(self, mock_message, mock_variant, *_) -> Non ): self.assertTrue(_systemd_unit_changed(mock_message)) + @patch( + "charms.operator_libs_linux.v0.juju_systemd_notices._observed_services", + return_value={"foobar": "foobar"}, + ) @patch("charms.operator_libs_linux.v0.juju_systemd_notices._juju_unit", "foobar/0") @patch("asyncio.create_subprocess_exec") - async def test_send_juju_notification(self, mock_subp, *_) -> None: + async def test_send_juju_notification(self, mock_subcmd, *_) -> None: # Scenario 1 - .service in service name and notification succeeds. mock_p = AsyncMock() mock_p.wait.return_value = None mock_p.returncode = 0 - mock_subp.return_value = mock_p + mock_subcmd.return_value = mock_p await _send_juju_notification("foobar.service", "active") # Scenario 2 - No .service in name and state is stopped but notification fails. mock_p = AsyncMock() mock_p.wait.return_value = None mock_p.returncode = 1 - mock_subp.return_value = mock_p + mock_subcmd.return_value = mock_p await _send_juju_notification("foobar", "inactive") async def test_get_service_state(self) -> None: @@ -205,28 +235,20 @@ async def test_get_service_state(self) -> None: state = await _get_service_state(mock_sysbus, "foobar") self.assertEqual(state, "unknown") + @patch( + "charms.operator_libs_linux.v0.juju_systemd_notices._observed_services", + {"foobar": "foobar", "snap.test.service": "service"}, + ) @patch("charms.operator_libs_linux.v0.juju_systemd_notices._get_service_state") - @patch("pathlib.Path.iterdir") - @patch("pathlib.Path.exists") - async def test_async_load_services(self, mock_exists, mock_iterdir, mock_state) -> None: - # Scenario 1 - Hooks dir does not exist. - mock_exists.return_value = False - self.assertIsNone(await _async_load_services()) - - # Scenario 2 - There are no services to watch. - mock_exists.return_value = True - mock_iterdir.return_value = [] - self.assertIsNone(await _async_load_services()) - - # Scenario 3 - Desired outcome (services are subscribed to for watching). - mock_exists.return_value = True - mock_iterdir.return_value = [ - Path("service-foobar-started"), - Path("service-foobar-stopped"), - Path("dispatch"), # Ensure that unmatched hooks are ignored/not registered. - ] + async def test_async_load_services(self, mock_state) -> None: mock_state.return_value = "active" - self.assertIsNone(await _async_load_services()) + await _async_load_services() + + # Somehow calling `_async_load_services()` fully + # covers the for-loop without any additional changes to + # the `_observed_services` global ¯\_(ツ)_/¯ + mock_state.return_value = "stopped" + await _async_load_services() @patch("pathlib.Path.exists", return_value=False) @patch("asyncio.Event.wait") @@ -239,12 +261,16 @@ async def test_juju_systemd_notices_daemon(self, *_) -> None: def test_main(self, mocked_args, *_) -> None: # Scenario 1 - Desired outcome (juju-systemd-notices daemon starts successfully) # and debug is set to True. - mocked_args.return_value = argparse.Namespace(debug=True, unit="foobar/0") + mocked_args.return_value = argparse.Namespace( + debug=True, unit="foobar/0", services=["foobar=foobar", "snap.test.service=service"] + ) _main() # Scenario 2 - Desired outcome (juju-systemd-notices daemon starts successfully) # and debug is set to False - mocked_args.return_value = argparse.Namespace(debug=False, unit="foobar/0") + mocked_args.return_value = argparse.Namespace( + debug=False, unit="foobar/0", services=["foobar=foobar", "snap.test.service=service"] + ) _main() # Scenario 3 - Debug flag is passed to script but no unit name. diff --git a/tox.ini b/tox.ini index 9d33c65..ba81759 100644 --- a/tox.ini +++ b/tox.ini @@ -32,7 +32,7 @@ deps = black ruff commands = - ruff --fix {[vars]all_dir} + ruff check --fix {[vars]all_dir} black {[vars]all_dir} [testenv:lint] @@ -43,7 +43,7 @@ deps = codespell commands = codespell {toxinidir} - ruff {[vars]all_dir} + ruff check {[vars]all_dir} black --check --diff {[vars]all_dir} [testenv:unit]