feat: fail loudly, recover automatically#143
Open
bmorton wants to merge 28 commits into
Open
Conversation
Audits the operator's silent failure modes (stalled upgrades, wedged schema Jobs, abandoned cleanup, dropped status conflicts) and designs bounded recovery plus condition-derived observability. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
Twenty TDD tasks across four phases: shared foundations, recovery semantics, condition-derived observability, and verification. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
Add Policy type for managing retry delays and Decision type for authorization decisions. Implement DeadlineExceeded and Remaining helper functions for bounded deadline checking. Include SchemaJobPolicy with 1m/5m/15m delays for database schema Job recreation. All functions are pure with time-injection to enable unit testing without clock dependencies. Coverage: 92.9% Signed-off-by: Brian Morton <brian@mmmhm.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb
Finding 1: Add 'since' column to TestDeadlineExceeded table, replacing
the name-equality override. Use plausible 'now' (start+4min) for the
zero-since row to properly guard the invariant that zero 'since' never
expires regardless of current time. This catches regressions where the
'if since.IsZero() { return false }' guard is removed.
Finding 2: Add 'wantAttempt' column to TestPolicyNext table and assert
on Decision.Attempt in both TestPolicyNext and
TestPolicyNextNegativeAttemptsTreatedAsZero.
Finding 3: Add test case for Remaining with zero 'since', covering the
'if since.IsZero() { return deadline }' branch.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb
Signed-off-by: Brian Morton <brian@mmmhm.com>
Wraps k8s.io/client-go/tools/events.EventRecorder with per-object, per-eventtype, per-reason deduplication so states re-evaluated on every reconcile (stalled upgrade, unreachable target) emit one Event per state change rather than one per requeue. - New(nil) returns nil; nil *Recorder drops all events safely - Forget(obj) clears an object's dedupe state for re-entry reporting - sync.Mutex protects the shared dedupe map for concurrent reconciles - Reuses reason as the action verb per existing convention Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
- Extend TestNilRecorderIsSafe to test Warning() and Forget() in addition to Normal() - Add TestForgetIsolationPreventsPrefixCollisions to verify Forget() does not delete entries for different objects when one UID is a substring of another - Add comment in emit() explaining fallback behavior for objects without usable key Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
Replaces seven near-identical setReady/statusUpdate pairs with the shared helper. Fixes the swallowed 409 Conflict in the TemporalClusterClient and TemporalDevServer reconcilers, and fixes the missing observedGeneration stamp on dev.Status that TemporalDevServer's setReady previously omitted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
Remove unused 'want' parameter from assertStamped by hoisting the generation constant to file scope as testGeneration, allowing the helper to reference it directly. This fixes the unparam lint failure. Also correct the comment attribution: TemporalDevServer (not TemporalClusterClient) was the controller that did not stamp Status.ObservedGeneration before the migration to internal/status. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
A rolling phase that exceeds upgradePhaseTimeout now sets UpgradeBlocked and Degraded with the Deployment's own failure reason, emits a warning event, and requeues. The condition clears automatically when the rollout completes. Also moves Rollbackable=false to the schema-migrating phase, matching the field's documented contract. The existing upgrade test's Rollbackable assertion (BeFalse during SchemaMigrating) encoded the bug: with the move, Rollbackable is still true when the phase is first observed at SchemaMigrating (the operator has not yet processed a schema step). Updated the assertion to BeTrue and revised the By description to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
Rejects retargeting a running upgrade to a third version, allows a revert to status.upgrade.fromVersion as the documented escape hatch, and warns when that revert happens after the schema has already migrated. Also fixes validateUpgradePath, which validated from spec.version (already the target mid-upgrade) instead of the true baseline in fromVersion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
A schema Job that exhausts its BackoffLimit is now deleted and recreated after 1m, 5m, then 15m before the operator gives up with a terminal SchemaReady=False and Degraded=True. The failed Job is retained on give-up so its pod logs stay inspectable. Recreation is safe: the schema tools run without --overwrite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
A target that is absent still forgets immediately, preserving the issue #58 guarantee that deletion always terminates. A target that exists but is unreachable now retries for up to cleanupDeadline before abandoning with a CleanupAbandoned warning event and a counter increment, rather than orphaning the Temporal object on the first transient error. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
grpc.NewClient is lazy, so a half-open connection hangs on the first RPC rather than at dial. Every client method now derives a bounded context, so an unreachable frontend can no longer stall a controller indefinitely. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
Tasks 7 and 9 added status fields (upgrade.phaseStartedAt, stalledService, message; persistence.schemaAttempts) but did not regenerate dist/chart. The Verify generated chart CI job would have failed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
- Create internal/metrics/domain.go with CleanupAbandoned, TargetUnreachable, UpgradePhaseSeconds, and SchemaJobAttempts metrics registered idempotently into controller-runtime's registry - Wire CleanupAbandoned and TargetUnreachable increments into all four cleanupUnreachable/reconcileDelete branches (TemporalNamespace, TemporalSchedule, TemporalSearchAttribute, TemporalClusterConnection) - Wire UpgradePhaseSeconds gauge in reconcileUpgrade with DeletePartialMatch on completion to clear stale series - Wire SchemaJobAttempts gauge in handleFailedSchemaJob and DeleteLabelValues in resetSchemaAttempts for stale-series hygiene - Fix missing Progressing=True condition set on TemporalClusterConnection cleanupRetry branch to match the other three controllers - Promote github.com/prometheus/client_golang to a direct dependency Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
Adds ConditionCollector, a prometheus.Collector that reads every condition on every Temporal CRD from the manager's cache on scrape. This avoids stale series for deleted objects and makes all conditions added in Phases 1-2 queryable with no per-condition plumbing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
…controller recorders - Call metrics.Register() and register ConditionCollector with controller-runtime's metrics registry after manager construction - Add Events *events.Recorder field to TemporalClusterReconciler, TemporalClusterClientReconciler, TemporalDevServerReconciler, and TemporalWorkflowRunReconciler (four reconcilers already had it) - Populate Events on all eight reconciler constructions in cmd/main.go with distinct per-controller recorder names Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
Add four PrometheusRule alerts covering the operator failure modes surfaced
in Tasks 13-15:
- TemporalClusterUpgradeStalled: fires after 5m when UpgradeBlocked=True,
indicating mixed server versions due to a stuck rolling upgrade.
- TemporalSchemaMigrationFailed: fires after 1m when SchemaReady=False with
reason SchemaMigrationFailed, meaning the retry budget is exhausted.
- TemporalResourceDegraded: fires after 15m when any resource has Degraded=True.
- TemporalCleanupAbandoned: fires after 1m when cleanup_abandoned_total rises,
meaning a Temporal-side object was orphaned.
All alerts have a 'for:' duration to prevent single-scrape flapping.
The chart generator (helmgen) does not pick up config/prometheus/ because it
is commented out in config/default/kustomization.yaml. A hand-maintained
override at hack/helm/overrides/templates/monitoring/alerts.yaml is used with
the chart's standard {{- if .Values.prometheus.enable }} gate and
{{- include "chart.labels" . | nindent 4 }} helper, matching the existing
servicemonitor.yaml convention.
The test in internal/metrics/alerts_test.go uses a relative path
../../config/prometheus/alerts.yaml. This couples the test to repo layout but
provides a meaningful drift guard (an alert referencing a nonexistent metric
or missing 'for:' fails the test). A future improvement could embed the file
via go:embed or accept the path via a test flag to reduce fragility.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb
Signed-off-by: Brian Morton <brian@mmmhm.com>
The temporal_operator_resource_condition gauge emits one series per condition with the condition status as a label. The gauge value is 1 only when the condition is True, so a False condition always carries value 0. The TemporalSchemaMigrationFailed alert matched status="False" but compared == 1, which can never be true. The alert was permanently silent for the critical path where a schema migration has failed and the cluster cannot start. Fix: change the comparison to == 0. Add an explanatory comment so the next reader understands why this alert compares to 0 while the others compare to 1. Add TestConditionAlertComparisons in alerts_test.go to enforce the invariant: exprs matching status="True" must compare == 1, and exprs matching status="False" must compare == 0. Regenerate dist/chart via make helm-chart. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
Covers the failure mode with no prior e2e coverage: an upgrade that cannot roll out must report UpgradeBlocked, reject retargeting, and recover when spec.version is reverted. Also adds --upgrade-phase-timeout flag (default 15m) to cmd/main.go so e2e can shorten it via --upgrade-phase-timeout=1m. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
…on check - Use a bash array for extra_args in e2e.yml to prevent glob expansion of [0]/[1] subscripts and word-splitting of multi-word strings - Capture stderr in reject-retarget-while-stalled and require the failure to contain an admission-rejection keyword; transient API errors no longer cause a false pass - Add status.version assertion to 01-assert.yaml to match the reference upgrade suite baseline Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Brian Morton <brian@mmmhm.com>
Add the -race flag to the go test invocation in the test target to enable the race detector for all unit and integration tests. The race detector has been verified to pass without detecting any data races in the full suite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
…anup Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
… connection deletion During deletion, local peers that cannot be resolved (non-NotFound error, e.g. TLS secret missing) or that resolve successfully but cannot be dialed were silently skipped: they never entered the locals map, removeErr stayed nil, and the finalizer was removed immediately. This orphaned remote-cluster registrations on surviving peers with no trace. Introduce localPeerErrors, which collects errors for local peers expected to participate in cleanup but unreachable. The result is joined with removeErr and passed through decideCleanup, giving those failures the same bounded-retry / CleanupAbandoned treatment as RemoveRemoteCluster failures already had. ErrTargetNotFound peers are excluded from the new path: resolvePeers never sets resolveErr for them and their empty address fails the ready+address gate, preserving the issue #58 immediate-forget guarantee. Update the connection controller test to verify the corrected behaviour: - within the cleanup deadline the finalizer is retained and a retry is scheduled (RequeueAfter > 0) - with cleanupDeadline collapsed to 0 the abandon path fires and the object is eventually garbage-collected Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
IsTrue has zero callers anywhere in the repository. Removing it shrinks the public API surface and eliminates dead code with no test coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
Tasks adding UpgradeStatus.phaseStartedAt/stalledService/message and PersistenceStatus.schemaAttempts ran `make generate manifests` and `make helm-chart` but not `make api-docs docs-crd-reference`, leaving the generated CRD reference stale and failing the Verify generated docs CI job. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fd4be6c-6d19-4724-a9ec-211a7a6daffb Signed-off-by: Brian Morton <brian@mmmhm.com>
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.
Implements
docs/superpowers/specs/2026-07-25-fail-loudly-recover-automatically-design.md.Principle: every abnormal state is (1) named by a condition, (2) either auto-recovered on a bounded schedule or terminated with evidence, and (3) observable without reading operator logs. No stall is silent; no recovery is unbounded.
The four silent failures this fixes
UpgradeBlocked+Degradedpast--upgrade-phase-timeout, naming the stalled service and the Deployment's own unready reason. Self-clears when the rollout completes — a report, not a latchSchemaReady=False+Degraded. The failed Job is retained so its pod logs stay readable. Safe to re-run: the schema tools run without--overwritecleanupDeadline, then abandons with aCleanupAbandonedwarning and a counterReadytransitionsAlso
temporal_operator_resource_condition, collected on scrape so deleted resources stop appearing and future conditions need no new plumbingPrometheusRulealerts, Events in all 8 controllers (was 2)DegradedandProgressing— declared but previously unused — are now setgrpc.NewClientis lazy, so an unreachable frontend previously hung a controller on first RPCfromVersionas the documented escape hatch (with a warning once the schema has migrated)upgrade-stalle2e suite;-raceenabled inmake testVerification
make build✓ ·make lint0 issues ✓ ·make testpasses with-race, no data races ✓ ·make generate manifests helm-chartzero drift ✓ · coverage 48.1% → 49.3%Issue #58's regression tests pass unmodified.
Notes for review
Three bugs were caught during implementation that would otherwise have shipped:
severity: criticalalert. The original expr matchedstatus="False"and compared== 1, but that gauge is1only when a condition is True — the alert for an unstartable cluster could never fire. Fixed, withTestConditionAlertComparisonsnow guarding the whole class.Known follow-up: a local peer that resolves but is
ready=falseis skipped during connection deletion. Pre-existing behavior, not introduced here, but a gap relative to the spec's wording.API changes are
status-only; nospecsurface added.