Provide the "Initial database schema" alembic migration #3174

portante · 2023-01-16T11:26:22Z

Since we already have been using the database for quite some time, we create an initial migration which will take an empty database to the database base schema version as of commit 6a764f1. That commit was the most recent version deployed in Red Hat's staging environment. The existing revision migration was removed so that we only have a migration from empty database to fully populated.

Starting with Alembic v1.9.0 the command, alembic check is available which will succeed if no migrations are required, and fail if it detects that a migration is necessary. So future PRs will have to provide a migration revision if a change to the database schema is made in the PR, and alembic check will fail if one is not provided.

The existing tox environment will now run the alembic check when the server side tests are executed. You can just run the check via:

tox -- server alembic

We update the alembic.ini file with the latest version generated by alembic 1.9.x, and it also assumes a localhost PostgreSQL database.

The previous requirement of needing a pbench-server.cfg file has been removed by restoring the alembic/env.py file back to the default with a small change to reference our database metadata.

We have removed the unneeded alembic.ini file in the alembic subdirectory.

portante · 2023-01-16T12:19:44Z

FWIW, PR #3114 has a series of commits which change the database schema and I'd really like to engage Alembic migrations to help ensure our deployments work properly.

More work needs to be done to integrate with Flask SQLAlchemy and Flask Migrate, but that can be done later.

lib/pbench/server/database/alembic.ini

lib/pbench/server/database/alembic.run

tox.ini

portante · 2023-01-16T15:38:14Z

I saw comments on this PR via email, but I don't see them now. Weird.

I'll record responses here ...

The over-arching reason that I am using a tox.ini environment with a file alembic.run, is that I have no way to start our migration story without a bootstrap somewhere, and we don't current use Flask migrations after the create_app, so we can't use our current container environment.

These kinds of changes will be removed when we can start using Flask SQLAlchemy and Flask Migrate.

In the mean time, these are part of the commit because they represent how I did this work. I used a tox environment to make sure I had the right path to our pbench modules (our current env.py fails the Alembic commands without it), and our pbench modules require a working pbench-server.cfg file (which we don't have outside of a containerized environment), and the change to alembic.ini was innocuous enough to be useful outside the container environment.

Once we adopt Flask SQLAlchemy and Flask Migrate, all this can go away. I can also just remove these changes from the PR, but it seemed better to include and not encourage mystery.

npalaska

I don't have any strict objection to the PR as long as we can document that these are intermittent changes subject to change again if and when we use Flask-Migrate and Flask-SQLAlchemy packages. As Dave said we also need to make sure that we have a mechanism to add this migration revision file inside the container (but I guess that can be a follow-up PR). On the Jira side, we also need a story to replace our current use of the python sqlalchemy package with Flask Flask-SQLAlchemy

lib/pbench/server/database/alembic.ini

webbnh

I assume that (other than the comments below) this is all good...so I'll defer to Dave and Nikhil.

lib/pbench/server/database/alembic.run

lib/pbench/server/database/pbench-server.cfg

portante

Ready for review.

webbnh

There's a typo in exec-tests which really should be fixed, and there's a typo in a commit in alembic.ini; the rest are trivialities which you might as well consider since we need another cycle anyway.

exec-tests

lib/pbench/server/database/alembic.ini

lib/pbench/server/database/alembic/env.py

portante · 2023-01-17T01:42:30Z

Bugger all, it doesn't work. I think I have a solution for the failed CI job. Working on it ...

…onfig

portante · 2023-01-17T02:43:12Z

Still working on a solution to the CI failure.

portante

Almost have this working for the CI. Just need to see about a pg_isready command for the CI environment.

portante

A bit ugly, but at least the CI now has a way to verify a commit does not add a database schema change without providing a migration.

Since we already have been using the database for quite some time, we create an initial migration which will take an empty database to the database base schema version as of commit 6a764f1. That commit was the most recent version deployed in Red Hat's staging environment. In order to make it convenient to work with migrations, we also create a `tox` environment, `alembic`, which simply invokes the new script `alembic.run` which one can modify to execute whatever Alembic commands are needed. The committed copy takes no actions, but has the initial commands used to create the first migration: alembic stamp head alembic revision --autogenerate -m "Initial tables" We also update the `alembic.ini` file to assume a `localhost` PostgreSQL database, and provide a Pbench Server configuration file that works in the Alembic environment.

Starting with Alembic v1.9.0 the command, `alembic check` is available which will succeed if no migrations are required, and fail if it detects that a migration is necessary. We also remove the unused `alembic/alembic.ini`, drop the need for a separate `tox` environment, remove the need for a `pbench-server.cfg` file entirely by reverting to the original env.py as generated by `alembic init alembic`. We also collapse the existing migrations to one since we just need one to move from an empty database to our current schema.

We now run a very simple PostgreSQL container against which we run all our known migrations and then ask Alembic to check that we don't have any additional model changes which need a migration. The CI build will fail when any migrations are missing, and a local `tox -- server alembic` will also fail (really, any `tox` run that ends up running the server side, e.g. `tox -- server`, `tox -- all`, or just `tox`).

The build.sh is invoked inside a container, so we can't use podman from there to launch an empty live database. So move to a CI only pipeline check. One can run it locally by just invoking: jenkins/run-alembic-migrations-check

jenkins/Pipeline.gy

npalaska

Looks good now 👍

webbnh

This is great stuff (although, I did find one nit which should be fixed).

However, I'm concerned that we don't have a coordinated source of truth for the PostgreSQL configuration stuff (version number, container image name, image tag, username, password, and database name): it's going to be too easy to forget that this test is out there when we decide to upgrade the canned server and the staging server to PG-15.

jenkins/run-alembic-migrations-check

webbnh

👍

portante added the Server label Jan 16, 2023

portante added this to the v0.72 milestone Jan 16, 2023

portante requested review from npalaska and dbutenhof January 16, 2023 11:26

portante self-assigned this Jan 16, 2023

portante force-pushed the alembic-init-db branch from a7471fc to 3684812 Compare January 16, 2023 13:11

dbutenhof reviewed Jan 16, 2023

View reviewed changes

lib/pbench/server/database/alembic.ini Show resolved Hide resolved

lib/pbench/server/database/alembic.run Outdated Show resolved Hide resolved

tox.ini Outdated Show resolved Hide resolved

npalaska reviewed Jan 16, 2023

View reviewed changes

lib/pbench/server/database/alembic.ini Show resolved Hide resolved

webbnh reviewed Jan 16, 2023

View reviewed changes

lib/pbench/server/database/alembic.run Outdated Show resolved Hide resolved

lib/pbench/server/database/pbench-server.cfg Outdated Show resolved Hide resolved

portante force-pushed the alembic-init-db branch from 3684812 to 32930cd Compare January 17, 2023 00:11

portante requested review from dbutenhof, npalaska and webbnh January 17, 2023 00:14

portante commented Jan 17, 2023

View reviewed changes

portante changed the title ~~Provide the "Initial tables" alembic migration~~ Provide the "Initial database schema" alembic migration Jan 17, 2023

webbnh requested changes Jan 17, 2023

View reviewed changes

portante added a commit to portante/pbench that referenced this pull request Jan 17, 2023

How PR distributed-system-analysis#3174 compares to initial Alembic c…

495ac72

…onfig

portante mentioned this pull request Jan 17, 2023

Show progression of changes from Alembic init generated files to what we have in PR #3174 #3180

Closed

portante added a commit to portante/pbench that referenced this pull request Jan 17, 2023

How PR distributed-system-analysis#3174 compares to initial Alembic c…

f0ebd38

…onfig

portante commented Jan 17, 2023

View reviewed changes

portante requested a review from webbnh January 17, 2023 04:50

dbutenhof previously approved these changes Jan 17, 2023

View reviewed changes

portante added 5 commits January 17, 2023 16:09

Restore changes lost messing around with alembic versions

ccd45be

Bugger all; can't run from tox invoked by build.sh

6732ab0

The build.sh is invoked inside a container, so we can't use podman from there to launch an empty live database. So move to a CI only pipeline check. One can run it locally by just invoking: jenkins/run-alembic-migrations-check

Make it work

be04088

portante dismissed dbutenhof’s stale review via be04088 January 17, 2023 21:09

portante force-pushed the alembic-init-db branch from 084849f to be04088 Compare January 17, 2023 21:09

webbnh reviewed Jan 17, 2023

View reviewed changes

jenkins/Pipeline.gy Show resolved Hide resolved

npalaska previously approved these changes Jan 17, 2023

View reviewed changes

webbnh previously approved these changes Jan 17, 2023

View reviewed changes

jenkins/run-alembic-migrations-check Outdated Show resolved Hide resolved

jenkins/run-alembic-migrations-check Show resolved Hide resolved

jenkins/run-alembic-migrations-check Show resolved Hide resolved

portante mentioned this pull request Jan 18, 2023

Consider reworking the alembic check to use podman remote functionality #3185

Closed

Remove tabs

644dad1

portante dismissed stale reviews from webbnh and npalaska via 644dad1 January 18, 2023 00:10

portante requested review from dbutenhof, npalaska and webbnh January 18, 2023 00:10

dbutenhof approved these changes Jan 18, 2023

View reviewed changes

portante mentioned this pull request Jan 18, 2023

Rework database modules and class names #3191

Merged

webbnh approved these changes Jan 18, 2023

View reviewed changes

portante merged commit fd48da7 into distributed-system-analysis:main Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide the "Initial database schema" alembic migration #3174

Provide the "Initial database schema" alembic migration #3174

portante commented Jan 16, 2023 •

edited

Loading

portante commented Jan 16, 2023

portante commented Jan 16, 2023

npalaska left a comment

webbnh left a comment

portante left a comment

webbnh left a comment

portante commented Jan 17, 2023

portante commented Jan 17, 2023

portante left a comment

portante left a comment

npalaska left a comment

webbnh left a comment

webbnh left a comment

Provide the "Initial database schema" alembic migration #3174

Provide the "Initial database schema" alembic migration #3174

Conversation

portante commented Jan 16, 2023 • edited Loading

portante commented Jan 16, 2023

portante commented Jan 16, 2023

npalaska left a comment

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

portante left a comment

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

portante commented Jan 17, 2023

portante commented Jan 17, 2023

portante left a comment

Choose a reason for hiding this comment

portante left a comment

Choose a reason for hiding this comment

npalaska left a comment

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

portante commented Jan 16, 2023 •

edited

Loading