contrib/init: Better systemd integration #25975

dongcarl · 2022-09-01T17:50:25Z

1. Make logs available to journalctl (systemd's logging system) by not
   specifying -daemonwait, which rightfully has its own set of stdout
   and stderr descriptors (a user invoking with -daemonwait on the
   command line should not see any logs). It makes more sense not to
   daemonize in the systemd context anyway.

2. Make systemd aware of when bitcoind is started and in steady state by
   specifying -startupnotify='systemd-notify --ready' and Type=notify.
   NotifyAccess=all is necessary so that the spawned thread for
   startupnotify is allowed to inform systemd of bitcoind's readiness.

   Note that NotifyAccess=exec won't work because it only allows
   sd_notify readiness signalling from Exec*= declarations in the
   .service file.

Note that we currently don't allow multiple startupnotify commands, but
users can override it in systemd via:

  # systemctl edit bitcoind

By specifying something like:

  [Service]
  ExecStart=/usr/bin/bitcoind -pid=/run/bitcoind/bitcoind.pid \
                              -conf=/etc/bitcoin/bitcoin.conf \
                              -datadir=/var/lib/bitcoind \
                              -startupnotify='systemd-notify --ready; mycommandhere'

jamesob · 2022-09-06T16:24:17Z

Concept ACK

luke-jr · 2022-09-09T02:35:40Z

Related: #22372

DrahtBot · 2022-09-23T04:26:53Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	real-or-random
Concept ACK	jamesob, fanquake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

dongcarl · 2022-11-22T19:03:20Z

Anything else needed here?

fanquake · 2022-11-23T12:32:04Z

Anything else needed here?

Someone to actually sanity-check / test / review / leave an ACK? I don't use systemd, so can't test, and don't generally know if this is an improvement or not.

Looks like it at least partially reverts #21418, which was done in combination with #21007; where Type=forking was chosen specifically over Type=notify:

An alternative would be to implement support for Type=notify and do readyness notification through sd_notify. But I opted for daemonwait because it seems more generally useful.

Does this change break / compormise that behaviour?

kristapsk · 2022-11-23T12:56:26Z

Will try to look at these proposed changes with my RaspiBolt, which uses systemd (although with custom systemd unit file, but it is similar).

dongcarl · 2022-12-12T17:35:52Z

Does this change break / compormise that behaviour?

I think what laanwj was saying there was that if we had integrated libsystemd and called sd_notify ourselves, then we'd have startup notification logic that was only specific to systemd which is less than ideal since it's an extra dependency and we (at least strive to) support other init systems.

So that's why we started using -daemonwait, which is a somewhat portable readiness notification method for almost all init systems.

However, IIRC, in using -daemonwait, the forked/exec'd (I'm probably using this terminology wrong) bitcoind process will have a different set of stdout/stderr than the original process, which means that the logs that it prints will be discarded.

With the introduction of -startupnotify, however, we can run bitcoind without forking/exec-ing, and instead invoke systemd-notify --ready (basically just a CLI wrapper around libsystemd that all systemd systems have) to signal readiness. This means that the logs in stdout/stderr will flow to journald as expected on systemd systems.

Sidenote: There's currently no way I can find to tell systemd: My spawned process will append logs to /var/lib/bitcoind/debug.log so have journald watch that file for logs.

Sidenote 2: While it doesn't make sense to modify -daemonwait such that the child process inherits the parent's stdout/stderr (those invoking -daemonwait on the CLI expect the process to be fully detached), it might make sense to add a -daemonwaitinherit or something like that, but I think that might be an unwanted bloat of the CLI interface and that -startupnotify is good enough here.

achow101 · 2023-04-25T15:43:18Z

The feature request didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.

Closing due to lack of interest. Pull requests with improvements are always welcome.

real-or-random

ACK mod nits

This is very useful for systemd users. The changes match systemd docs and I tested that process management and logging works as intended.

@achow101 Can you reopen this?

One minor issue: This means what (with default config), we have log output both in systemd's journal and in the log file. In principle, it may make sense to also pass -nodebuglogfile, but I'm not sure if this is really what we want because it means that the user can't override this option in the config file anymore... Maybe a Changelog entry should be added, recommending systemd users to consider disabling writing logs to a file.

A more sophisticated solution would be to add a --systemd-notify command line option, that sets modifies the defaults of startupnotify, shutdownnotify, and debuglogfile (or even better avoids the problem that users who want to use further startup commands need to modify the systemd unit file). But I'm not convinced that adding this option is worth the hassle. It's probably good to have all systemd-specific code just in this unit file here. Anyway, this PR is clearly an improvement for systemd users, so even if --systemd-notify is a good idea for the future, it should not hold up this PR.

contrib/init/bitcoind.service

1. Make logs available to journalctl (systemd's logging system) by not specifying -daemonwait, which rightfully has its own set of stdout and stderr descriptors (a user invoking with -daemonwait on the command line should not see any logs). It makes more sense not to daemonize in the systemd context anyway. 2. Make systemd aware of when bitcoind is started and in steady state by specifying -startupnotify='systemd-notify --ready' and Type=notify. NotifyAccess=all is necessary so that the spawned thread for startupnotify is allowed to inform systemd of bitcoind's readiness. Note that NotifyAccess=exec won't work because it only allows sd_notify readiness signalling from Exec*= declarations in the .service file. 3. Also make systemd aware of when bitcoind is stopping by specifying -shutdownnotify='systemd-notify --stopping' Note that we currently don't allow multiple *notify commands, but users can override it in systemd via: # systemctl edit bitcoind By specifying something like: [Service] ExecStart=/usr/bin/bitcoind -pid=/run/bitcoind/bitcoind.pid \ -conf=/etc/bitcoin/bitcoin.conf \ -datadir=/var/lib/bitcoind \ -startupnotify='systemd-notify --ready; mystartupcommandhere' \ -shutdownnotify='systemd-notify --stopping; myshutdowncommandhere'

dongcarl · 2023-05-28T17:13:43Z

Pushed 689a65d:

Addressed: contrib/init: Better systemd integration #25975 (comment)

dongcarl · 2023-05-28T17:18:03Z

I think whether or not to to add a -nodebuglogfile is somewhat tangential to the systemd integration and more related to whether or not -daemonwait is specified or even more specifically whether or not we're writing logs to stdout+stderr.

As in: Does a user of bitcoind that doesn't specify -daemonwait and expects logs on stdout+stderr also expect the debug log file to be written to or no?

In any case, I think we can leave that discussion for another time.

real-or-random

ACK 689a65d tested this service file with 25.0

Sjors · 2023-06-16T14:16:39Z

This is great. Can you (or someone else) make a release note? ~~Otherwise I'm probably to forget to use these changes myself on the next release :-)~~ I'll go ahead and try them now.

I certainly prefer to keep using my debug.log file. I generally only use journalctl if there's a problem starting a service (or to see if indeed got the right version after an update).

Remove .service file patch since it's now in the release See: bitcoin/bitcoin#25975

maflcko added the Scripts and tools label Sep 2, 2022

kristapsk mentioned this pull request Nov 23, 2022

Support multiple -*notify commands #22372

Closed

dongcarl mentioned this pull request Mar 20, 2023

"Unable to parse settings.json" at startup after disk was full #21340

Closed

achow101 closed this Apr 25, 2023

real-or-random reviewed May 2, 2023

View reviewed changes

contrib/init/bitcoind.service Outdated Show resolved Hide resolved

maflcko reopened this May 2, 2023

dongcarl force-pushed the 2022-09-systemd-improve branch from 42bcabb to 689a65d Compare May 28, 2023 17:13

real-or-random approved these changes May 29, 2023

View reviewed changes

fanquake merged commit 769dd1e into bitcoin:master May 29, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 29, 2023

Merge bitcoin#25975: contrib/init: Better systemd integration

d6b4972

archlinux-github pushed a commit to archlinux/aur that referenced this pull request Dec 24, 2023

version: 26.0-1

f6c5f39

Remove .service file patch since it's now in the release See: bitcoin/bitcoin#25975

bitcoin locked and limited conversation to collaborators Oct 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

contrib/init: Better systemd integration #25975

contrib/init: Better systemd integration #25975

Uh oh!

dongcarl commented Sep 1, 2022

Uh oh!

jamesob commented Sep 6, 2022

Uh oh!

luke-jr commented Sep 9, 2022

Uh oh!

DrahtBot commented Sep 23, 2022 •

edited

Loading

Uh oh!

dongcarl commented Nov 22, 2022

Uh oh!

fanquake commented Nov 23, 2022

Uh oh!

kristapsk commented Nov 23, 2022

Uh oh!

dongcarl commented Dec 12, 2022 •

edited

Loading

Uh oh!

achow101 commented Apr 25, 2023

Uh oh!

real-or-random left a comment

Uh oh!

Uh oh!

dongcarl commented May 28, 2023

Uh oh!

dongcarl commented May 28, 2023

Uh oh!

real-or-random left a comment

Uh oh!

Sjors commented Jun 16, 2023 •

edited

Loading

Uh oh!

Uh oh!

contrib/init: Better systemd integration #25975

contrib/init: Better systemd integration #25975

Uh oh!

Conversation

dongcarl commented Sep 1, 2022

Uh oh!

jamesob commented Sep 6, 2022

Uh oh!

luke-jr commented Sep 9, 2022

Uh oh!

DrahtBot commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

dongcarl commented Nov 22, 2022

Uh oh!

fanquake commented Nov 23, 2022

Uh oh!

kristapsk commented Nov 23, 2022

Uh oh!

dongcarl commented Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Apr 25, 2023

Uh oh!

real-or-random left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongcarl commented May 28, 2023

Uh oh!

dongcarl commented May 28, 2023

Uh oh!

real-or-random left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Sep 23, 2022 •

edited

Loading

dongcarl commented Dec 12, 2022 •

edited

Loading

Sjors commented Jun 16, 2023 •

edited

Loading