feat(observability): report why a process runs without a pending drops cleaner - #1063
Draft
aparajon wants to merge 7 commits into
Draft
feat(observability): report why a process runs without a pending drops cleaner#1063aparajon wants to merge 7 commits into
aparajon wants to merge 7 commits into
Conversation
The quarantine and its cleaner are two halves of one feature, but only the quarantine was on by default. A deployment that could rename a dropped table into `_pending_drops` on its target could not necessarily reach that same target to reap it later, so quarantined tables piled up on target servers with nothing scheduled to remove them. Default `pending_drops.enabled` to false. With no `pending_drops` block a `DROP TABLE` executes as written, and turning the quarantine on is a deliberate statement that this deployment reaps its own targets.
The declarative differ emits a bare DROP TABLE, and the DROP phase re-runs from its first statement every time an apply resumes. A phase that dropped some of its tables before being stopped failed on the second attempt with "unknown table" and never reached the tables still standing, so the apply could not finish without an operator editing the schema. Check each target before dropping it and skip the ones that are already absent, which converges on exactly the state the plan asked for. The skip is logged and surfaced on the apply log, because on a first attempt it means something outside the apply removed the table.
A table removed between the existence check and the drop has already reached the state the plan asked for, so failing the apply on it defeats the convergence the check exists to provide. The check still decides what is reported as already absent.
…opt-in # Conflicts: # pkg/engine/spirit/pending_drops.go
…s cleaner Quarantining without reaping is what leaves tables on a target forever, so the reason a process is not reaping is the thing an operator needs to see. A single predicate reported all of them with one message, which could not distinguish a process that has nothing to reap from one that quarantines and leaves reaping to a deployment that may not exist. Split the decline into its causes, state the consequence of each definitively, and count them by reason so the dangerous combinations are alertable. The no-local-targets message now carries the routed target count, which is what separates a control plane whose deployments reap their own targets from a process with no MySQL topology at all.
The metadata key the servers send is how a deployment states whether it quarantines, but the local client still read an absent key as "quarantine". An embedder that builds a LocalConfig itself, bypassing both server paths, therefore got the quarantine with no cleaner guaranteed — the orphaned quarantine this opt-in exists to prevent. Read the key as an explicit opt-in, and have both server paths state it either way rather than only when disabled, so a data plane that predates the new default cannot read an omitted key as consent during a rolling deploy. Also record a counter when a DROP target is already absent, so the case is alertable rather than only greppable; cover the direct path's view/temporary/IF EXISTS forwarding, which was only exercised with the quarantine enabled; and correct a comment that still called the quarantine the default.
…cleaner-decline-reasons
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #1057 — review that first; this PR's diff is the last commit only.
Quarantining without reaping is what leaves tables on a target forever, so the reason a process is not reaping is the thing an operator needs to see. A single predicate reported all four causes with one message, which could not distinguish a process that has nothing to reap from one that quarantines and leaves reaping to a deployment that may not exist.
Each cause is now reported separately, states its consequence definitively, and is counted by reason so the dangerous combinations are alertable:
quarantine_disabledDROP TABLEexecutes as written, so there is nothing to reap.no_local_targetscleanup_disabled_for_processinvalid_retentionThe
no_local_targetsmessage now carries the routed-target count, which is what separates a control plane whose deployments reap their own targets from a process with no MySQL topology at all. That distinction is also why this case stays at info: warning on it would fire on every control plane for a benign state.Authored by Claude Code (claude-opus-5).