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'
contrib/init: Better systemd integration #25975
pull dongcarl wants to merge 1 commits into bitcoin:master from dongcarl:2022-09-systemd-improve changing 1 files +7 −4-
dongcarl commented at 5:50 pm on September 1, 2022: contributor
-
maflcko added the label Scripts and tools on Sep 2, 2022
-
jamesob commented at 4:24 pm on September 6, 2022: contributorConcept ACK
-
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: contributorAnything 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 overType=notify
:Does this change break / compormise that behaviour?
-
kristapsk commented at 12:56 pm on November 23, 2022: contributorWill 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 calledsd_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 runbitcoind
without forking/exec-ing, and instead invokesystemd-notify --ready
(basically just a CLI wrapper aroundlibsystemd
that allsystemd
systems have) to signal readiness. This means that the logs in stdout/stderr will flow tojournald
as expected onsystemd
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 havejournald
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: contributorACK 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 ofstartupnotify
,shutdownnotify
, anddebuglogfile
(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'
dongcarl force-pushed on May 28, 2023dongcarl commented at 5:13 pm on May 28, 2023: contributorPushed 689a65d878:
- Addressed: #25975 (review)
dongcarl commented at 5:18 pm on May 28, 2023: contributorI think whether or not to to add a
-nodebuglogfile
is somewhat tangential to thesystemd
integration and more related to whether or not-daemonwait
is specified or even more specifically whether or not we’re writing logs tostdout
+stderr
.As in: Does a user of
bitcoind
that doesn’t specify-daemonwait
and expects logs onstdout
+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 approvedreal-or-random commented at 8:40 am on May 29, 2023: contributorACK 689a65d878638ae67b07f111dd77ea3c624e4f58 tested this service file with 25.0fanquake merged this on May 29, 2023fanquake closed this on May 29, 2023
sidhujag referenced this in commit d6b4972cbc on May 29, 2023Sjors commented at 2:16 pm on June 16, 2023: memberThis 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).
archlinux-github referenced this in commit f6c5f39c33 on Dec 24, 2023bitcoin locked this on Oct 19, 2024
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-21 15:12 UTC
More mirrored repositories can be found on mirror.b10c.me