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.

Sign up for GitHub

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

Jump to bottom

Updating the *before_notebook* 20_start-postgresql.sh and 40_prepare_aiida.sh #476

Merged
merged 8 commits into from
Jul 4, 2024
Merged

Updating the *before_notebook* 20_start-postgresql.sh and 40_prepare_aiida.sh #476

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6428b39
Updating the *before_notebook* 20_ and 40_ files. Fixings are done as
mikibonacci Jul 3, 2024
9bb9234
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 3, 2024
604d0e6
Update stack/base-with-services/before-notebook.d/20_start-postgresql.sh
mikibonacci Jul 3, 2024
23c6120
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 3, 2024
8be6bf6
Clean up stale PID-files before `verdi migrate --force` in `40_prepar…
mikibonacci Jul 4, 2024
75e6811
Update stack/base/before-notebook.d/40_prepare-aiida.sh
mikibonacci Jul 4, 2024
201ba78
pgrep verdi daemon
danielhollas Jul 4, 2024
25e937b
Fix rm command
danielhollas Jul 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion stack/base-with-services/before-notebook.d/20_start-postgresql.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@ else
chmod -R g-rwxs "${PGDATA}"

if [[ -f ${PGDATA}/logfile ]]; then
mv "${PSQL_LOGFILE}" "${PSQL_LOGFILE}.1" && gzip "${PSQL_LOGFILE}.1"
if [[ -f ${PSQL_LOGFILE}.1.gz ]]; then
echo "Deleting old ${PSQL_LOGFILE}.1.gz compressed file"
rm "${PSQL_LOGFILE}.1.gz"
fi
mv "${PSQL_LOGFILE}" "${PSQL_LOGFILE}.1" && gzip "${PSQL_LOGFILE}.1"
mikibonacci marked this conversation as resolved.
Show resolved Hide resolved
fi
# Cleaning up the mess if PostgreSQL was not shutdown properly.
rm -vf "${PGDATA}/postmaster.pid"
Expand Down
2 changes: 2 additions & 0 deletions stack/base/before-notebook.d/40_prepare-aiida.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ load_computer('${computer_name}').set_minimum_job_poll_interval(${job_poll_inter
else

# Migration will run for the default profile.
danielhollas marked this conversation as resolved.
Show resolved Hide resolved
## We need to stop the daemon before.
verdi daemon stop
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to understand why this is needed, since the daemon cannot be running at the container startup.

@sphuber does verdi daemon stop do some kind of cleanup that might explain why it is needed here? It is true that we currently don't gracefully stop the daemon when we stop the container so there might perhaps be some stale pid files or something?

@mikibonacci what is the exact error that you saw coming from the verdi storage migrate command?

Copy link

Choose a reason for hiding this comment

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

Depends on the version. The behavior has changed a bit in the 2.x minor releases. But currently, yes, verdi daemon stop should clean up stale PID-files. And it is possible that some other commands will check if the daemon "is running" based on that pid file. Would have to look into more detail if you know exactly which version you are targeting

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! We're currently on 2.5, and hopefully will switch to 2.6 soon.

I was trying to get rid of verdi daemon stop because it adds 2s to container startup. Perhaps we can just remove the stale pid files manually?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @danielhollas and @sphuber, the exact error printed in the log is:

2024-07-03 15:41:58 Critical: Migration aborted, the daemon for the profile is still running.

The target AiiDA version is aiida-core==2.5.1 (build=pyhca7485f_0, channel=conda-forge) indeed.

Copy link

Choose a reason for hiding this comment

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

I was trying to get rid of verdi daemon stop because it adds 2s to container startup. Perhaps we can just remove the stale pid files manually?

Well if you are absolutely certain that the daemon shouldn't be running, you could just manually remove the pid file .aiida/daemon/circus-{profile-name}.pid which should be very fast. The verdi daemon stop command will actually try to reach the daemon, which has a 2 second timeout by default.

Copy link
Member

@unkcpz unkcpz Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Command verdi storage migrate has access to daemon client, so probably another solution is when --force flag is used, it does clean stale PID files before migrate.

Copy link

Choose a reason for hiding this comment

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

I am not sure that it would be a good idea to have --force ignore a running daemon. The --force flag is there to not have to type out the confirmation message. This has been the case for a long time and users may have grown used to that. If we now also have it start ignoring a running daemon, that might be dangerous.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, then manually delete it as Miki did here is fine.

verdi storage migrate --force

fi
Expand Down