thin-edge.io icon indicating copy to clipboard operation
thin-edge.io copied to clipboard

watchdog: Use "systemd" crate instead of CLI of systemd

Open matthiasbeyer opened this issue 2 years ago • 12 comments

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)

matthiasbeyer avatar Jul 22 '22 06:07 matthiasbeyer

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?

matthiasbeyer avatar Jul 22 '22 06:07 matthiasbeyer

Is there a compilation error? No package 'libsystemd' found

cmosd avatar Jul 22 '22 06:07 cmosd

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?

matthiasbeyer avatar Jul 22 '22 06:07 matthiasbeyer

I tested it, works as expected!.

What tests have you done?

didier-wenzek avatar Jul 22 '22 13:07 didier-wenzek

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?

PradeepKiruvale avatar Jul 22 '22 14:07 PradeepKiruvale

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?

matthiasbeyer avatar Aug 01 '22 09:08 matthiasbeyer

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.

albinsuresh avatar Aug 02 '22 04:08 albinsuresh

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?

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.

albinsuresh avatar Aug 02 '22 14:08 albinsuresh

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...

matthiasbeyer avatar Aug 02 '22 14:08 matthiasbeyer

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...

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.

albinsuresh avatar Aug 03 '22 07:08 albinsuresh

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.

matthiasbeyer avatar Aug 03 '22 07:08 matthiasbeyer

Note to self: We might need to include a featureflag for this nonetheless because Yocto might not ship with systemd (haven't checked yet).

matthiasbeyer avatar Aug 08 '22 06:08 matthiasbeyer

Closing as this raises issues while not being a priority

didier-wenzek avatar Nov 21 '22 17:11 didier-wenzek