Merge "db: Migrate to alembic"

7 files changed, 160 insertions, 223 deletions

diff --git a/bin/heat-db-setup b/bin/heat-db-setup
index b0e39ac74..8df02aa08 100755
--- a/bin/heat-db-setup
+++ b/bin/heat-db-setup
@@ -289,7 +289,7 @@ rm $log_conf
 
 # Do a final sanity check on the database.
 
-echo "SELECT * FROM migrate_version;" | mysql -u heat --password=${MYSQL_HEAT_PW} heat > /dev/null
+echo "SELECT * FROM alembic_version;" | mysql -u heat --password=${MYSQL_HEAT_PW} heat > /dev/null
 if ! [ $? -eq 0 ]
 then
 	echo "Final sanity check failed." >&2
diff --git a/heat/cmd/manage.py b/heat/cmd/manage.py
index c853d9ac2..9f7b8e316 100644
--- a/heat/cmd/manage.py
+++ b/heat/cmd/manage.py
@@ -26,17 +26,17 @@ from heat.common.i18n import _
 from heat.common import messaging
 from heat.common import service_utils
 from heat.db.sqlalchemy import api as db_api
+from heat.db.sqlalchemy import migration as db_migration
 from heat.objects import service as service_objects
 from heat.rpc import client as rpc_client
 from heat import version
 
-
 CONF = cfg.CONF
 
 
 def do_db_version():
     """Print database's current migration level."""
-    print(db_api.db_version(db_api.get_engine()))
+    print(db_migration.db_version())
 
 
 def do_db_sync():
@@ -44,7 +44,7 @@ def do_db_sync():
 
     Creating first if necessary.
     """
-    db_api.db_sync(db_api.get_engine(), CONF.command.version)
+    db_migration.db_sync(CONF.command.version)
 
 
 class ServiceManageCommand(object):
diff --git a/heat/db/sqlalchemy/api.py b/heat/db/sqlalchemy/api.py
index df627d43c..1556d2d76 100644
--- a/heat/db/sqlalchemy/api.py
+++ b/heat/db/sqlalchemy/api.py
@@ -39,7 +39,6 @@ from heat.common import crypt
 from heat.common import exception
 from heat.common.i18n import _
 from heat.db.sqlalchemy import filters as db_filters
-from heat.db.sqlalchemy import migration
 from heat.db.sqlalchemy import models
 from heat.db.sqlalchemy import utils as db_utils
 from heat.engine import environment as heat_environment
@@ -1622,19 +1621,6 @@ def sync_point_update_input_data(context, entity_id,
     return rows_updated
 
 
-def db_sync(engine, version=None):
-    """Migrate the database to `version` or the most recent version."""
-    if version is not None and int(version) < db_version(engine):
-        raise exception.Error(_("Cannot migrate to lower schema version."))
-
-    return migration.db_sync(engine, version=version)
-
-
-def db_version(engine):
-    """Display the current database version."""
-    return migration.db_version(engine)
-
-
 def _crypt_action(encrypt):
     if encrypt:
         return _('encrypt')
diff --git a/heat/db/sqlalchemy/migration.py b/heat/db/sqlalchemy/migration.py
index 7f030d90a..67ac4c3b4 100644
--- a/heat/db/sqlalchemy/migration.py
+++ b/heat/db/sqlalchemy/migration.py
@@ -1,4 +1,3 @@
-#
 #    Licensed under the Apache License, Version 2.0 (the "License"); you may
 #    not use this file except in compliance with the License. You may obtain
 #    a copy of the License at
@@ -13,26 +12,106 @@
 
 import os
 
-from oslo_db.sqlalchemy import migration as oslo_migration
+from alembic import command as alembic_api
+from alembic import config as alembic_config
+from alembic import migration as alembic_migration
+from oslo_log import log as logging
+import sqlalchemy as sa
+
+from heat.db.sqlalchemy import api as db_api
+
+LOG = logging.getLogger(__name__)
+
+ALEMBIC_INIT_VERSION = 'c6214ca60943'
+
+
+def _migrate_legacy_database(engine, connection, config):
+    """Check if database is a legacy sqlalchemy-migrate-managed database.
+
+    If it is, migrate it by "stamping" the initial alembic schema.
+    """
+    # If the database doesn't have the sqlalchemy-migrate legacy migration
+    # table, we don't have anything to do
+    if not sa.inspect(engine).has_table('migrate_version'):
+        return
+
+    # Likewise, if we've already migrated to alembic, we don't have anything to
+    # do
+    context = alembic_migration.MigrationContext.configure(connection)
+    if context.get_current_revision():
+        return
+
+    # We have legacy migrations but no alembic migration. Stamp (dummy apply)
+    # the initial alembic migration.
+
+    LOG.info(
+        'The database is still under sqlalchemy-migrate control; '
+        'fake applying the initial alembic migration'
+    )
+    alembic_api.stamp(config, ALEMBIC_INIT_VERSION)
+
+
+def _find_alembic_conf():
+    """Get the project's alembic configuration
+
+    :returns: An instance of ``alembic.config.Config``
+    """
+    path = os.path.join(
+        os.path.abspath(os.path.dirname(__file__)),
+        'alembic.ini',
+    )
+    config = alembic_config.Config(os.path.abspath(path))
+    # we don't want to use the logger configuration from the file, which is
+    # only really intended for the CLI
+    # https://stackoverflow.com/a/42691781/613428
+    config.attributes['configure_logger'] = False
+    return config
+
+
+def _upgrade_alembic(engine, config, version):
+    # re-use the connection rather than creating a new one
+    with engine.begin() as connection:
+        config.attributes['connection'] = connection
+        _migrate_legacy_database(engine, connection, config)
+        alembic_api.upgrade(config, version or 'head')
 
 
-INIT_VERSION = 72
+def db_sync(version=None, engine=None):
+    """Migrate the database to `version` or the most recent version."""
+    # if the user requested a specific version, check if it's an integer: if
+    # so, we're almost certainly in sqlalchemy-migrate land and won't support
+    # that
+    if version is not None and version.isdigit():
+        raise ValueError(
+            'You requested an sqlalchemy-migrate database version; this is '
+            'no longer supported'
+        )
 
+    if engine is None:
+        engine = db_api.get_engine()
 
-def db_sync(engine, version=None):
-    path = os.path.join(os.path.abspath(os.path.dirname(__file__)),
-                        'migrate_repo')
-    return oslo_migration.db_sync(engine, path, version,
-                                  init_version=INIT_VERSION)
+    config = _find_alembic_conf()
 
+    # discard the URL encoded in alembic.ini in favour of the URL configured
+    # for the engine by the database fixtures, casting from
+    # 'sqlalchemy.engine.url.URL' to str in the process. This returns a
+    # RFC-1738 quoted URL, which means that a password like "foo@" will be
+    # turned into "foo%40". This in turns causes a problem for
+    # set_main_option() because that uses ConfigParser.set, which (by design)
+    # uses *python* interpolation to write the string out ... where "%" is the
+    # special python interpolation character! Avoid this mismatch by quoting
+    # all %'s for the set below.
+    engine_url = str(engine.url).replace('%', '%%')
+    config.set_main_option('sqlalchemy.url', str(engine_url))
 
-def db_version(engine):
-    path = os.path.join(os.path.abspath(os.path.dirname(__file__)),
-                        'migrate_repo')
-    return oslo_migration.db_version(engine, path, INIT_VERSION)
+    LOG.info('Applying migration(s)')
+    _upgrade_alembic(engine, config, version)
+    LOG.info('Migration(s) applied')
 
 
-def db_version_control(engine, version=None):
-    path = os.path.join(os.path.abspath(os.path.dirname(__file__)),
-                        'migrate_repo')
-    return oslo_migration.db_version_control(engine, path, version)
+def db_version():
+    """Get database version."""
+    engine = db_api.get_engine()
+    with engine.connect() as connection:
+        m_context = alembic_migration.MigrationContext.configure(connection)
+        return m_context.get_current_revision()
diff --git a/heat/db/sqlalchemy/migrations/env.py b/heat/db/sqlalchemy/migrations/env.py
index 138d28374..84413f313 100644
--- a/heat/db/sqlalchemy/migrations/env.py
+++ b/heat/db/sqlalchemy/migrations/env.py
@@ -24,7 +24,7 @@ config = context.config
 
 # Interpret the config file for Python logging.
 # This line sets up loggers basically.
-if config.config_file_name is not None:
+if config.attributes.get('configure_logger', True):
     fileConfig(config.config_file_name)
 
 # this is the MetaData object for the various models in the database
@@ -58,16 +58,30 @@ def run_migrations_online() -> None:
 
     In this scenario we need to create an Engine and associate a connection
     with the context.
+
+    This is modified from the default based on the below, since we want to
+    share an engine when unit testing so in-memory database testing actually
+    works.
+
+    https://alembic.sqlalchemy.org/en/latest/cookbook.html#connection-sharing
     """
-    connectable = engine_from_config(
-        config.get_section(config.config_ini_section, {}),
-        prefix="sqlalchemy.",
-        poolclass=pool.NullPool,
-    )
+    connectable = config.attributes.get('connection', None)
+
+    if connectable is None:
+        # only create Engine if we don't have a Connection from the outside
+        connectable = engine_from_config(
+            config.get_section(config.config_ini_section, {}),
+            prefix="sqlalchemy.",
+            poolclass=pool.NullPool,
+        )
+
+    # when connectable is already a Connection object, calling connect() gives
+    # us a *branched connection*
 
     with connectable.connect() as connection:
         context.configure(
-            connection=connection, target_metadata=target_metadata
+            connection=connection,
+            target_metadata=target_metadata,
         )
 
         with context.begin_transaction():
diff --git a/heat/tests/db/test_migrations.py b/heat/tests/db/test_migrations.py
index 0dbf85bf3..31485c1b0 100644
--- a/heat/tests/db/test_migrations.py
+++ b/heat/tests/db/test_migrations.py
@@ -1,4 +1,3 @@
-#
 #    Licensed under the Apache License, Version 2.0 (the "License"); you may
 #    not use this file except in compliance with the License. You may obtain
 #    a copy of the License at
@@ -11,31 +10,18 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-"""
-Tests for database migrations. This test case reads the configuration
-file test_migrations.conf for database connection settings
-to use in the tests. For each connection found in the config file,
-the test case runs a series of test cases to ensure that migrations work
-properly both upgrading and downgrading, and that no data loss occurs
-if possible.
-"""
+"""Tests for database migrations."""
 
 import fixtures
-import os
-
-from migrate.versioning import repository
 from oslo_db.sqlalchemy import enginefacade
 from oslo_db.sqlalchemy import test_fixtures
 from oslo_db.sqlalchemy import test_migrations
-from oslo_db.sqlalchemy import utils
 from oslotest import base as test_base
 import sqlalchemy
 import testtools
 
-from heat.db.sqlalchemy import migrate_repo
 from heat.db.sqlalchemy import migration
 from heat.db.sqlalchemy import models
-from heat.tests import common
 
 
 class DBNotAllowed(Exception):
@@ -80,168 +66,14 @@ class TestBannedDBSchemaOperations(testtools.TestCase):
             self.assertRaises(DBNotAllowed, table.alter)
 
 
-class HeatMigrationsCheckers(test_migrations.WalkVersionsMixin,
-                             common.FakeLogMixin):
-    """Test sqlalchemy-migrate migrations."""
-
-    snake_walk = False
-    downgrade = False
-
-    @property
-    def INIT_VERSION(self):
-        return migration.INIT_VERSION
-
-    @property
-    def REPOSITORY(self):
-        migrate_file = migrate_repo.__file__
-        return repository.Repository(
-            os.path.abspath(os.path.dirname(migrate_file))
-        )
-
-    @property
-    def migration_api(self):
-        temp = __import__('oslo_db.sqlalchemy.migration', globals(),
-                          locals(), ['versioning_api'], 0)
-        return temp.versioning_api
-
-    @property
-    def migrate_engine(self):
-        return self.engine
-
-    def migrate_up(self, version, with_data=False):
-        """Check that migrations don't cause downtime.
-
-        Schema migrations can be done online, allowing for rolling upgrades.
-        """
-        # NOTE(xek): This is a list of migrations where we allow dropping
-        # things. The rules for adding exceptions are very very specific.
-        # Chances are you don't meet the critera.
-        # Reviewers: DO NOT ALLOW THINGS TO BE ADDED HERE
-        exceptions = [
-            64,  # drop constraint
-            86,  # drop watch_rule/watch_data tables
-        ]
-        # Reviewers: DO NOT ALLOW THINGS TO BE ADDED HERE
-
-        # NOTE(xek): We start requiring things be additive in
-        # liberty, so ignore all migrations before that point.
-        LIBERTY_START = 63
-
-        if version >= LIBERTY_START and version not in exceptions:
-            banned = ['Table', 'Column']
-        else:
-            banned = None
-        with BannedDBSchemaOperations(banned):
-            super(HeatMigrationsCheckers, self).migrate_up(version, with_data)
-
-    def test_walk_versions(self):
-        self.walk_versions(self.snake_walk, self.downgrade)
-
-    def assertColumnExists(self, engine, table, column):
-        t = utils.get_table(engine, table)
-        self.assertIn(column, t.c)
-
-    def assertColumnType(self, engine, table, column, sqltype):
-        t = utils.get_table(engine, table)
-        col = getattr(t.c, column)
-        self.assertIsInstance(col.type, sqltype)
-
-    def assertColumnNotExists(self, engine, table, column):
-        t = utils.get_table(engine, table)
-        self.assertNotIn(column, t.c)
-
-    def assertColumnIsNullable(self, engine, table, column):
-        t = utils.get_table(engine, table)
-        col = getattr(t.c, column)
-        self.assertTrue(col.nullable)
-
-    def assertColumnIsNotNullable(self, engine, table, column_name):
-        table = utils.get_table(engine, table)
-        column = getattr(table.c, column_name)
-        self.assertFalse(column.nullable)
-
-    def assertIndexExists(self, engine, table, index):
-        t = utils.get_table(engine, table)
-        index_names = [idx.name for idx in t.indexes]
-        self.assertIn(index, index_names)
+class HeatModelsMigrationsSync(test_migrations.ModelsMigrationsSync):
 
-    def assertIndexMembers(self, engine, table, index, members):
-        self.assertIndexExists(engine, table, index)
-
-        t = utils.get_table(engine, table)
-        index_columns = []
-        for idx in t.indexes:
-            if idx.name == index:
-                for ix in idx.columns:
-                    index_columns.append(ix.name)
-                break
-
-        self.assertEqual(sorted(members), sorted(index_columns))
-
-    def _check_073(self, engine, data):
-        # check if column still exists and is not nullable.
-        self.assertColumnIsNotNullable(engine, 'resource_data', 'resource_id')
-        # Ensure that only one foreign key exists and is created as expected.
-        inspector = sqlalchemy.engine.reflection.Inspector.from_engine(engine)
-        resource_data_fkeys = inspector.get_foreign_keys('resource_data')
-        self.assertEqual(1, len(resource_data_fkeys))
-        fk = resource_data_fkeys[0]
-        self.assertEqual('fk_resource_id', fk['name'])
-        self.assertEqual(['resource_id'], fk['constrained_columns'])
-        self.assertEqual('resource', fk['referred_table'])
-        self.assertEqual(['id'], fk['referred_columns'])
-
-    def _check_079(self, engine, data):
-        self.assertColumnExists(engine, 'resource',
-                                'rsrc_prop_data_id')
-        self.assertColumnExists(engine, 'event',
-                                'rsrc_prop_data_id')
-        column_list = [('id', False),
-                       ('data', True),
-                       ('encrypted', True),
-                       ('updated_at', True),
-                       ('created_at', True)]
-
-        for column in column_list:
-            self.assertColumnExists(engine,
-                                    'resource_properties_data', column[0])
-            if not column[1]:
-                self.assertColumnIsNotNullable(engine,
-                                               'resource_properties_data',
-                                               column[0])
-            else:
-                self.assertColumnIsNullable(engine,
-                                            'resource_properties_data',
-                                            column[0])
-
-    def _check_080(self, engine, data):
-        self.assertColumnExists(engine, 'resource',
-                                'attr_data_id')
-
-
-class DbTestCase(test_fixtures.OpportunisticDBTestMixin,
-                 test_base.BaseTestCase):
     def setUp(self):
-        super(DbTestCase, self).setUp()
+        super().setUp()
 
         self.engine = enginefacade.writer.get_engine()
         self.sessionmaker = enginefacade.writer.get_sessionmaker()
 
-
-class TestHeatMigrationsMySQL(DbTestCase, HeatMigrationsCheckers):
-    FIXTURE = test_fixtures.MySQLOpportunisticFixture
-
-
-class TestHeatMigrationsPostgreSQL(DbTestCase, HeatMigrationsCheckers):
-    FIXTURE = test_fixtures.PostgresqlOpportunisticFixture
-
-
-class TestHeatMigrationsSQLite(DbTestCase, HeatMigrationsCheckers):
-    pass
-
-
-class ModelsMigrationSyncMixin(object):
-
     def get_metadata(self):
         return models.BASE.metadata
 
@@ -252,24 +84,28 @@ class ModelsMigrationSyncMixin(object):
         migration.db_sync(engine=engine)
 
     def include_object(self, object_, name, type_, reflected, compare_to):
-        if name in ['migrate_version'] and type_ == 'table':
-            return False
         return True
 
 
-class ModelsMigrationsSyncMysql(DbTestCase,
-                                ModelsMigrationSyncMixin,
-                                test_migrations.ModelsMigrationsSync):
+class ModelsMigrationsSyncMysql(
+    HeatModelsMigrationsSync,
+    test_fixtures.OpportunisticDBTestMixin,
+    test_base.BaseTestCase,
+):
     FIXTURE = test_fixtures.MySQLOpportunisticFixture
 
 
-class ModelsMigrationsSyncPostgres(DbTestCase,
-                                   ModelsMigrationSyncMixin,
-                                   test_migrations.ModelsMigrationsSync):
+class ModelsMigrationsSyncPostgres(
+    HeatModelsMigrationsSync,
+    test_fixtures.OpportunisticDBTestMixin,
+    test_base.BaseTestCase,
+):
     FIXTURE = test_fixtures.PostgresqlOpportunisticFixture
 
 
-class ModelsMigrationsSyncSQLite(DbTestCase,
-                                 ModelsMigrationSyncMixin,
-                                 test_migrations.ModelsMigrationsSync):
+class ModelsMigrationsSyncSQLite(
+    HeatModelsMigrationsSync,
+    test_fixtures.OpportunisticDBTestMixin,
+    test_base.BaseTestCase,
+):
     pass
diff --git a/releasenotes/notes/switch-to-alembic-7af6f8e71e4bf56b.yaml b/releasenotes/notes/switch-to-alembic-7af6f8e71e4bf56b.yaml
new file mode 100644
index 000000000..883f74db3
--- /dev/null
+++ b/releasenotes/notes/switch-to-alembic-7af6f8e71e4bf56b.yaml
@@ -0,0 +1,22 @@
+---
+upgrade:
+  - |
+    The database migration engine has changed from `sqlalchemy-migrate`__ to
+    `alembic`__. For most deployments, this should have minimal to no impact
+    and the switch should be mostly transparent. The main user-facing impact is
+    the change in schema versioning. While sqlalchemy-migrate used a linear,
+    integer-based versioning scheme, which required placeholder migrations to
+    allow for potential migration backports, alembic uses a distributed version
+    control-like schema where a migration's ancestor is encoded in the file and
+    branches are possible. The alembic migration files therefore use a
+    arbitrary UUID-like naming scheme and the ``heat-manage db_sync`` command
+    now expects such an version when manually specifying the version that should
+    be applied. For example::
+
+        $ heat-manage db_sync c6214ca60943
+
+    Attempting to specify an sqlalchemy-migrate-based version will result in an
+    error.
+
+    .. __: https://sqlalchemy-migrate.readthedocs.io/en/latest/
+    .. __: https://alembic.sqlalchemy.org/en/latest/