diff --git a/cinder/db/migration.py b/cinder/db/migration.py index 5c5458b644a..e3222f733ca 100644 --- a/cinder/db/migration.py +++ b/cinder/db/migration.py @@ -18,135 +18,152 @@ import os +from alembic import command as alembic_api +from alembic import config as alembic_config +from alembic import migration as alembic_migration from migrate import exceptions as migrate_exceptions from migrate.versioning import api as migrate_api -from migrate.versioning import repository as migrate_repository +from migrate.versioning import repository as migrate_repo from oslo_config import cfg -from oslo_db import exception from oslo_db import options -import sqlalchemy as sa +from oslo_log import log as logging from cinder.db.sqlalchemy import api as db_api -from cinder.i18n import _ options.set_defaults(cfg.CONF) -INIT_VERSION = 134 -LEGACY_MIGRATIONS_PATH = os.path.join( - os.path.abspath(os.path.dirname(__file__)), - 'legacy_migrations', -) +LOG = logging.getLogger(__name__) + +MIGRATE_INIT_VERSION = 134 +MIGRATE_MIGRATIONS_PATH = ALEMBIC_INIT_VERSION = '921e1a36b076' -def _find_migrate_repo(abs_path): +def _find_migrate_repo(): """Get the project's change script repository - :param abs_path: Absolute path to migrate repository + :returns: An instance of ``migrate.versioning.repository.Repository`` """ - if not os.path.exists(abs_path): - raise exception.DBMigrationError("Path %s not found" % abs_path) - return migrate_repository.Repository(abs_path) - - -def _migrate_db_version_control(engine, abs_path, version=None): - """Mark a database as under this repository's version control. - - Once a database is under version control, schema changes should - only be done via change scripts in this repository. - - :param engine: SQLAlchemy engine instance for a given database - :param abs_path: Absolute path to migrate repository - :param version: Initial database version - """ - repository = _find_migrate_repo(abs_path) - - try: - migrate_api.version_control(engine, repository, version) - except migrate_exceptions.InvalidVersionError as ex: - raise exception.DBMigrationError("Invalid version : %s" % ex) - except migrate_exceptions.DatabaseAlreadyControlledError: - raise exception.DBMigrationError("Database is already controlled.") - - return version - - -def _migrate_db_version(engine, abs_path, init_version): - """Show the current version of the repository. - - :param engine: SQLAlchemy engine instance for a given database - :param abs_path: Absolute path to migrate repository - :param init_version: Initial database version - """ - repository = _find_migrate_repo(abs_path) - try: - return migrate_api.db_version(engine, repository) - except migrate_exceptions.DatabaseNotControlledError: - pass - - meta = sa.MetaData() - meta.reflect(bind=engine) - tables = meta.tables - if ( - len(tables) == 0 or - 'alembic_version' in tables or - 'migrate_version' in tables - ): - _migrate_db_version_control(engine, abs_path, version=init_version) - return migrate_api.db_version(engine, repository) - - msg = _( - "The database is not under version control, but has tables. " - "Please stamp the current version of the schema manually." + path = os.path.join( + os.path.abspath(os.path.dirname(__file__)), 'legacy_migrations', ) - raise exception.DBMigrationError(msg) + return migrate_repo.Repository(path) -def _migrate_db_sync(engine, abs_path, version=None, init_version=0): - """Upgrade or downgrade a database. +def _find_alembic_conf(): + """Get the project's alembic configuration - Function runs the upgrade() or downgrade() functions in change scripts. - - :param engine: SQLAlchemy engine instance for a given database - :param abs_path: Absolute path to migrate repository. - :param version: Database will upgrade/downgrade until this version. - If None - database will update to the latest available version. - :param init_version: Initial database version + :returns: An instance of ``alembic.config.Config`` """ + path = os.path.join( + os.path.abspath(os.path.dirname(__file__)), 'alembic.ini') - if version is not None: - try: - version = int(version) - except ValueError: - raise exception.DBMigrationError(_("version should be an integer")) + 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 - current_version = _migrate_db_version(engine, abs_path, init_version) - repository = _find_migrate_repo(abs_path) + return config - if version is None or version > current_version: - try: - return migrate_api.upgrade(engine, repository, version) - except Exception as ex: - raise exception.DBMigrationError(ex) - else: - return migrate_api.downgrade(engine, repository, version) + +def _is_database_under_migrate_control(engine, repository): + try: + migrate_api.db_version(engine, repository) + return True + except migrate_exceptions.DatabaseNotControlledError: + return False + + +def _is_database_under_alembic_control(engine): + with engine.connect() as conn: + context = alembic_migration.MigrationContext.configure(conn) + return bool(context.get_current_revision()) + + +def _init_alembic_on_legacy_database(engine, repository, config): + """Init alembic in an existing environment with sqlalchemy-migrate.""" + LOG.info( + 'The database is still under sqlalchemy-migrate control; ' + 'applying any remaining sqlalchemy-migrate-based migrations ' + 'and fake applying the initial alembic migration' + ) + migrate_api.upgrade(engine, repository) + + # re-use the connection rather than creating a new one + with engine.begin() as connection: + config.attributes['connection'] = connection + alembic_api.stamp(config, ALEMBIC_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 + alembic_api.upgrade(config, version or 'head') def db_version(): """Get database version.""" - return _migrate_db_version( - db_api.get_engine(), - LEGACY_MIGRATIONS_PATH, - INIT_VERSION) + repository = _find_migrate_repo() + engine = db_api.get_engine() + + migrate_version = None + if _is_database_under_migrate_control(engine, repository): + migrate_version = migrate_api.db_version(engine, repository) + + alembic_version = None + if _is_database_under_alembic_control(engine): + alembic_version = alembic_api.current(engine) + + return alembic_version or migrate_version def db_sync(version=None, engine=None): - """Migrate the database to `version` or the most recent version.""" + """Migrate the database to `version` or the most recent version. + + We're currently straddling two migration systems, sqlalchemy-migrate and + alembic. This handles both by ensuring we switch from one to the other at + the appropriate moment. + """ + + # 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() - return _migrate_db_sync( - engine=engine, - abs_path=LEGACY_MIGRATIONS_PATH, - version=version, - init_version=INIT_VERSION) + repository = _find_migrate_repo() + 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)) + + # if we're in a deployment where sqlalchemy-migrate is already present, + # then apply all the updates for that and fake apply the initial alembic + # migration; if we're not then 'upgrade' will take care of everything + # this should be a one-time operation + if ( + _is_database_under_migrate_control(engine, repository) and + not _is_database_under_alembic_control(engine) + ): + _init_alembic_on_legacy_database(engine, repository, config) + + # apply anything later + LOG.info('Applying migration(s)') + _upgrade_alembic(engine, config, version) + LOG.info('Migration(s) applied') diff --git a/cinder/db/migrations/env.py b/cinder/db/migrations/env.py index 49efd90f3f8..3a139ad17c8 100644 --- a/cinder/db/migrations/env.py +++ b/cinder/db/migrations/env.py @@ -20,9 +20,10 @@ from sqlalchemy import pool # access to the values within the .ini file in use. config = context.config -# Interpret the config file for Python logging. +# Interpret the config file for Python logging unless we're told not to. # This line sets up loggers basically. -fileConfig(config.config_file_name) +if config.attributes.get('configure_logger', True): + fileConfig(config.config_file_name) # add your model's MetaData object here # for 'autogenerate' support @@ -57,12 +58,25 @@ def run_migrations_online(): 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( diff --git a/cinder/tests/unit/db/test_migration.py b/cinder/tests/unit/db/test_migration.py index 1a1de9da1ca..2caa5dffae2 100644 --- a/cinder/tests/unit/db/test_migration.py +++ b/cinder/tests/unit/db/test_migration.py @@ -10,243 +10,177 @@ # License for the specific language governing permissions and limitations # under the License. -import os -import tempfile from unittest import mock -from migrate import exceptions as migrate_exception +from alembic import command as alembic_api +from alembic.runtime import migration as alembic_migration +from migrate import exceptions as migrate_exceptions from migrate.versioning import api as migrate_api -from migrate.versioning import repository as migrate_repository -from oslo_db import exception as db_exception -from oslo_db.sqlalchemy import enginefacade -from oslo_db.sqlalchemy import test_fixtures as db_fixtures from oslotest import base as test_base -import sqlalchemy from cinder.db import migration -from cinder import utils +from cinder.db.sqlalchemy import api as db_api -class TestMigrationCommon( - db_fixtures.OpportunisticDBTestMixin, test_base.BaseTestCase, -): +class TestDBSync(test_base.BaseTestCase): - def setUp(self): - super().setUp() + def test_db_sync_legacy_version(self): + """We don't allow users to request legacy versions.""" + self.assertRaises(ValueError, migration.db_sync, '402') - self.engine = enginefacade.writer.get_engine() - - self.path = tempfile.mkdtemp('test_migration') - self.path1 = tempfile.mkdtemp('test_migration') - self.return_value = '/home/openstack/migrations' - self.return_value1 = '/home/extension/migrations' - self.init_version = 1 - self.test_version = 123 - - self.patcher_repo = mock.patch.object(migrate_repository, 'Repository') - self.repository = self.patcher_repo.start() - self.repository.side_effect = [self.return_value, self.return_value1] - - self.mock_api_db = mock.patch.object(migrate_api, 'db_version') - self.mock_api_db_version = self.mock_api_db.start() - self.mock_api_db_version.return_value = self.test_version - - def tearDown(self): - os.rmdir(self.path) - self.mock_api_db.stop() - self.patcher_repo.stop() - super().tearDown() - - def test_find_migrate_repo_path_not_found(self): - self.assertRaises( - db_exception.DBMigrationError, - migration._find_migrate_repo, - "/foo/bar/", + @mock.patch.object(migration, '_upgrade_alembic') + @mock.patch.object(migration, '_init_alembic_on_legacy_database') + @mock.patch.object(migration, '_is_database_under_alembic_control') + @mock.patch.object(migration, '_is_database_under_migrate_control') + @mock.patch.object(migration, '_find_alembic_conf') + @mock.patch.object(migration, '_find_migrate_repo') + @mock.patch.object(db_api, 'get_engine') + def _test_db_sync( + self, has_migrate, has_alembic, mock_get_engine, mock_find_repo, + mock_find_conf, mock_is_migrate, mock_is_alembic, mock_init, + mock_upgrade, + ): + mock_is_migrate.return_value = has_migrate + mock_is_alembic.return_value = has_alembic + migration.db_sync() + mock_get_engine.assert_called_once_with() + mock_find_repo.assert_called_once_with() + mock_find_conf.assert_called_once_with() + mock_find_conf.return_value.set_main_option.assert_called_once_with( + 'sqlalchemy.url', str(mock_get_engine.return_value.url), ) + mock_is_migrate.assert_called_once_with( + mock_get_engine.return_value, mock_find_repo.return_value) + if has_migrate: + mock_is_alembic.assert_called_once_with( + mock_get_engine.return_value) + else: + mock_is_alembic.assert_not_called() - def test_find_migrate_repo_called_once(self): - my_repository = migration._find_migrate_repo(self.path) - self.repository.assert_called_once_with(self.path) - self.assertEqual(self.return_value, my_repository) + # we should only attempt the upgrade of the remaining + # sqlalchemy-migrate-based migrations and fake apply of the initial + # alembic migrations if sqlalchemy-migrate is in place but alembic + # hasn't been used yet + if has_migrate and not has_alembic: + mock_init.assert_called_once_with( + mock_get_engine.return_value, + mock_find_repo.return_value, mock_find_conf.return_value) + else: + mock_init.assert_not_called() - def test_find_migrate_repo_called_few_times(self): - repo1 = migration._find_migrate_repo(self.path) - repo2 = migration._find_migrate_repo(self.path1) - self.assertNotEqual(repo1, repo2) + # however, we should always attempt to upgrade the requested migration + # to alembic + mock_upgrade.assert_called_once_with( + mock_get_engine.return_value, mock_find_conf.return_value, None) - def test_db_version_control(self): - with utils.nested_contexts( - mock.patch.object(migration, '_find_migrate_repo'), - mock.patch.object(migrate_api, 'version_control'), - ) as (mock_find_repo, mock_version_control): - mock_find_repo.return_value = self.return_value + def test_db_sync_new_deployment(self): + """Mimic a new deployment without existing sqlalchemy-migrate cruft.""" + has_migrate = False + has_alembic = False + self._test_db_sync(has_migrate, has_alembic) - version = migration._migrate_db_version_control( - self.engine, self.path, self.test_version) + def test_db_sync_with_existing_migrate_database(self): + """Mimic a deployment currently managed by sqlalchemy-migrate.""" + has_migrate = True + has_alembic = False + self._test_db_sync(has_migrate, has_alembic) - self.assertEqual(self.test_version, version) - mock_version_control.assert_called_once_with( - self.engine, self.return_value, self.test_version) + def test_db_sync_with_existing_alembic_database(self): + """Mimic a deployment that's already switched to alembic.""" + has_migrate = True + has_alembic = True + self._test_db_sync(has_migrate, has_alembic) - @mock.patch.object(migration, '_find_migrate_repo') - @mock.patch.object(migrate_api, 'version_control') - def test_db_version_control_version_less_than_actual_version( - self, mock_version_control, mock_find_repo, + +@mock.patch.object(alembic_api, 'current') +@mock.patch.object(migrate_api, 'db_version') +@mock.patch.object(migration, '_is_database_under_alembic_control') +@mock.patch.object(migration, '_is_database_under_migrate_control') +@mock.patch.object(db_api, 'get_engine') +@mock.patch.object(migration, '_find_migrate_repo') +class TestDBVersion(test_base.BaseTestCase): + + def test_db_version_migrate( + self, mock_find_repo, mock_get_engine, mock_is_migrate, + mock_is_alembic, mock_migrate_version, mock_alembic_version, ): - mock_find_repo.return_value = self.return_value - mock_version_control.side_effect = \ - migrate_exception.DatabaseAlreadyControlledError - self.assertRaises( - db_exception.DBMigrationError, - migration._migrate_db_version_control, self.engine, - self.path, self.test_version - 1) + """Database is controlled by sqlalchemy-migrate.""" + mock_is_migrate.return_value = True + mock_is_alembic.return_value = False + ret = migration.db_version() + self.assertEqual(mock_migrate_version.return_value, ret) + mock_find_repo.assert_called_once_with() + mock_get_engine.assert_called_once_with() + mock_is_migrate.assert_called_once() + mock_is_alembic.assert_called_once() + mock_migrate_version.assert_called_once_with( + mock_get_engine.return_value, mock_find_repo.return_value) + mock_alembic_version.assert_not_called() - @mock.patch.object(migration, '_find_migrate_repo') - @mock.patch.object(migrate_api, 'version_control') - def test_db_version_control_version_greater_than_actual_version( - self, mock_version_control, mock_find_repo, + def test_db_version_alembic( + self, mock_find_repo, mock_get_engine, mock_is_migrate, + mock_is_alembic, mock_migrate_version, mock_alembic_version, ): - mock_find_repo.return_value = self.return_value - mock_version_control.side_effect = \ - migrate_exception.InvalidVersionError - self.assertRaises( - db_exception.DBMigrationError, - migration._migrate_db_version_control, self.engine, - self.path, self.test_version + 1) + """Database is controlled by alembic.""" + mock_is_migrate.return_value = False + mock_is_alembic.return_value = True + ret = migration.db_version() + self.assertEqual(mock_alembic_version.return_value, ret) + mock_find_repo.assert_called_once_with() + mock_get_engine.assert_called_once_with() + mock_is_migrate.assert_called_once() + mock_is_alembic.assert_called_once() + mock_migrate_version.assert_not_called() + mock_alembic_version.assert_called_once_with( + mock_get_engine.return_value) - def test_db_version_return(self): - ret_val = migration._migrate_db_version( - self.engine, self.path, self.init_version) - self.assertEqual(self.test_version, ret_val) + def test_db_version_not_controlled( + self, mock_find_repo, mock_get_engine, mock_is_migrate, + mock_is_alembic, mock_migrate_version, mock_alembic_version, + ): + """Database is not controlled.""" + mock_is_migrate.return_value = False + mock_is_alembic.return_value = False + ret = migration.db_version() + self.assertIsNone(ret) + mock_find_repo.assert_called_once_with() + mock_get_engine.assert_called_once_with() + mock_is_migrate.assert_called_once() + mock_is_alembic.assert_called_once() + mock_migrate_version.assert_not_called() + mock_alembic_version.assert_not_called() - def test_db_version_raise_not_controlled_error_first(self): - with mock.patch.object( - migration, '_migrate_db_version_control', - ) as mock_ver: - self.mock_api_db_version.side_effect = [ - migrate_exception.DatabaseNotControlledError('oups'), - self.test_version] - ret_val = migration._migrate_db_version( - self.engine, self.path, self.init_version) - self.assertEqual(self.test_version, ret_val) - mock_ver.assert_called_once_with( - self.engine, self.path, version=self.init_version) +class TestDatabaseUnderVersionControl(test_base.BaseTestCase): - def test_db_version_raise_not_controlled_error_tables(self): - with mock.patch.object(sqlalchemy, 'MetaData') as mock_meta: - self.mock_api_db_version.side_effect = \ - migrate_exception.DatabaseNotControlledError('oups') - my_meta = mock.MagicMock() - my_meta.tables = {'a': 1, 'b': 2} - mock_meta.return_value = my_meta + @mock.patch.object(migrate_api, 'db_version') + def test__is_database_under_migrate_control__true(self, mock_db_version): + ret = migration._is_database_under_migrate_control('engine', 'repo') + self.assertTrue(ret) + mock_db_version.assert_called_once_with('engine', 'repo') - self.assertRaises( - db_exception.DBMigrationError, migration._migrate_db_version, - self.engine, self.path, self.init_version) + @mock.patch.object(migrate_api, 'db_version') + def test__is_database_under_migrate_control__false(self, mock_db_version): + mock_db_version.side_effect = \ + migrate_exceptions.DatabaseNotControlledError() + ret = migration._is_database_under_migrate_control('engine', 'repo') + self.assertFalse(ret) + mock_db_version.assert_called_once_with('engine', 'repo') - @mock.patch.object(migrate_api, 'version_control') - def test_db_version_raise_not_controlled_error_no_tables(self, mock_vc): - with mock.patch.object(sqlalchemy, 'MetaData') as mock_meta: - self.mock_api_db_version.side_effect = ( - migrate_exception.DatabaseNotControlledError('oups'), - self.init_version) - my_meta = mock.MagicMock() - my_meta.tables = {} - mock_meta.return_value = my_meta + @mock.patch.object(alembic_migration.MigrationContext, 'configure') + def test__is_database_under_alembic_control__true(self, mock_configure): + context = mock_configure.return_value + context.get_current_revision.return_value = 'foo' + engine = mock.MagicMock() + ret = migration._is_database_under_alembic_control(engine) + self.assertTrue(ret) + context.get_current_revision.assert_called_once_with() - migration._migrate_db_version( - self.engine, self.path, self.init_version) - - mock_vc.assert_called_once_with( - self.engine, self.return_value1, self.init_version) - - @mock.patch.object(migrate_api, 'version_control') - def test_db_version_raise_not_controlled_alembic_tables(self, mock_vc): - # When there are tables but the alembic control table - # (alembic_version) is present, attempt to version the db. - # This simulates the case where there is are multiple repos (different - # abs_paths) and a different path has been versioned already. - with mock.patch.object(sqlalchemy, 'MetaData') as mock_meta: - self.mock_api_db_version.side_effect = [ - migrate_exception.DatabaseNotControlledError('oups'), None] - my_meta = mock.MagicMock() - my_meta.tables = {'alembic_version': 1, 'b': 2} - mock_meta.return_value = my_meta - - migration._migrate_db_version( - self.engine, self.path, self.init_version) - - mock_vc.assert_called_once_with( - self.engine, self.return_value1, self.init_version) - - @mock.patch.object(migrate_api, 'version_control') - def test_db_version_raise_not_controlled_migrate_tables(self, mock_vc): - # When there are tables but the sqlalchemy-migrate control table - # (migrate_version) is present, attempt to version the db. - # This simulates the case where there is are multiple repos (different - # abs_paths) and a different path has been versioned already. - with mock.patch.object(sqlalchemy, 'MetaData') as mock_meta: - self.mock_api_db_version.side_effect = [ - migrate_exception.DatabaseNotControlledError('oups'), None] - my_meta = mock.MagicMock() - my_meta.tables = {'migrate_version': 1, 'b': 2} - mock_meta.return_value = my_meta - - migration._migrate_db_version( - self.engine, self.path, self.init_version) - - mock_vc.assert_called_once_with( - self.engine, self.return_value1, self.init_version) - - def test_db_sync_wrong_version(self): - self.assertRaises( - db_exception.DBMigrationError, - migration._migrate_db_sync, self.engine, self.path, 'foo') - - @mock.patch.object(migrate_api, 'upgrade') - def test_db_sync_script_not_present(self, upgrade): - # For non existent migration script file sqlalchemy-migrate will raise - # VersionNotFoundError which will be wrapped in DBMigrationError. - upgrade.side_effect = migrate_exception.VersionNotFoundError - self.assertRaises( - db_exception.DBMigrationError, - migration._migrate_db_sync, self.engine, self.path, - self.test_version + 1) - - @mock.patch.object(migrate_api, 'upgrade') - def test_db_sync_known_error_raised(self, upgrade): - upgrade.side_effect = migrate_exception.KnownError - self.assertRaises( - db_exception.DBMigrationError, - migration._migrate_db_sync, self.engine, self.path, - self.test_version + 1) - - def test_db_sync_upgrade(self): - init_ver = 55 - with utils.nested_contexts( - mock.patch.object(migration, '_find_migrate_repo'), - mock.patch.object(migrate_api, 'upgrade') - ) as (mock_find_repo, mock_upgrade): - mock_find_repo.return_value = self.return_value - self.mock_api_db_version.return_value = self.test_version - 1 - - migration._migrate_db_sync( - self.engine, self.path, self.test_version, init_ver) - - mock_upgrade.assert_called_once_with( - self.engine, self.return_value, self.test_version) - - def test_db_sync_downgrade(self): - with utils.nested_contexts( - mock.patch.object(migration, '_find_migrate_repo'), - mock.patch.object(migrate_api, 'downgrade') - ) as (mock_find_repo, mock_downgrade): - mock_find_repo.return_value = self.return_value - self.mock_api_db_version.return_value = self.test_version + 1 - - migration._migrate_db_sync( - self.engine, self.path, self.test_version) - - mock_downgrade.assert_called_once_with( - self.engine, self.return_value, self.test_version) + @mock.patch.object(alembic_migration.MigrationContext, 'configure') + def test__is_database_under_alembic_control__false(self, mock_configure): + context = mock_configure.return_value + context.get_current_revision.return_value = None + engine = mock.MagicMock() + ret = migration._is_database_under_alembic_control(engine) + self.assertFalse(ret) + context.get_current_revision.assert_called_once_with() diff --git a/cinder/tests/unit/db/test_migrations.py b/cinder/tests/unit/db/test_migrations.py index ca81a7d4772..f1f3d0d00bb 100644 --- a/cinder/tests/unit/db/test_migrations.py +++ b/cinder/tests/unit/db/test_migrations.py @@ -11,18 +11,17 @@ # 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, +Tests for database migrations. For each database backend supported by cinder, 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. +properly and that no data loss occurs if possible. """ import os +from alembic import command as alembic_api +from alembic import script as alembic_script import fixtures -from migrate.versioning import api as migration_api +from migrate.versioning import api as migrate_api from migrate.versioning import repository from oslo_db.sqlalchemy import enginefacade from oslo_db.sqlalchemy import test_fixtures @@ -38,7 +37,83 @@ from cinder.tests.unit import utils as test_utils from cinder.volume import volume_types -class MigrationsMixin(test_migrations.WalkVersionsMixin): +class MigrationsWalk( + test_fixtures.OpportunisticDBTestMixin, test_base.BaseTestCase, +): + def setUp(self): + super().setUp() + self.engine = enginefacade.writer.get_engine() + self.config = migration._find_alembic_conf() + self.init_version = migration.ALEMBIC_INIT_VERSION + + def _migrate_up(self, revision): + if revision == self.init_version: # no tests for the initial revision + return + + self.assertIsNotNone( + getattr(self, '_check_%s' % revision, None), + ( + 'API DB Migration %s does not have a test; you must add one' + ) % revision, + ) + alembic_api.upgrade(self.config, revision) + + def test_single_base_revision(self): + """Ensure we only have a single base revision. + + There's no good reason for us to have diverging history, so validate + that only one base revision exists. This will prevent simple errors + where people forget to specify the base revision. If this fail for your + change, look for migrations that do not have a 'revises' line in them. + """ + script = alembic_script.ScriptDirectory.from_config(self.config) + self.assertEqual(1, len(script.get_bases())) + + def test_single_head_revision(self): + """Ensure we only have a single head revision. + + There's no good reason for us to have diverging history, so validate + that only one head revision exists. This will prevent merge conflicts + adding additional head revision points. If this fail for your change, + look for migrations with the same 'revises' line in them. + """ + script = alembic_script.ScriptDirectory.from_config(self.config) + self.assertEqual(1, len(script.get_heads())) + + def test_walk_versions(self): + with self.engine.begin() as connection: + self.config.attributes['connection'] = connection + script = alembic_script.ScriptDirectory.from_config(self.config) + for revision_script in script.walk_revisions(): + revision = revision_script.revision + self._migrate_up(revision) + + +class TestMigrationsWalkSQLite( + MigrationsWalk, + test_fixtures.OpportunisticDBTestMixin, + test_base.BaseTestCase, +): + pass + + +class TestMigrationsWalkMySQL( + MigrationsWalk, + test_fixtures.OpportunisticDBTestMixin, + test_base.BaseTestCase, +): + FIXTURE = test_fixtures.MySQLOpportunisticFixture + + +class TestMigrationsWalkPostgreSQL( + MigrationsWalk, + test_fixtures.OpportunisticDBTestMixin, + test_base.BaseTestCase, +): + FIXTURE = test_fixtures.PostgresqlOpportunisticFixture + + +class LegacyMigrationsWalk(test_migrations.WalkVersionsMixin): """Test sqlalchemy-migrate migrations.""" BOOL_TYPE = sqlalchemy.types.BOOLEAN @@ -47,9 +122,13 @@ class MigrationsMixin(test_migrations.WalkVersionsMixin): VARCHAR_TYPE = sqlalchemy.types.VARCHAR TEXT_TYPE = sqlalchemy.types.Text + def setUp(self): + super().setUp() + self.engine = enginefacade.writer.get_engine() + @property def INIT_VERSION(self): - return migration.INIT_VERSION + return migration.MIGRATE_INIT_VERSION @property def REPOSITORY(self): @@ -59,16 +138,7 @@ class MigrationsMixin(test_migrations.WalkVersionsMixin): @property def migration_api(self): - return migration_api - - def setUp(self): - super(MigrationsMixin, self).setUp() - - # (zzzeek) This mixin states that it uses the - # "self.engine" attribute in the migrate_engine() method. - # So the mixin must set that up for itself, oslo_db no longer - # makes these assumptions for you. - self.engine = enginefacade.writer.get_engine() + return migrate_api @property def migrate_engine(self): @@ -81,7 +151,7 @@ class MigrationsMixin(test_migrations.WalkVersionsMixin): class BannedDBSchemaOperations(fixtures.Fixture): """Ban some operations for migrations""" def __init__(self, banned_resources=None): - super(MigrationsMixin.BannedDBSchemaOperations, self).__init__() + super().__init__() self._banned_resources = banned_resources or [] @staticmethod @@ -92,7 +162,7 @@ class MigrationsMixin(test_migrations.WalkVersionsMixin): resource, op)) def setUp(self): - super(MigrationsMixin.BannedDBSchemaOperations, self).setUp() + super().setUp() for thing in self._banned_resources: self.useFixture(fixtures.MonkeyPatch( 'sqlalchemy.%s.drop' % thing, @@ -123,8 +193,9 @@ class MigrationsMixin(test_migrations.WalkVersionsMixin): banned = ['Table', 'Column'] else: banned = None - with MigrationsMixin.BannedDBSchemaOperations(banned): - super(MigrationsMixin, self).migrate_up(version, with_data) + + with LegacyMigrationsWalk.BannedDBSchemaOperations(banned): + super().migrate_up(version, with_data) def __check_cinderbase_fields(self, columns): """Check fields inherited from CinderBase ORM class.""" @@ -217,9 +288,11 @@ class MigrationsMixin(test_migrations.WalkVersionsMixin): self.assert_each_foreign_key_is_part_of_an_index() -class TestSqliteMigrations(test_fixtures.OpportunisticDBTestMixin, - MigrationsMixin, - test_base.BaseTestCase): +class TestLegacyMigrationsWalkSQLite( + test_fixtures.OpportunisticDBTestMixin, + LegacyMigrationsWalk, + test_base.BaseTestCase, +): def assert_each_foreign_key_is_part_of_an_index(self): # Skip the test for SQLite because SQLite does not list @@ -228,9 +301,11 @@ class TestSqliteMigrations(test_fixtures.OpportunisticDBTestMixin, pass -class TestMysqlMigrations(test_fixtures.OpportunisticDBTestMixin, - MigrationsMixin, - test_base.BaseTestCase): +class TestLegacyMigrationsWalkMySQL( + test_fixtures.OpportunisticDBTestMixin, + LegacyMigrationsWalk, + test_base.BaseTestCase, +): FIXTURE = test_fixtures.MySQLOpportunisticFixture BOOL_TYPE = sqlalchemy.dialects.mysql.TINYINT @@ -241,7 +316,10 @@ class TestMysqlMigrations(test_fixtures.OpportunisticDBTestMixin, # add this to the global lists to make reset work with it, it's removed # automatically in tearDown so no need to clean it up here. # sanity check - migration.db_sync(engine=self.migrate_engine) + repo = migration._find_migrate_repo() + migrate_api.version_control( + self.migrate_engine, repo, migration.MIGRATE_INIT_VERSION) + migrate_api.upgrade(self.migrate_engine, repo) total = self.migrate_engine.execute( "SELECT count(*) " @@ -268,13 +346,14 @@ class TestMysqlMigrations(test_fixtures.OpportunisticDBTestMixin, # Depending on the MariaDB version, and the page size, we may not have # been able to change quota_usage_resource to 300 chars, it could still # be 255. - self.assertIn(quota_usage_resource.c.resource.type.length, - (255, 300)) + self.assertIn(quota_usage_resource.c.resource.type.length, (255, 300)) -class TestPostgresqlMigrations(test_fixtures.OpportunisticDBTestMixin, - MigrationsMixin, - test_base.BaseTestCase): +class TestLegacyMigrationsWalkPostgreSQL( + test_fixtures.OpportunisticDBTestMixin, + LegacyMigrationsWalk, + test_base.BaseTestCase, +): FIXTURE = test_fixtures.PostgresqlOpportunisticFixture TIME_TYPE = sqlalchemy.types.TIMESTAMP diff --git a/releasenotes/notes/switch-to-alembic-2bbe27749fde70ff.yaml b/releasenotes/notes/switch-to-alembic-2bbe27749fde70ff.yaml new file mode 100644 index 00000000000..f0c3e6e0495 --- /dev/null +++ b/releasenotes/notes/switch-to-alembic-2bbe27749fde70ff.yaml @@ -0,0 +1,25 @@ +--- +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 ``cinder-manage db sync`` command + now expects such an version when manually specifying the version that should + be applied. For example:: + + $ cinder-manage db sync 921e1a36b076 + + It is no longer possible to specify an sqlalchemy-migrate-based version. + When the ``cinder-manage db sync`` command is run, all remaining + sqlalchemy-migrate-based migrations will be automatically applied. + 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/