thin-edge.io
thin-edge.io copied to clipboard
watchdog: Use "systemd" crate instead of CLI of systemd
This patch changes the talking to the systemd process from using the CLI interface of systemd to using the "systemd" crate.
The Error type had to be adapted to be able to bubble up errors from the
systemd crate interface.
If the talking to systemd failed (indicated by a false
returned from
systemd::daemon::pid_notify
, we return a WatchdogError::TalkingFailed.
There seems to be nothing more we can do.
You should probably have a very close look whether I transformed the code in the right way (so behavior is equal to before and test this, because I didn't :laughing:
Types of changes
- [ ] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
- [ ] Documentation Update (if none of the other choices apply)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
Checklist
- [x] I have read the CONTRIBUTING doc
- [x] I have signed the CLA (in all commits with git commit -s)
- [x] I ran
cargo fmt
as mentioned in CODING_GUIDELINES - [x] I used
cargo clippy
as mentioned in CODING_GUIDELINES - [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] I have added necessary documentation (if appropriate)
Ah, of course, we would depend on libsystemd now in the CI containers. I can add commits for adding this dependency, of course, if you want me to...
But first: Do we want this patch?
Is there a compilation error? No package 'libsystemd' found
Is there a compilation error? No package 'libsystemd' found
Yes that's because the systemd
crate I am using here depends on libsystemd
on the system. So we would have to include this in the CI image. I can totally do this, but I wanted some discussion whether this change is wanted or not - maybe @didier-wenzek wants to have a look at this first, too?
I tested it, works as expected!.
What tests have you done?
I tested it, works as expected!.
What tests have you done?
I tested the tedge_mapper c8y/az
and tedge_agent
updating the tedge_watchdog
systemd files. One can follow the steps from this document.
https://github.com/thin-edge/thin-edge.io/blob/main/docs/src/howto-guides/021_enable_tedge_watchdog_using_systemd.md
Observe the health status messages sent by these (Agent/mapper) processes on health status MQTT topics by subscribing to these topics.
Did I miss any other tests here?
Doesn't the debian package description of this crate need any update to define libsystemd as a dependency of this tedge-watchdog package?
Probably! Can you point me to the package definition?
Probably! Can you point me to the package definition?
All the debian package configurations are in the Cargo.toml of this crate itself, under the [package.metadata.deb]
section. We use this cargo helper command for building the debian packages.
Is there a compilation error? No package 'libsystemd' found
Yes that's because the
systemd
crate I am using here depends onlibsystemd
on the system. So we would have to include this in the CI image. I can totally do this, but I wanted some discussion whether this change is wanted or not - maybe @didier-wenzek wants to have a look at this first, too?
It's okay to expect a user using the tedge-watchdog
component, designed specifically for systemd based OS flavours to have this libsystemd
dependency installed in-order to use this package. But, having this dependency even to build the project would be too much of an ask for all the devs using non-Debian flavours, right? Is there any way to avoid enforcing such a hard dependency on everyone?
While looking at alternatives, I came across this pure-rust systemd crate as well: https://crates.io/crates/libsystemd. Is it something that you evaluated already? Their documentation says that it's not feature-complete, which is concerning. But I was just wondering if it's sufficient for our needs at least.
While looking at alternatives, I came across this pure-rust systemd crate as well: https://crates.io/crates/libsystemd. Is it something that you evaluated already? Their documentation says that it's not feature-complete, which is concerning. But I was just wondering if it's sufficient for our needs at least.
I had a quick look and thought that it is nice, but I'd rather use the "real deal" instead of some possibly not-as-good maintained alternative implementation that might introduce bugs where using the native thing would be just no issue.
But, having this dependency even to build the project would be too much of an ask for all the devs using non-Debian flavours, right?
I'm not sure whether I understand what you mean. If you want to work on the codebase, you'd still not need debian, just the dependency libsystemd
, nothing else than using openssl
or zlib
which is required by other parts of the project... Maybe I'm misunderstanding something though...
I'm not sure whether I understand what you mean. If you want to work on the codebase, you'd still not need debian, just the dependency
libsystemd
, nothing else than usingopenssl
orzlib
which is required by other parts of the project... Maybe I'm misunderstanding something though...
What I was asking is, if this libsystemd
dependency would make the life difficult for devs using non-systemd based systems for development, like a macOS or some other Linux flavour without systemd support, preventing them from even building this project? They would not want to install or use this tedge-watchdog
component on their machines, as it's not meant for them. But, that dependency shouldn't be stopping them from just building this project, right? So, I was asking if there's any way to make the tedge-watchdog
module build only on systemd based OSes so that this libsystemd
dependency is enforced only on those.
I see where you're going!
What I was asking is, if this libsystemd dependency would make the life difficult for devs using non-systemd based systems for development, like a macOS or some other Linux flavour without systemd support
For Apple, these devs wouldn't build tedge_watchdog I guess. I am not 100% sure whether libsystemd can be installed on macos just for the sake of building.
Linux users don't have a problem, because all distributions can ship libsystemd, even if the distribution itself does not use systemd at all.
So the answer to
But, that dependency shouldn't be stopping them from just building this project, right?
is: "depends". Linux users don't have a problem, Apple users might be able to install libsystemd, but I am not sure about that, I have never used any Apple devices.
So, I was asking if there's any way to make the tedge-watchdog module build only on systemd based OSes so that this libsystemd dependency is enforced only on those.
Yes, we can use the cfg attribute to check whether we're compiling for linux and then include this crate or not. This is not done by this PR, though. I think we should get this issue sorted out before this PR goes in (if it goes in). I will file some PR for this shortly.
Note to self: We might need to include a featureflag for this nonetheless because Yocto might not ship with systemd (haven't checked yet).
Closing as this raises issues while not being a priority