feat: enable juju-systemd-notices to observe snap services #128
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original `ruff $@` is no longer supported by latest versions of ruff Signed-off-by: Jason C. Nucciarone <[email protected]>
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 <[email protected]>
Signed-off-by: Jason C. Nucciarone <[email protected]>
* Consolidates metadata.yaml and actions.yaml into charmcraft.yaml for test charm. Less files to worry about. Signed-off-by: Jason C. Nucciarone <[email protected]>
Signed-off-by: Jason C. Nucciarone <[email protected]>
Signed-off-by: Jason C. Nucciarone <[email protected]>
Signed-off-by: Jason C. Nucciarone <[email protected]>
`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 <[email protected]>
Signed-off-by: Jason C. Nucciarone <[email protected]>
benhoyt
reviewed
Jul 17, 2024
There was a problem hiding this comment.
Hi @NucciTheBoss -- nice work on this. A couple of comments about backwards compatibility, and a suggestion to simplify by using CLI arguments instead of watch.yaml.
While you're here, would you also have a few spare cycles to look at the test_snap_set_and_get_with_typed integration test failure? (In a separate PR.) If you can, that would be amazing.
- 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 <[email protected]>
- Reverted SystemdNotices in test charm back to the original constructor API. Signed-off-by: Jason C. Nucciarone <[email protected]>
- Fixes unit tests failing on Python 3.10 due to incorrect patching. Signed-off-by: Jason C. Nucciarone <[email protected]>
NucciTheBoss
changed the title
4 tasks
benhoyt
requested changes
Jul 19, 2024
Signed-off-by: Jason C. Nucciarone <[email protected]>
Signed-off-by: Jason C. Nucciarone <[email protected]>
Signed-off-by: Jason C. Nucciarone <[email protected]>
|
Addressed latest feedback comments and bumped Let me know if you have any more questions or comments! |
benhoyt
approved these changes
Jul 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #126
This pull request enables support for observing snap services using the
juju-systemd-noticescharm library.The key change in this pull request is modifying how the notices daemon configures how it observes services on the machine. Rather than configure which services to observe by reading the
hooksdirectory, the daemon instead reads a watch.yaml file in the charm directory. Here's what the watch.yaml file would look like in a deployed charm using the notices daemon:Originally,
juju-systemd-noticesused a regex that parsed hook names to determine which services to observe with the notices daemon. This approach worked until we needed to start working with nested services - e.g. have.in the service name likesnap.slurm.slurmd- so we had to find a new way to configure how to observe services, but also ensure that the correct hook would be fired by the charm. We decided to use a key value approach where the key is the service name to observe over DBus, and the value is the valid event name alias used to trigger the start/stop events.Now, when the notices daemon captures the event, it uses the configured alias to determine which hook to fire, but can still watch DBus using the correct service name.
Misc.
snap.slurm.slurmd- and not the unit aliasslurmd.ruffnow requires the subcommandcheckto perform linting actions.SystemdNoticesinit constructor now takes a variable amount ofServiceobjects rather than aList[str]. We used aServicedataclass since it made it easy to provide service name aliases and set a default alias if none was provided by the charm author.