Shared dispatcher pool: default for new ER destinations + fleet migration tooling#450
Shared dispatcher pool: default for new ER destinations + fleet migration tooling#450chrisdoehring wants to merge 10 commits into
Conversation
When ER_SHARED_DISPATCHER_TOPIC is set, new EarthRanger integrations get the shared topic in additional (signal-safe update) and no dedicated dispatcher; additional[dedicated_dispatcher]=true opts back in. Empty setting preserves today's behavior everywhere.
Batched topic flip onto the shared ER pool with rollback bookkeeping (pre_migration_topic + shared_pool_migrated_at in additional); old dispatchers stay running as the rollback lever. Refuses when ER_SHARED_DISPATCHER_TOPIC is unset; skips dedicated_dispatcher integrations.
Restore the brief's module-level _make_er_destination_with_deployment helper and exact test bodies (Tasks 3-4 append tests that call it), and guard _get_migratable_er_integrations against --max 0. Also fixes pre-existing session-wide settings pollution surfaced by the restored tests: tests that combined @override_settings( GCP_ENVIRONMENT_ENABLED=True) with mid-test request.getfixturevalue of fixtures that set the same flag via the pytest-django settings fixture leaked GCP_ENVIRONMENT_ENABLED=True into every later test (the decorator's disable() and the fixture finalizer run out of order). The decorator was redundant there - the fixtures set the flag for the test's duration - so it is removed on those three tests.
Restores the pre-migration topic while the dormant dispatcher still exists; refuses with guidance after teardown.
Tears down dormant pre-migration dispatchers after the 7-day cooling period (overridable, loudly). Refuses to delete any deployment that records the shared topic as its own (deletion deletes the topic), and skips subscriptions that still hold undelivered messages. Manually migrated integrations get stamped on first sight instead of torn down blind.
The drain check now reads the Monitoring num_undelivered_messages metric (pull is rejected on push subscriptions, so the old peek could never succeed); teardown gets a per-item error envelope; migrated integrations are excluded from --update-source/--recreate selection (a redeploy would attach a zombie subscription to the shared topic); pre_migration_topic can no longer be stored falsy.
…test - dispatchers.py: Replace rollback guidance (--deploy won't work on gcp_pubsub integrations) - utils.py: Document subscription_is_drained caveat with out-of-band deletions - test_commands.py: Add healthy integration to verify teardown continues after malformed stamp Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Implements EarthRanger’s “shared dispatcher pool” migration path: new ER integrations attach to a shared Pub/Sub topic by default (dedicated dispatchers become opt-in), and the dispatchers management command gains verbs to migrate/rollback/teardown legacy dedicated dispatchers with safety checks (including a Cloud Monitoring–based drain check).
Changes:
- Add
ER_SHARED_DISPATCHER_TOPICsetting and default new ER destinations to that shared topic (unlessadditional["dedicated_dispatcher"]=true). - Extend
dispatcherscommand with--migrate-to-shared,--rollback-shared, and--teardown-migrated(cooling period + drain guard). - Add Cloud Monitoring client dependency and unit tests for the Monitoring-based drain check and shared-pool workflows.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| dependencies/requirements.in | Adds google-cloud-monitoring pin used by the new drain-check implementation. |
| dependencies/requirements.txt | Compiled requirements update reflecting the Monitoring dependency. |
| cdip_admin/cdip_admin/settings.py | Introduces ER_SHARED_DISPATCHER_TOPIC setting and documents behavior. |
| cdip_admin/integrations/models/v2/models.py | Defaults new ER integrations to shared topic (signal-safe update) unless dedicated opt-in. |
| cdip_admin/deployments/utils.py | Adds Monitoring-based subscription_is_drained() helper used by teardown safety checks. |
| cdip_admin/deployments/management/commands/dispatchers.py | Adds shared-pool migrate/rollback/teardown verbs and excludes migrated ER from redeploy/update-source flows. |
| cdip_admin/deployments/tests/test_drain_check.py | New unit tests for subscription_is_drained() behavior. |
| cdip_admin/deployments/tests/test_commands.py | Adds command-level tests for migrate/rollback/teardown and update-source exclusion behavior. |
| cdip_admin/deployments/tests/test_automatic_deployments.py | Adds tests ensuring new ER integrations default to shared pool and dedicated opt-in preserves old behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| env_vars = (configuration or {}).get("env_vars", {}) | ||
| project_id = env_vars.get("GCP_PROJECT_ID") | ||
| project_name = f"projects/{project_id}" |
There was a problem hiding this comment.
Fixed in 40391e3: subscription_is_drained now returns False early (with a warning naming the subscription) when GCP_PROJECT_ID is missing/falsy, before any Monitoring request is built, so we never query projects/None; added unit tests covering the early return.
| now = time.time() | ||
| interval = monitoring_v3.TimeInterval() | ||
| interval.end_time.seconds = int(now) | ||
| interval.end_time.nanos = int((now - interval.end_time.seconds) * 10 ** 9) | ||
| interval.start_time.seconds = int(now - _DRAIN_CHECK_LOOKBACK_SECONDS) | ||
| interval.start_time.nanos = interval.end_time.nanos | ||
|
|
There was a problem hiding this comment.
Fixed in 40391e3: the TimeInterval is now derived explicitly from a single now=time.time() — end_seconds/end_nanos from now, start_seconds = end_seconds - 600 reusing end's nanos — giving a correctly aligned 10-minute window, with a comment explaining it.
| end_seconds = point.interval.end_time.seconds | ||
| if latest_end_seconds is None or end_seconds > latest_end_seconds: | ||
| latest_end_seconds = end_seconds | ||
| latest_value = point.value.int64_value |
There was a problem hiding this comment.
Fixed in 40391e3: most-recent-point selection now compares the full (end_time.seconds, end_time.nanos) tuple instead of seconds alone, so sub-second-close points tie-break correctly.
| ).order_by("name")[:options["max"]] | ||
| ) | ||
| new_settings = None | ||
| if options.get("v2") and settings.ER_SHARED_DISPATCHER_TOPIC: |
There was a problem hiding this comment.
Fixed in 40391e3: the --update-source/--recreate v2 shared-pool exclusion is now scoped to type_cleaned == "earth_ranger", so a non-ER integration whose topic happens to coincide with the shared topic value is no longer filtered out; the existing (ER-typed) test still passes.
| from datetime import datetime | ||
| migrated_at = datetime.fromisoformat(stamp) | ||
| if migrated_at > cutoff: |
There was a problem hiding this comment.
Fixed in 40391e3: teardown_migrated now coerces a naive parsed shared_pool_migrated_at to aware UTC (timezone.is_naive/make_aware) before comparing against the aware cooling cutoff, so hand-edited naive stamps age out instead of being skipped forever by an aware/naive TypeError; added a unit test with a naive stamp.
destination-earthranger-stage per the standardized naming; prod follows after the stage validation cycle and the legacy-topic cutover. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0112VwWNyti6VJbbKH7CrSDn
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| )) | ||
| processed += 1 | ||
| continue | ||
| subscription_name = f"{deployment.name[:250]}-sub".replace("--", "-") |
There was a problem hiding this comment.
Fixed in fc0a641 — the drain check now derives the name via deployment.name or get_default_dispatcher_name(integration=integration), mirroring deploy's behavior so a null-named row is teardownable rather than skipped forever. (In practice migrated deployments always have a name, since create_dispatcher_for_integration sets it — but name is nullable, so the fallback closes the gap.)
DispatcherDeployment.name is nullable and deploy derives a default via get_default_dispatcher_name without persisting it; mirror that so a name-less row is still teardownable rather than skipped forever. Addresses PR #450 review comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0112VwWNyti6VJbbKH7CrSDn
Summary
Implements the shared-pool migration design (spec/plan in
gundi-dispatcher-er/docs/superpowers/): all EarthRanger destinations move to a single shared dispatcher deployment, and dedicated dispatchers become an explicit opt-in special case.ER_SHARED_DISPATCHER_TOPICsetting — single source of truth; empty (default) disables everything (standardized names:destination-earthranger-{env}; this PR sets the stage value in.github/workflows/values/stage.yml, prod follows after the stage validation cycle and the legacy-topic cutover documented in the spec).Integration._post_saveassigns the shared topic (signal-safe.update()) and creates no deployment;additional["dedicated_dispatcher"]=trueopts back into a dedicated function. SMART/WPSWatch/TrapTagger/v1 unchanged.dispatchers --migrate-to-shared [--max N]— batched topic flip with rollback bookkeeping (pre_migration_topic,shared_pool_migrated_at); the old function keeps running as the rollback lever; refuses when the setting is unset; skips dedicated/already-migrated; never stores an undeterminable pre-migration topic.dispatchers --rollback-shared --integration <id>— restores the old topic while the dormant dispatcher exists; manual-recovery guidance after teardown.dispatchers --teardown-migrated [--cooling-days N]— after a 7-day cooling period (override logged loudly): hard refusal for any deployment recording the shared topic (deployment deletion deletes its topic), a Cloud Monitoringnum_undelivered_messagesdrain check (pull is rejected on push subscriptions — caught in review), stamp-on-first-sight for previously hand-migrated integrations, FK-detach → delete → bookkeeping cleanup, per-item error envelope.--update-source/--recreate(v2) now exclude migrated integrations — redeploying one would attach a zombie push subscription to the shared root topic (duplicate delivery of all traffic).Review trail
Executed task-by-task with independent reviews; the final whole-branch review caught a Critical plan defect (pull-based drain check can never succeed on push subscriptions) that was fixed with the Monitoring API before merge. Two pre-existing test bugs fixed along the way (session-wide
override_settingspollution in three tests).Follow-ups (non-blocking)
monitoring.timeSeries.listto the portal service account in each environment before running--teardown-migrated.google-cloud-monitoring==2.31.0was pinned by hand in the compiled requirements; run a real dependency compile before the next release.--cooling-days) → set in prod → batches.Testing
deployments/package + health-calc suite: 96 passed, 1 pre-existing skip. New coverage: shared-pool default/dedicated flag/non-ER isolation; migrate bookkeeping/skips/refusal; rollback round-trip + post-teardown refusal; teardown cooling gate, shared-topic refusal, undrained skip, stamp-on-first-sight, malformed-stamp continuation; drain-check unit tests; update-source exclusion.🤖 Generated with Claude Code
https://claude.ai/code/session_0112VwWNyti6VJbbKH7CrSDn