Browse Source

fix(tools) Fix postdeploy migrations creating unsafe indexes (#62937)

The postdeploy migrations tool was not loading our monkey patches, or
enabling 'safe' mode on the patched executor. Added tests for the
postdeploy migration tool covering concurrent index creation.

Refs HC-1054
Mark Story 1 year ago
parent
commit
a638492f43

+ 0 - 0
fixtures/safe_migrations_apps/migration_test_app/__init__.py


+ 30 - 0
fixtures/safe_migrations_apps/migration_test_app/migrations/0001_create_migration_run_test.py

@@ -0,0 +1,30 @@
+from django.db import migrations, models
+
+import sentry.db.models.fields.bounded
+from sentry.new_migrations.migrations import CheckedMigration
+
+
+class Migration(CheckedMigration):
+    initial = True
+    is_dangerous = True
+
+    dependencies = []
+
+    operations = [
+        migrations.CreateModel(
+            name="MigrationRunTest",
+            fields=[
+                (
+                    "id",
+                    sentry.db.models.fields.bounded.BoundedBigAutoField(
+                        primary_key=True, serialize=False
+                    ),
+                ),
+                ("name", models.CharField(max_length=255)),
+            ],
+        ),
+        migrations.AddIndex(
+            model_name="migrationruntest",
+            index=models.Index(name="migration_run_test_name_idx", fields=["name"]),
+        ),
+    ]

+ 0 - 0
fixtures/safe_migrations_apps/migration_test_app/migrations/__init__.py


+ 6 - 0
fixtures/safe_migrations_apps/migration_test_app/models.py

@@ -0,0 +1,6 @@
+from django.db import models
+
+
+class MigrationRunTest(models.Model):
+    name = models.CharField(max_length=255)
+    indexes = models.Index(name="migration_run_test_name_idx", fields=["name"])

+ 14 - 3
src/sentry/runner/commands/migrations.py

@@ -1,20 +1,28 @@
+import os
+
 import click
 import click
 
 
 from sentry.runner.decorators import configuration
 from sentry.runner.decorators import configuration
 
 
 
 
 @click.group()
 @click.group()
+@configuration
 def migrations():
 def migrations():
-    "Manage migrations."
+    from sentry.runner.initializer import monkeypatch_django_migrations
+
+    # Include our monkeypatches for migrations.
+    monkeypatch_django_migrations()
+
+    # Allow dangerous/postdeploy migrations to be run.
+    os.environ["MIGRATION_SKIP_DANGEROUS"] = "0"
 
 
 
 
 @migrations.command()
 @migrations.command()
 @click.argument("app_name")
 @click.argument("app_name")
 @click.argument("migration_name")
 @click.argument("migration_name")
-@configuration
 @click.pass_context
 @click.pass_context
 def run(ctx, app_name, migration_name):
 def run(ctx, app_name, migration_name):
-    "Manually run a single data migration. Will error if migration is not data only."
+    "Manually run a single data migration. Will error if migration is not post-deploy/dangerous"
     del ctx  # assertion: unused argument
     del ctx  # assertion: unused argument
 
 
     from django.db import connection, connections
     from django.db import connection, connections
@@ -36,6 +44,9 @@ def run(ctx, app_name, migration_name):
     click.secho("Running post-deployment migration:", fg="cyan")
     click.secho("Running post-deployment migration:", fg="cyan")
     click.secho(f"  {migration.name}", bold=True)
     click.secho(f"  {migration.name}", bold=True)
     with connection.schema_editor() as schema_editor:
     with connection.schema_editor() as schema_editor:
+        # Enable 'safe' migration execution. This enables concurrent mode on index creation
+        setattr(schema_editor, "safe", True)
+
         for op in migration.operations:
         for op in migration.operations:
             click.echo(f"    * {op.describe()}... ", nl=False)
             click.echo(f"    * {op.describe()}... ", nl=False)
             new_state = project_state.clone()
             new_state = project_state.clone()

+ 33 - 0
tests/sentry/runner/commands/test_migrations.py

@@ -0,0 +1,33 @@
+from click.testing import CliRunner
+from django.db import connection
+from django.test import override_settings
+
+from sentry.runner.commands.migrations import migrations
+from sentry.testutils.cases import TransactionTestCase
+
+
+class MigrationsRunTest(TransactionTestCase):
+    command = migrations
+
+    # Copy paste from CliTest as this test needs to escape auto transactions
+    @property
+    def runner(self) -> CliRunner:
+        return CliRunner()
+
+    def invoke(self, *args, **kwargs):
+        return self.runner.invoke(self.command, args, obj={}, **kwargs)
+
+    def test_index_creation(self):
+        with override_settings(
+            INSTALLED_APPS=("fixtures.safe_migrations_apps.migration_test_app",),
+            MIGRATION_MODULES={},
+        ):
+            result = self.invoke("run", "migration_test_app", "0001")
+
+            assert result.exit_code == 0, result.output
+            assert "Running post-deployment migration" in result.output
+            assert "Migration complete" in result.output
+
+            queries = [q["sql"] for q in connection.queries]
+            expected = 'CREATE INDEX CONCURRENTLY "migration_run_test_name_idx" ON "migration_test_app_migrationruntest" ("name")'
+            assert expected in queries, queries