OLS-3566 Add TTL lifecycle CRD fields and reconciler enforcement - #412
OLS-3566 Add TTL lifecycle CRD fields and reconciler enforcement#412sriroopar wants to merge 1 commit into
Conversation
Add automatic garbage collection of terminal AgenticRun resources following the Kubernetes Jobs ttlSecondsAfterFinished pattern. CRD changes: - AgenticOLSConfig.spec.lifecycle.terminalTTL (cluster-wide default) - AgenticRun.spec.ttlAfterTerminal (per-run override, mutable) - AgenticRun.status.terminalTime (stamped once at terminal state) Reconciler changes: - Stamp terminalTime and ttlAfterTerminal on terminal state - Delete expired runs; RequeueAfter for non-expired - ttlAfterTerminal=0 disables auto-deletion - Pre-set ttlAfterTerminal preserved (not overwritten) - No AgenticOLSConfig CR = no auto-deletion (backwards-compatible) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesAgenticRun now supports per-run and cluster-wide terminal TTL configuration. The controller records terminal timestamps, persists configured TTLs, requeues active terminal runs, and deletes expired runs. Tests cover all terminal phases and configuration states. AgenticRun terminal cleanup
Sequence Diagram(s)sequenceDiagram
participant AgenticRunReconciler
participant AgenticOLSConfig
participant KubernetesAPI
AgenticRunReconciler->>AgenticOLSConfig: Read lifecycle terminal TTL
AgenticRunReconciler->>KubernetesAPI: Persist TerminalTime and TTLAfterTerminal
AgenticRunReconciler->>AgenticRunReconciler: Calculate expiration
AgenticRunReconciler->>KubernetesAPI: Requeue until expiration
AgenticRunReconciler->>KubernetesAPI: Delete expired AgenticRun
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controller/agenticrun/reconciler.go`:
- Around line 278-281: Update the enqueue condition in the reconciler around
DerivePhase and isTerminal so terminal runs are enqueued whenever either
TerminalTime or TTLAfterTerminal is missing. Preserve enqueueing for all
non-terminal runs and the existing handling for terminal runs with incomplete
TTL metadata.
In `@controller/agenticrun/ttl_test.go`:
- Line 98: Check every getAgenticRun call in controller/agenticrun/ttl_test.go
at lines 98, 141, 280, 314, and 350: capture its returned error and call
t.Fatalf before dereferencing got, preserving the existing status/spec
assertions after successful retrieval.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 364a3193-6b60-4ee6-8602-6f3929d5792c
⛔ Files ignored due to path filters (2)
config/crd/bases/agentic.openshift.io_agenticolsconfigs.yamlis excluded by!config/crd/bases/**config/crd/bases/agentic.openshift.io_agenticruns.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (5)
api/v1alpha1/agenticolsconfig_types.goapi/v1alpha1/agenticrun_types.gocontroller/agenticrun/helpers.gocontroller/agenticrun/reconciler.gocontroller/agenticrun/ttl_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
| phase := agenticv1alpha1.DerivePhase(p.Status.Conditions) | ||
| // Enqueue non-terminal runs (normal workflow) and terminal runs | ||
| // that may need TTL stamping (terminalTime set but no ttlAfterTerminal yet). | ||
| if !isTerminal(phase) || (p.Status.TerminalTime != nil && p.Spec.TTLAfterTerminal == nil) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enqueue terminal runs with incomplete TTL metadata.
Line 281 skips a terminal run when TerminalTime is nil, even if TTLAfterTerminal is also nil. Terminal runs created before this change have both fields unset. A later AgenticOLSConfig update will not reconcile them, so the default TTL is never applied.
Enqueue terminal runs when either TTL metadata field is missing.
Proposed fix
- if !isTerminal(phase) || (p.Status.TerminalTime != nil && p.Spec.TTLAfterTerminal == nil) {
+ if !isTerminal(phase) || p.Status.TerminalTime == nil || p.Spec.TTLAfterTerminal == nil {
reqs = append(reqs, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(&p)})
}Based on PR objectives, configuration changes must enqueue terminal runs that require TTL metadata.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| phase := agenticv1alpha1.DerivePhase(p.Status.Conditions) | |
| // Enqueue non-terminal runs (normal workflow) and terminal runs | |
| // that may need TTL stamping (terminalTime set but no ttlAfterTerminal yet). | |
| if !isTerminal(phase) || (p.Status.TerminalTime != nil && p.Spec.TTLAfterTerminal == nil) { | |
| phase := agenticv1alpha1.DerivePhase(p.Status.Conditions) | |
| // Enqueue non-terminal runs (normal workflow) and terminal runs | |
| // that may need TTL stamping (terminalTime set but no ttlAfterTerminal yet). | |
| if !isTerminal(phase) || p.Status.TerminalTime == nil || p.Spec.TTLAfterTerminal == nil { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/agenticrun/reconciler.go` around lines 278 - 281, Update the
enqueue condition in the reconciler around DerivePhase and isTerminal so
terminal runs are enqueued whenever either TerminalTime or TTLAfterTerminal is
missing. Preserve enqueueing for all non-terminal runs and the existing handling
for terminal runs with incomplete TTL metadata.
| t.Fatalf("reconcile: %v", err) | ||
| } | ||
|
|
||
| got, _ := getAgenticRun(r, "fix-crash") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check every getAgenticRun error before dereferencing its result. Ignoring these errors can hide the failed client operation or cause a nil dereference.
controller/agenticrun/ttl_test.go#L98-L98: capture the error and callt.Fatalfbefore readinggot.Status.controller/agenticrun/ttl_test.go#L141-L141: capture the error and callt.Fatalfbefore readinggot.Spec.controller/agenticrun/ttl_test.go#L280-L280: capture the error and callt.Fatalfbefore readinggot.Status.controller/agenticrun/ttl_test.go#L314-L314: capture the error and callt.Fatalfbefore readinggot.Status.controller/agenticrun/ttl_test.go#L350-L350: capture the error and callt.Fatalfbefore readinggot.Status.
As per path instructions, “Never ignore error returns”.
📍 Affects 1 file
controller/agenticrun/ttl_test.go#L98-L98(this comment)controller/agenticrun/ttl_test.go#L141-L141controller/agenticrun/ttl_test.go#L280-L280controller/agenticrun/ttl_test.go#L314-L314controller/agenticrun/ttl_test.go#L350-L350
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/agenticrun/ttl_test.go` at line 98, Check every getAgenticRun call
in controller/agenticrun/ttl_test.go at lines 98, 141, 280, 314, and 350:
capture its returned error and call t.Fatalf before dereferencing got,
preserving the existing status/spec assertions after successful retrieval.
Source: Path instructions
|
@sriroopar: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Summary
AgenticRunresources following the Kubernetes JobsttlSecondsAfterFinishedpatternAgenticOLSConfig.spec.lifecycle.terminalTTLas a cluster-wide default TTL andAgenticRun.spec.ttlAfterTerminalas a per-run mutable overridestatus.terminalTimeandspec.ttlAfterTerminalwhen a run reaches terminal state, then deletes expired runs or requeues withRequeueAfterfor the remaining durationCRD Changes
AgenticOLSConfig.spec.lifecycle.terminalTTL*int32(optional)AgenticRun.spec.ttlAfterTerminal*int32(optional, mutable)0disables auto-deletionAgenticRun.status.terminalTime*metav1.Time(optional)Reconciler Changes
handleTerminalTTL()method integrated into all terminal phase branches (Completed, Failed, Denied, Escalated, EmergencyStopped, NoActionRequired)terminalTimeandttlAfterTerminalon first terminal reconcilettlAfterTerminal(by adapter/admin) is never overwrittenttlAfterTerminal=0explicitly disables auto-deletion for that runAgenticOLSConfigCR → no auto-deletion (backwards-compatible)fanOutToActiveRunsupdated to enqueue terminal runs needing TTL stamping on config changesFiles Changed
api/v1alpha1/agenticolsconfig_types.goLifecycleConfigstruct andLifecyclefieldapi/v1alpha1/agenticrun_types.goTTLAfterTerminalto spec,TerminalTimeto statuscontroller/agenticrun/reconciler.gohandleTerminalTTL(), integrated into all terminal branchescontroller/agenticrun/helpers.gogetTerminalTTL()helpercontroller/agenticrun/ttl_test.goconfig/crd/bases/*.yamlmake manifestsTest plan
make manifestsregenerates CRD YAMLs without errormake testpasses (all existing + 9 new TTL tests)make api-lintpassesterminalTimeandttlAfterTerminalstampedttlAfterTerminal=0prevents auto-deletionttlAfterTerminalis not overwritten by cluster defaultAgenticOLSConfigCR means no auto-deletionSpec Reference:
lightspeed-agentic-operator/.ai/spec/what/agentic-lifecycle.md— rules 1-10🤖 Generated with Claude Code