pid1: add new Type=notify-reload service type for a service reload protocol based on SIGHUP #25916

poettering · 2023-01-02T18:09:23Z

Fixes: #18484 #6162

poettering · 2023-01-02T18:09:44Z

(i guess still needs an integration test)

poettering · 2023-01-03T14:46:09Z

Test added. Should be ready for review.

test/units/testsuite-59.sh

src/core/service.c

bluca · 2023-01-10T14:01:00Z

bit of bikeshedding, but maybe it would be nicer from the usability point of view to have a separate setting, rather than a different Type=? Say we want to add one more protocol next year, then we'd have Type=notify-reload-newproto and so on

poettering · 2023-01-10T14:08:34Z

bit of bikeshedding, but maybe it would be nicer from the usability point of view to have a separate setting, rather than a different Type=? Say we want to add one more protocol next year, then we'd have Type=notify-reload-newproto and so on

the last 10y or so we didn't extend Type=notify in such a major way that we introduced a new type. i don't really see anything coming up for the next 10y in that regard. Hence, if history tells us anything I think it's OK to expose this as type.

Note that the many many extensions we did to sd_notify() we always did without intrdoucing a new type (watchdog, fdstore, barrier, …), and I am pretty sure for 99% of future additions this is going to be the same. Just this extension is one of the few that actually touches the state machine of the service, hence it's a bit more involved. Given that with this change the whole state machine of the service is basically exposed via sd_notify() i really can't see anything else coming...

Fixes: systemd#6162

…reloading And send READY=1 again when we are done with it. We do this not only for "daemon-reload" but also for "daemon-reexec" and "switch-root", since from the perspective of an encapsulating service manager these three operations are not that different.

We are basically already there, just need to add MONOTONIC_USEC= to the RELOADING=1 message, and make sure the message is generated in really all cases.

So close already. Let's add the two missing notifications too. Fixes: systemd#18484

These wrap RELOADING=1 and STOPPING=1 messages. The former is particularly useful, since we want to insert the MONOTONIC_USEC= field into the message automatically, which is easy from C but harder from shell.

yuwata · 2023-01-12T02:12:00Z

@poettering Sorry for late comment. Let me confirm one point. Why the new type notify-reload is necessary, instead of silently extending Type=notify? I guess if we reuse notify type then that breaks backward compatibility. But I'd like to know in what scenario the difference between two types are significant. This is a pure question, and does not mean I dislike the new type.

Werkov · 2023-01-12T10:36:11Z

I'm also wondering what warrants the necessity of the new type.

Just this extension is one of the few that actually touches the state machine of the service, hence it's a bit more involved. Given that with this change the whole state machine of the service is basically exposed via sd_notify() i really can't see anything else coming...

Is this state machine change somewhat relevant to service authors? IIUC, reload behavior could have been buggy when a spontaneous (by the daemon) and PID1-initiated reload coincide.
(Since this implicitly requires a richer protocol between PID1 and daemon, I dismiss all but Type=notify.) Good citizen daemons already issue RELOADING=1 and READY=1. If the problems is matching READY=1 with its reload-request, then daemons could rectify that by sending MONOTONIC_USEC= (and PID1 would track state properly even for Type=notify service).

IOW, even with the new type introduced, the old type would still have problems that could be solved with an optional MONOTONIC_USEC= message.

poettering · 2023-01-16T09:53:48Z

@poettering Sorry for late comment. Let me confirm one point. Why the new type notify-reload is necessary, instead of silently extending Type=notify?

We need to know if services implement reload or not, so that we can implement things such as "systemctl reload-or-restart" properly. Type=notify means no reload is implemented (or implemented via ExecReload=), while Type=notify-reload indicates that reload is implemented.

I guess if we reuse notify type then that breaks backward compatibility. But I'd like to know in what scenario the difference between two types are significant. This is a pure question, and does not mean I dislike the new type.

Note that most of our own service don#t actually support reloading (they probably should be), hence you'll actually see more Type=notify than Type=notify-reload if you grep through units/

poettering · 2023-01-16T09:59:00Z

Is this state machine change somewhat relevant to service authors? IIUC, reload behavior could have been buggy when a spontaneous (by the daemon) and PID1-initiated reload coincide. (Since this implicitly requires a richer protocol between PID1 and daemon, I dismiss all but Type=notify.) Good citizen daemons already issue RELOADING=1 and READY=1. If the problems is matching READY=1 with its reload-request, then daemons could rectify that by sending MONOTONIC_USEC= (and PID1 would track state properly even for Type=notify service).

Well, service authors can drop ExecReload= if they use Type=notify-reload.

This is for example interesting for delegated services, because it means pid1 won't ever have to fork anything into the delegated cgroup, just to do a reload.

And yeah, services that already implement RELOADING=1 + READY=1 can wit minimal effort extend their stuff to be compatible with Type=notify-reload, they just have to add MONOTONIC_USEC=.

If an externally triggered reload and one by PID1 happen at the same time this should be entirely fine, they can be coalesced. what pid 1 cares about is one RELOADING=1 with MONOTONIC_USEC= being sent, with a timestamp beyond the timestamp it requested the reload.

IOW, even with the new type introduced, the old type would still have problems that could be solved with an optional MONOTONIC_USEC= message.

I don't grok this comment?

Werkov · 2023-01-16T17:27:37Z

I don't grok this comment?

Suppose a service like

Type=notify
ExecReload=kill -SIGHUP $MAINPID

The service follows the protocol with RELOADING=1, READY=1.

There can be a race:

via signal              spontaneous reload
                        RELOADING=1
RELOADING=1             
                        READY=1
READY=1

PID wouldn't know which message is associated with its reload request and which
is just from the daemon.

The fix would be for the service to attach MONOTONIC_USEC= to the READY=
message. (Type=notify could remain.)

I.e. the new

Type=notify-reload

could be equivalent to

Type=notify
ReloadSignal=SIGHUP

And BC default would be

Type=notify
ReloadSignal=

(relying on a possibly unsynced ExecReload=).

poettering · 2023-01-17T11:41:42Z

And yes, MONOTONIC_USEC= I added to deal with the race you describe, if that was the question? otherwise we wouldn't need it.

Type=notify + ReloadSignal=SIGHUP would certainly be another way to encode this mechanism. It's a bit of a bikeshedding question how to expose this best. My thinking was that I want to default to SIGHUP for this, so that people don't ever get the idea it was a good idea to pick anything else. But if we require people to explicitly spell down SIGHUP then they might get bad ideas... By Type=notify-reload we dictate a protocol and a default signal, and if you then want to fuck around with the signal used, you need to jump through extra hoops to do so by also adding ReloadSignal=, which is nice.

Werkov · 2023-01-17T14:53:04Z

I remember 5686391 that was 5 years ago, so by your reasoning, budget for the next new Type= should only be in 2028 :-)

Encoding same-but-modified behavior just within the Type= directive doesn't look normal.
Type= is how is lifecycle defined.
Optional extensions on top of that with independent directives are more conceptual (similar to what we have with Exec*{Post,Pre}= etc.).
Re: bad ideas from spotting SIGHUP, if they just write Type=notify-reload

unit file author: expecting reload functionality, blindly relying on given daemon handling SIGHUP properly (but terminating by default),
daemon author: getting equally bad idea by implementing the singal handler for a (different) signal.

Ad bike-shedding -- if this weren't just a comment but a PR ¹, would it be better?

Where also I could more easily find possible loopholes in my argument. ↩

commit f74b856 added an ExecReload to the systemd unit, which allows users to signal the daemon to reload its config through systemd (`systemctl reload docker.service`). While reloading works, systemd expects the `ExecReload` command to be synchronous, so that it knows when the reload completes, and can account for this when managing dependent services. > Note however that reloading a daemon by enqueuing a signal (...) is usually > not a good choice, because this is an asynchronous operation and hence not > suitable when ordering reloads of multiple services against each other. Systemd 253 introduced a new Type (Type=notify-reload, see [systemd#25916]), which allows setting a "ReloadSignal" instead, in addition to sending a [MONOTONIC_USEC]. We currently still support distros that do not provide systemd 253, so cannot use this new feature, but sending "RELOADING=1 / "READY=1" should at least provide more information to systemd for the time being. This patch: - adds reload notifications to the daemon to notify when the daemon got signaled to reload its configuration see [sd_notify(3)]. - adds a [notifyReady] to notify when the reload finished, which we currently send regardless if the reload was successful or failed (which could be due to an invalid config). We can re-implement this using `Type=notify-reload` once we no longer have to support systemd versions before 253. [sd_notify(3)]: https://www.freedesktop.org/software/systemd/man/latest/sd_notify.html#RELOADING=1 [systemd.service(5)]: https://www.freedesktop.org/software/systemd/man/latest/systemd.service.html#Type= [ExecReload]: https://www.freedesktop.org/software/systemd/man/latest/systemd.service.html#ExecReload= [MONOTONIC_USEC]: https://www.freedesktop.org/software/systemd/man/latest/sd_notify.html#MONOTONIC_USEC=… [systemd#25916]: systemd/systemd#25916 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

@sbindir

As per the `sd_notify` manual: > A field carrying the monotonic timestamp (as per CLOCK_MONOTONIC) formatted > in decimal in μs, when the notification message was generated by the client. > This is typically used in combination with "RELOADING=1", to allow the > service manager to properly synchronize reload cycles. See systemd.service(5) > for details, specifically "Type=notify-reload". Thus this change allows users with a recent systemd to switch to `Type=notify-reload`, should they desire to do so. Correct behavior was verified with a Fedora 39 VM. see systemd/systemd#25916 [wla: the service file should be updated this way:] diff --git a/admin/systemd/haproxy.service.in b/admin/systemd/haproxy.service.in index 22a53d8..8c6dadb5e5 100644 --- a/admin/systemd/haproxy.service.in +++ b/admin/systemd/haproxy.service.in @@ -8,12 +8,11 @@ EnvironmentFile=-/etc/default/haproxy EnvironmentFile=-/etc/sysconfig/haproxy Environment="CONFIG=/etc/haproxy/haproxy.cfg" "PIDFILE=/run/haproxy.pid" "EXTRAOPTS=-S /run/haproxy-master.sock" ExecStart=@sbindir@/haproxy -Ws -f $CONFIG -p $PIDFILE $EXTRAOPTS -ExecReload=@sbindir@/haproxy -Ws -f $CONFIG -c $EXTRAOPTS -ExecReload=/bin/kill -USR2 $MAINPID KillMode=mixed Restart=always SuccessExitStatus=143 -Type=notify +Type=notify-reload +ReloadSignal=SIGUSR2 # The following lines leverage SystemD's sandboxing options to provide # defense in depth protection at the expense of restricting some flexibility Signed-off-by: William Lallemand <wlallemand@haproxy.com>

commit f74b856 added an ExecReload to the systemd unit, which allows users to signal the daemon to reload its config through systemd (`systemctl reload docker.service`). While reloading works, systemd expects the `ExecReload` command to be synchronous, so that it knows when the reload completes, and can account for this when managing dependent services. > Note however that reloading a daemon by enqueuing a signal (...) is usually > not a good choice, because this is an asynchronous operation and hence not > suitable when ordering reloads of multiple services against each other. Systemd 253 introduced a new Type (Type=notify-reload, see [systemd#25916]), which allows setting a "ReloadSignal" instead, in addition to sending a [MONOTONIC_USEC]. We currently still support distros that do not provide systemd 253, so cannot use this new feature, but sending "RELOADING=1 / "READY=1" should at least provide more information to systemd for the time being. This patch: - adds reload notifications to the daemon to notify when the daemon got signaled to reload its configuration see [sd_notify(3)]. - adds a [notifyReady] to notify when the reload finished, which we currently send regardless if the reload was successful or failed (which could be due to an invalid config). We can re-implement this using `Type=notify-reload` once we no longer have to support systemd versions before 253. [sd_notify(3)]: https://www.freedesktop.org/software/systemd/man/latest/sd_notify.html#RELOADING=1 [systemd.service(5)]: https://www.freedesktop.org/software/systemd/man/latest/systemd.service.html#Type= [ExecReload]: https://www.freedesktop.org/software/systemd/man/latest/systemd.service.html#ExecReload= [MONOTONIC_USEC]: https://www.freedesktop.org/software/systemd/man/latest/sd_notify.html#MONOTONIC_USEC=… [systemd#25916]: systemd/systemd#25916 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

poettering added udev network login documentation pid1 units labels Jan 2, 2023

github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Jan 2, 2023

poettering added the new-feature label Jan 2, 2023

poettering force-pushed the reload-notify branch from 54a87be to 6441ff8 Compare January 3, 2023 14:45

poettering added the notify label Jan 3, 2023

github-advanced-security bot found potential problems Jan 3, 2023

View reviewed changes

test/units/testsuite-59.sh Fixed Show fixed Hide fixed

poettering force-pushed the reload-notify branch 2 times, most recently from a521995 to e5748e5 Compare January 6, 2023 11:21

poettering added this to the v253 milestone Jan 6, 2023

poettering force-pushed the reload-notify branch 2 times, most recently from 190add2 to 6f8d880 Compare January 9, 2023 08:49

poettering mentioned this pull request Jan 9, 2023

RFC Pre-create cgroup subtree of services with enabled Delegate= #24961

Closed

bluca reviewed Jan 10, 2023

View reviewed changes

src/core/service.c Show resolved Hide resolved

bluca approved these changes Jan 10, 2023

View reviewed changes

bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels Jan 10, 2023

poettering force-pushed the reload-notify branch from 6f8d880 to 361cd3a Compare January 10, 2023 14:41

poettering added 4 commits January 10, 2023 18:28

manager: add one more assert()

7fa4928

pid1: add new Type=notify-reload service type

3bd28bf

Fixes: systemd#6162

man: document Type=notify-reload

81e19b6

poettering added 6 commits January 10, 2023 18:28

networkd: implement Type=notify-reload protocol

0e07cdb

udevd: implement the full Type=notify-reload protocol

f843315

We are basically already there, just need to add MONOTONIC_USEC= to the RELOADING=1 message, and make sure the message is generated in really all cases.

logind: implement Type=notify-reload protocol properly

5d71e46

So close already. Let's add the two missing notifications too. Fixes: systemd#18484

notify: add --stopping + --reloading switches

fd0f4da

These wrap RELOADING=1 and STOPPING=1 messages. The former is particularly useful, since we want to insert the MONOTONIC_USEC= field into the message automatically, which is easy from C but harder from shell.

test: add Type=notify-reload testcase

ee52bbc

update TODO

6fee784

poettering force-pushed the reload-notify branch from 361cd3a to 6fee784 Compare January 10, 2023 17:28

poettering merged commit 833504f into systemd:main Jan 11, 2023

jimklimov mentioned this pull request Apr 17, 2023

Driver reload - basic support networkupstools/nut#1906

Merged

kwilczynski mentioned this pull request Oct 25, 2023

GracefulNodeShutdown tests failing due to connection with dbus kubernetes/kubernetes#121124

Closed

thaJeztah mentioned this pull request Feb 8, 2024

Add sd_notify "reload" notifications to assist with systemctl reload moby/moby#47353

Open

thaJeztah mentioned this pull request Feb 8, 2024

cmd/dockerd: add basic support for sd_notify "reload" notifications moby/moby#47358

Draft

kannon92 mentioned this pull request Mar 20, 2024

e2e_node: remove Dbus test case kubernetes/kubernetes#124009

Merged

Kranzes mentioned this pull request Apr 3, 2025

Remove systemd notify-reload kanidm/kanidm#3541

Closed

3 tasks

arianvp mentioned this pull request Apr 3, 2025

implement notify-reload protocol correctly kanidm/kanidm#3540

Merged

3 tasks

bluca mentioned this pull request May 6, 2025

core: Also refresh confext extensions when reloading notify-reload service #33995

Merged

Uh oh!

pid1: add new Type=notify-reload service type for a service reload protocol based on SIGHUP #25916

pid1: add new Type=notify-reload service type for a service reload protocol based on SIGHUP #25916

Uh oh!

Conversation

poettering commented Jan 2, 2023

Uh oh!

poettering commented Jan 2, 2023

Uh oh!

poettering commented Jan 3, 2023

Uh oh!

Uh oh!

Uh oh!

bluca commented Jan 10, 2023

Uh oh!

poettering commented Jan 10, 2023

Uh oh!

yuwata commented Jan 12, 2023

Uh oh!

Werkov commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poettering commented Jan 16, 2023

Uh oh!

poettering commented Jan 16, 2023

Uh oh!

Werkov commented Jan 16, 2023

Uh oh!

poettering commented Jan 17, 2023

Uh oh!

Werkov commented Jan 17, 2023

Footnotes

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Werkov commented Jan 12, 2023 •

edited

Loading