From 64621053c298d6f7be97d7735d843e721e5692e9 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 22 Mar 2023 17:54:25 +0000 Subject: [PATCH] db: Migrate to alembic This is significantly easier than Cinder, Nova etc. as Heat hasn't had any database migrations in multiple cycles, meaning we don't need to worry about having to apply any older sqlalchemy-migrate migrations before switching to alembic. Instead, we simply need to determine we're upgrading a deployment that was previously using sqlalchemy-migrate, upgrading a deployment that has already migrated to alembic, or deploying a new deployment, adjusting accordingly. Signed-off-by: Stephen Finucane Change-Id: I808af9cb21ba21808209b1daddac7426f4cad310 --- bin/heat-db-setup | 2 +- heat/cmd/manage.py | 6 +- heat/db/sqlalchemy/api.py | 14 -- heat/db/sqlalchemy/migration.py | 111 ++++++++-- heat/db/sqlalchemy/migrations/env.py | 28 ++- heat/tests/db/test_migrations.py | 200 ++---------------- .../switch-to-alembic-7af6f8e71e4bf56b.yaml | 22 ++ 7 files changed, 160 insertions(+), 223 deletions(-) create mode 100644 releasenotes/notes/switch-to-alembic-7af6f8e71e4bf56b.yaml 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/