From a96774a1fdbe18a4977ee33eef2daa71b27742bf Mon Sep 17 00:00:00 2001 From: "Owen W. Taylor" Date: Fri, 22 Jan 2021 14:41:05 -0500 Subject: [PATCH] Go back to using a file-backed SQLite database for tests Using a memory database causes tests/test_build to intermittently fail, because using the same pysqlite3 connection object from multiple threads - as was done so that the threads shared the same memory database - is not, in the end, thread safe. One thread will stomp on the transaction state of other threads, resulting in errors from starting a new transaction when another is already in progress, or trying to commit a transaction that is not in progress. To avoid a significant speed penalty, the session-scope fixture sets up a database in the pytest temporary directory, which will typically be on tmpfs. Time to complete all tests: memory backend: 38 seconds file on tmpfs: 40 seconds file on nvme ssd with btrfs: 137 seconds MBSSQLAlchemy, which attempted to make the memory backend work, is removed. Session hooks are installed on the Session class rather than on the scoped_session instance - this works better when we're changing from one database to another at test setup time. --- module_build_service/__init__.py | 23 +------------------- module_build_service/common/config.py | 2 ++ module_build_service/scheduler/db_session.py | 6 ++--- tests/__init__.py | 16 +++++++++++++- tests/conftest.py | 15 +++++++++++-- 5 files changed, 34 insertions(+), 28 deletions(-) diff --git a/module_build_service/__init__.py b/module_build_service/__init__.py index 292ffa67..5772c857 100644 --- a/module_build_service/__init__.py +++ b/module_build_service/__init__.py @@ -24,7 +24,6 @@ import pkg_resources from flask import Flask, has_app_context, url_for from flask_sqlalchemy import SQLAlchemy -from sqlalchemy.pool import StaticPool from module_build_service.common.config import config_section from module_build_service.web.proxy import ReverseProxy @@ -40,27 +39,7 @@ app.wsgi_app = ReverseProxy(app.wsgi_app) app.config.from_object(config_section) -class MBSSQLAlchemy(SQLAlchemy): - """ - Inherits from SQLAlchemy and if SQLite in-memory database is used, - sets the driver options so multiple threads can share the same database. - - This is used *only* during tests to make them faster. - """ - - def apply_driver_hacks(self, app, info, options): - if info.drivername == "sqlite" and info.database in (None, "", ":memory:"): - options["poolclass"] = StaticPool - options["connect_args"] = {"check_same_thread": False} - try: - del options["pool_size"] - except KeyError: - pass - - super(MBSSQLAlchemy, self).apply_driver_hacks(app, info, options) - - -db = MBSSQLAlchemy(app) +db = SQLAlchemy(app) def get_url_for(*args, **kwargs): diff --git a/module_build_service/common/config.py b/module_build_service/common/config.py index ea6ad0f3..57f2c1e5 100644 --- a/module_build_service/common/config.py +++ b/module_build_service/common/config.py @@ -41,6 +41,8 @@ class BaseConfiguration(object): class TestConfiguration(BaseConfiguration): LOG_LEVEL = "debug" + # This is just for initialization - we override this with a database in the + # pytest temporary directory SQLALCHEMY_DATABASE_URI = os.environ.get("DATABASE_URI", "sqlite:///:memory:") DEBUG = True MESSAGING = "in_memory" diff --git a/module_build_service/scheduler/db_session.py b/module_build_service/scheduler/db_session.py index b91e373d..a73a7833 100644 --- a/module_build_service/scheduler/db_session.py +++ b/module_build_service/scheduler/db_session.py @@ -4,7 +4,7 @@ from __future__ import absolute_import import sqlalchemy.event from sqlalchemy.pool import NullPool -from sqlalchemy.orm import scoped_session, sessionmaker +from sqlalchemy.orm import scoped_session, sessionmaker, Session from module_build_service.common.config import conf from module_build_service.common.models import ( @@ -24,8 +24,8 @@ def _setup_event_listeners(db_session): ) for event, handler in event_hooks: - if not sqlalchemy.event.contains(db_session, event, handler): - sqlalchemy.event.listen(db_session, event, handler) + if not sqlalchemy.event.contains(Session, event, handler): + sqlalchemy.event.listen(Session, event, handler) # initialize DB event listeners from the monitor module from module_build_service.common.monitor import db_hook_event_listeners diff --git a/tests/__init__.py b/tests/__init__.py index c995d6b3..729de01d 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -13,6 +13,7 @@ import yaml from traceback import extract_stack import koji +import flask_sqlalchemy from mock import patch from six import string_types @@ -132,7 +133,7 @@ def truncate_tables(): db_session.commit() -def clean_database(add_platform_module=True, add_default_arches=True): +def clean_database(database_uri=None, add_platform_module=True, add_default_arches=True): """Initialize the test database This function is responsible for dropping all the data in the database and @@ -142,6 +143,19 @@ def clean_database(add_platform_module=True, add_default_arches=True): Flask-SQLAlchemy. """ + if database_uri is not None: + # When starting tests, we switch from the memory backend to a + # database in the pytest temporary directory, requiring some + # hacks + db.session.remove() + db.session = None + + # clear any cached engine connections + flask_sqlalchemy.get_state(module_build_service.app).connnectors = {} + + module_build_service.app.config['SQLALCHEMY_DATABASE_URI'] = database_uri + db.session = db.create_scoped_session() + # Helpful for writing tests if any changes were made using the database # session but the test didn't commit or rollback. # diff --git a/tests/conftest.py b/tests/conftest.py index d6a959a2..68c3a42c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -281,6 +281,17 @@ def provide_test_client(request): @pytest.fixture(autouse=True, scope="session") -def create_database(request): +def create_database(request, tmpdir_factory): """Drop and recreate all tables""" - clean_database(add_platform_module=False, add_default_arches=False) + + # pysqlite + the SQLite memory backend doesn't handle multiple threads usefully: + # either each thread gets its own backend, or the threads stomp on each others + # transaction state. So, before running tests, replace the memory backend with + # a file backend - if this ends up on tmpfs, its almost as fast as the memory + # backend. + database_uri = None + if module_build_service.app.config["SQLALCHEMY_DATABASE_URI"] == 'sqlite:///:memory:': + database_path = str(tmpdir_factory.getbasetemp() / "test.db") + database_uri = 'sqlite:///' + database_path + + clean_database(database_uri=database_uri, add_platform_module=False, add_default_arches=False)