contrib/init: Better systemd integration

dongcarl commented at 5:50 pm on September 1, 2022: contributor

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

maflcko added the label Scripts and tools on Sep 2, 2022

jamesob commented at 4:24 pm on September 6, 2022: member

Concept ACK

luke-jr commented at 2:35 am on September 9, 2022: member

Related: #22372

DrahtBot commented at 4:26 am on September 23, 2022: contributor

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 commented at 7:03 pm on November 22, 2022: contributor

Anything else needed here?

fanquake commented at 12:32 pm on November 23, 2022: member

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 commented at 12:56 pm on November 23, 2022: contributor

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 commented at 5:35 pm on December 12, 2022: contributor

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 commented at 3:43 pm on April 25, 2023: member

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.

achow101 closed this on Apr 25, 2023

in contrib/init/bitcoind.service:24 in 42bcabbd99 outdated

22-                            -pid=/run/bitcoind/bitcoind.pid \
23+ExecStart=/usr/bin/bitcoind -pid=/run/bitcoind/bitcoind.pid \
24                             -conf=/etc/bitcoin/bitcoin.conf \
25-                            -datadir=/var/lib/bitcoind
26+                            -datadir=/var/lib/bitcoind \
27+                            -startupnotify='systemd-notify --ready'

real-or-random commented at 10:31 am on May 2, 2023:

0                            -startupnotify='systemd-notify --ready' \ 
1                            -shutdownnotify='systemd-notify --stopping'

Shutdown notifications are just “nice to have” and I don’t think anyone will rely on this, but now that we have the command-line option, why not use it here.

dongcarl commented at 5:13 pm on May 28, 2023:

Incorporated, thanks!

real-or-random commented at 10:41 am on May 2, 2023: contributor

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.

maflcko reopened this on May 2, 2023

contrib/init: Better systemd integration

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'

689a65d878

dongcarl force-pushed on May 28, 2023

dongcarl commented at 5:13 pm on May 28, 2023: contributor

Pushed 689a65d878:

Addressed: #25975 (review)

dongcarl commented at 5:18 pm on May 28, 2023: contributor

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 approved

real-or-random commented at 8:40 am on May 29, 2023: contributor

ACK 689a65d878638ae67b07f111dd77ea3c624e4f58 tested this service file with 25.0

fanquake merged this on May 29, 2023

fanquake closed this on May 29, 2023

sidhujag referenced this in commit d6b4972cbc on May 29, 2023

Sjors commented at 2:16 pm on June 16, 2023: member

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

contrib/init: Better systemd integration #25975

Reviews

Conflicts