feature: Added ability for users to add a startup command #15367
Conversation
412ef2a to
b58073e
Compare
|
I don't think this should be the scope of Bitcoin Core. Service dependencies should be handled by the init / service management system you are using. Or you can just create a simple shell script, to run all the stuff you need. Besides, it is usually recommended to run different services under different users, if possible, to increase security. |
|
@kristapsk Startup services can't trigger when bitcoin is ready to be actually used. That is the big reason for a notify here. Yes, as an alternative software could just poll until its ready but the same could be said for blocknotifys. Historically speaking-- startup scripts people have created for bitcoin have been uniformly low quality (e.g. making the software unusable providing no way to get access to the rpc cookie, killing the process and corrupting its state on shutdown, etc). I have generally recommended against using these package scripts due to their low quality, so I think it would be asking a lot of them to expect them to start intelligently starting other services when so far not corrupting the Bitcoin install has been a challenge for them. |
|
I understand the need, I'm not fond of the solution. I think it's perfectly sane to poll the interface or locally run something like netstat. Does anyone knows an example using this strategy? |
Huh. Netstat can not tell you if bitcoind is done starting. If you connect as soon as the socket is listening you'll get "bitcoin is starting" errors returned. The gain here over simply retrying is that you can start connecting only when it's actually ready. (And yes, true you could just sit in a retry loop, but you could also poll for all the other notifys and that didn't stop us from adding them. :) ) |
Didn't thought about that. Yes, then it makes sense, concept ACK from me too. |
b58073e to
b515ccf
Compare
|
Moved function call to very end of init sequence |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Still not convinced about this. I think the general approach is to poll the interface of interest. Docker, for instance, supports a directive to check if a container is healthy https://docs.docker.com/v17.09/engine/reference/builder/#healthcheck See also https://docs.docker.com/compose/startup-order/ Anyway, what if you want to trigger some command when both bitcoind and other service are up and available? You would have to poll the second. Take your example, what if you want to run something else after Also, from the client point of view, I think it's a good practice to have retry mechanisms, no because of the startup, but because in runtime a service can be unavailable for some reason. |
|
Electrum (and most likely lnd as well) already have a retry mechanism for rpc and the rpc can be unavailable for other reasons as well so this doesn't add much functionality. Not specific to this pr but running random commands is not a good security practice and can lead to disasters like privilege escalation. Consider the scenario where someone out of hurry and not wanting to deal with file permissions puts his notifywhatever script in a world writable place and forgets about it only to have his wallet flushed some months later. |
While that is true the same could be done with the other notify options, which are ran much more often. |
b515ccf to
bfe463b
Compare
bfe463b to
4b6987c
Compare
4b6987c to
7981a11
Compare
128d9fe to
799865b
Compare
|
Concept ACK |
799865b to
2a247df
Compare
|
Rebased and added |
2a247df to
962d730
Compare
962d730 to
090530c
Compare
|
Incorporated @MarcoFalke's suggestions |
|
re-ACK 090530c @dongcarl or @kristapsk would be good to get another re-ACK or so, so that this can be merged into the upcoming major release. |
|
tACK 090530c |
|
Posthumous ACK |
|
Thank you all for working on this! Very helpful to me and came in pretty much exactly when needed. And thanks @dongcarl for pointers - didn't know about One more tip for other readers:
The problem was solved, I actually had overloaded system. |
This comment has been minimized.
This comment has been minimized.
When other initscripts depend on bitcoind, it's because their daemons want to be able to invoke bitcoin-cli or to communicate with bitcoind via RPC. That can't happen until some time after bitcoind has forked into the background. The -startupnotify option was added in 090530c (bitcoin#15367) to support exactly this use case, so let's use it. The ideal OpenRC mechanism of marking the service inactive at startup and later calling back into the start() function with IN_BACKGROUND=yes won't work here because bitcoind runs as an unprivileged user that is not allowed to call "rc-service ${SVCNAME} start", and OpenRC has no mechanism like systemd-notify, so instead we make start-stop-daemon lock a startup dummy file with flock(1) before exec'ing bitcoind, and then we use -startupnotify to release the lock when bitcoind is ready to service RPC requests. In the initscript's start_post() function we mark the service as inactive and fork a subshell process into the background to wait for bitcoind to release the lock file, which will happen when bitcoind either executes the -startupnotify command or dies. After acquiring and releasing the lock, the subshell checks whether the pid file still exists and marks the service as started or stopped appropriately.
When other initscripts depend on bitcoind, it's because their daemons want to be able to invoke bitcoin-cli or to communicate with bitcoind via RPC. That can't happen until some time after bitcoind has forked into the background. The -startupnotify option was added in 090530c (bitcoin#15367) to support exactly this use case, so let's use it. A straightforward and reliable way to wait for bitcoind to invoke its -startupnotify command is to make bitcoind hold a lock on a dummy file and release that lock via the -startupnotify command. The initscript's start_post() function initially marks the service as inactive and forks a subshell process into the background to wait for bitcoind to release the lock file, which will happen when bitcoind either executes the -startupnotify command or dies. After acquiring and releasing the lock, the subshell checks whether the pid file still exists and marks the service as started or stopped appropriately.
Summary: > Thoughts for adding the feature is for users to be able to add things like electrum-personal-server or lnd to run whenever Bitcoin Core is running. Open to feedback about the feature. This is a backport of [[bitcoin/bitcoin#15367 | core#15367]] Test Plan: `ninja && src/bitcoind -startupnotify="notify-send 'bitcoind started' 'test message'"` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10346
Summary: > Thoughts for adding the feature is for users to be able to add things like electrum-personal-server or lnd to run whenever Bitcoin Core is running. Open to feedback about the feature. This is a backport of [[bitcoin/bitcoin#15367 | core#15367]] Test Plan: `ninja && src/bitcoind -startupnotify="notify-send 'bitcoind started' 'test message'"` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10346
Thoughts for adding the feature is for users to be able to add things like electrum-personal-server or lnd to run whenever Bitcoin Core is running. Open to feedback about the feature.