From a3597530069aa7b55b1d3a033f50e0deebcb9786 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Mon, 13 Sep 2021 14:07:12 +0200 Subject: [PATCH] Fix nova-manage db version When nova switched to alembic implementation was added to nova-manage db version CLI to query the current db revision from alembic. However multiple mistake was made. The code called alembic_api.current[1] with an Engine object while that call expects a Config object instead. This leads to but/1943436. Also the same code expected that this call returns the revision. But that call just prints the revision to the standard output instead. So the implementations has been change from calling the alembic command API which is mostly created for CLI consumption to MigrationContext.get_current_revision() call that is intended to be used as a python API instead. [1] https://alembic.sqlalchemy.org/en/latest/api/commands.html#alembic.command.current Co-Authored-By: Sean Mooney Closes-Bug: #1943436 Change-Id: I9fa7c03310c5bdb82e9a9c39727edb12eeae77f0 --- nova/db/migration.py | 4 +++- nova/tests/unit/db/api/test_migrations.py | 15 +++----------- nova/tests/unit/db/main/test_migrations.py | 15 +++----------- nova/tests/unit/db/test_migration.py | 24 ++++++++++++---------- 4 files changed, 22 insertions(+), 36 deletions(-) diff --git a/nova/db/migration.py b/nova/db/migration.py index e06e21a038e2..80410c319273 100644 --- a/nova/db/migration.py +++ b/nova/db/migration.py @@ -170,6 +170,8 @@ def db_version(database='main', context=None): alembic_version = None if _is_database_under_alembic_control(engine): - alembic_version = alembic_api.current(engine) + with engine.connect() as conn: + m_context = alembic_migration.MigrationContext.configure(conn) + alembic_version = m_context.get_current_revision() return alembic_version or migrate_version diff --git a/nova/tests/unit/db/api/test_migrations.py b/nova/tests/unit/db/api/test_migrations.py index c5e2069bd83b..1281b164f255 100644 --- a/nova/tests/unit/db/api/test_migrations.py +++ b/nova/tests/unit/db/api/test_migrations.py @@ -244,18 +244,9 @@ class NovaMigrationsWalk( def test_db_version_alembic(self): migration.db_sync(database='api') - # FIXME(gibi): this is bug/1943436 - ex = self.assertRaises( - AttributeError, migration.db_version, database='api') - self.assertIn( - "'Engine' object has no attribute 'get_main_option'", - str(ex) - ) - - # It should return the head of the migrations instead - # head = alembic_script.ScriptDirectory.from_config( - # self.config).get_current_head() - # self.assertEqual(head, migration.db_version(database='api')) + head = alembic_script.ScriptDirectory.from_config( + self.config).get_current_head() + self.assertEqual(head, migration.db_version(database='api')) class TestMigrationsWalkSQLite( diff --git a/nova/tests/unit/db/main/test_migrations.py b/nova/tests/unit/db/main/test_migrations.py index 0b3bf1b05c21..1271c31fe002 100644 --- a/nova/tests/unit/db/main/test_migrations.py +++ b/nova/tests/unit/db/main/test_migrations.py @@ -287,18 +287,9 @@ class NovaMigrationsWalk( def test_db_version_alembic(self): migration.db_sync(database='main') - # FIXME(gibi): this is bug/1943436 - ex = self.assertRaises( - AttributeError, migration.db_version, database='main') - self.assertIn( - "'Engine' object has no attribute 'get_main_option'", - str(ex) - ) - - # It should return the head of the migrations instead - # head = alembic_script.ScriptDirectory.from_config( - # self.config).get_current_head() - # self.assertEqual(head, migration.db_version(database='main')) + head = alembic_script.ScriptDirectory.from_config( + self.config).get_current_head() + self.assertEqual(head, migration.db_version(database='main')) class TestMigrationsWalkSQLite( diff --git a/nova/tests/unit/db/test_migration.py b/nova/tests/unit/db/test_migration.py index 3d4269a296b1..c05826b92360 100644 --- a/nova/tests/unit/db/test_migration.py +++ b/nova/tests/unit/db/test_migration.py @@ -17,7 +17,6 @@ import urllib import fixtures import mock -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 @@ -151,7 +150,7 @@ class TestDBSync(test.NoDBTestCase): self._test_db_sync(has_migrate, has_alembic) -@mock.patch.object(alembic_api, 'current') +@mock.patch.object(alembic_migration.MigrationContext, 'configure') @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') @@ -161,7 +160,7 @@ class TestDBVersion(test.NoDBTestCase): def test_db_version_invalid_databse( self, mock_find_repo, mock_get_engine, mock_is_migrate, - mock_is_alembic, mock_migrate_version, mock_alembic_version, + mock_is_alembic, mock_migrate_version, mock_m_context_configure, ): """We only have two databases.""" self.assertRaises( @@ -169,7 +168,7 @@ class TestDBVersion(test.NoDBTestCase): 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_is_alembic, mock_migrate_version, mock_m_context_configure, ): """Database is controlled by sqlalchemy-migrate.""" mock_is_migrate.return_value = True @@ -184,30 +183,33 @@ class TestDBVersion(test.NoDBTestCase): 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_m_context_configure.assert_not_called() 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_is_alembic, mock_migrate_version, mock_m_context_configure, ): """Database is controlled by alembic.""" mock_is_migrate.return_value = False mock_is_alembic.return_value = True ret = migration.db_version('main') - self.assertEqual(mock_alembic_version.return_value, ret) + mock_m_context = mock_m_context_configure.return_value + self.assertEqual( + mock_m_context.get_current_revision.return_value, + ret + ) mock_find_repo.assert_called_once_with('main') mock_get_engine.assert_called_once_with('main', context=None) 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) + mock_m_context_configure.assert_called_once() 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, + mock_is_alembic, mock_migrate_version, mock_m_context_configure, ): """Database is not controlled.""" mock_is_migrate.return_value = False @@ -221,7 +223,7 @@ class TestDBVersion(test.NoDBTestCase): mock_is_migrate.assert_called_once() mock_is_alembic.assert_called_once() mock_migrate_version.assert_not_called() - mock_alembic_version.assert_not_called() + mock_m_context_configure.assert_not_called() class TestGetEngine(test.NoDBTestCase):