Skip to content

Commit

Permalink
Move dev DB creation to a separate process
Browse files Browse the repository at this point in the history
Context
-------

At startup (when you run `make dev`) Via's Gunicorn processes attempt to
create the DB tables if they don't already exist. This is done in dev
only, not in production:

    # via/db.py

    def includeme(config):
        ...

        if config.registry.settings.get("dev"):
            # Create the database tables if they don't already exist.
            Base.metadata.create_all(engine)

Problem
-------

Via actually has four Gunicorn workers in dev (see the `workers = 4` in
`conf/gunicorn/dev.conf.py`). All 4 try to create the DB tables at the
same time and 3 of them crash, printing out error messages.

The issue is kind of difficult to reproduce:

1. You need to actually have a model, so you won't be able to produce
   the issue on the `db` branch without changes. See
   #1072 and
   #1093 for branches on which you
   can reproduce the issue.

2. The model's table needs to actually be missing from the DB table. You
   can do that by running this command:

       make services args=down && make services && make dev

3. Since the issue is a race condition you won't be able to reproduce it
   reliably, but if you run the above command several times you should
   see it.

Solution
--------

This commit moves the `sqlalchemy.create_all()` call out of the Gunicorn
worker process and instead adds a separate `make_db` process to the list
of processes that `supervisord-dev.conf` runs when you run `make dev`.
This new `make_db` process starts up, calls `create_all()`, and then
exits, and since it's a singular process there's no collision.
  • Loading branch information
seanh committed Jul 30, 2023
1 parent 7f96bf1 commit 2a63b4b
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 26 deletions.
4 changes: 4 additions & 0 deletions .cookiecutter/includes/conf/supervisord-dev.conf.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
{
"programs": {
"make_db": {
"command": "bin/make_db",
"startsecs": "0"
},
"web": {
"command": "gunicorn -c conf/gunicorn/dev.conf.py --paste conf/development.ini"
},
Expand Down
36 changes: 33 additions & 3 deletions bin/make_db
Original file line number Diff line number Diff line change
@@ -1,5 +1,35 @@
"""Initialize the dev environment's DB."""
#!/usr/bin/env python3
import sys
"""Initialize the dev environment's DB."""
import alembic.command
import alembic.config
from pyramid.paster import bootstrap
bootstrap("conf/development.ini")
from sqlalchemy import text
from sqlalchemy.exc import ProgrammingError

from via.db import Base, create_engine


def is_stamped(engine) -> bool:
"""Return True if the DB is stamped with an Alembic revision ID."""
with engine.connect() as connection:
try:
if connection.execute(text("select * from alembic_version")).first():
return True
except ProgrammingError:
pass

return False


def main():
with bootstrap("conf/development.ini") as env:
settings = env["registry"].settings
engine = create_engine(settings)

if not is_stamped(engine):
Base.metadata.create_all(engine)
alembic.command.stamp(alembic.config.Config("conf/alembic.ini"), "head")


if __name__ == "__main__":
main()
8 changes: 8 additions & 0 deletions conf/supervisord-dev.conf
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@
nodaemon=true
silent=true

[program:make_db]
command=bin/make_db
stdout_events_enabled=true
stderr_events_enabled=true
stopsignal=KILL
stopasgroup=true
startsecs=0

[program:web]
command=gunicorn -c conf/gunicorn/dev.conf.py --paste conf/development.ini
stdout_events_enabled=true
Expand Down
29 changes: 6 additions & 23 deletions via/db.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import alembic.command
import alembic.config
import sqlalchemy
import zope.sqlalchemy
from sqlalchemy import MetaData, create_engine, text
from sqlalchemy.exc import ProgrammingError
from sqlalchemy import MetaData
from sqlalchemy.orm import DeclarativeBase, MappedAsDataclass, Session


Expand All @@ -18,23 +16,13 @@ class Base(MappedAsDataclass, DeclarativeBase):
)


def stamp(engine): # pragma: no cover
"""If the DB isn't already stamped then stamp it with the latest revision."""
stamped = False

with engine.connect() as connection:
try:
if connection.execute(text("select * from alembic_version")).first():
stamped = True
except ProgrammingError:
pass

if not stamped:
alembic.command.stamp(alembic.config.Config("conf/alembic.ini"), "head")
def create_engine(settings): # pragma: no cover
"""Return a SQLAlchemy engine."""
return sqlalchemy.create_engine(settings["database_url"])


def includeme(config): # pragma: no cover
engine = create_engine(config.registry.settings["database_url"])
engine = create_engine(config.registry.settings)

def db_session(request):
"""Return the SQLAlchemy session for the given request."""
Expand All @@ -47,11 +35,6 @@ def db_session(request):

return session

if config.registry.settings.get("dev"):
# Create the database tables if they don't already exist.
Base.metadata.create_all(engine)
stamp(engine)

# Make the SQLAlchemy session available as `request.db`.
# `reify=True` means it'll create only one session per request.
config.add_request_method(db_session, name="db", reify=True)

0 comments on commit 2a63b4b

Please sign in to comment.