Move dev DB schema creation and stamping to a separate process #1123

seanh · 2023-07-26T14:42:03Z

This will make Via out of sync with the cookiecutter. There's a couple of follow-up cookiecutter PRs to fix this: hypothesis/cookiecutters#146, hypothesis/cookiecutters#147

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 produce:

You need to actually have a model, so you won't be able to produce
the issue on the db branch without changes. See
Cache YouTube transcripts in the DB #1072 and
Cache YouTube video titles in the DB #1093 for branches on which you
can reproduce the issue.
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
```
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.

seanh · 2023-07-26T14:43:01Z

bin/make_db

 #!/usr/bin/env python3
-import sys
+"""Initialize the dev environment's DB."""


The #! needs to be above the docstring or it doesn't work! 🤦

bin/make_db actually comes from the cookiecutter, so I'll need to move these changes into the cookiecutter

seanh · 2023-07-26T14:49:50Z

via/db.py

+def create_engine(settings):
+    """Return a SQLAlchemy engine."""
+    return sqlalchemy.create_engine(settings["database_url"])
+
+
+def create_all(engine):
+    """Create all DB tables that don't already exist."""
+    Base.metadata.create_all(engine)


Pulled create_all() out into a separate process that's not called by the includeme() below anymore, and is imported and called by bin/make_db instead.

create_engine() also had to be pulled out because make_db needs to call it in order to get an engine to pass to create_all(engine).

I think we could probably call "Base.metadata.create_all(engine)" directly in the script? Save ourselves a test. I'm not sure wrapping it here provides benefit.

I've inlined the create_all() and also the stamping, but left create_engine() in db.py because it's actually called in two places

jon-betts · 2023-07-26T14:58:18Z

via/db.py

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


Honestly not having dev specific stuff in here seems like a win to me.

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.

seanh · 2023-07-27T15:50:51Z

.cookiecutter/includes/conf/supervisord-dev.conf.json

@@ -1,5 +1,9 @@
 {
  "programs": {
+    "make_db": {
+      "command": "bin/make_db",
+      "startsecs": "0"


By default if bin/make_db exits in under a second (which it easily can do) Supervisor will consider this a crash regardless of the exit code and will restart the process. Setting startsecs to 0 tells Supervisor not to consider this a crash and not to restart the process (unless the exit code is non-zero)

seanh · 2023-07-27T15:51:24Z

bin/make_db

+
+
+def main():
+    with bootstrap("conf/development.ini") as env:


with is the recommended way to use bootstrap() in the Pyramid docs, make_db wasn't doing this before

marcospri · 2023-07-28T13:16:27Z

bin/make_db

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


As I mentioned on the other PR I reckon either here or in the Makefile
we should run the migrations as part of the make db process

https://github.com/hypothesis/lms/blob/main/Makefile#L18

I reckon with that we'll get a "it just works" experience for dev that are not making changes here often and just want to have the service running.

Via does run the migrations in Makefile. I think make db has always done this for all our apps?

It may be a bit confusing that there's a bin/make_db script, but calling that script is only one of two commands that make db runs. May want to move the alembic uprade command into the make_db script somehow

marcospri

I've seen Jon already took a look here.

Made a comment about the migration in the make db process. I reckon that's the right solution but if you prefer we can chat about that once we have actual migrations in this project.

seanh commented Jul 26, 2023

View reviewed changes

jon-betts reviewed Jul 26, 2023

View reviewed changes

seanh force-pushed the make_db branch 2 times, most recently from 6cb8fbd to 884c296 Compare July 26, 2023 16:01

seanh changed the title ~~Move dev DB creation to a separate process~~ Move dev DB schema creation and stamping to a separate process Jul 26, 2023

seanh force-pushed the db branch 2 times, most recently from 1ec445e to 7f96bf1 Compare July 27, 2023 13:26

seanh force-pushed the make_db branch from 884c296 to 8b1d2d9 Compare July 27, 2023 13:53

seanh force-pushed the make_db branch from 8b1d2d9 to c97bcea Compare July 27, 2023 15:48

seanh commented Jul 27, 2023

View reviewed changes

seanh requested a review from marcospri July 27, 2023 15:51

seanh marked this pull request as ready for review July 27, 2023 15:51

seanh mentioned this pull request Jul 27, 2023

Fix make db and add schema creation and auto-stamping hypothesis/cookiecutters#147

Merged

marcospri reviewed Jul 28, 2023

View reviewed changes

marcospri approved these changes Jul 28, 2023

View reviewed changes

seanh merged commit 2a63b4b into db Jul 30, 2023
7 checks passed

seanh deleted the make_db branch July 30, 2023 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move dev DB schema creation and stamping to a separate process #1123

Move dev DB schema creation and stamping to a separate process #1123

seanh commented Jul 26, 2023 •

edited

Loading

seanh Jul 26, 2023

seanh Jul 26, 2023

jon-betts Jul 26, 2023 •

edited

Loading

seanh Jul 26, 2023

jon-betts Jul 26, 2023

seanh Jul 26, 2023

seanh Jul 27, 2023

seanh Jul 27, 2023

marcospri Jul 28, 2023

seanh Jul 30, 2023

marcospri left a comment

Move dev DB schema creation and stamping to a separate process #1123

Move dev DB schema creation and stamping to a separate process #1123

Conversation

seanh commented Jul 26, 2023 • edited Loading

Context

Problem

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jon-betts Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcospri left a comment

Choose a reason for hiding this comment

seanh commented Jul 26, 2023 •

edited

Loading

jon-betts Jul 26, 2023 •

edited

Loading