Schema migrations

[redshiftzero]

Some new features (e.g. #1170) are going to require schema migrations. We should integrate e.g. sqlalchemy-migrate to make it easier for us to make changes to the databases on the production instances.

[redshiftzero]

Getting this done is blocking some new and frequently requested features (e.g. #1422 - the tagging of sources and submissions and #1359 - the assignment of particular users to address a given source), so I've been testing how to go about doing this in the feature branch db-migrations. In that branch, the first steps of integrating database migration support into SecureDrop using SQLAlchemy-migrate are implemented. Here I'll describe in more detail how this might work and I welcome feedback or suggestions.

Requirements

• This will need to work for both new installs and databases already created in the production instances
• This should not require manual intervention by administrators for future migrations, so future versions of SecureDrop should check if a migration is needed and perform it if is necessary
• If a migration fails for some reason (more below) and the database cannot be upgraded, then we will need to keep the SD instance running on the existing database version using the existing app code.

How it currently works

• A new directory SECUREDROP_DATA_ROOT/db_repository tracks database versioning data.
• In new installs, this versioning directory is set up at database initialization time - the code for this has been added to init_db() in db.py.
• In existing installs, the versioning directory should be set up at first run of the app after update. I was thinking that this could be done in a one-off Ansible task that is run by admins for the 0.4 release (something we are going to do anyway). A new Ansible task copies the SECUREDROP_DATA_ROOT/db_repository location into the existing config.py so db.py can find it.
• When a database migration needs to be performed, migrate() in db.py can be executed to perform a migration. First, the database is backed up, and then the migration is executed. This is something that pre-release can be tested to make sure that the migrations that SQLAlchemy-migrate generates work and no problems on the SQL side are encountered.

Potential Issues

We should enumerate as many possible issues that could arise and handle them if we can:

• Insufficient disk space to backup the database
  • In this situation, the instance should continue to run on the old version of the code, let the administrators know they need to resolve the disk space issue, and then a backup and upgrade can be attempted again in the future when the issue is resolved.
• Any other failure during the migration occurs
  • Pre-release testing should be done in order to ensure that no problems with the generated migrations to minimize this possibility
  • However, if it does happen, SD should copy the backup back, and continue to run on the old version of the code using the existing database.

When to migrate

One thing I'm trying to figure out is when to best attempt to perform a migration. It could be done during postinst. Another option is to have the SD app code check the db models upon launch of the Flask app, see if they have changed, and if so do the migration during app initialization. I think the former is probably the better way to handle this but thoughts welcome on this.

[redshiftzero]

Note: I just checked the diff between 0.3.11 and develop, and there are already merged changes to the database models. I'm fine pushing e.g. #1422 to post-0.4, but in order to remove #1419 from 0.4 we will need to back out these changes in develop

[redshiftzero]

After discussion, we are going to push this until the next release.

[redshiftzero]

(Again, this requires the backing out of changes from develop)

[heartsucker]

I would like to add a second proposition instead of using sqlalchemy-migrate. One thing about it that feels awkward is that you have to duplicate the models to both the models.py in the main application and the migration scripts. The docs even say to do this:

To avoid the above problem, you should use SQLAlchemy schema reflection as shown above or copy-paste your table definition into each change script rather than importing parts of your application.

Also, since it's possible we might only ever support Postgres (#2225), using the SQLAlchemy automagic isn't even necessary.

For example, the Rust ORM (diesel) takes the lazy approch. There's just a sorted dir with {up,down}.sql that are run against the DB. I like this better because I want full control over the generated SQL, and I don't want to have have to print each generated migration and read over it to ensure it does what I want.

migrations/
└── 0001-init.sql
...

And the DB is versioned by the numeric prefix. I actually wrote this once for an app because we kept having problems with sqlalchemy-migrate and alembic.

import os
from flask_sqlalchemy import SQLAlchemy
from os import path
from sqlalchemy import text
from sqlalchemy.exc import InternalError, OperationalError, ProgrammingError

db = SQLAlchemy()


def migrate(migrations_dir):
    version = _initialize_db()
    migrations = [(v, s)
                  for (v, s) in _list_migrations(migrations_dir)
                  if v > version]
    for (version, sql) in migrations:
        try:
            db.session.execute(sql)
            sql = text('UPDATE db_version SET version = :version') \
                .bindparams(version=version)
            db.session.execute(sql)
            db.session.commit()
        except Exception:
            db.session.rollback()
            raise


def _initialize_db() -> int:
    version = None
    try:
        sql = text('SELECT version FROM db_version')
        version = list(db.session.execute(sql))[0][0]
    except (InternalError, OperationalError, ProgrammingError):
        db.session.rollback()
        sql = text('CREATE TABLE db_version (version INT)')
        db.session.execute(sql)
    if version is None:
        sql = text('INSERT INTO db_version (version) VALUES (0)')
        db.session.execute(sql)
        version = 0
    db.session.commit()
    return version


def _list_migrations(migrations_dir) -> list:
    migrations = []
    for migration in os.listdir(migrations_dir):
        full_path = path.join(migrations_dir, migration)
        try:
            version = int(migration.split('-')[0])
        except ValueError:
            continue
        with open(full_path, 'r') as f:
            sql = f.read()
        migrations.append((version, sql))
    return list(sorted(migrations, key=lambda x: x[0]))

This is one of those times we have to ask about whether or not it makes sense to add a dependency or if we want to just roll our own code. This is only 60 lines of Python + SQL versus having to fiddle with a DSL to get it to generate the code we actually want.

The downside to this method is that it doesn't allow arbitrary Python code in the migrations, but realistically, if you need that you're probably doing something overly complicated with your migration. i can't think of a time at work we (at work) have actually needed code for a migration.

[heartsucker]

Pinging @redshiftzero for thoughts on this.

[heartsucker]

I'm working on this now, and will be using Alembic. I'm also setting it up so that the first migration will optionally dump everything into Postgres if the SQLite database exists. This will cover new and existing instances.

[heartsucker]

Blocked by #2866.

[redshiftzero]

Here's a proposed breakdown for this ticket:

1. Integrate autogenerated Alembic migrations support
2. Add database backup in postinst
3. Add alembic migration in postinst
4. Add realistic data upload script for creating a prod-like database for testing/QA of database migrations

[heartsucker]

Where do we want the database backups saved? What's the naming scheme. I propose gzipped backups as /var/lib/securedrop/backups/YYYY-MM-DD-sqlite.sql.tgz.

[heartsucker]

Also this may go in to the current PR or may be a second PR since it will be even larger than the original, but the gist is.

For each revision A -> B there's a .sql file (because we can't use anything from models.py) that loads some data into the DB and then a test that checks some certain expected behavior works (uses can still log in, FKs aren't broken, whatever).

We have a test the does all the migrations up to A, loads the SQL, runs some sanity checks. Another test checks the downgrade from B -> A and might even run sqldump on A and then A' where A' == A -> B -> A. If A == A', this would check that alembic upgrade +1 followed by alembic downgrade -1 is idempotent.

This would mean that every migration would need to have a set up dummy data generated for it as well some manual tests written.

[heartsucker]

Further note. Should we do a DB downgrade when we downgrade the SD app version? This could be in the maintainer scripts.

[eloquence]

NB: We've moved this off the 0.7 (May 8) milestone, given that we don't think we'll have quite enough time to properly QA this for the release, but we should be able to ship it with 0.8, and will continue to work on it through this sprint and following ones.