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

remove stale daemon pid in status check #3118

Merged
merged 2 commits into from
Jul 16, 2019
Merged

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Jul 3, 2019

The problem (#2795) that stale daemon PID not found after reboot when daemon is not stop correctly also occurred in command verdi daemon status and verdi status.

Also calling function delete_stale_pid_file in verdi status check.

Or maybe it is better to print tips on how to solve the problem in get_daemon_status rather than remove files automatically?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 74.354% when pulling 3677b18 on unkcpz:daemon-stale into 406dbb4 on aiidateam:develop.

@sphuber
Copy link
Contributor

sphuber commented Jul 3, 2019

Or maybe it is better to print tips on how to solve the problem in get_daemon_status rather than remove files automatically?

The problem with this approach is that it is difficult to determine the source of the problem when a call to daemon times out. Note that the delete_stale_pid_file should only remove the pid file if the corresponding process can no longer be found. This should be fine in principle. Did you find a situation where the pid file was removed unjustly? If not, why do you think we should remove these lines?

@unkcpz
Copy link
Member Author

unkcpz commented Jul 3, 2019

I didn't find situation where the pid file was removed unjustly.

My PR suggests also adding delete_stale_pid_file in verdi status and verdi daemon status.

@sphuber
Copy link
Contributor

sphuber commented Jul 3, 2019

Ah you are right, I glossed fast over the diff and interpreted it exactly the other way around.

@unkcpz
Copy link
Member Author

unkcpz commented Jul 3, 2019

I think this is not an urgent PR, but it can be confusing for users, especially beginners.

Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @unkcpz!

In the long term, if we realise we are starting to add this in too many places, we should probably find a better way to avoid putting the call in many places. But for now this a useful fix for user and we approve it.

@sphuber sphuber merged commit dd3a43b into aiidateam:develop Jul 16, 2019
@unkcpz unkcpz deleted the daemon-stale branch July 16, 2019 10:25
d-tomerini pushed a commit to d-tomerini/aiida_core that referenced this pull request Sep 30, 2019
Added stale pid removal to `verdi status` and `verdi daemon status`
d-tomerini pushed a commit to d-tomerini/aiida_core that referenced this pull request Oct 16, 2019
Added stale pid removal to `verdi status` and `verdi daemon status`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants