Skip to content

Commit f644ccf

Browse files
authored
fix(migrations): Fail migrations that delete models if no historical_silo_assignment is found (#103702)
We have been silently failing the final deletion of tables for a while, because we need to add them to `historical_silo_assignments` for it to work correctly. This pr makes sure that we fail in CI if we aren't able to find an entry there.
1 parent 396a475 commit f644ccf

File tree

3 files changed

+89
-0
lines changed

3 files changed

+89
-0
lines changed

src/sentry/db/router.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,13 @@ class SiloRouter:
6868
historical_silo_assignments = {
6969
"authidentity_duplicate": SiloMode.CONTROL,
7070
"authprovider_duplicate": SiloMode.CONTROL,
71+
"feedback_feedback": SiloMode.REGION,
72+
"releases_commit": SiloMode.REGION,
73+
"releases_commitfilechange": SiloMode.REGION,
7174
"sentry_actor": SiloMode.REGION,
7275
"sentry_alertruleactivations": SiloMode.REGION,
76+
"sentry_dashboardwidgetsnapshot": SiloMode.REGION,
77+
"sentry_datasecrecywaiver": SiloMode.REGION,
7378
"sentry_incidentseen": SiloMode.REGION,
7479
"sentry_incidentsubscription": SiloMode.REGION,
7580
"sentry_monitorlocation": SiloMode.REGION,
@@ -78,6 +83,8 @@ class SiloRouter:
7883
"sentry_projectavatar": SiloMode.REGION,
7984
"sentry_scheduledjob": SiloMode.CONTROL,
8085
"sentry_teamavatar": SiloMode.REGION,
86+
"uptime_projectuptimesubscription": SiloMode.REGION,
87+
"workflow_engine_actiongroupstatus": SiloMode.REGION,
8188
"workflow_engine_workflowaction": SiloMode.REGION,
8289
}
8390
"""

src/sentry/new_migrations/monkey/models.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
1+
from django.db import router
12
from django.db.migrations import DeleteModel
23
from django_zero_downtime_migrations.backends.postgres.schema import UnsafeOperationException
34

45
from sentry.db.postgres.schema import SafePostgresDatabaseSchemaEditor
56
from sentry.new_migrations.monkey.state import DeletionAction, SentryProjectState
7+
from sentry.utils.env import in_test_environment
68

79

810
class SafeDeleteModel(DeleteModel):
@@ -35,6 +37,27 @@ def database_forwards(
3537
return
3638

3739
model = from_state.get_pending_deletion_model(app_label, self.name)
40+
table = model._meta.db_table
41+
42+
# Check if we can determine the model's database to detect missing
43+
# historical_silo_assignments entries
44+
resolved_db = None
45+
for db_router in router.routers:
46+
if hasattr(db_router, "_db_for_table"):
47+
resolved_db = db_router._db_for_table(table, app_label)
48+
if resolved_db is not None:
49+
break
50+
51+
# If we can't determine the database and we're in CI/tests, fail loudly
52+
# This indicates the table is missing from historical_silo_assignments
53+
if resolved_db is None and in_test_environment():
54+
raise ValueError(
55+
f"Cannot determine database for deleted model {app_label}.{self.name} "
56+
f"(table: {table}). This table must be added to historical_silo_assignments "
57+
f"in src/sentry/db/router.py (or getsentry/db/router.py for getsentry models) "
58+
f"with the appropriate SiloMode before the deletion migration can run. "
59+
)
60+
3861
if self.allow_migrate_model(schema_editor.connection.alias, model):
3962
schema_editor.delete_model(model, is_safe=True)
4063

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
"""
2+
Tests for SafeDeleteModel validation that ensures deleted models are added to
3+
historical_silo_assignments.
4+
"""
5+
6+
from typing import cast
7+
from unittest.mock import Mock
8+
9+
import pytest
10+
from django.db import connection
11+
12+
from sentry.db.postgres.schema import SafePostgresDatabaseSchemaEditor
13+
from sentry.new_migrations.monkey.models import SafeDeleteModel
14+
from sentry.new_migrations.monkey.state import DeletionAction, SentryProjectState
15+
from sentry.testutils.cases import TestCase
16+
17+
18+
class SafeDeleteModelTest(TestCase):
19+
"""
20+
Tests that SafeDeleteModel fails loudly when a deleted model is not in
21+
historical_silo_assignments.
22+
"""
23+
24+
def test_delete_model_without_historical_assignment_fails(self) -> None:
25+
"""
26+
When deleting a model that is not in historical_silo_assignments and
27+
cannot be found, SafeDeleteModel should raise a ValueError in test
28+
environments.
29+
"""
30+
fake_meta = Mock()
31+
fake_meta.db_table = "sentry_fake_deleted_table_not_in_router"
32+
fake_meta.app_label = "sentry"
33+
fake_meta.model_name = "fakedeletedmodel"
34+
35+
FakeDeletedModel = Mock()
36+
FakeDeletedModel._meta = fake_meta
37+
38+
from_state = SentryProjectState()
39+
to_state = SentryProjectState()
40+
41+
# Manually add the model to pending deletion in the from_state
42+
# This simulates what happens when a model was marked for pending deletion
43+
# and is now being deleted
44+
from_state.pending_deletion_models[("sentry", "fakedeletedmodel")] = FakeDeletedModel
45+
46+
operation = SafeDeleteModel(name="FakeDeletedModel", deletion_action=DeletionAction.DELETE)
47+
48+
with connection.schema_editor() as schema_editor:
49+
with pytest.raises(ValueError) as exc_info:
50+
operation.database_forwards(
51+
"sentry",
52+
cast(SafePostgresDatabaseSchemaEditor, schema_editor),
53+
from_state,
54+
to_state,
55+
)
56+
57+
assert "Cannot determine database for deleted model" in str(exc_info.value)
58+
assert "sentry_fake_deleted_table_not_in_router" in str(exc_info.value)
59+
assert "historical_silo_assignments" in str(exc_info.value)

0 commit comments

Comments
 (0)