Browse Source

migration(crons): Remove invalidly slugged monitors (#52896)

Correctly marks monitors as `PENDING_DELETION` instead of trying to
delete them mid run.
Richard Ortenberg 1 year ago
parent
commit
536fa53fb9

+ 1 - 1
migrations_lockfile.txt

@@ -6,5 +6,5 @@ To resolve this, rebase against latest master and regenerate your migration. Thi
 will then be regenerated, and you should be able to merge without conflicts.
 
 nodestore: 0002_nodestore_no_dictfield
-sentry: 0514_migrate_priority_saved_searches
+sentry: 0515_slugify_invalid_monitors
 social_auth: 0001_initial

+ 100 - 0
src/sentry/migrations/0515_slugify_invalid_monitors.py

@@ -0,0 +1,100 @@
+# Generated by Django 2.2.28 on 2023-07-07 15:51
+from django.db import IntegrityError, migrations
+from django.utils.text import slugify
+
+from sentry.constants import ObjectStatus
+from sentry.new_migrations.migrations import CheckedMigration
+from sentry.utils.query import RangeQuerySetWrapperWithProgressBar
+
+
+def schedule(cls, instance, days=30):
+    from datetime import timedelta
+
+    from django.utils import timezone
+
+    model = type(instance)
+    model_name = model.__name__
+    cls.objects.update_or_create(
+        app_label=instance._meta.app_label,
+        model_name=model_name,
+        object_id=instance.pk,
+        defaults={
+            "actor_id": None,
+            "data": {},
+            "date_scheduled": timezone.now() + timedelta(days=days, hours=0),
+        },
+    )
+
+
+def migrate_monitor_slugs(apps, schema_editor):
+    Monitor = apps.get_model("sentry", "Monitor")
+    Rule = apps.get_model("sentry", "Rule")
+    RegionScheduledDeletion = apps.get_model("sentry", "RegionScheduledDeletion")
+    ScheduledDeletion = apps.get_model("sentry", "ScheduledDeletion")
+
+    MAX_SLUG_LENGTH = 50
+
+    for monitor in RangeQuerySetWrapperWithProgressBar(Monitor.objects.all()):
+        monitor_slug = monitor.slug
+        slugified = slugify(monitor_slug)[:MAX_SLUG_LENGTH].strip("-")
+
+        # Nothing to migrate if the monitor already has a valid slug
+        if monitor_slug == slugified:
+            continue
+
+        try:
+            monitor.slug = slugified
+            monitor.save()
+        except IntegrityError:
+            # If there is a collision, delete the old monitor as the new one is receiving all check-ins
+            alert_rule_id = monitor.config.get("alert_rule_id")
+            if alert_rule_id:
+                rule = (
+                    Rule.objects.filter(
+                        project_id=monitor.project_id,
+                        id=alert_rule_id,
+                    )
+                    .exclude(
+                        status__in=[
+                            ObjectStatus.PENDING_DELETION,
+                            ObjectStatus.DELETION_IN_PROGRESS,
+                        ]
+                    )
+                    .first()
+                )
+                if rule:
+                    rule.status = ObjectStatus.PENDING_DELETION
+                    rule.save()
+                    schedule(RegionScheduledDeletion, rule, days=0)
+
+            # Revert slug so as not to attempt re-saving clean slug
+            monitor.slug = monitor_slug
+            monitor.status = ObjectStatus.PENDING_DELETION
+            monitor.save()
+            schedule(ScheduledDeletion, monitor, days=0)
+
+
+class Migration(CheckedMigration):
+    # This flag is used to mark that a migration shouldn't be automatically run in production. For
+    # the most part, this should only be used for operations where it's safe to run the migration
+    # after your code has deployed. So this should not be used for most operations that alter the
+    # schema of a table.
+    # Here are some things that make sense to mark as dangerous:
+    # - Large data migrations. Typically we want these to be run manually by ops so that they can
+    #   be monitored and not block the deploy for a long period of time while they run.
+    # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to
+    #   have ops run this and not block the deploy. Note that while adding an index is a schema
+    #   change, it's completely safe to run the operation after the code has deployed.
+    is_dangerous = False
+
+    dependencies = [
+        ("sentry", "0514_migrate_priority_saved_searches"),
+    ]
+
+    operations = [
+        migrations.RunPython(
+            migrate_monitor_slugs,
+            migrations.RunPython.noop,
+            hints={"tables": ["sentry_monitor"]},
+        ),
+    ]

+ 18 - 1
src/sentry/monitors/endpoints/organization_monitor_details.py

@@ -21,7 +21,13 @@ from sentry.apidocs.constants import (
 from sentry.apidocs.parameters import GlobalParams, MonitorParams
 from sentry.apidocs.utils import inline_sentry_response_serializer
 from sentry.constants import ObjectStatus
-from sentry.models import Rule, RuleActivity, RuleActivityType, ScheduledDeletion
+from sentry.models import (
+    RegionScheduledDeletion,
+    Rule,
+    RuleActivity,
+    RuleActivityType,
+    ScheduledDeletion,
+)
 from sentry.monitors.models import Monitor, MonitorEnvironment, MonitorStatus
 from sentry.monitors.serializers import MonitorSerializer, MonitorSerializerResponse
 from sentry.monitors.utils import create_alert_rule, update_alert_rule
@@ -213,6 +219,17 @@ class OrganizationMonitorDetailsEndpoint(MonitorEndpoint):
                         RuleActivity.objects.create(
                             rule=rule, user_id=request.user.id, type=RuleActivityType.DELETED.value
                         )
+                        scheduled_rule = RegionScheduledDeletion.schedule(
+                            rule, days=0, actor=request.user
+                        )
+                        self.create_audit_entry(
+                            request=request,
+                            organization=project.organization,
+                            target_object=rule.id,
+                            event=audit_log.get_event_id("RULE_REMOVE"),
+                            data=rule.get_audit_log_data(),
+                            transaction_id=scheduled_rule,
+                        )
 
             # create copy of queryset as update will remove objects
             monitor_objects_list = list(monitor_objects)

+ 85 - 0
tests/sentry/migrations/test_0515_slugify_invalid_monitors.py

@@ -0,0 +1,85 @@
+from django.utils.text import slugify
+
+from sentry.constants import ObjectStatus
+from sentry.models import ScheduledDeletion
+from sentry.monitors.models import Monitor, MonitorType, ScheduleType
+from sentry.testutils.cases import TestMigrations
+
+
+class MigrateSlugifyInvalidMonitorTest(TestMigrations):
+    migrate_from = "0514_migrate_priority_saved_searches"
+    migrate_to = "0515_slugify_invalid_monitors"
+
+    def setup_before_migration(self, apps):
+        self.invalid_monitor_1 = Monitor.objects.create(
+            name="invalid_1",
+            slug="/api/analytics/admin.get?token=123",
+            organization_id=self.organization.id,
+            project_id=self.project.id,
+            type=MonitorType.CRON_JOB,
+            config={"schedule": "* * * * *", "schedule_type": ScheduleType.CRONTAB},
+        )
+
+        self.invalid_monitor_2 = Monitor.objects.create(
+            name="invalid_2",
+            slug="/api/cron",
+            organization_id=self.organization.id,
+            project_id=self.project.id,
+            type=MonitorType.CRON_JOB,
+            config={"schedule": "* * * * *", "schedule_type": ScheduleType.CRONTAB},
+        )
+
+        self.invalid_monitor_3 = Monitor.objects.create(
+            name="invalid_2",
+            slug="/api/cron/3",
+            organization_id=self.organization.id,
+            project_id=self.project.id,
+            type=MonitorType.CRON_JOB,
+            config={"schedule": "* * * * *", "schedule_type": ScheduleType.CRONTAB},
+        )
+
+        self.valid_monitor_1 = Monitor.objects.create(
+            name="valid_1",
+            slug="valid_1",
+            organization_id=self.organization.id,
+            project_id=self.project.id,
+            type=MonitorType.CRON_JOB,
+            config={"schedule": "* * * * *", "schedule_type": ScheduleType.CRONTAB},
+        )
+
+        self.valid_monitor_2 = Monitor.objects.create(
+            name="valid_2",
+            slug="apicron3",
+            organization_id=self.organization.id,
+            project_id=self.project.id,
+            type=MonitorType.CRON_JOB,
+            config={"schedule": "* * * * *", "schedule_type": ScheduleType.CRONTAB},
+        )
+
+    def test(self):
+        MAX_SLUG_LENGTH = 50
+
+        invalid_monitor_1 = Monitor.objects.get(id=self.invalid_monitor_1.id)
+        invalid_monitor_2 = Monitor.objects.get(id=self.invalid_monitor_2.id)
+
+        valid_monitor_1 = Monitor.objects.get(id=self.valid_monitor_1.id)
+        valid_monitor_2 = Monitor.objects.get(id=self.valid_monitor_2.id)
+
+        assert invalid_monitor_1.slug == slugify(self.invalid_monitor_1.slug)[
+            :MAX_SLUG_LENGTH
+        ].strip("-")
+        assert invalid_monitor_2.slug == slugify(self.invalid_monitor_2.slug)[
+            :MAX_SLUG_LENGTH
+        ].strip("-")
+
+        # assert colliding monitor is in a pending deletion state
+        assert (
+            ObjectStatus.PENDING_DELETION
+            == Monitor.objects.get(id=self.invalid_monitor_3.id).status
+        )
+        assert ScheduledDeletion.objects.filter(
+            object_id=self.invalid_monitor_3.id, model_name="Monitor"
+        ).exists()
+
+        assert valid_monitor_1.slug == self.valid_monitor_1.slug
+        assert valid_monitor_2.slug == self.valid_monitor_2.slug