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 2 commits into
Conversation
|
[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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesAgenticRun now supports cluster-wide and per-run terminal TTL settings. The controller records terminal timestamps, persists TTL values, requeues unexpired runs, and deletes expired runs. Tests and lifecycle specifications cover terminal phases, configuration states, and revision resets. 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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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)
6550c99 to
ebb339e
Compare
Adversarial reviewThis is the origin of the TTL/lifecycle feature that #413 cherry-picks on top of. I checked out the branch, built it ( 🔴 Critical1. Likely nil-pointer panic in if run.Status.TerminalTime == nil {
base := run.DeepCopy()
run.Status.TerminalTime = &now
if err := r.statusPatch(ctx, run, base); err != nil { ... }
}
if run.Spec.TTLAfterTerminal == nil {
clusterTTL, err := getTerminalTTL(ctx, r.Client)
...
if clusterTTL != nil {
// Re-fetch to avoid conflicts after the status patch above.
if err := r.Get(ctx, client.ObjectKeyFromObject(run), run); err != nil { ... }
original := run.DeepCopy()
run.Spec.TTLAfterTerminal = clusterTTL
...
}
}
...
terminalTime := run.Status.TerminalTime.Time // <-- can panic
This is timing-dependent so it won't always fire, but it's a real crash risk on every terminal The re- 2. TTL stamping bumps
original := run.DeepCopy()
run.Spec.TTLAfterTerminal = clusterTTL
if err := r.Patch(ctx, run, client.MergeFrom(original)); err != nil { ... }Any spec write bumps 🟠 Should fix3. Spec docs not updated, and the PR description cites a spec file that doesn't exist The PR body says "Spec Reference: 🟡 Minor / nits
What looks solid
Given 1–2 are real correctness bugs invisible to the existing test suite, I'd hold this for a fix + regression test before merge — and note that #413 currently duplicates this entire diff via an unsquashed cherry-pick commit, so fixing it here (the origin) and rebasing #413 on top once merged would avoid the two copies drifting apart. |
7de7501 to
a1d4cdc
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
controller/agenticrun/ttl_test.go (1)
73-116: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test that preserves the first terminal timestamp.
Run reconciliation twice for the same terminal run. Store
TerminalTimeafter the first reconciliation. Assert that the second reconciliation leaves that value unchanged.Without this regression test, a future timestamp refresh can postpone expiry on every reconciliation and prevent TTL deletion.
🤖 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` around lines 73 - 116, The test TestHandleTerminalTTL_StampsTerminalTimeAndTTL should reconcile the same terminal run twice, save TerminalTime after the first reconciliation, then assert the second reconciliation preserves the identical timestamp. Keep the existing TTL assertions and verify the timestamp is not refreshed between reconciliations.
🤖 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 @.ai/spec/what/crd-api.md:
- Line 57: Clarify the TTL policy in the specification, especially rules 48 and
23, by stating whether an absent AgenticOLSConfig disables only default TTL
stamping or also deletion of already pre-set spec.ttlAfterTerminal values. Align
handleTerminalTTL and the related reconciliation tests with that explicit
policy, preserving pre-set TTL behavior if the intended contract is that
configuration controls defaults only.
In @.ai/spec/what/run-lifecycle.md:
- Line 65: The watched-configuration/policy/ConfigMap fan-out rule must only
re-enqueue terminal runs for TTL stamping when an effective cluster terminal TTL
is configured; do not enqueue runs solely because spec.ttlAfterTerminal is unset
when AgenticOLSConfig is absent or terminalTTL is unset. Keep any terminalTime
stamping behavior as a separate, explicit path.
- Line 57: Update the revision-detection logic described by rules 20 and 24 so a
change containing only spec.ttlAfterTerminal does not satisfy needsRevision,
including when spec.revisionFeedback is non-empty; distinguish revisionFeedback
changes from TTL-only changes while preserving operator TTL-write handling. Add
a regression test covering a user or adapter TTL-only update with non-empty
revisionFeedback and verify that re-analysis is not started.
- Around line 61-64: The run lifecycle must start a new terminal TTL epoch after
revision feedback. Update the revision-start transition for NoActionRequired
runs to clear or replace status.terminalTime before the next terminal state,
ensuring TTL deletion and RequeueAfter calculations use the new terminal
timestamp; add a regression test covering revision followed by
re-terminalization.
---
Nitpick comments:
In `@controller/agenticrun/ttl_test.go`:
- Around line 73-116: The test TestHandleTerminalTTL_StampsTerminalTimeAndTTL
should reconcile the same terminal run twice, save TerminalTime after the first
reconciliation, then assert the second reconciliation preserves the identical
timestamp. Keep the existing TTL assertions and verify the timestamp is not
refreshed between reconciliations.
🪄 Autofix
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: e51380d3-0fb5-469a-86ba-89a9b8452b30
⛔ Files ignored due to path filters (1)
config/crd/bases/agentic.openshift.io_agenticruns.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (5)
.ai/spec/what/crd-api.md.ai/spec/what/run-lifecycle.mdapi/v1alpha1/agenticrun_types.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)
🚧 Files skipped from review as they are similar to previous changes (2)
- controller/agenticrun/reconciler.go
- api/v1alpha1/agenticrun_types.go
a1d4cdc to
5844651
Compare
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 @.ai/spec/what/crd-api.md:
- Line 57: Update the AgenticOLSConfig.spec.lifecycle.terminalTTL description to
state that pre-set spec.ttlAfterTerminal: 0 disables deletion, while only
non-zero values remain eligible. Replace the claim that runs without a
configured default are never auto-deleted with the rule 23 behavior: a later
effective terminalTTL is applied retroactively to terminal AgenticRun resources
lacking spec.ttlAfterTerminal.
In `@controller/agenticrun/handlers_test.go`:
- Line 820: Update the test calls to getAgenticRun in the affected test cases to
capture and assert the returned errors instead of discarding them. Ensure each
failed read causes the test to fail while preserving the existing agentic-run
assertions.
🪄 Autofix
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: 14c7a961-9e89-470f-ac71-69a3685a401a
⛔ Files ignored due to path filters (1)
config/crd/bases/agentic.openshift.io_agenticruns.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (7)
.ai/spec/what/crd-api.md.ai/spec/what/run-lifecycle.mdapi/v1alpha1/agenticrun_types.gocontroller/agenticrun/handlers.gocontroller/agenticrun/handlers_test.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)
🚧 Files skipped from review as they are similar to previous changes (4)
- controller/agenticrun/ttl_test.go
- .ai/spec/what/run-lifecycle.md
- controller/agenticrun/reconciler.go
- api/v1alpha1/agenticrun_types.go
6415171 to
736f092
Compare
blublinsky
left a comment
blublinsky
left a comment
There was a problem hiding this comment.
Three error paths in handleTerminalTTL return raw errors without wrapping via fmt.Errorf("%s: %w", ErrFoo, err), which is inconsistent with the project convention and with the rest of this file.
The statusPatch errors for stamping terminalTime, the r.Patch error for stamping ttlAfterTerminal, and the statusPatch error for advancing observedGeneration are all logged but returned unwrapped — while other error paths in the same method (getTerminalTTL → ErrStampTerminalTTL, r.Delete → ErrDeleteExpiredRun) correctly use the fmt.Errorf("%s: %w", …) pattern.
These are the only unwrapped Patch/statusPatch error returns in the entire file. The existing ErrStampTerminalTTL constant is the natural fit for all three, e.g.:
return ctrl.Result{}, false, fmt.Errorf("%s: %w", ErrStampTerminalTTL, err)
736f092 to
aa77989
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.ai/spec/what/crd-api.md:
- Around line 12-14: The revision-detection logic in needsRevision must
distinguish user revisions from operator-only spec.ttlAfterTerminal updates, so
TTL-only generation changes do not re-enter analysis for terminal
NoActionRequired, advisory-only Completed, or execution-less Failed runs. Update
the corresponding lifecycle specification to document this exception, while
preserving re-analysis for genuine revisionFeedback changes, and add regression
tests covering each affected terminal scenario in addition to the existing
TTL-stamping test.
🪄 Autofix
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: 35fb90bb-2edc-4a6a-b40d-e9581b5ed16b
📒 Files selected for processing (4)
.ai/spec/what/crd-api.mdcontroller/agenticrun/handlers_test.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)
🚧 Files skipped from review as they are similar to previous changes (3)
- controller/agenticrun/reconciler.go
- controller/agenticrun/ttl_test.go
- controller/agenticrun/handlers_test.go
a360871 to
2b49daa
Compare
blublinsky
left a comment
blublinsky
left a comment
There was a problem hiding this comment.
Review: OLS-3566 TTL lifecycle
Summary
Clean implementation of terminal-run garbage collection following the Kubernetes Jobs ttlSecondsAfterFinished pattern. Spec documentation, test coverage, and backwards compatibility are all strong.
Issue: crash-recovery desync in handleTerminalTTL (must-fix)
Location: controller/agenticrun/reconciler.go, handleTerminalTTL
The observedGeneration sync (step 3 of the three-patch sequence) is nested inside if run.Spec.TTLAfterTerminal == nil. If the reconciler crashes between the spec patch (which stamps ttlAfterTerminal and bumps metadata.generation) and the status patch (which syncs Analyzed.observedGeneration), the generation sync is permanently skipped on subsequent reconciles — because TTLAfterTerminal is no longer nil, the entire block is bypassed.
For runs that carry stale revisionFeedback (non-empty, never cleared by design), needsRevision() returns true due to the stale observedGeneration. Several terminal phase blocks (NoActionRequired, advisory-only Completed, execution-less Failed) use needsRevision() as a gate before reaching handleTerminalTTL, so the run exits the terminal branch and spuriously re-enters analysis.
Suggested fix: Move the observedGeneration sync outside the if run.Spec.TTLAfterTerminal == nil block — make it an unconditional idempotent check:
// Unconditional: heal stale observedGeneration regardless of how
// ttlAfterTerminal was set (crash recovery, pre-set by adapter, etc.)
if analyzed := meta.FindStatusCondition(run.Status.Conditions,
agenticv1alpha1.AgenticRunConditionAnalyzed); analyzed != nil &&
analyzed.ObservedGeneration < run.Generation {
base := run.DeepCopy()
analyzed.ObservedGeneration = run.Generation
if err := r.statusPatch(ctx, run, base); err != nil {
return ctrl.Result{}, false, fmt.Errorf("%s: %w", ErrStampTerminalTTL, err)
}
}This fires only during recovery (when generation is already synced, no API call is made).
What's good
- Thorough test coverage — 9 tests covering stamping, idempotency, preset preservation, zero-disables, expiry, requeue, no-config, and the critical observedGeneration sync. The three regression tests (NoActionRequired, advisory Completed, execution-less Failed) show deep understanding of the revision-loop interaction.
- Spec rules 23/24 and 6a/6b/48 are precise and internally consistent.
handleRevisioncorrectly clearsTerminalTimeto prevent stale timestamps across revision cycles.- Fan-out logic is well-reasoned: only re-enqueues terminal runs for TTL stamping when a cluster default actually exists, avoiding churn.
- Full backwards compatibility: no AgenticOLSConfig CR = no auto-deletion.
2b49daa to
91d953b
Compare
blublinsky
left a comment
blublinsky
left a comment
There was a problem hiding this comment.
Should-fix: doc references non-existent sub-rule "23a"
crd-api.md rule 6b says "rule 6/23a" but run-lifecycle.md has no labeled sub-rule 23a — only rule 23 with unlabeled markdown sub-bullets. Should be "rule 6/23" (the first sub-bullet of rule 23 covers clearing terminalTime on revision). Note: #413 already has this fixed to "rule 6/23".
Suggest doing a broader sweep of all spec/doc files to ensure consistency with the actual implementation — especially around rule numbering, cross-references, and behavioral claims.
…gression tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
91d953b to
fe967da
Compare
|
@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. |
|
/retest |
|
Should-fix: Duplicate terminal-phase boilerplate in reconciler switch The sandbox cleanup → audit cleanup → func (r *AgenticRunReconciler) handleTerminalCleanup(ctx context.Context, run *agenticv1alpha1.AgenticRun, phase string) (ctrl.Result, error) {
if hasSandboxClaims(run) {
if err := r.Agent.ReleaseSandboxes(ctx, run); err != nil {
logf.FromContext(ctx).Error(err, "sandbox cleanup failed at terminal phase")
}
}
if r.Audit != nil {
r.Audit.EmitTerminalSpan(ctx, run, phase, terminalReason(run))
r.Audit.Cleanup(run)
}
if result, requeue, err := r.handleTerminalTTL(ctx, run); requeue || err != nil {
return result, err
}
return ctrl.Result{}, nil
}Not blocking — the code is correct as-is, but this would reduce maintenance risk. ( |
|
Nice-to-have: Collapse three structurally identical regression tests into a table-driven test
Nice-to-have:
|
2 participants
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