feat: replace pooled Trino instances before voluntary node disruption

[benben]

Summary

Add default-off handling for voluntary Karpenter node replacement in shared Trino pools. Legacy Trino is unchanged.

• Protect existing coordinator and worker pod metadata only after verifying exact pod → ReplicaSet → recorded Deployment ownership.
• Record the first matching Node/NodeClaim drift or deletion evidence under the pool fence (nullable migration 000043).
• Cordon the exact node with UID/resource-version preconditions, then reuse normal create → admit → drain → retirement → UID-scoped deletion. No node deletion, pod eviction, query timeout, or repair-budget bypass.
• Require a serving replacement before infrastructure-triggered drain, including when desired count exceeds the serving floor.
• Reject unsafe candidate placement while preserving exact admission-journal replay and admission/retirement CAS races. Infrastructure lookup errors do not block health checks or explicitly authorized recovery.
• Add typed read-only replacement evidence, structured first-request logging, rollout/recovery documentation, and an opt-in pod-protection harness assertion.

Validation completed

• Planner regression failed before implementation, then passed.
• Rebased on merged fix: recover draining Trino instances after admitted pod loss #1242; 213 combined Trino/pool/effects/recovery/node tests passed after both rounds of review fixes.
• Reproduced stuck PREPARING candidates and false scheduling failures before fixing them. Definitively invalid candidates now enter guarded retirement; scheduling waits quietly and API errors remain retryable.
• Added drift/deletion observation → persisted evidence → replacement admission → drain coverage, populated/sanitized admin evidence coverage, and typed-invalid placement checks during admission.
• Retained ownership, leader-fencing, pagination, lost-admission replay, and admission/retirement race coverage.
• All 57 pure planner tests passed.
• Migration and immutable first-write/leader-fencing test passed against isolated local PostgreSQL; that server and scratch database were removed afterward.
• just lint passed with an isolated cache; Kubernetes-tagged changed-code lint passed.
• Shell syntax and whitespace checks passed.

Latest review and CI follow-up

• Reproduced and fixed replacement planning blocked by later observation failures, discarded positive drift evidence with incomplete pod inventory, and node-name-dependent drift detection.
• Preserve independent positive evidence without authorizing cordons through API or incomplete-inventory errors; malformed or absent evidence remains fail-closed.
• Candidate checks use fresh named NodeClaim GETs rather than repeated cluster-wide LISTs. Regression tests cover fresh drift/cordon changes, exact owner UID checks, per-check caching, and compound errors.
• Document the Karpenter v1 ownership prerequisite. Failed candidates retain their capacity until cleanup; unsupported scheduling can still cause repeated failures afterward.
• Fixed the failed configstore CI job: historical migration fixtures and current-version assertions now include migration 43, with JSONB/nullability assertions. Reproduced the three failures before fixing; all 204 configstore integration tests passed against isolated local PostgreSQL, which was stopped afterward.
• Independent adversarial review found no remaining blockers. Required lint and Kubernetes-tagged lint passed; commits are signed. CI is not being monitored after the push.

Live validation still pending

The opt-in live pod-protection harness has not been run, and no physical Karpenter/EC2 replacement or zero-loss client test has been performed. Unit simulations do not establish that guarantee. This PR must not be represented as production-validated.

The required cluster-scoped Node patch / NodeClaim read permissions are approved and prepared in charts #16443. Keep that activation PR unmerged until the prerequisite source image and migration have fully rolled out. No live permission changes or node mutations were performed here. Deploy the new Duckgres image to every eligible control-plane replica before enabling the chart flag. New blueprint pod annotations are chart-owned; the source flag defaults to false.

Hardware failures, forced pod/node deletion, interruption deadlines and finite NodeClaim termination grace remain outside the zero-loss claim. Once accepted, a durable replacement request is not canceled by disabling new observation. Owned cordons are deliberately not automatically removed; the runbook covers drift clearing while unrelated workloads remain.

[github-actions]

Test Impact Plan

Deterministic summary of how this PR changes tests, CI runners, and coverage-risk signals.

Summary

Area	Added	Changed	Deleted
Test files	3	4	0
E2E/journey files	0	0	0
Workflow files	0	0	0

Signals

• Test cases: +26 / -0
• Assertions: +131 / -0
• Skips or known failures added: 0
• Workflow continue-on-error added: 0
• Workflow path filters added: 0
• Test commands removed from justfile: 0
• E2E/journey retry lines added: 0

Coverage risk: neutral or increased

No coverage-reduction warnings detected.

[bill-ph]

Review

I read the PR description and the full diff, then the surrounding code in repo/: the planner (controlplane/trinopool/plan.go), the reconcile/progress/durable-step paths (trino_pool_operator.go, trino_pool_progress.go, trino_pool_durable.go, trino_pool_failure.go), the new node guard and evidence store, the recovery API projection, and the migration. The design is conservative and matches the described boundaries: default-off, no node deletion or pod eviction, UID/resourceVersion-bound cordon, first-write-wins evidence under the pool fence, Gateway drain/retirement still authoritative for deletion. The planner change correctly requires a serving replacement (serving > DesiredInstances) before draining a node-replacement instance, and the admission race/read-back handling is sound.

P1

1. A candidate on an unsafe node can stay in PREPARING and wedge the whole pool's lifecycle. validateCandidate only fails the candidate when instance.NodeReplacementEvidence != nil; the new candidateNodeCheck errors are returned as (false, err) and are treated as retryable. If inspect reports errTrinoCandidateNodeReplacement (drift/deletion observed after protectPoolNodes ran, or on a node whose NodeClaim is missing/mismatched) or errTrinoUnschedulableNode without a Drifted/Deleting condition, the candidate never advances, and PlanNext returns PlanActionNone whenever preparing > 0. That suppresses every create — including capacity repairs — and every drain/rollout for the pool until the node situation changes. The evidence branch shows the intended remedy (fail the candidate so it is cleaned up and replaced); the error branch should do the same for the replacement-needed class instead of returning a transient error. This contradicts the PR's "fresh candidates reject unsafe nodes" claim: they are blocked, not rejected.

P2

2. Incomplete pod inventories are reported as reconcile faults during normal scheduling. inspect requires scheduled == replicas for both Deployments and otherwise returns errTrinoPodProtectionIncomplete. A freshly created instance (CREATING/PREPARING) has pods that are not yet scheduled, so protectPoolNodes records a failure, reconcileOnce returns/logs it and marks the instance RolloutBlocked on every tick until scheduling settles. Since the feature is opt-in this is not a default-path problem, but it turns a healthy rollout into a stream of "reconcile failed" warnings/metrics. Consider treating "pods still being scheduled" as a wait rather than an error. Relatedly, when a node-protection error does block a serving instance, the planner's fallback reason is "a serving instance has unreadable rollout evidence; repair its record", which is misleading for a node-lookup failure.

3. No operator-level positive test for the record/cordon path. Every test either pre-seeds NodeReplacementEvidence on the instance or installs a guard backed by an empty fake clientset, so nothing exercises protectPoolNodes discovering drift/deletion from inspect, persisting the first evidence under the fence, and then the planner acting on it. Add a test with a Drifted trinoNodeClaim that asserts the evidence is recorded first-write-wins and that the instance becomes a create/drain candidate.

4. The new node_replacement projection is only key-set tested. The admin unit test only covers the nil case (len(row) == 4), and the e2e harness only relaxes its key-set assertion. Since the populated object now exposes internal node and NodeClaim names/UIDs, add a unit assertion that a valid evidence row is projected correctly and still passes the existing sanitization checks.

No P0 issues found.

— Robo Bill v2 (deepseek-flash, high reasoning)

[bill-ph]

Review

I read the PR description, the combined diff (f1aae3b..02b8704, including the follow-up commit that answers the earlier round), and the surrounding code in repo/: the guard (trino_pool_node_guard.go), the disruption orchestration (trino_pool_node_disruption.go), the reconcile/plan/progress paths (trino_pool_operator.go, trinopool/plan.go, trino_pool_progress.go), the fenced evidence writer and migration 000043, the admin projection, and the runbook.

The earlier round's four points are addressed: definitiveTrinoNodePlacement now fails a stuck candidate instead of wedging PlanNext behind preparing > 0; errTrinoPodProtectionIncomplete is treated as convergence for CREATING/PREPARING; TestTrinoNodeObservedReplacementPersistsAndUsesNormalRollout covers observe → persist → cordon → surge → drain end to end; and TestTrinoRecoveryInstancesExposeTypedNodeReplacementEvidence asserts the populated projection and re-runs the sanitization checks. I re-verified the parts most likely to break: nodeSafeAdmission's GetMember read-back is safe because coordinator_pod_uid, coordinator_boot_id and gateway_incarnation are all persisted at registration before PREPARING, so the identity comparison does not spuriously fail; the ACTIVE branch replays through the same Step id, so idempotency holds; the cordon patch's uid + resourceVersion preconditions and the if node.Spec.Unschedulable { return nil } short-circuit correctly avoid clobbering a foreign cordon's annotation; and RecordTrinoPoolNodeReplacement is genuinely first-write-wins under the pool fence.

The issues below are all in the guard's error handling, not in the durable or Gateway-facing state machine.

P1

1. A node-protection error suppresses the rollout of an instance whose replacement was already accepted and cordoned. applyPlan sets view.RolloutBlocked = true for any instance present in o.nodeProtectionErrors, with no exception for an instance that already carries valid, durable node_replacement_evidence (controlplane/trino_pool_operator.go:559). outdatedServing skips RolloutBlocked instances, so while the error persists the planner will neither surge the replacement (rule 5 needs len(outdated) > 0) nor drain the original (rule 4) — it falls through to "a serving instance has unreadable rollout evidence; repair its record" and does nothing. Any transient Nodes().Get failure, NodeClaim list error, or momentarily unscheduled worker pod on the drifted instance therefore stalls a replacement the controller has already committed to and cordoned for, which is in tension with the runbook's "Recorded replacement is deliberate: it completes even if drift later clears." The intended tolerance already exists one layer up — protectPoolNodes clears errTrinoUnschedulableNode once instance.NodeReplacementEvidence != nil (trino_pool_node_disruption.go:59) — it just is not extended to the RolloutBlocked decision. Suggest gating line 559 on the evidence being absent or unparseable, so only unreadable evidence blocks rollout.

P2

2. Drift evidence observed in the same pass is discarded when the pod inventory is incomplete. inspect deliberately returns (evidence, errTrinoPodProtectionIncomplete) (trino_pool_node_guard.go:242), but protectPoolNodes only records when err == nil (trino_pool_node_disruption.go:62), so a serving instance that is one unscheduled worker short never persists drift it has already proven. Combined with (1) that makes the partially-degraded instance — the one most likely to be sitting on a node that is going away — the one that never gets a replacement request. Recording evidence is a pure observation and is safe to do even when the inventory is incomplete; only the cordon needs the stricter gate.

3. errTrinoUnschedulableNode returns from the middle of the node loop, so an unrelated cordon can mask drift. inspect iterates nodes in sorted name order and bails out on the first unschedulable-and-undrifted node (trino_pool_node_guard.go:236). If an instance's pods span an admin-cordoned node a-... and a Drifted node z-..., the drift on z-... is never reached and no evidence is ever recorded — whether the instance gets replaced depends on node-name sort order. Collecting evidence across all nodes first and only then reporting the unschedulable condition would make this order-independent.

4. A node that is not owned by a karpenter.sh/v1 NodeClaim turns every candidate into a create → fail → recreate cycle. errTrinoNodePlacementInvalid is now "definitive", so validateCandidate calls failCandidate (controlplane/trino_pool_progress.go:296). PhaseFailedPreparing is terminal, so it stops occupying capacity and PlanNext immediately creates another instance, which lands on the same kind of node and fails again — an unbounded churn of Deployments/ConfigMaps/Services and instance rows rather than a stable wait. This is the right trade against the previous wedge, but it makes "every node eligible for pool pods is owned by a Karpenter v1 NodeClaim" a hard prerequisite that the runbook's "Enable safely" section does not list alongside the RBAC and annotation steps. Worth calling out explicitly, and/or bounding the retries.

5. Cluster-scoped NodeClaim listing cost per tick. trinoPoolReconcileInterval is 5s. Each protectPoolNodes pass does one paginated NodeClaim list (up to 20 requests at the 500/page, 10k cap), and candidateNodeCheck / nodeSafeAdmission each call g.reset() and re-list, so a tick with a PREPARING or VALIDATING candidate does two or three full inventories. On a large cluster that is a sustained cluster-scoped list load added per pool operator, and the runbook's "One paginated NodeClaim inventory is shared within the pass" is accurate only for the protection pass. Caching the inventory for the whole tick rather than per reset() would remove most of it.

No P0 issues found. Agreed with keeping this a draft until the live pod-protection harness and an isolated physical replacement test have actually run — the unit coverage here is good but, as the description says, it does not establish the zero-loss property.

Inline comments

• controlplane/trino_pool_operator.go:559 — P1: this blocks rollout even for an instance that already has valid, durable node_replacement_evidence and a cordoned node. outdatedServing skips RolloutBlocked instances, so a transient Nodes().Get/NodeClaim-list failure or a momentarily unscheduled worker stalls both the surge create and the drain of a replacement that was already accepted — the planner falls through to "a serving instance has unreadable rollout evidence". protectPoolNodes already encodes the opposite intent by clearing errTrinoUnschedulableNode once evidence exists. Suggest only setting RolloutBlocked here when the evidence is absent or unparseable.
• controlplane/trino_pool_node_disruption.go:62 — P2: inspect intentionally returns (evidence, errTrinoPodProtectionIncomplete) (guard.go:242), but this only records when err == nil, so drift already proven for a serving instance is thrown away whenever one worker pod is unscheduled. Recording is a pure observation and is safe here; only the cordon below needs the stricter gate.
• controlplane/trino_pool_node_guard.go:236 — P2: this returns from the middle of the sorted node loop, so an unrelated unschedulable node whose name sorts first hides a Drifted node later in ordered and no evidence is ever recorded. Whether the instance gets replaced then depends on node-name ordering. Collecting evidence over all nodes and reporting the unschedulable condition only afterwards makes this order-independent.
• controlplane/trino_pool_progress.go:296 — P2: errTrinoNodePlacementInvalid is definitive, and PhaseFailedPreparing is terminal, so it frees its capacity slot and PlanNext creates another instance right away. On a node that is not owned by a karpenter.sh/v1 NodeClaim that becomes an unbounded create → fail → recreate cycle instead of a wait. Better than the previous wedge, but it makes "all nodes eligible for pool pods are Karpenter v1 NodeClaim-owned" a hard prerequisite that the runbook's "Enable safely" list does not mention — worth documenting and/or bounding the retries.

— Robo Bill v2 (opus, high reasoning)