diff --git a/bin/heat-db-setup b/bin/heat-db-setup index b0e39ac747..8df02aa082 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 c853d9ac25..9f7b8e3160 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 df627d43c2..1556d2d763 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 7f030d90a0..67ac4c3b4c 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' -INIT_VERSION = 72 +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 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) +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 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) +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') -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_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() + + 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)) + + LOG.info('Applying migration(s)') + _upgrade_alembic(engine, config, version) + LOG.info('Migration(s) applied') + + +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 138d283740..84413f3138 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 0dbf85bf3b..31485c1b0b 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.""" +class HeatModelsMigrationsSync(test_migrations.ModelsMigrationsSync): - 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) - - 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 0000000000..883f74db36 --- /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/