fix(agentruntime): degrade gracefully when target workload is missing - #529
fix(agentruntime): degrade gracefully when target workload is missing#529mrsabath wants to merge 1 commit into
Conversation
… (#2490) When an AgentRuntime's spec.targetRef workload (Sandbox for agents, Deployment for tools) is deleted but the AgentRuntime CR remains, the operator previously logged an Error and emitted a Warning event every ~30s while leaving a stale Ready: True. resolveTargetRef now wraps the IsNotFound case with a sentinel; Reconcile branches on it and treats a missing target as a recoverable degraded state: sets Ready=False and TargetResolved=False (reason TargetNotFound), clears the now-stale status.Card, logs at V(1) instead of Error, emits the Warning event only on transition into the degraded state, and requeues at 60s (recovery is watch-driven, not bound by the interval). Genuine (non-IsNotFound) API errors keep the loud Error path with a distinct reason, TargetResolveError. Target-resolution reasons are extracted to constants. Adds envtest coverage: degraded sets Ready=False; the Warning event is deduped to once across cycles; status.Card is cleared on degrade; recovery to Ready=True when the target reappears. The context's AfterEach now drains the kagenti.io/cleanup finalizer so specs do not leak state. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
e4dbd4d to
505e927
Compare
cwiklik
left a comment
There was a problem hiding this comment.
Clean, well-reasoned fix for the rossoctl#2490 reconcile error-loop: a missing spec.targetRef workload is now a recoverable degraded state (Ready=False/TargetResolved=False, reason TargetNotFound, stale status.Card cleared, V(1) log) instead of per-30s Error+Warning spam, with the Warning emitted only on transition (deduped via the pre-existing condition) and recovery driven by the existing workload watch.
The logic is idiomatic and thoughtful:
errTargetNotFoundsentinel +%w+errors.Is— correct.- The transition-dedup reads the pre-reconcile condition, so the Warning fires only on entry into degraded — and the
at most once across cyclestest proves it via a FakeRecorder. - Using a distinct
TargetResolveErrorreason for genuine API errors is a nice detail: it stops a transient API error from settingalreadyDegradedand suppressing a later legitimate degrade event. status.Cardcleared on degrade (tested); recovery assertsRequeueAfter==0+Ready=True/Configured.
Tests are assertive and cover every claim (degrade conditions, at-most-once event, Card clearing, recovery), with AfterEach draining the finalizer to avoid state leakage between specs.
One non-blocking observation (not introduced here): the degraded path's unconditional every-60s Status().Update mirrors the pre-existing updateErrorStatus pattern, and For() has no generation predicate — a possible future optimization (guard the update on an actual change), but pre-existing and out of scope for this fix.
Areas reviewed: Go (reconciler control flow, status/condition handling, error wrapping), envtest coverage
Commits: 1, signed-off: yes (Assisted-By trailer correct)
CI: passing (Unit, Integration, E2E all green)
LGTM.
Summary
Fixes the operator reconcile error-loop described in rossoctl/rossoctl#2490.
When an
AgentRuntime'sspec.targetRefworkload (aSandboxfor agents, aDeploymentfor tools) is deleted while the parentAgentRuntimeCR remains, the operator previously:Ready: Truefrom the last successful reconcile.This treats a genuinely-missing target as a recoverable degraded state instead.
What changed
resolveTargetRefwraps theIsNotFoundcase with a sentinel error (errTargetNotFound, via%w).Reconcilebranches onerrors.Is: for a missing target it nowReady=FalseandTargetResolved=False(reasonTargetNotFound) — fixing the staleReady: True,status.Card(it was discovered from the absent workload),TargetNotFoundWarning event only on transition into the degraded state (deduped via the pre-existing condition), andIsNotFound) API errors keep the original loud Error path, now with a distinct reasonTargetResolveError(so a transient API error can't suppress a later legitimate degraded-transition event).ReasonTargetFound/ReasonTargetNotFound/ReasonTargetResolveError).Recovery is automatic and prompt: the existing
SetupWithManagerworkload watches enqueue a reconcile the moment the target reappears (it is not bound by the 60s requeue). No owner references or finalizers are added to the child workload — the operator does not own it (linkage is viaspec.targetRef).Tests
New envtest coverage in
agentruntime_controller_test.go(context "When the target Deployment does not exist"):Ready=False/TargetResolved=Falsewith reasonTargetNotFound;TargetNotFoundWarning event is emitted at most once across consecutive degraded reconciles;status.Cardis cleared on degrade;Ready=True/Configured(andTargetResolved=True/TargetFound) when the target is recreated.The context's
AfterEachnow drains thekagenti.io/cleanupfinalizer (drives reconciles until the object is gone) so specs don't leak state into each other.Full controller suite passes (
go test ./internal/controller/). Verified on a local Kind cluster: on target deletion the operator emits exactly oneTargetNotFoundWarning (with the exact wrapped message) instead of the previous per-30s spam, and recovers cleanly when the workload returns.Notes
AgentRuntimeCR are handled separately (Docs: Fix advanced weather-agent demo (partial, in-progress run) cortex#819).Assisted-By: Claude (Anthropic AI) noreply@anthropic.com