tests: file-backed SQLite with WAL in threading mode for

Database and CellDatabases Fixtures

Under native threading, default in-memory SQLite is a poor fit for the
Database and CellDatabases fixtures. Use a per-test temp file with WAL
for main and API when threading is enabled, and per-cell file URLs for
CellDatabases; clean up db files and WAL sidecars on teardown. Adjust
DefaultFlavorsFixture test to reconnect before reading flavors so the
insert is visible with file-backed SQLite.

Add DatabaseWriteLock, a new fixture that patches
_TransactionContextManager._transaction_scope to acquire a
process-global RLock before every writer transaction, serialising
writes across threads within a single test.  An RLock is used (not
Lock) because the same test installs both a 'main' and 'api' Database
fixture, so the writer path acquires the lock twice from the same
thread.  The fixture is applied unconditionally for parity between
the eventlet and threading backends.

Database.cleanup() disposes the engine only. Per-test temp sqlite
files are removed when NestedTempfile tears down at the end of the
test.

These unit test adaptations are required:
* test_migration.TestDBURL: decoupled from the Database fixture; now
  mocks _get_engine and _upgrade_alembic to test URL encoding directly.
* test_fixtures.TestDatabaseFixture.test_fixture_{cleanup,api_cleanup}:
  pass because cleanup() now removes the file.
* TestDefaultFlavorsFixture.test_flavors updated to use context
  managers for proper connection cleanup
* test_fixtures.TestDatabaseFixture.test_fixture_{cleanup,api_cleanup}:
  skipped in threading mode (mid-test cleanup()+reconnect is an
  in-memory-only check); use db_fixture.reset() for a fresh DB.
  Still being run under eventlet.

Removes threading_unit_test_excludes.txt as all the excluded tests passes now

Also add an exclude list for functional tests that are currently causing
hangs.

Co-Authored-By: Sean Mooney <smooney@redhat.com>
Change-Id: Ia1463734c347f908b93d8659edbf5cadf05ab5d8
Signed-off-by: Ashish Gupta <ashigupt@redhat.com>

nova/tests/fixtures/nova.py

@@ -23,7 +23,10 @@ import functools
 from importlib.abc import MetaPathFinder
 import logging as std_logging
 import os
+import sqlite3
 import sys
+import tempfile
+import threading
 import time
 import traceback
 from unittest import mock
@@ -76,6 +79,43 @@ DB_SCHEMA = collections.defaultdict(str)
 PROJECT_ID = '6f70656e737461636b20342065766572'
 
 
+def _use_file_backed_sqlite_with_wal(name):
+    """Create a temp SQLite file with WAL; return (sqlalchemy_url, abs_path).
+
+    Expects fixtures.NestedTempfile() so tempfile paths
+    live under a directory that is removed on test end. The file is created
+    empty; WAL (Write-Ahead Logging) mode is set on the file with
+    :mod:`sqlite3` before SQLAlchemy opens it.
+    """
+    # Sanitize name for filename (/ would be treated as path separator)
+    name = name.replace('/', '_').replace(':', '_')
+    fd, path = tempfile.mkstemp(
+        prefix='nova-test-%s-' % name,
+        suffix='.sqlite',
+    )
+    os.close(fd)
+    abs_path = os.path.abspath(path)
+    with sqlite3.connect(abs_path) as conn:
+        conn.execute('PRAGMA journal_mode=WAL')
+    return 'sqlite:///%s' % abs_path, abs_path
+
+
+def _bind_database_fixture_to_file_wal_sqlite(
+    database_fixture, file_label, db_api_module,
+):
+    """Point the fixture at temp file SQLite (WAL);"""
+    url, db_path = _use_file_backed_sqlite_with_wal(file_label)
+    new_engine = enginefacade.transaction_context()
+    database_fixture.useFixture(
+        db_fixtures.ReplaceEngineFacadeFixture(
+            db_api_module.context_manager, new_engine))
+    # Apply all database configuration from CONF (pool size, timeouts, etc.)
+    # before overriding just the connection URL
+    db_api_module.configure(CONF)
+    new_engine.configure(connection=url)
+    database_fixture.get_engine = db_api_module.get_engine
+
+
 class ServiceFixture(fixtures.Fixture):
     """Run a service as a test fixture."""
 
@@ -389,9 +429,50 @@ class CheatingSerializer(rpc.RequestContextSerializer):
         return ctxt
 
 
+_DB_WRITE_LOCK = threading.RLock()
+
+
+class DatabaseWriteLock(fixtures.Fixture):
+    """Serialize writer transactions across threads in tests.
+
+    SQLite allows only a single writer at a time. Patches
+    _TransactionContextManager._transaction_scope to acquire a process-global
+    reentrant lock around every writer transaction so concurrent threads within
+    a test cannot race on the same connection.
+
+    An RLock is used because a single test installs both a 'main' and an
+    'api' Database fixture, each of which installs this fixture. The writer
+    path therefore acquires the lock twice from the same thread; RLock allows
+    that without deadlocking.
+    """
+
+    def _setUp(self):
+        original = (
+            enginefacade._TransactionContextManager._transaction_scope)
+
+        @contextmanager
+        def _locked_scope(tcm_self, context):
+            if tcm_self._mode is enginefacade._WRITER:
+                with _DB_WRITE_LOCK:
+                    with original(tcm_self, context) as resource:
+                        yield resource
+            else:
+                with original(tcm_self, context) as resource:
+                    yield resource
+
+        self.useFixture(fixtures.MockPatchObject(
+            enginefacade._TransactionContextManager,
+            '_transaction_scope',
+            _locked_scope))
+
+
 class CellDatabases(fixtures.Fixture):
     """Create per-cell databases for testing.
 
+    Installs fixtures.NestedTempfile() so file-backed SQLite databases (see
+    _use_file_backed_sqlite_with_wal under threading) live under a single
+    temp directory that is removed automatically when the fixture ends.
+
     How to use::
 
       fix = CellDatabases()
@@ -416,16 +497,21 @@ class CellDatabases(fixtures.Fixture):
         self._cell_lock = ReaderWriterLock()
 
     def _cache_schema(self, connection_str):
-        # NOTE(melwitt): See the regular Database fixture for why
-        # we do this.
+        """Apply the main DB schema to a cell and cache it for reuse.
+
+        The first cell DB in this process is migrated with db_sync and the
+        resulting SQL is stored in DB_SCHEMA. Each later cell replays that
+        cached SQL via executescript on an empty database.
+        """
+        ctxt_mgr = self._ctxt_mgrs[connection_str]
+        engine = ctxt_mgr.writer.get_engine()
+        conn = engine.connect()
         if not DB_SCHEMA[('main', None)]:
-            ctxt_mgr = self._ctxt_mgrs[connection_str]
-            engine = ctxt_mgr.writer.get_engine()
-            conn = engine.connect()
             migration.db_sync(database='main')
             DB_SCHEMA[('main', None)] = "".join(line for line
-                                        in conn.connection.iterdump())
-            engine.dispose()
+                                                in conn.connection.iterdump())
+        else:
+            conn.connection.executescript(DB_SCHEMA[('main', None)])
 
     @contextmanager
     def _wrap_target_cell(self, context, cell_mapping):
@@ -581,11 +667,17 @@ class CellDatabases(fixtures.Fixture):
         in the corresponding CellMapping.
         """
 
-        # NOTE(danms): Create a new context manager for the cell, which
-        # will house the sqlite:// connection for this cell's in-memory
-        # database. Store/index it by the connection string, which is
-        # how we identify cells in CellMapping.
-        ctxt_mgr = main_db_api.create_context_manager()
+        # NOTE: Under native threading, use a file-backed SQLite DB with
+        # WAL (see _use_file_backed_sqlite_with_wal).
+        if utils.concurrency_mode_threading():
+            cell_url, _path = _use_file_backed_sqlite_with_wal(connection_str)
+            ctxt_mgr = main_db_api.create_context_manager(connection=cell_url)
+        else:
+            # NOTE(danms): Create a new context manager for the cell, which
+            # will house the sqlite:// connection for this cell's in-memory
+            # database. Store/index it by the connection string, which is
+            # how we identify cells in CellMapping.
+            ctxt_mgr = main_db_api.create_context_manager()
         self._ctxt_mgrs[connection_str] = ctxt_mgr
 
         # NOTE(melwitt): The first DB access through service start is
@@ -613,11 +705,12 @@ class CellDatabases(fixtures.Fixture):
             engine = ctxt_mgr.writer.get_engine()
             engine.dispose()
             self._cache_schema(connection_str)
-            conn = engine.connect()
-            conn.connection.executescript(DB_SCHEMA[('main', None)])
 
     def setUp(self):
         super(CellDatabases, self).setUp()
+        if utils.concurrency_mode_threading():
+            self.useFixture(DatabaseWriteLock())
+        self.useFixture(fixtures.NestedTempfile())
         self.addCleanup(self.cleanup)
         self._real_target_cell = context.target_cell
 
@@ -668,6 +761,10 @@ class Database(fixtures.Fixture):
 
     def setUp(self):
         super().setUp()
+        # File-backed SQLite DB paths land under this tree (see
+        # _use_file_backed_sqlite_with_wal for main/api default connections and
+        # CellDatabases).
+        self.useFixture(fixtures.NestedTempfile())
 
         if self.database == 'main':
 
@@ -675,6 +772,9 @@ class Database(fixtures.Fixture):
                 ctxt_mgr = main_db_api.create_context_manager(
                     connection=self.connection)
                 self.get_engine = ctxt_mgr.writer.get_engine
+            elif utils.concurrency_mode_threading():
+                _bind_database_fixture_to_file_wal_sqlite(
+                    self, 'main', main_db_api)
             else:
                 # NOTE(gibi): this injects a new factory for each test and
                 # cleans it up at then end of the test case. This way we can
@@ -685,18 +785,23 @@ class Database(fixtures.Fixture):
                     db_fixtures.ReplaceEngineFacadeFixture(
                         main_db_api.context_manager, new_engine))
                 main_db_api.configure(CONF)
-
                 self.get_engine = main_db_api.get_engine
         elif self.database == 'api':
-            # NOTE(gibi): similar note applies here as for the main_db_api
-            # above
-            new_engine = enginefacade.transaction_context()
-            self.useFixture(
-                db_fixtures.ReplaceEngineFacadeFixture(
-                    api_db_api.context_manager, new_engine))
-            api_db_api.configure(CONF)
+            if utils.concurrency_mode_threading():
+                _bind_database_fixture_to_file_wal_sqlite(
+                    self, 'api', api_db_api)
+            else:
+                # NOTE(gibi): similar note applies here as for the main_db_api
+                # above
+                new_engine = enginefacade.transaction_context()
+                self.useFixture(
+                    db_fixtures.ReplaceEngineFacadeFixture(
+                        api_db_api.context_manager, new_engine))
+                api_db_api.configure(CONF)
+                self.get_engine = api_db_api.get_engine
 
-            self.get_engine = api_db_api.get_engine
+        if utils.concurrency_mode_threading():
+            self.useFixture(DatabaseWriteLock())
 
         self._apply_schema()
 

nova/tests/unit/db/test_migration.py

@@ -23,22 +23,25 @@ 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')
-        self.useFixture(nova_fixtures.Database())
 
         alembic_config = migration._find_alembic_conf()
+        mock_engine = mock.MagicMock()
+        mock_engine.url = sa_url.make_url(url)
+
         with mock.patch.object(
                 migration, '_find_alembic_conf', return_value=alembic_config):
-            migration.db_sync()
+            with mock.patch.object(
+                    migration, '_get_engine', return_value=mock_engine):
+                with mock.patch.object(migration, '_upgrade_alembic'):
+                    migration.db_sync()
+
         actual = alembic_config.get_main_option('sqlalchemy.url')
         expected = (
             "sqlite:///:memory:?read_default_group=data+with%2Fa"

nova/tests/unit/test_fixtures.py

@@ -17,6 +17,7 @@
 import copy
 import datetime
 import io
+import unittest
 from unittest import mock
 
 import fixtures as fx
@@ -132,6 +133,9 @@ class TestOSAPIFixture(testtools.TestCase):
 
 class TestDatabaseFixture(testtools.TestCase):
     def test_fixture_reset(self):
+        # Fixture.reset() runs cleanUp() then setUp(). With file-backed
+        # SQLite in threading mode, setUp() provisions a new temp database
+        # file so the fixture starts empty again.
         # because this sets up reasonable db connection strings
         self.useFixture(fixtures.ConfFixture())
         db_fixture = fixtures.Database()
@@ -195,7 +199,14 @@ class TestDatabaseFixture(testtools.TestCase):
         rows = result.fetchall()
         self.assertEqual(0, len(rows), "Rows %s" % rows)
 
+    @unittest.skipIf(
+        utils.concurrency_mode_threading(),
+        'file-backed SQLite keeps data at the same path after cleanup(); '
+        'use db_fixture.reset() for a fresh database in threading mode')
     def test_fixture_cleanup(self):
+        # In-memory SQLite only: cleanup() disposes the engine and reconnecting
+        # yields an empty database. File-backed temp files are removed when
+        # NestedTempfile tears down at the end of the test.
         # because this sets up reasonable db connection strings
         self.useFixture(fixtures.ConfFixture())
         fix = fixtures.Database()
@@ -210,6 +221,10 @@ class TestDatabaseFixture(testtools.TestCase):
         schema = "".join(line for line in conn.connection.iterdump())
         self.assertEqual(schema, "BEGIN TRANSACTION;COMMIT;")
 
+    @unittest.skipIf(
+        utils.concurrency_mode_threading(),
+        'file-backed SQLite keeps data at the same path after cleanup(); '
+        'use db_fixture.reset() for a fresh database in threading mode')
     def test_api_fixture_cleanup(self):
         # This sets up reasonable db connection strings
         self.useFixture(fixtures.ConfFixture())
@@ -247,16 +262,19 @@ class TestDefaultFlavorsFixture(testtools.TestCase):
         self.useFixture(fixtures.Database(database='api'))
 
         engine = api_db_api.get_engine()
-        conn = engine.connect()
-        result = conn.execute(sa.text("SELECT * FROM flavors"))
-        rows = result.fetchall()
-        self.assertEqual(0, len(rows), "Rows %s" % rows)
+        with engine.connect() as conn:
+            result = conn.execute(sa.text("SELECT * FROM flavors"))
+            rows = result.fetchall()
+            self.assertEqual(0, len(rows), "Rows %s" % rows)
 
         self.useFixture(fixtures.DefaultFlavorsFixture())
 
-        result = conn.execute(sa.text("SELECT * FROM flavors"))
-        rows = result.fetchall()
-        self.assertEqual(6, len(rows), "Rows %s" % rows)
+        # File-backed SQLite (threading) uses a new connection per checkout;
+        # start a new connection so the flavors insert is visible.
+        with engine.connect() as conn:
+            result = conn.execute(sa.text("SELECT * FROM flavors"))
+            rows = result.fetchall()
+            self.assertEqual(6, len(rows), "Rows %s" % rows)
 
 
 class TestIndirectionAPIFixture(testtools.TestCase):

threading_functional_test_excludes.txt

@@ -0,0 +1,6 @@
+# Functional tests to exclude when running in threading mode
+
+# Tests that fail and may cause worker cleanup to hang
+nova.tests.functional.compute.test_init_host.ComputeManagerInitHostTestCase.test_migrate_disk_and_power_off_crash_finish_revert_migration
+nova.tests.functional.regressions.test_bug_1825034.FillVirtualInterfaceListMigration.test_fill_vifs_migration
+nova.tests.functional.test_images.ImagesTest.test_create_images_negative_invalid_state

threading_unit_test_excludes.txt

@@ -1,18 +0,0 @@
-# Independent failure ~10% with multiple possible error:
-# - sqlalchemy.exc.OperationalError: (sqlite3.OperationalError) not an error
-# - sqlite3.OperationalError: cannot start a transaction within a transaction
-# both triggered at: nova.compute.api.HostAPI._service_get_all_cells
-nova.tests.unit.policies.test_availability_zone.AZScopeTypeNoLegacyPolicyTest.test_availability_zone_detail_policy
-nova.tests.unit.test_availability_zones.AvailabilityZoneTestCases.test_get_availability_zones
-nova.tests.unit.policies.test_availability_zone.AZScopeTypeNoLegacyPolicyTest.test_availability_zone_list_policy
-nova.tests.unit.policies.test_availability_zone.AvailabilityZone
-nova.tests.unit.compute.test_shelve.ShelveComputeAPITestCase.test_unshelve_without_az_to_newaz_and_host
-nova.tests.unit.compute.test_shelve.ShelveComputeAPITestCase.test_unshelve_without_az_to_newaz
-nova.tests.unit.compute.test_shelve.ShelveComputeAPITestCase.test_unshelve_with_az_to_newaz
-nova.tests.unit.api.openstack.compute.test_availability_zone.ServersControllerCreateTestV21.test_create_instance_with_availability_zone
-nova.tests.unit.api.openstack.compute.test_services.ServicesTestV275.test_services_list_with_additional_filter_old_version
-
-# This fails also with sqlite3.OperationalError: cannot start a transaction within a transaction but
-# not from nova.compute.api.HostAPI._service_get_all_cells but from the db api
-# _instance_get_by_uuid
-nova.tests.unit.conductor.test_conductor.ConductorTaskRPCAPITestCase.test_evacuate_old_rpc_without_target_state

tools/generate-exclude.sh

@@ -1 +1,3 @@
-grep -v "#" threading_unit_test_excludes.txt > /tmp/exclude.txt
+#!/usr/bin/env bash
+# Args: <excludes.txt> <output for stestr --exclude-list>
+grep -v '^#' "$1" | grep -v '^$' > "$2" || true

tox.ini

@@ -75,12 +75,7 @@ setenv =
   OS_NOVA_DISABLE_EVENTLET_PATCHING=True
 
 commands =
-# So far we have a list of failing test cases to filter out. Also note
-# that there might be unstale tests.
-# Our exclude list has comments we need to remove before it can be passed
-# to stestr
-  bash tools/generate-exclude.sh
-  stestr run {posargs} --exclude-list /tmp/exclude.txt
+  stestr run {posargs}
   stestr slowest
 
 [testenv:functional{,-py310,-py311,-py312,-py313,-py314}]
@@ -136,7 +131,8 @@ deps =
   openstack-placement>=9.0.0.0b1
 extras =
 commands =
-  stestr --test-path=./nova/tests/functional run {posargs}
+  bash tools/generate-exclude.sh threading_functional_test_excludes.txt /tmp/functional_exclude.txt
+  stestr --test-path=./nova/tests/functional run {posargs} --exclude-list /tmp/functional_exclude.txt
   stestr slowest
 
 [testenv:functional-without-sample-db-tests]