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 <[email protected]>
Closes-Bug: #1943436

Change-Id: I9fa7c03310c5bdb82e9a9c39727edb12eeae77f0

Author: Balazs Gibizer

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

nova/tests/unit/db/api/test_migrations.py

@@ -244,18 +244,9 @@ def test_walk_versions(self):
     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(

nova/tests/unit/db/main/test_migrations.py

@@ -287,18 +287,9 @@ def test_walk_versions(self):
     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(

nova/tests/unit/db/test_migration.py

@@ -17,7 +17,6 @@
 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 @@ def test_db_sync_with_existing_alembic_database(self):
         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,15 +160,15 @@ 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(
             exception.Invalid, migration.db_version, database='invalid')
 
     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 @@ def test_db_version_migrate(
         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 @@ def test_db_version_not_controlled(
         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):