opensafely-core / job-server

A server for mediating jobs that can be run in an OpenSAFELY secure environment. q.v. job-runner
https://jobs.opensafely.org
Other
5 stars 10 forks source link

Squash migrations #4393

Closed Jongmassey closed 2 months ago

Jongmassey commented 3 months ago

Following process from django docs

First step of #4182

Jongmassey commented 3 months ago

Once you’ve squashed your migration, you should then commit it alongside the migrations it replaces and distribute this change to all running instances of your application, making sure that they run migrate to store the change in their database.

$ python manage.py migrate
Operations to perform:
  Apply all migrations: applications, auth, contenttypes, interactive, jobserver, redirects, sessions, social_django
Running migrations:
  No migrations to apply.

on a freshly downloaded prod database dump, after this commit

Jongmassey commented 3 months ago

You must then transition the squashed migration to a normal migration by:

  • Deleting all the migration files it replaces.
  • Updating all migrations that depend on the deleted migrations to depend on the squashed migration instead.
  • Removing the replaces attribute in the Migration class of the squashed migration (this is how Django tells that it is a squashed migration).
$ python manage.py migrate
Operations to perform:
  Apply all migrations: applications, auth, contenttypes, interactive, jobserver, redirects, sessions, social_django
Running migrations:
  No migrations to apply.

Ignore below, now fixed in 474134e

Having done this, running migrate again fails:

$python manage.py migrate
Traceback (most recent call last):
  File "/home/jmassey/opensafely-core/job-server/manage.py", line 21, in <module>
    main()
  File "/home/jmassey/opensafely-core/job-server/manage.py", line 17, in main
    execute_from_command_line(sys.argv)
  File "/home/jmassey/opensafely-core/job-server/.venv/lib/python3.12/site-packages/django/core/management/__init__.py", line 442, in execute_from_command_line
    utility.execute()
  File "/home/jmassey/opensafely-core/job-server/.venv/lib/python3.12/site-packages/django/core/management/__init__.py", line 436, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/jmassey/opensafely-core/job-server/.venv/lib/python3.12/site-packages/django/core/management/base.py", line 413, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/home/jmassey/opensafely-core/job-server/.venv/lib/python3.12/site-packages/django/core/management/base.py", line 459, in execute
    output = self.handle(*args, **options)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/jmassey/opensafely-core/job-server/.venv/lib/python3.12/site-packages/django/core/management/base.py", line 107, in wrapper
    res = handle_func(*args, **kwargs)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/jmassey/opensafely-core/job-server/.venv/lib/python3.12/site-packages/django/core/management/commands/migrate.py", line 117, in handle
    executor = MigrationExecutor(connection, self.migration_progress_callback)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/jmassey/opensafely-core/job-server/.venv/lib/python3.12/site-packages/django/db/migrations/executor.py", line 18, in __init__
    self.loader = MigrationLoader(self.connection)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/jmassey/opensafely-core/job-server/.venv/lib/python3.12/site-packages/django/db/migrations/loader.py", line 58, in __init__
    self.build_graph()
  File "/home/jmassey/opensafely-core/job-server/.venv/lib/python3.12/site-packages/django/db/migrations/loader.py", line 276, in build_graph
    self.graph.validate_consistency()
  File "/home/jmassey/opensafely-core/job-server/.venv/lib/python3.12/site-packages/django/db/migrations/graph.py", line 198, in validate_consistency
    [n.raise_error() for n in self.node_map.values() if isinstance(n, DummyNode)]
     ^^^^^^^^^^^^^^^
  File "/home/jmassey/opensafely-core/job-server/.venv/lib/python3.12/site-packages/django/db/migrations/graph.py", line 60, in raise_error
    raise NodeNotFoundError(self.error_message, self.key, origin=self.origin)
django.db.migrations.exceptions.NodeNotFoundError: Migration applications.0001_initial dependencies reference nonexistent parent node ('jobserver', '0001_initial')
Jongmassey commented 3 months ago

Ah, I think it's because the applications and interactive applications' migrations reference the old initial 0001 migration!

Jongmassey commented 3 months ago

Consider naming the squashed migration:

I agree, I've re-run with a datestamped name as I think that might be helpful over just "squashed" in the future

Jongmassey commented 3 months ago

I think we should mark these -- and the intervening RunPython instances -- as elidable

Is my understanding correct that this requires editing the Migrations that contain RunPython instances (those from which I copied the functions per the instructions in the comment at the head of the output from the sqaushmigration command) such that they are marked as elidable e.g. 00003 would become

class Migration(migrations.Migration):
    dependencies = [
        ("jobserver", "0002_ensure_project_slugs_have_a_value"),
    ]

    operations = [
        migrations.RunPython(
            add_redirects_to_remove_org_from_orgs,
            reverse_code=remove_redirects_for_orgs,
            elidable=True,
        )
    ]

Then run the squashmigrations command which would automatically handle the situation I addressed manually?

iaindillingham commented 3 months ago

Is my understanding correct...

Your understanding is correct!

Jongmassey commented 3 months ago

OK, I've taken it up to the point of

The recommended process is to squash, keeping the old files, commit and release, wait until all systems are upgraded with the new release ... and then remove the old files, commit and do a second release.

with a named squashed output and with prior marking of RunPythons as elidable - can you take another look @iaindillingham

Jongmassey commented 2 months ago

@iaindillingham you mentioned in the meeting yesterday that there was more to squashing migrations than you'd recalled - are there any more changes required on this PR? Ta

Jongmassey commented 2 months ago

@iaindillingham you mentioned in the meeting yesterday that there was more to squashing migrations than you'd recalled - are there any more changes required on this PR? Ta

iaindillingham commented 2 months ago

@iaindillingham you mentioned in the meeting yesterday that there was more to squashing migrations than you'd recalled - are there any more changes required on this PR? Ta

There's one more change required: I've commented on the name of the squashed migration. My comment in yesterday's meeting was more "I'd forgotten how fiddly squashing migrations is".