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 08157c9d..6a12463a 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 243f52a3..00000000 --- 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 e543430c..4fc0954c 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 c5a88502..00000000 --- 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 ff6cf87d..56426d22 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 59f79872..2e252c9d 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 9d33c65b..ba817594 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]