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
  1. 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'
    
  2. maflcko added the label Scripts and tools on Sep 2, 2022
  3. jamesob commented at 4:24 pm on September 6, 2022: contributor
    Concept ACK
  4. luke-jr commented at 2:35 am on September 9, 2022: member
    Related: #22372
  5. 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.

  6. dongcarl commented at 7:03 pm on November 22, 2022: contributor
    Anything else needed here?
  7. 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?

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

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

  11. achow101 closed this on Apr 25, 2023

  12. 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!
  13. 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.

  14. maflcko reopened this on May 2, 2023

  15. 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
  16. dongcarl force-pushed on May 28, 2023
  17. dongcarl commented at 5:13 pm on May 28, 2023: contributor

    Pushed 689a65d878:

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

  19. real-or-random approved
  20. real-or-random commented at 8:40 am on May 29, 2023: contributor
    ACK 689a65d878638ae67b07f111dd77ea3c624e4f58 tested this service file with 25.0
  21. fanquake merged this on May 29, 2023
  22. fanquake closed this on May 29, 2023

  23. sidhujag referenced this in commit d6b4972cbc on May 29, 2023
  24. 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).

  25. archlinux-github referenced this in commit f6c5f39c33 on Dec 24, 2023
  26. bitcoin locked this on Oct 19, 2024

github-metadata-mirror

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

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me