Fix interference in db unit test

We discovered that two unit test cases added in
I0647bb8545c1464b521a1d866cf5ee674aea2eae cause errors like
  oslo_db.sqlalchemy.enginefacade.AlreadyStartedError:
  this TransactionFactory is already started

when the db tests run selectively with tox -e py38 nova.tests.unit.db
but does not cause errors if the whole unit test suit is run.

This error happened because our db code uses two global transaction
factory, one for the api DB and one for the main DB. There was a global
flag SESSION_CONFIGURED in our Database fixture that guarded against
double initialization of the factory. But the faulty test cases in
question do not use our Database fixture but use the
OpportunisticDBTestMixin from oslo_db. Obviously that fixture does not
know about our SESSION_CONFIGURED global. So if one of the offending
test case ran first in an executor then that initialized the
transaction factory globally and a later test that uses our Database
fixture tried to configure it again leading to the error. For some
unknown reason if these tests were run in the opposite order the faulty
re-initialization did not happen. Probably the OpportunisticDBTestMixin
was able to prevent that.

A previous patch already removed the global SESSION_CONFIGURED flag
from our fixture and replaced it with a per DB specific patch_factory
calls that allow resetting the state of the factory at the end of each
test case. This would already solve the current test case issue as only
our offending test cases would initialize the global factory without
cleanup and we have one test case per DB. So there would be no
interference. However if in the future we add similar test cases then
those can still interfere through the global factory.

So this patch fixes the two offending test case. Also it extends the
DatabasePoisonFixture used for the NoDbTestCase tesst. The poison now
detects if the test case starts any transaction factory.

This poison caught another offender test case,
test_db_sync_with_special_symbols_in_connection_string, that was marked
NoDb but actually using the database. It is now changed to declare
itself as a test that sets up the DB manually and also it is changed to
use the Database fixture instead of touching the global factory
directly.

Closes-Bug: #1948963

Change-Id: Id96f1795034490c13125ebbab49b029fb96af1c7

4 files changed, 30 insertions, 18 deletions

diff --git a/nova/tests/fixtures/nova.py b/nova/tests/fixtures/nova.py
index 5161c7c3c0..f0ee5f00f4 100644
--- a/nova/tests/fixtures/nova.py
+++ b/nova/tests/fixtures/nova.py
@@ -203,6 +203,18 @@ class DatabasePoisonFixture(fixtures.Fixture):
             '_create_session',
             self._poison_configure))
 
+        # NOTE(gibi): not just _create_session indicates a manipulation on the
+        # DB but actually any operation that actually initializes (starts) a
+        # transaction factory. If a test does this without using the Database
+        # fixture then that test i) actually a database test and should declare
+        # it so ii) actually manipulates a global state without proper cleanup
+        # and test isolation. This could lead that later tests are failing with
+        # the error: oslo_db.sqlalchemy.enginefacade.AlreadyStartedError: this
+        # TransactionFactory is already started
+        self.useFixture(fixtures.MonkeyPatch(
+           'oslo_db.sqlalchemy.enginefacade._TransactionFactory._start',
+           self._poison_configure))
+
     def _poison_configure(self, *a, **k):
         # If you encounter this error, you might be tempted to just not
         # inherit from NoDBTestCase. Bug #1568414 fixed a few hundred of these
diff --git a/nova/tests/unit/db/api/test_migrations.py b/nova/tests/unit/db/api/test_migrations.py
index 10c0c8feb9..1b14d569db 100644
--- a/nova/tests/unit/db/api/test_migrations.py
+++ b/nova/tests/unit/db/api/test_migrations.py
@@ -275,11 +275,14 @@ class NovaMigrationsWalk(
                 self._migrate_up(connection, revision)
 
     def test_db_version_alembic(self):
-        migration.db_sync(database='api')
+        engine = enginefacade.writer.get_engine()
 
-        script = alembic_script.ScriptDirectory.from_config(self.config)
-        head = script.get_current_head()
-        self.assertEqual(head, migration.db_version(database='api'))
+        with mock.patch.object(migration, '_get_engine', return_value=engine):
+            migration.db_sync(database='api')
+
+            script = alembic_script.ScriptDirectory.from_config(self.config)
+            head = script.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 478920c248..cb253dfc5b 100644
--- a/nova/tests/unit/db/main/test_migrations.py
+++ b/nova/tests/unit/db/main/test_migrations.py
@@ -308,11 +308,14 @@ class NovaMigrationsWalk(
                 self._migrate_up(connection, revision)
 
     def test_db_version_alembic(self):
-        migration.db_sync(database='main')
+        engine = enginefacade.writer.get_engine()
 
-        script = alembic_script.ScriptDirectory.from_config(self.config)
-        head = script.get_current_head()
-        self.assertEqual(head, migration.db_version(database='main'))
+        with mock.patch.object(migration, '_get_engine', return_value=engine):
+            migration.db_sync(database='main')
+
+            script = alembic_script.ScriptDirectory.from_config(self.config)
+            head = script.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 7251b174b1..6657bc48e0 100644
--- a/nova/tests/unit/db/test_migration.py
+++ b/nova/tests/unit/db/test_migration.py
@@ -17,33 +17,27 @@ import os
 import urllib
 
 from alembic.runtime import migration as alembic_migration
-import fixtures
 from migrate import exceptions as migrate_exceptions
 from migrate.versioning import api as migrate_api
 import mock
-from oslo_db.sqlalchemy import enginefacade
 
 from nova.db.api import api as api_db_api
 from nova.db.main import api as main_db_api
 from nova.db import migration
 from nova import exception
 from nova import test
+from nova.tests import fixtures as nova_fixtures
 
 
 class TestDBURL(test.NoDBTestCase):
+    USES_DB_SELF = True
 
     def test_db_sync_with_special_symbols_in_connection_string(self):
         qargs = 'read_default_group=data with/a+percent_%-and%20symbols!'
         url = f"sqlite:///:memory:?{qargs}"
         self.flags(connection=url, group='database')
-        # since the engine.url is immutable it will never get updated
-        # once its created so reusing the engine instance would break
-        # this test.
-        engine = enginefacade.writer.get_engine()
-        self.useFixture(
-            fixtures.MonkeyPatch(
-                'nova.db.migration._get_engine',
-                mock.Mock(return_value=engine)))
+        self.useFixture(nova_fixtures.Database())
+
         alembic_config = migration._find_alembic_conf()
         with mock.patch.object(
                 migration, '_find_alembic_conf', return_value=alembic_config):