From 4bbea58cbb0f24f5be52738d8361c37d286506eb Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sat, 13 Feb 2021 16:45:30 +0000 Subject: [PATCH] db: Move db.sqalchemy.migration to db.migration At this point is nova's lifecycle, we're not going to change our ORM, making the layer of indirection that is 'nova.db.sqlalchemy' is a useless one. Start removing it with the 'migration' module. Change-Id: I5fd6c6f74b7de2b422359cbe72defb07252e6b1e Signed-off-by: Stephen Finucane --- nova/db/migration.py | 116 ++++++++++++++- nova/db/sqlalchemy/migration.py | 140 ------------------ .../functional/db/api/test_migrations.py | 8 +- nova/tests/unit/cmd/test_manage.py | 31 ++-- ...alchemy_migration.py => test_migration.py} | 2 +- nova/tests/unit/db/test_migrations.py | 15 +- 6 files changed, 135 insertions(+), 177 deletions(-) delete mode 100644 nova/db/sqlalchemy/migration.py rename nova/tests/unit/db/{test_sqlalchemy_migration.py => test_migration.py} (99%) diff --git a/nova/db/migration.py b/nova/db/migration.py index c4aa47612c..c7a32423ac 100644 --- a/nova/db/migration.py +++ b/nova/db/migration.py @@ -14,23 +14,127 @@ # License for the specific language governing permissions and limitations # under the License. -"""Database setup and migration commands.""" +import os -from nova.db.sqlalchemy import migration +from migrate import exceptions as versioning_exceptions +from migrate.versioning import api as versioning_api +from migrate.versioning.repository import Repository +from oslo_log import log as logging +import sqlalchemy -IMPL = migration +from nova.db.sqlalchemy import api as db_session +from nova import exception +from nova.i18n import _ + +INIT_VERSION = {} +INIT_VERSION['main'] = 401 +INIT_VERSION['api'] = 66 +_REPOSITORY = {} + +LOG = logging.getLogger(__name__) + + +def get_engine(database='main', context=None): + if database == 'main': + return db_session.get_engine(context=context) + + if database == 'api': + return db_session.get_api_engine() + + +def find_migrate_repo(database='main'): + """Get the path for the migrate repository.""" + global _REPOSITORY + rel_path = os.path.join('sqlalchemy', 'migrate_repo') + if database == 'api': + rel_path = os.path.join('sqlalchemy', 'api_migrations', 'migrate_repo') + path = os.path.join(os.path.abspath(os.path.dirname(__file__)), rel_path) + assert os.path.exists(path) + if _REPOSITORY.get(database) is None: + _REPOSITORY[database] = Repository(path) + return _REPOSITORY[database] def db_sync(version=None, database='main', context=None): """Migrate the database to `version` or the most recent version.""" - return IMPL.db_sync(version=version, database=database, context=context) + if version is not None: + try: + version = int(version) + except ValueError: + raise exception.NovaException(_("version should be an integer")) + + current_version = db_version(database, context=context) + repository = find_migrate_repo(database) + engine = get_engine(database, context=context) + if version is None or version > current_version: + return versioning_api.upgrade(engine, repository, version) + else: + return versioning_api.downgrade(engine, repository, version) def db_version(database='main', context=None): """Display the current database version.""" - return IMPL.db_version(database=database, context=context) + repository = find_migrate_repo(database) + + # NOTE(mdbooth): This is a crude workaround for races in _db_version. The 2 + # races we have seen in practise are: + # * versioning_api.db_version() fails because the migrate_version table + # doesn't exist, but meta.tables subsequently contains tables because + # another thread has already started creating the schema. This results in + # the 'Essex' error. + # * db_version_control() fails with pymysql.error.InternalError(1050) + # (Create table failed) because of a race in sqlalchemy-migrate's + # ControlledSchema._create_table_version, which does: + # if not table.exists(): table.create() + # This means that it doesn't raise the advertised + # DatabaseAlreadyControlledError, which we could have handled explicitly. + # + # I believe the correct fix should be: + # * Delete the Essex-handling code as unnecessary complexity which nobody + # should still need. + # * Fix the races in sqlalchemy-migrate such that version_control() always + # raises a well-defined error, and then handle that error here. + # + # Until we do that, though, we should be able to just try again if we + # failed for any reason. In both of the above races, trying again should + # succeed the second time round. + # + # For additional context, see: + # * https://bugzilla.redhat.com/show_bug.cgi?id=1652287 + # * https://bugs.launchpad.net/nova/+bug/1804652 + try: + return _db_version(repository, database, context) + except Exception: + return _db_version(repository, database, context) + + +def _db_version(repository, database, context): + engine = get_engine(database, context=context) + try: + return versioning_api.db_version(engine, repository) + except versioning_exceptions.DatabaseNotControlledError as exc: + meta = sqlalchemy.MetaData() + meta.reflect(bind=engine) + tables = meta.tables + if len(tables) == 0: + db_version_control( + INIT_VERSION[database], database, context=context) + return versioning_api.db_version(engine, repository) + else: + LOG.exception(exc) + # Some pre-Essex DB's may not be version controlled. + # Require them to upgrade using Essex first. + raise exception.NovaException( + _("Upgrade DB using Essex release first.")) def db_initial_version(database='main'): """The starting version for the database.""" - return IMPL.db_initial_version(database=database) + return INIT_VERSION[database] + + +def db_version_control(version=None, database='main', context=None): + repository = find_migrate_repo(database) + engine = get_engine(database, context=context) + versioning_api.version_control(engine, repository, version) + return version diff --git a/nova/db/sqlalchemy/migration.py b/nova/db/sqlalchemy/migration.py deleted file mode 100644 index 5c0b41a7dc..0000000000 --- a/nova/db/sqlalchemy/migration.py +++ /dev/null @@ -1,140 +0,0 @@ -# Copyright 2010 United States Government as represented by the -# Administrator of the National Aeronautics and Space Administration. -# All Rights Reserved. -# -# 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 -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT -# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the -# License for the specific language governing permissions and limitations -# under the License. - -import os - -from migrate import exceptions as versioning_exceptions -from migrate.versioning import api as versioning_api -from migrate.versioning.repository import Repository -from oslo_log import log as logging -import sqlalchemy - -from nova.db.sqlalchemy import api as db_session -from nova import exception -from nova.i18n import _ - -INIT_VERSION = {} -INIT_VERSION['main'] = 401 -INIT_VERSION['api'] = 66 -_REPOSITORY = {} - -LOG = logging.getLogger(__name__) - - -def get_engine(database='main', context=None): - if database == 'main': - return db_session.get_engine(context=context) - - if database == 'api': - return db_session.get_api_engine() - - -def find_migrate_repo(database='main'): - """Get the path for the migrate repository.""" - global _REPOSITORY - rel_path = 'migrate_repo' - if database == 'api': - rel_path = os.path.join('api_migrations', 'migrate_repo') - path = os.path.join(os.path.abspath(os.path.dirname(__file__)), rel_path) - assert os.path.exists(path) - if _REPOSITORY.get(database) is None: - _REPOSITORY[database] = Repository(path) - return _REPOSITORY[database] - - -def db_sync(version=None, database='main', context=None): - """Migrate the database to `version` or the most recent version.""" - if version is not None: - try: - version = int(version) - except ValueError: - raise exception.NovaException(_("version should be an integer")) - - current_version = db_version(database, context=context) - repository = find_migrate_repo(database) - engine = get_engine(database, context=context) - if version is None or version > current_version: - return versioning_api.upgrade(engine, repository, version) - else: - return versioning_api.downgrade(engine, repository, version) - - -def db_version(database='main', context=None): - """Display the current database version.""" - repository = find_migrate_repo(database) - - # NOTE(mdbooth): This is a crude workaround for races in _db_version. The 2 - # races we have seen in practise are: - # * versioning_api.db_version() fails because the migrate_version table - # doesn't exist, but meta.tables subsequently contains tables because - # another thread has already started creating the schema. This results in - # the 'Essex' error. - # * db_version_control() fails with pymysql.error.InternalError(1050) - # (Create table failed) because of a race in sqlalchemy-migrate's - # ControlledSchema._create_table_version, which does: - # if not table.exists(): table.create() - # This means that it doesn't raise the advertised - # DatabaseAlreadyControlledError, which we could have handled explicitly. - # - # I believe the correct fix should be: - # * Delete the Essex-handling code as unnecessary complexity which nobody - # should still need. - # * Fix the races in sqlalchemy-migrate such that version_control() always - # raises a well-defined error, and then handle that error here. - # - # Until we do that, though, we should be able to just try again if we - # failed for any reason. In both of the above races, trying again should - # succeed the second time round. - # - # For additional context, see: - # * https://bugzilla.redhat.com/show_bug.cgi?id=1652287 - # * https://bugs.launchpad.net/nova/+bug/1804652 - try: - return _db_version(repository, database, context) - except Exception: - return _db_version(repository, database, context) - - -def _db_version(repository, database, context): - engine = get_engine(database, context=context) - try: - return versioning_api.db_version(engine, repository) - except versioning_exceptions.DatabaseNotControlledError as exc: - meta = sqlalchemy.MetaData() - meta.reflect(bind=engine) - tables = meta.tables - if len(tables) == 0: - db_version_control( - INIT_VERSION[database], database, context=context) - return versioning_api.db_version(engine, repository) - else: - LOG.exception(exc) - # Some pre-Essex DB's may not be version controlled. - # Require them to upgrade using Essex first. - raise exception.NovaException( - _("Upgrade DB using Essex release first.")) - - -def db_initial_version(database='main'): - """The starting version for the database.""" - return INIT_VERSION[database] - - -def db_version_control(version=None, database='main', context=None): - repository = find_migrate_repo(database) - engine = get_engine(database, context=context) - versioning_api.version_control(engine, repository, version) - return version diff --git a/nova/tests/functional/db/api/test_migrations.py b/nova/tests/functional/db/api/test_migrations.py index 907032b1c8..5b0d10f290 100644 --- a/nova/tests/functional/db/api/test_migrations.py +++ b/nova/tests/functional/db/api/test_migrations.py @@ -40,7 +40,6 @@ import testtools from nova.db import migration from nova.db.sqlalchemy.api_migrations import migrate_repo from nova.db.sqlalchemy import api_models -from nova.db.sqlalchemy import migration as sa_migration from nova import test from nova.tests import fixtures as nova_fixtures @@ -53,9 +52,8 @@ class NovaAPIModelsSync(test_migrations.ModelsMigrationsSync): self.engine = enginefacade.writer.get_engine() def db_sync(self, engine): - with mock.patch.object(sa_migration, 'get_engine', - return_value=engine): - sa_migration.db_sync(database='api') + with mock.patch.object(migration, 'get_engine', return_value=engine): + migration.db_sync(database='api') @property def migrate_engine(self): @@ -160,7 +158,7 @@ class NovaAPIMigrationsWalk(test_migrations.WalkVersionsMixin): @property def migration_api(self): - return sa_migration.versioning_api + return migration.versioning_api @property def migrate_engine(self): diff --git a/nova/tests/unit/cmd/test_manage.py b/nova/tests/unit/cmd/test_manage.py index a42980871b..5b41ce7930 100644 --- a/nova/tests/unit/cmd/test_manage.py +++ b/nova/tests/unit/cmd/test_manage.py @@ -30,7 +30,7 @@ from nova.cmd import manage from nova import conf from nova import context from nova.db import api as db -from nova.db.sqlalchemy import migration as sqla_migration +from nova.db import migration from nova import exception from nova import objects from nova.scheduler.client import report @@ -618,18 +618,17 @@ Cell %s: 456 self.assertEqual(4, ret) self.assertIn('Unable to get cell list', self.output.getvalue()) - @mock.patch.object(sqla_migration, 'db_version', return_value=2) - def test_version(self, sqla_migrate): + @mock.patch.object(migration, 'db_version', return_value=2) + def test_version(self, mock_db_version): self.commands.version() - sqla_migrate.assert_called_once_with(context=None, database='main') + mock_db_version.assert_called_once_with() - @mock.patch.object(sqla_migration, 'db_sync') - def test_sync(self, sqla_sync): + @mock.patch.object(migration, 'db_sync') + def test_sync(self, mock_db_sync): self.commands.sync(version=4, local_cell=True) - sqla_sync.assert_called_once_with(context=None, - version=4, database='main') + mock_db_sync.assert_called_once_with(4) - @mock.patch('nova.db.migration.db_sync') + @mock.patch.object(migration, 'db_sync') @mock.patch.object(objects.CellMapping, 'get_by_uuid', return_value='map') def test_sync_cell0(self, mock_get_by_uuid, mock_db_sync): ctxt = context.get_admin_context() @@ -810,17 +809,15 @@ class ApiDbCommandsTestCase(test.NoDBTestCase): self.useFixture(fixtures.MonkeyPatch('sys.stdout', self.output)) self.commands = manage.ApiDbCommands() - @mock.patch.object(sqla_migration, 'db_version', return_value=2) - def test_version(self, sqla_migrate): + @mock.patch.object(migration, 'db_version', return_value=2) + def test_version(self, mock_db_version): self.commands.version() - sqla_migrate.assert_called_once_with(context=None, - database='api') + mock_db_version.assert_called_once_with(database='api') - @mock.patch.object(sqla_migration, 'db_sync') - def test_sync(self, sqla_sync): + @mock.patch.object(migration, 'db_sync') + def test_sync(self, mock_db_sync): self.commands.sync(version=4) - sqla_sync.assert_called_once_with(context=None, - version=4, database='api') + mock_db_sync.assert_called_once_with(4, database='api') @ddt.ddt diff --git a/nova/tests/unit/db/test_sqlalchemy_migration.py b/nova/tests/unit/db/test_migration.py similarity index 99% rename from nova/tests/unit/db/test_sqlalchemy_migration.py rename to nova/tests/unit/db/test_migration.py index bfeb72ef37..15496a2ee7 100644 --- a/nova/tests/unit/db/test_sqlalchemy_migration.py +++ b/nova/tests/unit/db/test_migration.py @@ -17,8 +17,8 @@ from migrate.versioning import api as versioning_api import mock import sqlalchemy +from nova.db import migration from nova.db.sqlalchemy import api as db_api -from nova.db.sqlalchemy import migration from nova import test diff --git a/nova/tests/unit/db/test_migrations.py b/nova/tests/unit/db/test_migrations.py index 2755fa2a03..ef06420434 100644 --- a/nova/tests/unit/db/test_migrations.py +++ b/nova/tests/unit/db/test_migrations.py @@ -48,7 +48,6 @@ import testtools from nova.db import migration from nova.db.sqlalchemy import migrate_repo -from nova.db.sqlalchemy import migration as sa_migration from nova.db.sqlalchemy import models from nova import test from nova.tests import fixtures as nova_fixtures @@ -78,7 +77,7 @@ class NovaMigrationsCheckers(test_migrations.ModelsMigrationsSync, @property def migration_api(self): - return sa_migration.versioning_api + return migration.versioning_api @property def migrate_engine(self): @@ -136,9 +135,8 @@ class NovaMigrationsCheckers(test_migrations.ModelsMigrationsSync, # Implementations for ModelsMigrationsSync def db_sync(self, engine): - with mock.patch.object(sa_migration, 'get_engine', - return_value=engine): - sa_migration.db_sync() + with mock.patch.object(migration, 'get_engine', return_value=engine): + migration.db_sync() def get_engine(self, context=None): return self.migrate_engine @@ -239,9 +237,10 @@ class TestNovaMigrationsMySQL(NovaMigrationsCheckers, FIXTURE = test_fixtures.MySQLOpportunisticFixture def test_innodb_tables(self): - with mock.patch.object(sa_migration, 'get_engine', - return_value=self.migrate_engine): - sa_migration.db_sync() + with mock.patch.object( + migration, 'get_engine', return_value=self.migrate_engine, + ): + migration.db_sync() total = self.migrate_engine.execute( "SELECT count(*) "