Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notify systemd for start, stop, watchdog #20482

Merged
merged 2 commits into from
Jan 21, 2024
Merged

Conversation

chrthi
Copy link
Contributor

@chrthi chrthi commented Jan 1, 2024

As already suggested in #20413, this PR implements Type=notify. This makes zigbee2mqtt send a notification to systemd when it has finished starting up and when it starts shutting down. My use case for example was to add an ExecStartPost= line to the service file that changes the ACL of the web interface socket. For this I needed a reliable way to know when this socket has actually been created.
It is also possible to specify a watchdog timeout in the service file; in this case zigbee2mqtt will regularly notify systemd that it's main loop is still alive, and systemd will restart it if it fails to do so for too long.

One drawback is that requiring sd-notify adds a dependency on the libsystemd-dev deb package. So I've made the sd-notify dependency optional in an attempt to not break builds on other platforms. This might need some refinement or updates to the install instructions. The deb package might also need to be added to the Docker images.

@chrthi chrthi force-pushed the feature/sd-notify branch from dfff208 to a6216a7 Compare January 1, 2024 20:31
lib/controller.ts Outdated Show resolved Hide resolved
@@ -39,6 +39,16 @@ const AllExtensions = [
type ExtensionArgs = [Zigbee, MQTT, State, PublishEntityState, EventBus,
(enable: boolean, name: string) => Promise<void>, () => void, (extension: Extension) => Promise<void>];

try {
if (process.env.NOTIFY_SOCKET) {
var sdNotify = require('sd-notify');
Copy link
Owner

@Koenkk Koenkk Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use import/from syntax, also I suggest to use let sdNotify = null on line 42, then you can skip the other sdNotify = null statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, import is a static import, so we can't do it conditionally. I tried to avoid that because sd-notify requires the libsystemd-dev C library to build and would be a hassle to install for Mac or Windows users. I've now switched to optional-require which seems like a good solution to me and also simplifies this code a lot.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems optional-require only do a try/catch, in my opinion not worth adding a dependency for. I think we can easily add such function to utils.ts

@Koenkk
Copy link
Owner

Koenkk commented Jan 2, 2024

Could you take a look at the failing test and also make a PR for the docs?

@chrthi chrthi force-pushed the feature/sd-notify branch 3 times, most recently from 15568c1 to e619c56 Compare January 7, 2024 17:29
@chrthi
Copy link
Contributor Author

chrthi commented Jan 7, 2024

The "failing tests" were in fact just a failing coverage check because the CI tests run without optional dependencies. I now mock the module in the controller tests so the code is covered. I haven't added an actual test case since the application logic is rather trivial and testing end-to-end by running under systemd is the only way to be sure it really works.
Working on the docs update now

chrthi added a commit to chrthi/zigbee2mqtt.io that referenced this pull request Jan 7, 2024
@chrthi
Copy link
Contributor Author

chrthi commented Jan 7, 2024

Documentation PR added: Koenkk/zigbee2mqtt.io#2475

@chrthi chrthi force-pushed the feature/sd-notify branch from e619c56 to 57490c6 Compare January 7, 2024 22:18
@IgnacioHR
Copy link

IgnacioHR commented Jan 9, 2024

Hi, I'm glad to see the progress on this task!

I just wanted to share some thoughts about the dependency: It should be possible to avoid the dependency at all by using exec with the right parameters. In fact, there is an executable systemd-notify that accepts parameters for userid, pid and status.

The documentation is here

Note, the pid parameter is important because the exec will be launched in a new process and the pid to send is the pid of the running node.

There is more information here. I wonder why for the solution 2 the article don't mention the pid parameter (maybe the pid parameter was added afterwards to resolve the race condition)

@chrthi chrthi force-pushed the feature/sd-notify branch from 57490c6 to 73f3498 Compare January 21, 2024 18:31
@chrthi chrthi marked this pull request as draft January 21, 2024 18:35
@chrthi chrthi force-pushed the feature/sd-notify branch from 73f3498 to 46eb038 Compare January 21, 2024 19:25
@chrthi chrthi force-pushed the feature/sd-notify branch from 46eb038 to 27835a0 Compare January 21, 2024 19:42
chrthi added a commit to chrthi/zigbee2mqtt.io that referenced this pull request Jan 21, 2024
@chrthi
Copy link
Contributor Author

chrthi commented Jan 21, 2024

Hi, I'm glad to see the progress on this task!

I just wanted to share some thoughts about the dependency: It should be possible to avoid the dependency at all by using exec with the right parameters. In fact, there is an executable systemd-notify that accepts parameters for userid, pid and status.

The documentation is here

Note, the pid parameter is important because the exec will be launched in a new process and the pid to send is the pid of the running node.

There is more information here. I wonder why for the solution 2 the article don't mention the pid parameter (maybe the pid parameter was added afterwards to resolve the race condition)

Hi!
I was aware that systemd-notify exists, but as the documentation says, it is more intended for (shell) scripts. In a service that is written in a proper programming language, it would be somewhat wasteful to start a process when all one really needs to do is write a message to a socket. Especially if it happens every few seconds when the watchdog is used.

Now node is a bit of an in-between case; it can write to unix sockets directly just fine, but they dropped support for datagram sockets -.- https://groups.google.com/g/nodejs/c/iCzhcuxGP1I so the sd-notify module that uses the libsystemd C library for that seems to be the next-best solution.

@chrthi
Copy link
Contributor Author

chrthi commented Jan 21, 2024

Removed the optional-require dependency and used the try/catch suggested on https://www.npmjs.com/package/optional-require instead. Unfortunately the let sdNotify; causes some implicit-any warnings and eslint complains about the empty catch, so the result became a bit ugly. If someone knows better, feel free to make suggestions...

@chrthi chrthi marked this pull request as ready for review January 21, 2024 20:00
@Koenkk Koenkk changed the base branch from master to dev January 21, 2024 20:46
@Koenkk Koenkk merged commit 97eac16 into Koenkk:dev Jan 21, 2024
8 checks passed
@Koenkk
Copy link
Owner

Koenkk commented Jan 21, 2024

Thanks!

@IgnacioHR
Copy link

Great work!

Koenkk pushed a commit to Koenkk/zigbee2mqtt.io that referenced this pull request Feb 1, 2024
@sdunster
Copy link

sdunster commented Feb 6, 2024

This doesn't work in the docker images published on docker.io and I think this is because the Dockerfile's npm ci specifies --no-optional. Would you accept a PR to remove that?

@IgnacioHR
Copy link

IgnacioHR commented Feb 8, 2024

I've created a PR to update the documentation

Koenkk/zigbee2mqtt.io#2558

In order for this to work, the user needs to install the sd-notify requirements

@sdunster
Copy link

sdunster commented Feb 9, 2024

I was referring to the fact the Docker images built by this repo's Dockerfile don't currently include libsystemd so the sd_notify stuff doesn't work. Sure I could build my own Docker image but I'd rather just get it into this repo if that is OK. Would any committers accept a PR to remove --no-optional?

I should say that my use case here is that I'm trying to run this (and mosquitto which also doesn't have it enabled in their Docker images) in Podman via systemd and whenever I configured it wrong not having sd_notify resulted in less than ergonomic debugging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants