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

fix crash when stopping/restarting with an invalid pidfile #440

Merged
merged 3 commits into from
Aug 24, 2017

Conversation

lpirl
Copy link
Contributor

@lpirl lpirl commented Aug 23, 2017

I use Tahoe as an vehicle for some fault injection experiments.
For a reason I am not interested in, a node ended up with an empty PID file in its directory (maybe due to a forced VM crash?).
When trying to stop or restart the node, it crashed since it did not handle non-numeric values in the PID file.
This PR attempts to fix this issue.
I decided to not remove the broken PID file, so operators can investigate if they wish to do so.

@lpirl
Copy link
Contributor Author

lpirl commented Aug 23, 2017

Oh by the way, a node with an invalid PID file does not start. Is this intended behavior?

@meejah
Copy link
Contributor

meejah commented Aug 23, 2017

Patch looks good to me.

As far as "what to do", it does seem somewhat safer to not-start .. but by the same token, and invalid PID file could mean anything and if you just did tahoe run or tahoe start you must mean "I want it running", so trying to run seems fine to me if the PID-file is bogus. Other thoughts?

@lpirl
Copy link
Contributor Author

lpirl commented Aug 23, 2017

Exactly, that is roughly what I considered.
Introducing a --keep-going/--try-harder/--force seems to be a bit overkill – or are there possible more use cases for such a flag?
For sure, when fully automating Tahoe (as I do), it is sort of annoying to check for and handle stale PID files because Tahoe cannot deal with them in all cases.

@meejah
Copy link
Contributor

meejah commented Aug 23, 2017

@warner any thoughts? I think we should merge this, and then also possibly merge something that just deletes a completely-invalid PID file and then keeps going.

@codecov-io
Copy link

codecov-io commented Aug 23, 2017

Codecov Report

Merging #440 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #440      +/-   ##
==========================================
+ Coverage   87.91%   87.92%   +0.01%     
==========================================
  Files         148      148              
  Lines       27553    27557       +4     
  Branches     3953     3953              
==========================================
+ Hits        24222    24230       +8     
+ Misses       2621     2614       -7     
- Partials      710      713       +3
Impacted Files Coverage Δ
src/allmydata/scripts/startstop_node.py 34.59% <100%> (+7.49%) ⬆️
src/allmydata/mutable/servermap.py 93.38% <0%> (-0.49%) ⬇️
src/allmydata/immutable/upload.py 95.25% <0%> (-0.28%) ⬇️
src/allmydata/web/status.py 82.97% <0%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04fc0e4...e79eb2f. Read the comment docs.

@exarkun
Copy link
Member

exarkun commented Aug 24, 2017

@lpirl I've created lpirl#1 which should fix the coverage checks. Can you merge that into your branch and then hopefully we can get a green build here - at which point I'd be happy to see this merged.

Thanks.

Add a test for the non-numeric pidfile contents case
@meejah meejah merged commit 5483c86 into tahoe-lafs:master Aug 24, 2017
@meejah
Copy link
Contributor

meejah commented Aug 24, 2017

\o/ Good job everyone

@meejah
Copy link
Contributor

meejah commented Aug 24, 2017

BTW, we're planning on deprecating tahoe start and encouraging people to use tahoe run alongside their favorite daemonization-tool. Also, tahoe start doesn't do daemonization on Windows (it does what tahoe run does).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants