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

DM-42384: Add support for database migrations with Alembic #959

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

rra
Copy link
Member

@rra rra commented Feb 10, 2024

Add a new oidc token type as an initial migration, which is not yet used but will be used in a subsequent change.

Use an async engine for Alembic, since that avoids needing to also install psycopg2 alongside asyncpg only for Alembic to use it. Since Alembic only sort-of supports async via asyncio.run, this requires moving the body of gafaelfawr init into a separate coroutine and running that with asyncio.run so that Alembic can be run outside of the running async loop.

The Alembic environment is a bit complicated at the moment and duplicates a bit of Safir code. A lot of this can be lifted into Safir in the future.

Fix a few problems with secret loading uncovered by loading the example configuration: the example OIDC client secrets were missing the new return_uri field, and newlines weren't removed from the end of secrets.

@rra rra force-pushed the tickets/DM-42384 branch 2 times, most recently from 4b86596 to ffb6faf Compare February 10, 2024 01:09
@rra
Copy link
Member Author

rra commented Feb 10, 2024

This is the first time that I've used Alembic, and there were some non-obvious aspects to how to generate migrations and how to use an async database engine, so this seemed worth a code review.

Copy link
Member

@jonathansick jonathansick left a comment

Choose a reason for hiding this comment

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

This looks great. I think it overall matches up with my experience using Alembic with LTD Keeper.

Big picture, I do wonder about running alembic on app startup, especially in Kubernetes. In LTD Keeper I always ran the migrations from a specific Kubernetes Job before deploying the new codebase. My thinking was coloured by this article: https://pythonspeed.com/articles/schema-migrations-server-startup/

In practice it might be totally fine always running alembic, but I wanted to bring this up.

@rra
Copy link
Member Author

rra commented Feb 13, 2024

Yeah, this was bothering me too and is a really good point. One of the things that's sort of entangled with this is that currently we run gafaelfawr init during startup, which creates the schema and would create any new tables, although it's somewhat safer since it doesn't modify any tables that already exist.

I feel like the right way to do this is to probably add a Helm chart parameter that says whether to automatically run schema updates (similar to the one for JupyterHub) and, if it is enabled, do a schema upgrade (and gafaelfawr init) as part of a Helm pre-install and pre-upgrade hook in a separate Kubernetes Job like you do with LTD Keeper.

I'll add this to my to-do list and in the meantime I'll drop the automatic Alembic run, and will do this update manually if I don't get around to the more complicated thing before I am ready to push out this release.

Add a new oidc token type as an initial migration, which is not yet
used but will be used in a subsequent change.

Use an async engine for Alembic, since that avoids needing to also
install psycopg2 alongside asyncpg only for Alembic to use it. Since
Alembic only sort-of supports async via asyncio.run, this requires
moving the body of gafaelfawr init into a separate coroutine and
running that with asyncio.run so that Alembic can be run outside of
the running async loop.

The Alembic environment is a bit complicated at the moment and
duplicates a bit of Safir code. A lot of this can be lifted into
Safir in the future.

Fix a few problems with secret loading uncovered by loading the
example configuration: the example OIDC client secrets were missing
the new return_uri field, and newlines weren't removed from the end
of secrets.
@rra
Copy link
Member Author

rra commented Feb 13, 2024

Oh, and that uncovered another bug: I was running gafaelfawr init first, but that's wrong, since that will stamp the database with the latest Alembic version. Okay, I think I will have to implement the Helm hook thing for both gafaelfawr init and for Alembic to make this safe. I'll drop both from the startup script now so that I remember to do that.

It's not generally safe to run Alembic upgrades on startup since
we may have multiple replicas, and it's not safe to run
gafaelfawr init before Alembic because it will stap the database
with the current version even though it didn't make all the required
changes.

Drop both from the startup script for now. We will need to find a
better way of doing both database initialization and schema changes,
probably using Helm hooks.
@rra rra enabled auto-merge February 13, 2024 00:40
@rra rra merged commit 10184c5 into main Feb 13, 2024
5 checks passed
@rra rra deleted the tickets/DM-42384 branch February 13, 2024 00:49
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

2 participants