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

CLI: interface for manully clean stale PID file #6007

Merged
merged 4 commits into from May 11, 2023

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented May 10, 2023

In 8d963c9 orphaned PID files can now only be removed by starting the daemon again. In order to provide a way to clean the stale pid file manually. In AiiDAlab docker stack, we run verdi daemon stop before database migration. But if the container is shut down ungracefully, the daemon is not properly stopped and leaves a stale PID file which causes the exception when running verdi daemon stop next time.
With the CLI interface in this PR, it allows running the verdi daemon clean-stale-pid-file first, to make sure verdi daemon stop will not block the starting of the container.

EDIT: the argument clean-stale-pid-file seems nasty, maybe call it verdi daemon clean?

@sphuber
Copy link
Contributor

sphuber commented May 11, 2023

Thanks @unkcpz . When I refactored the daemon implementation, it was a request of @ltalirz to only have stale PID files be cleaned when starting the daemon. I agree though, that when this message is shown when asking the daemon status, it makes sense to me as well to resolve it by stopping the daemon. I believe the main problem @ltalirz had with the original implementation was that the PID was cleaned when asking the status as well. Which would be an unexpected side-effect for a status command. But I would argue that it makes sense for both starting and stopping the daemon.

So how about, instead of adding a new dedicated command, we just have stopping the daemon also clean stale PID files? Would that work for you @unkcpz and @ltalirz ?

@ltalirz
Copy link
Member

ltalirz commented May 11, 2023

That sounds reasonable to me as well, thanks Seb

@unkcpz unkcpz force-pushed the fix/aiidalab-docker-stack-pid-daemon branch from 841e0d2 to 210ed38 Compare May 11, 2023 07:36
@unkcpz
Copy link
Member Author

unkcpz commented May 11, 2023

Thanks @sphuber, it is even better for me. Then we just need to bump the aiida-core version in aiidalab-docker-stack. I update this PR.

@sphuber
Copy link
Contributor

sphuber commented May 11, 2023

@unkcpz Please make sure to make the change in the DaemonClient.stop_daemon function and not in the verdi command. We want all functionality in the Python API so that its behavior and verdi are identical

@unkcpz
Copy link
Member Author

unkcpz commented May 11, 2023

Yes it is in DaemonClient.stop_daemon 😉

sphuber
sphuber previously approved these changes May 11, 2023
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @unkcpz

@sphuber sphuber self-requested a review May 11, 2023 08:21
@sphuber sphuber dismissed their stale review May 11, 2023 08:21

need additional changes

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Actually, sorry. I think we should also update the warning message if the stale PID is found. Now it suggest to start the daemon. I think suggesting to stop it makes more sense, or at least to suggest running either. As either will solve the problem now.

@unkcpz
Copy link
Member Author

unkcpz commented May 11, 2023

@sphuber thanks!

@sphuber sphuber merged commit 2bfc3c6 into aiidateam:main May 11, 2023
12 checks passed
@danielhollas
Copy link
Collaborator

@unkcpz thanks for taking this on. @sphuber is the plan here to release this as part of 2.3.1? Or do we need to still have a workaround in AiiDAlab for now?

@sphuber
Copy link
Contributor

sphuber commented May 12, 2023

I am indeed planning to release this with v2.3.1, but that is blocked by #6000 . As soon as that is merged, I can make the release. It has already been reviewed by @chrisjsewell but I needed to add one more commit, because it turns out that the refactoring of v2.0 broke the mechanism for any new migrations. They would work but not actually be committed.

sphuber pushed a commit that referenced this pull request May 22, 2023
The daemon client was recently refactored to only clean the stale PID
file in the `start_daemon` command. Before it was done when asking the
status which was considered an unexpected side-effect.

Sometimes, the user wants to make sure the daemon is no longer running.
Currently, the status would raise a warning if a stale PID file is found
suggesting the user to start the daemon to fix it. However, this is
counterintuitve and not desirable if the goal is for the daemon to be
stopped.

The `stop_daemon` method is updated to also clean any potentially stale
PID files. The error message is updated to suggest to either start or
stop the daemon to return it to a nominal state.

Co-authored-by: Sebastiaan Huber <mail@sphuber.net>

Cherry-pick: 2bfc3c6
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.

None yet

4 participants