ref(crons): Add null constraint to monitorcheckin monitor_environment

[evanpurkhiser]

All old check-ins that didn't have this set have fallen out of
retention. This was added as compat while we added environments

select count(1) from sentry_monitorcheckin where monitor_environment_id is null;

All return 0 across US / DE

[github-actions]

This PR has a migration; here is the generated SQL for src/sentry/migrations/0818_enforce_check_in_environment_not_null.py ()

--
-- Alter field monitor_environment on monitorcheckin
--
SET CONSTRAINTS "sentry_monitorchecki_monitor_environment__1e84da50_fk_sentry_mo" IMMEDIATE; ALTER TABLE "sentry_monitorcheckin" DROP CONSTRAINT "sentry_monitorchecki_monitor_environment__1e84da50_fk_sentry_mo";
ALTER TABLE "sentry_monitorcheckin" ADD CONSTRAINT "sentry_monitorchecki_monitor_environment__1e84da50_fk_sentry_mo" FOREIGN KEY ("monitor_environment_id") REFERENCES "sentry_monitorenvironment" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_monitorcheckin" VALIDATE CONSTRAINT "sentry_monitorchecki_monitor_environment__1e84da50_fk_sentry_mo";
ALTER TABLE "sentry_monitorcheckin" ADD CONSTRAINT "sentry_monitorcheckin_monitor_environment_id_1e84da50_notnull" CHECK ("monitor_environment_id" IS NOT NULL) NOT VALID;
ALTER TABLE "sentry_monitorcheckin" VALIDATE CONSTRAINT "sentry_monitorcheckin_monitor_environment_id_1e84da50_notnull";
ALTER TABLE "sentry_monitorcheckin" ALTER COLUMN "monitor_environment_id" SET NOT NULL;
ALTER TABLE "sentry_monitorcheckin" DROP CONSTRAINT "sentry_monitorcheckin_monitor_environment_id_1e84da50_notnull";

[asottile-sentry]

surprised the migrations codeowners didn't kick in 🤔

[evanpurkhiser]

Going to get some @getsentry/owners-migrations eyes on this, though I believe this is generating the correct DDL according to

https://develop.sentry.dev/backend/application-domains/database-migrations/#adding-constraints-to-columns-including-not-null

We want the DEFERRED and VALIDATE after

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #83832   +/-   ##
=======================================
  Coverage   87.63%   87.63%           
=======================================
  Files        9510     9510           
  Lines      542008   541992   -16     
  Branches    21222    21222           
=======================================
- Hits       474968   474959    -9     
+ Misses      66674    66667    -7     
  Partials      366      366           

[vgrozdanic]

This table has over 600M rows just in US , so the validations of constraints very likely won't finish in 5 seconds (our time limit for in-deployment migrations).

We should mark this as post-deployment migration and run it manually

[markstory]

Interesting that Django is dropping and rebuilding the foreign key constraint. I agree with @vgrozdanic that this should be done as a post_deploy migration to avoid query timeouts.

[wedamija]

This PR has a migration; here is the generated SQL for src/sentry/migrations/0818_enforce_check_in_environment_not_null.py ()

--
-- Alter field monitor_environment on monitorcheckin
--
SET CONSTRAINTS "sentry_monitorchecki_monitor_environment__1e84da50_fk_sentry_mo" IMMEDIATE; ALTER TABLE "sentry_monitorcheckin" DROP CONSTRAINT "sentry_monitorchecki_monitor_environment__1e84da50_fk_sentry_mo";
ALTER TABLE "sentry_monitorcheckin" ADD CONSTRAINT "sentry_monitorchecki_monitor_environment__1e84da50_fk_sentry_mo" FOREIGN KEY ("monitor_environment_id") REFERENCES "sentry_monitorenvironment" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_monitorcheckin" VALIDATE CONSTRAINT "sentry_monitorchecki_monitor_environment__1e84da50_fk_sentry_mo";
ALTER TABLE "sentry_monitorcheckin" ADD CONSTRAINT "sentry_monitorcheckin_monitor_environment_id_1e84da50_notnull" CHECK ("monitor_environment_id" IS NOT NULL) NOT VALID;
ALTER TABLE "sentry_monitorcheckin" VALIDATE CONSTRAINT "sentry_monitorcheckin_monitor_environment_id_1e84da50_notnull";
ALTER TABLE "sentry_monitorcheckin" ALTER COLUMN "monitor_environment_id" SET NOT NULL;
ALTER TABLE "sentry_monitorcheckin" DROP CONSTRAINT "sentry_monitorcheckin_monitor_environment_id_1e84da50_notnull";

Don't know why it feels the need to drop/recreate the fk constraint here... not a huge problem, it'll just slow it down

[vgrozdanic]

I was also interested in drop/add of a non-related constraint and started searching through Django tickets to see if there is already a bug reported because it feels like a bug.

There were a few reports and a fix for a similar behavior, but I would have to check in demo project first if the newest version of Django has this fixed and if maybe this problems comes because of our mocks in migrations (although unlikely) 🤷

[evanpurkhiser]

Fixed tests and changed over to a post deploy

[markstory]

This code will be active before the migration is run. Will that behave correctly?

[asottile-sentry]

it's never None so it's currently dead code

[evanpurkhiser]

Yep, what Anthony said.