Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve PID file error handling #15278

Merged
merged 3 commits into from Feb 21, 2019

Conversation

Projects
None yet
5 participants
@hebasto
Copy link
Member

commented Jan 28, 2019

Digging into #15240 the lack of the proper logging has been discovered.
Fixed by this PR.

UPDATE (inspired by @laanwj's comment):
Not being able to create the PID file is fatal now.

Output of bitcoind:

$ src/bitcoind -pid=/run/bitcoind/bitcoind.pid
2019-02-01T23:20:10Z Bitcoin Core version v0.17.99.0-561e375c7 (release build)
2019-02-01T23:20:10Z Assuming ancestors of block 0000000000000037a8cd3e06cd5edbfe9dd1dbcc5dacab279376ef7cfc2b4c75 have valid signatures.
2019-02-01T23:20:10Z Setting nMinimumChainWork=00000000000000000000000000000000000000000000007dbe94253893cbd463
2019-02-01T23:20:10Z Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation
2019-02-01T23:20:10Z Using RdRand as an additional entropy source
2019-02-01T23:20:11Z Error: Unable to create the PID file '/run/bitcoind/bitcoind.pid': No such file or directory
Error: Unable to create the PID file '/run/bitcoind/bitcoind.pid': No such file or directory
2019-02-01T23:20:11Z Shutdown: In progress...
2019-02-01T23:20:11Z Shutdown: Unable to remove PID file: File does not exist
2019-02-01T23:20:11Z Shutdown: done

Output of bitcoin-qt:
screenshot from 2019-02-02 01-19-05

Notes for reviewers

  1. CreatePidFile() has been moved from util/system.cpp to init.cpp for the following reasons:
  • to get the ability to use InitError()
  • now init.cpp contains code of both creating PID file and removing it
  1. Regarding 0.18 release process: this PR modifies 1 string and introduces 2 new ones.
@@ -967,10 +968,11 @@ fs::path GetPidFile()
void CreatePidFile(const fs::path &path, pid_t pid)

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 31, 2019

Member

I don't quite understand why this doesn't return an error status, and that not being able to create the PID file is not fatal.

This comment has been minimized.

Copy link
@hebasto

hebasto Feb 1, 2019

Author Member

Fixed.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

Concept ACK, thanks for adding more relevant logging for diagnostics.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

Concept ACK. I suggest adding the NODISCARD annotations to fs::remove and CreatePidFile (after making CreatePidFile return an error status as suggested by @laanwj).

@hebasto hebasto force-pushed the hebasto:20190128-pidfile-logging branch from f5fb1bc to 8b4c5ba Feb 1, 2019

@hebasto hebasto force-pushed the hebasto:20190128-pidfile-logging branch from 8b4c5ba to 561e375 Feb 1, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15454 (Remove the automatic creation and loading of the default wallet by achow101)
  • #14776 (Add process based prctl spectre mitigation controls. by jameshilliard)
  • #14045 (refactor: Fix the chainparamsbase -> util circular dependency by Empact)
  • #13746 (-masterdatadir for datadir bootstrapping by kallewoof)
  • #12833 ([qt] move QSettings to bitcoin_rw.conf where possible by Sjors)
  • #11082 (Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr)

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.

@hebasto hebasto changed the title Improve pidfile logging Improve PID file error handling Feb 1, 2019

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

@laanwj @practicalswift thank you for your reviews.
Your comments have been addressed. PR description updated.

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

utACK 561e375
Thanks for fixing this

@@ -1190,12 +1194,29 @@ bool AppInitLockDataDirectory()
return true;
}

#ifndef WIN32
NODISCARD static bool CreatePidFile()

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 13, 2019

Member

If you're moving this function anyway, why not move GetPidFile as well, it's only used from init.cpp too

This comment has been minimized.

Copy link
@hebasto

hebasto Feb 14, 2019

Author Member

Fixed.

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Tested:

  • Removing the PId file while running, this results in the following warning in the log:
2019-02-13T22:07:02Z Shutdown: Unable to remove PID file: File does not exist
  • After making the pid file unaccessible by chowning it to root:
2019-02-13T22:08:36Z Error: Unable to create the PID file '/.../.bitcoin/regtest/bitcoind.pid': Permission denied
Error: Unable to create the PID file '/.../.bitcoin/regtest/bitcoind.pid': Permission deniez

Looks good to me.

Move all PID file stuff to init.cpp
It is only used from init.cpp.
Move-only refactoring.
@hebasto

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

@laanwj's #15278 (comment) has been addressed.

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

utACK 3782075

@laanwj laanwj merged commit 3782075 into bitcoin:master Feb 21, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Feb 21, 2019

Merge #15278: Improve PID file error handling
3782075 Move all PID file stuff to init.cpp (Hennadii Stepanov)
561e375 Make PID file creating errors fatal (Hennadii Stepanov)
745a2ac Improve PID file removing errors logging (Hennadii Stepanov)

Pull request description:

  Digging into #15240 the lack of the proper logging has been discovered.
  Fixed by this PR.

  UPDATE (inspired by @laanwj's [comment](#15278 (comment))):
  Not being able to create the PID file is fatal now.

  Output of `bitcoind`:

  ```
  $ src/bitcoind -pid=/run/bitcoind/bitcoind.pid
  2019-02-01T23:20:10Z Bitcoin Core version v0.17.99.0-561e375c7 (release build)
  2019-02-01T23:20:10Z Assuming ancestors of block 0000000000000037a8cd3e06cd5edbfe9dd1dbcc5dacab279376ef7cfc2b4c75 have valid signatures.
  2019-02-01T23:20:10Z Setting nMinimumChainWork=00000000000000000000000000000000000000000000007dbe94253893cbd463
  2019-02-01T23:20:10Z Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation
  2019-02-01T23:20:10Z Using RdRand as an additional entropy source
  2019-02-01T23:20:11Z Error: Unable to create the PID file '/run/bitcoind/bitcoind.pid': No such file or directory
  Error: Unable to create the PID file '/run/bitcoind/bitcoind.pid': No such file or directory
  2019-02-01T23:20:11Z Shutdown: In progress...
  2019-02-01T23:20:11Z Shutdown: Unable to remove PID file: File does not exist
  2019-02-01T23:20:11Z Shutdown: done
  ```

  Output of `bitcoin-qt`:
  ![screenshot from 2019-02-02 01-19-05](https://user-images.githubusercontent.com/32963518/52154886-9349b600-2688-11e9-8128-470f16790305.png)

  **Notes for reviewers**
  1. `CreatePidFile()` has been moved from `util/system.cpp` to `init.cpp` for the following reasons:
  - to get the ability to use `InitError()`
  - now `init.cpp` contains code of both creating PID file and removing it

  2. Regarding 0.18 release process: this PR modifies 1 string and introduces 2 new ones.

Tree-SHA512: ac07d0f800e61ec759e427d0afc0ca43d67f232e977662253963afdd0a220d34b871050f58149fc9fabd427bfc8e0d3f6a6032f2a38f30ad366fc0d074b0f2b3

@hebasto hebasto deleted the hebasto:20190128-pidfile-logging branch Feb 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.