Only remove my process' pidfile. #15491
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gtanzillo
approved these changes
Jun 30, 2017
Fryguy
reviewed
Jun 30, 2017
lib/pid_file.rb
Outdated
| @@ -27,7 +27,7 @@ def remove | |||
| def create(remove_on_exit = true) | |||
| FileUtils.mkdir_p(File.dirname(@fname)) | |||
| File.open(@fname, "w") { |f| f.write(Process.pid) } | |||
| at_exit { PidFile.remove(@fname) } if remove_on_exit | |||
| at_exit { PidFile.remove(@fname) if PidFile.new(@fname).pid == Process.pid } if remove_on_exit | |||
There was a problem hiding this comment.
Can this logic be pushed down into PidFile? Perhaps PidFile.remove! or PidFile.safe_remove?
There was a problem hiding this comment.
Gah...I didn't realize this was PidFile class 😖
Even so, what do you think of a different method to separate the concerns?
There was a problem hiding this comment.
We're in pidfile, let me try pushing it down to remove
jrafanie
force-pushed
the
do_not_remove_other_process_pidfile
branch
from
9d39865
to
800c6e0
Compare
jrafanie
commented
Jun 30, 2017
| @@ -21,7 +21,7 @@ def pid | |||
| end | |||
|
|
|||
| def remove | |||
| FileUtils.rm(@fname) if File.file?(@fname) | |||
| FileUtils.rm(@fname) if pid == Process.pid | |||
There was a problem hiding this comment.
The pid method checks if the File exists... good idea @Fryguy
Fryguy
approved these changes
Jun 30, 2017
jrafanie
force-pushed
the
do_not_remove_other_process_pidfile
branch
from
800c6e0
to
2134728
Compare
https://bugzilla.redhat.com/show_bug.cgi?id=1464698 at_exit handlers created in the server process are inherited when the server forks workers. As soon as one of these workers exits, this at_exit was being run and removing the server's pidfile. This allowed a subsequent rake evm:start or systemctl start evmserverd to fail to detect an existing server process, allowing a second MIQ Server to start. We can workaround this by only removing a pidfile that matches my Process.pid.
jrafanie
force-pushed
the
do_not_remove_other_process_pidfile
branch
from
2134728
to
9090b3e
Compare
|
Checked commit jrafanie@9090b3e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
simaishi
pushed a commit
that referenced
this pull request
Jul 7, 2017
…idfile Only remove my process' pidfile. (cherry picked from commit 8dff3ce) https://bugzilla.redhat.com/show_bug.cgi?id=1468612
|
Euwe backport details: |
simaishi
pushed a commit
that referenced
this pull request
Aug 4, 2017
…idfile Only remove my process' pidfile. (cherry picked from commit 8dff3ce) https://bugzilla.redhat.com/show_bug.cgi?id=1478434
|
Fine backport details: |
simaishi
pushed a commit
that referenced
this pull request
Aug 23, 2017
…idfile Only remove my process' pidfile. (cherry picked from commit 8dff3ce) https://bugzilla.redhat.com/show_bug.cgi?id=1484485
|
Darga backport details: |
d-m-u
pushed a commit
to d-m-u/manageiq
that referenced
this pull request
Jun 6, 2018
…rocess_pidfile Only remove my process' pidfile. (cherry picked from commit 8dff3ce) https://bugzilla.redhat.com/show_bug.cgi?id=1478434
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://bugzilla.redhat.com/show_bug.cgi?id=1464698
at_exit handlers created in the server process are inherited when the
server forks workers. As soon as one of these workers exits, this
at_exit was being run and removing the server's pidfile. This allowed a
subsequent rake evm:start or systemctl start evmserverd to fail to
detect an existing server process, allowing a second MIQ Server to
start.
We can workaround this by only removing a pidfile that matches my
Process.pid.