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

Initialize DB during image build #457

Merged
merged 34 commits into from
Jun 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
f48a799
Reduce number of verdi invocations on startup
danielhollas Apr 25, 2024
bdd8ef1
Set debug outputs
danielhollas Apr 25, 2024
f02bc28
Cleanup 20_start-postgresql.sh
danielhollas Apr 25, 2024
6f639aa
More tweaks
danielhollas May 17, 2024
da9cffd
Prepare DB during build
danielhollas May 20, 2024
79231b6
Try without root
danielhollas May 20, 2024
a6320de
Use PGDATA
danielhollas May 21, 2024
fac06ff
Revert "Try without root"
danielhollas May 21, 2024
b4e6145
PGDATA everywhere
danielhollas May 21, 2024
ac248db
back
danielhollas May 21, 2024
2e5ba46
Fix?
danielhollas May 21, 2024
50e2611
Pass PGDATA for full-stack
danielhollas May 21, 2024
be5846d
This?
danielhollas May 21, 2024
e89f505
Update stack/full-stack/Dockerfile
danielhollas May 24, 2024
9aaa20d
Update stack/full-stack/Dockerfile
danielhollas May 24, 2024
163e4f2
Update stack/full-stack/Dockerfile
danielhollas May 24, 2024
e4c45e1
Add comment about Erlang
danielhollas May 23, 2024
1ac56fb
Come on!
danielhollas May 24, 2024
bdf3907
Remove meta targets
danielhollas May 24, 2024
c8c1cef
Preserve logfile
danielhollas May 24, 2024
2ddeac0
sigh
danielhollas May 24, 2024
7e9c91c
fix
danielhollas May 25, 2024
2d0e8f4
remove pg_ctl status
danielhollas May 26, 2024
d03f11b
To hell with it
danielhollas May 26, 2024
94ab041
Try thisA
danielhollas May 26, 2024
ae59bdb
?
danielhollas May 26, 2024
8378fa5
Sigh
danielhollas May 26, 2024
236902b
pretty print json output
danielhollas May 26, 2024
5b2d0bd
Pretty please
danielhollas May 26, 2024
2cfe15a
hmm
danielhollas May 26, 2024
d480087
...
danielhollas May 26, 2024
daa56ea
Getting closer
danielhollas May 26, 2024
c08dd3d
Screw this
danielhollas May 26, 2024
7f80842
Merge branch 'main' into initdb
danielhollas Jun 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
2 changes: 1 addition & 1 deletion .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,6 @@ jobs:
- name: Set output variables
id: bake_metadata
run: |
.github/workflows/extract-image-names.sh | tee -a "${GITHUB_OUTPUT}"
.github/workflows/extract-image-names.sh | tee -a "${GITHUB_OUTPUT}" | awk -F'=' '{print $2}' | jq
Copy link
Member

Choose a reason for hiding this comment

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

This I don't understand, what different it will make?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, this just pretty prints the json output so that it is easier to read in the Github Actions logs. Previously it was on a single line, and it was a bit annoying when I needed to copy the image name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how it looks now in the amd64 image build

image

env:
BAKE_METADATA: ${{ steps.build-upload.outputs.metadata }}
22 changes: 4 additions & 18 deletions docker-bake.hcl
Original file line number Diff line number Diff line change
Expand Up @@ -54,22 +54,8 @@ group "default" {
targets = "${TARGETS}"
}

target "base-meta" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated cleanup

tags = tags("base")
}
target "base-with-services-meta" {
tags = tags("base-with-services")
}
target "lab-meta" {
tags = tags("lab")
}

target "full-stack-meta" {
tags = tags("full-stack")
}

target "base" {
inherits = ["base-meta"]
tags = tags("base")
context = "stack/base"
platforms = "${PLATFORMS}"
args = {
Expand All @@ -78,7 +64,7 @@ target "base" {
}
}
target "base-with-services" {
inherits = ["base-with-services-meta"]
tags = tags("base-with-services")
context = "stack/base-with-services"
contexts = {
base = "target:base"
Expand All @@ -90,7 +76,7 @@ target "base-with-services" {
}
}
target "lab" {
inherits = ["lab-meta"]
tags = tags("lab")
context = "stack/lab"
contexts = {
base = "target:base"
Expand All @@ -103,7 +89,7 @@ target "lab" {
}
}
target "full-stack" {
inherits = ["full-stack-meta"]
tags = tags("full-stack")
context = "stack/full-stack"
contexts = {
base-with-services = "target:base-with-services"
Expand Down
9 changes: 8 additions & 1 deletion stack/base-with-services/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,20 @@ ARG AIIDA_VERSION
ARG PGSQL_VERSION
ARG TARGETARCH

# Location of the Postgresql DB
# This variable is automatically picked up by initdb and pg_ctl
# WARNING: If you change this, you have to change it in full-stack as well!
ENV PGDATA=/home/${NB_USER}/.postgresql

# Install RabbitMQ and PostgreSQL in a dedicated conda environment.
#
# RabbitMQ is not available on conda-forge at the time being, see:
# https://github.com/conda-forge/rabbitmq-server-feedstock/issues/67If
# Instead we need install erlang via apt and RabbitMQ as a "Generic Unix Build", see:
# https://www.rabbitmq.com/install-generic-unix.html
# Note that this version must be compatible with system's erlang version.
# Currently installed Erlang version is 23.3, so the latest supported RMQ version is 3.9.21
# https://www.rabbitmq.com/docs/which-erlang#old-timers
# Note that system erlang from arm64 is already installed in the base image,
# together with other APT dependencies to save build time.
RUN if [ "$TARGETARCH" = "amd64" ]; then \
Expand Down Expand Up @@ -58,4 +65,4 @@ USER ${NB_USER}
WORKDIR "/home/${NB_USER}"

# Initialize the database
RUN mamba run -n aiida-core-services initdb -D aiida_db -U aiida
RUN mamba run -n aiida-core-services initdb
Copy link
Member

Choose a reason for hiding this comment

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

What is the DB name used when create aiida profile after this change? We set it explicitly in config-quick-setup.yaml as aiida_db.

BTW, we need keep in mind to adapt the change to use verdi presto after we move to 2.6.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. -D parameter doesn't set the db name, it sets the DB location.

https://www.postgresql.org/docs/current/app-initdb.html#APP-INITDB-OPTION-PGDATA

in this case it would set it to ~/aiida_db. But this was broken anyway, since the location in stack/base-with-services/before-notebook.d/20_start-postgresql.sh was set to ~/.postgresql, so essentially this initdb command was being ignored. Now the location is set via the PGDATA variable and thus is guaranteed to be the same everywhere to prevent these kind of mismatches in the future.

Note that the the -U parameter sets the so-called bootstrap superuser I am not sure if this would have any effect, but we are not using this parameter in stack/base-with-services/before-notebook.d/20_start-postgresql.sh so I deleted it here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, we need keep in mind to adapt the change to use verdi presto after we move to 2.6.0

I don't think we'll want to use verdi presto since we'd loose some config options. Instead I believe we should migrate from verdi quicksetup to the full verdi profile setup. But that's not that urgent, quicksetup has been deprecated but not removed.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, thanks!

40 changes: 21 additions & 19 deletions stack/base-with-services/before-notebook.d/20_start-postgresql.sh
Original file line number Diff line number Diff line change
@@ -1,30 +1,32 @@
#!/bin/bash
set -x

if [[ -z $PGDATA ]]; then
echo "ERROR: PGDATA variable not set, cannot start PostgreSQL!"
exit 1
fi

PSQL_LOGFILE="${PGDATA}/logfile"
# -w waits until server is up
PSQL_START_CMD="pg_ctl --timeout=180 -w -D /home/${NB_USER}/.postgresql -l /home/${NB_USER}/.postgresql/logfile start"
PSQL_STOP_CMD="pg_ctl -w -D /home/${NB_USER}/.postgresql stop"
PSQL_STATUS_CMD="pg_ctl -D /home/${NB_USER}/.postgresql status"
PSQL_START_CMD="pg_ctl --timeout=180 -w -l ${PSQL_LOGFILE} start"

MAMBA_RUN="mamba run -n aiida-core-services"

# make DB directory, if not existent
if [ ! -d /home/${NB_USER}/.postgresql ]; then
mkdir /home/${NB_USER}/.postgresql
${MAMBA_RUN} initdb -D /home/${NB_USER}/.postgresql
echo "unix_socket_directories = '/tmp'" >> /home/${NB_USER}/.postgresql/postgresql.conf
${MAMBA_RUN} ${PSQL_START_CMD}

# Initialize DB directory if it does not exist
if [[ ! -d ${PGDATA} ]]; then
mkdir "${PGDATA}"
${MAMBA_RUN} initdb
echo "unix_socket_directories = '/tmp'" >> "${PGDATA}/postgresql.conf"
else
# Fix problem with kubernetes cluster that adds rws permissions to the group
# for more details see: https://github.com/materialscloud-org/aiidalab-z2jh-eosc/issues/5
chmod g-rwxs /home/${NB_USER}/.postgresql -R
chmod -R g-rwxs "${PGDATA}"

if ! ${MAMBA_RUN} ${PSQL_STATUS_CMD}; then
# Cleaning up the mess if Postgresql was not shutdown properly.
# TODO: Rotate logfile
echo "" > /home/${NB_USER}/.postgresql/logfile
rm -vf /home/${NB_USER}/.postgresql/postmaster.pid
${MAMBA_RUN} ${PSQL_START_CMD}
fi
if [[ -f ${PGDATA}/logfile ]]; then
mv "${PSQL_LOGFILE}" "${PSQL_LOGFILE}.1" && gzip "${PSQL_LOGFILE}.1"
fi
# Cleaning up the mess if PostgreSQL was not shutdown properly.
rm -vf "${PGDATA}/postmaster.pid"
fi

# Start the server
${MAMBA_RUN} ${PSQL_START_CMD}
6 changes: 6 additions & 0 deletions stack/full-stack/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,10 @@ RUN fix-permissions "/home/${NB_USER}/.aiida"

USER ${NB_USER}

# WARNING: If you change this, you have to change it in base-with-services as well
ENV PGDATA="/home/${NB_USER}/.postgresql"

# Initialize the database
RUN mamba run -n aiida-core-services initdb

WORKDIR "/home/${NB_USER}"