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

Implement automated PGSQL DB migration. #63

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yakutovicha
Copy link
Contributor

@yakutovicha yakutovicha commented Oct 31, 2022

fixes #41

TODO:

@yakutovicha yakutovicha marked this pull request as draft October 31, 2022 16:21
@danielhollas
Copy link

@yakutovicha I am a bit confused here. I believe that @unkcpz mentioned that aiida-prerequisites is no longer used to build the new AiiDA-2.0 images. So presumably the migration script needs to live in aiidalab-docker-stack repo?

@yakutovicha
Copy link
Contributor Author

@yakutovicha I am a bit confused here. I believe that @unkcpz mentioned that aiida-prerequisites is no longer used to build the new AiiDA-2.0 images. So presumably the migration script needs to live in aiidalab-docker-stack repo?

Maybe I missed that part, sorry. For the moment aiida-core images are still based on aiida-prerequisites. We will at some point replace them, but we haven't agreed on the strategy yet.

Anyways, I am going to implement the migration here and then we port it elsewhere.

yakutovicha and others added 2 commits November 28, 2022 11:52
Co-authored-by: Daniel Hollas <danekhollas@gmail.com>
* Use pg_dumpall to dump the whole server.
* Automatically determine the old and the new DB version.
* Copy the migration script to the /opt folder.
@yakutovicha yakutovicha marked this pull request as ready for review November 28, 2022 16:03
@@ -29,6 +29,11 @@ else
if ! $running ; then
echo "" > /home/${SYSTEM_USER}/.postgresql/logfile # empty log files
rm -vf /home/${SYSTEM_USER}/.postgresql/postmaster.pid
${PSQL_START_CMD}
${PSQL_START_CMD} || cant_start=true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As @danielhollas suggested, we should distinguish the "migration required" case from other failures and properly handle it here.

Copy link

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

Hi @yakutovicha,

Thanks for doing this annoying work. Just a couple of quick comments. I don't think I'll have time for a more thorough review, unfortunately.

# This script is intended to be run in a container. Its job is to migrate the PGSQL database to a newer version.

# The following variables are required:
DB_FOLDER=/home/${SYSTEM_USER}/.postgresql

Choose a reason for hiding this comment

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

Just noting for future reference that in the new AiiDAlab docker stack we use ${NB_USER}

# Part 1: dump the old database.

# Make a backup of the database.
mv ${DB_FOLDER} ${BACKUP_DB_FOLDER}

Choose a reason for hiding this comment

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

Where would this script show it's output?
I would put an echo here to show where the backup is

(although when the script is executed in the debug mode (set -x), each command is echoed, so I guess and extra echo make it a bit more clear what is happening)

Suggested change
mv ${DB_FOLDER} ${BACKUP_DB_FOLDER}
echo "Creating a DB backup to ${BACKUP_DB_FOLDER}"
mv ${DB_FOLDER} ${BACKUP_DB_FOLDER}

Copy link
Member

Choose a reason for hiding this comment

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

If we can retrieve the debug information, yes, I agree it is more clear to print this massage. But I don't know where to check these printed-out messages. Is it write to some log files?

@@ -93,7 +93,7 @@ RUN cd /tmp && \
conda clean --all -f -y

# Install PostgreSQL in a dedicated conda environment.
RUN conda create -c conda-forge -n pgsql postgresql=10 && conda clean --all -f -y
RUN conda create -c conda-forge -n pgsql postgresql=12 && conda clean --all -f -y

Choose a reason for hiding this comment

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

Any reason why not jump straight to version 14? (just asking, I think it is probably better to do it incrementally)

fi

# Start the old version of PostgreSQL.
conda run -n ${OLD_DB_CONDA_ENV_NAME} pg_ctl -D ${BACKUP_DB_FOLDER} -l ${BACKUP_DB_FOLDER}/logfile start

Choose a reason for hiding this comment

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

Using the ${BACKUP_DB_FOLDER} for this makes me a bit nervous, I don't think we should touch it at all during the migration. But perhaps I am misunderstanding....

Suggested change
conda run -n ${OLD_DB_CONDA_ENV_NAME} pg_ctl -D ${BACKUP_DB_FOLDER} -l ${BACKUP_DB_FOLDER}/logfile start
conda run -n ${OLD_DB_CONDA_ENV_NAME} pg_ctl -D ${DB_FOLDER} -l ${DB_FOLDER}/logfile start

Choose a reason for hiding this comment

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

Never mind, I misunderstood how the backup folder works.

conda run -n ${OLD_DB_CONDA_ENV_NAME} pg_ctl -D ${BACKUP_DB_FOLDER} stop

# Delete the environment with old version of PostgreSQL.
conda env remove -n ${OLD_DB_CONDA_ENV_NAME}

Choose a reason for hiding this comment

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

Should this command be run only at the very and once we're sure the migration was successfull. Otherwise might be good to keep around for debugging?

@unkcpz
Copy link
Member

unkcpz commented Jan 10, 2023

I test using aiidalab-launch with this change. This is how I did the test:

  1. (new image) build aiida-prerequisites image based on this PR -> build aiida-core 1.6.9 -> build aiidalab-docker-stack support-1.x.
  2. (old image) Start an aiidalab instance of legacy version (running aiida-core 1.6.9 from the image aiidalab/aiidalab-docker-stack:22.8.1) and install QeApp and running a calculation to store more data to the database.
  3. Create a new profile using the volume from old image but container using the new image to apply the change made in this PR.

I check the psql version and it is all correct as expected. However, verdi status show the error:

(base) aiida@fb1488a6d2fe:~$ verdi status
 ✔ config dir:  /home/aiida/.aiida
 ✔ profile:     On profile default
 ✔ repository:  /home/aiida/.aiida/repository/default
 ✘ postgres:    Unable to connect as aiida_qs_aiida_477d3dfc78a2042156110cb00ae3618f@localhost:5432
Error: OperationalError: could not connect to server: Connection refused
        Is the server running on host "localhost" (127.0.0.1) and accepting
        TCP/IP connections on port 5432?
could not connect to server: Cannot assign requested address
        Is the server running on host "localhost" (::1) and accepting
        TCP/IP connections on port 5432?

 ✔ rabbitmq:    Connected to RabbitMQ v3.8.14 as amqp://guest:guest@127.0.0.1:5672?heartbeat=600

I did anything wrong? How I confirm the migrate script is executed and the database is successfully migrated?

@unkcpz
Copy link
Member

unkcpz commented Jan 10, 2023

As mentioned by @yakutovicha in the meeting, the migration script needs to be run by hand.

I manually trigger the migration and it seems failed at last step with:

(1 row)

SET
SET
SET
COMMENT
CREATE EXTENSION
COMMENT
You are now connected to database "template1" as user "aiida".
SET
SET
SET
SET
SET
SET
 set_config
------------

(1 row)

SET
SET
SET
COMMENT
CREATE EXTENSION
COMMENT

psql:/home/aiida/pgsql-dump-10-20230110134114.sql:14: ERROR:  role "aiida" already exists

@danielhollas
Copy link

@unkcpz I am wondering if the error that you so could simply be ignored (e.g. user aiida already exist, never mind). Per
https://dba.stackexchange.com/questions/318379/role-postgres-already-exists-when-restoring-cluster

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.

Enable automated migration of PostgreSQL database.
3 participants