feat: add spec.ttl for automatic job cleanup after completion - #118
feat: add spec.ttl for automatic job cleanup after completion#118Harshith-umesh wants to merge 5 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 |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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 |
c2dc107 to
70f4078
Compare
Add a TTL field to FournosJob that defines the delay after job termination before the CR is automatically deleted. When not set, jobs are never auto-pruned (preserving current behavior). - Add spec.ttl to CRD schema (Go duration format: "12h", "7d", etc.) - Add duration parser utility (fournos/core/duration.py) - Add _gc_expired_jobs() to the operator GC loop - Add unit tests for TTL logic Co-authored-by: Cursor <cursoragent@cursor.com>
70f4078 to
f1170ca
Compare
kpouget
left a comment
kpouget
left a comment
There was a problem hiding this comment.
thanks @Harshith-umesh , looks good mostly,
just that I think it would be nicer to store the completion timestamp in the status,
in a similar way Pods (containers) do, instead of searching the transition timestamps
|
can you also validate that this works even if added later with an |
Co-authored-by: Kevin Pouget <kpouget@redhat.com>
- Add status.completionTime to CRD schema, set atomically on all terminal transitions via new set_terminal_phase() helper - Validate spec.ttl in on_create; fail immediately if unparseable - Refactor _get_completion_time to read status.completionTime with fallback to metadata.creationTimestamp for creation-time failures - Downgrade GC log for invalid/missing TTL from warning to debug - Fix indentation from accepted early-continue suggestion - Update tests to match completionTime-based logic Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
| def _get_completion_time(job: dict) -> datetime | None: | ||
| """Return the time the job entered its terminal phase. | ||
|
|
||
| Prefers status.completionTime (set by the operator on terminal transitions). | ||
| Falls back to metadata.creationTimestamp for jobs that failed at creation. | ||
| """ |
There was a problem hiding this comment.
I'm not sure we want to fallback on the creation time in the get_completion_time method (it's counter-intuitive to get a completion time if the job is still running)
Falls back to metadata.creationTimestamp for jobs that failed at creation.
jobs shouldn't fail at creation time AFAIU
There was a problem hiding this comment.
maybe this method should check that the fjob is in a terminal state?
| patch.status["conditions"] = result | ||
|
|
||
|
|
||
| def set_terminal_phase(patch, phase: str, message: str) -> None: |
There was a problem hiding this comment.
for consistency, this method should check that phase is in the TERMINAL_PHASES
kpouget
left a comment
kpouget
left a comment
There was a problem hiding this comment.
after addressing the reviews, please deploy the controller in the WIP namespace with this:
/test deploy-fournos-wip
and test it manually by creating fjobs in psap-automation-wip
2 participants
Summary
spec.ttlfield to the FournosJob CRD that defines the delay after job termination (Succeeded/Failed/Stopped) before the CR is automatically deleted12h,30m,7d)Changes
manifests/crd.yaml— newspec.ttlfield in OpenAPI schemafournos/core/duration.py— Go-style duration parser ("12h"->timedelta)fournos/operator.py—_gc_expired_jobs()added to GC loop, deletes terminal jobs past their TTLtests/test_ttl.py— unit tests for duration parsing and TTL GC logicTest plan
pytest tests/test_ttl.py)spec.ttlaccepted on new jobsttl: 1m, wait for completion, confirm auto-deletion after ~5 min (GC interval)ttlare not affectedMade with Cursor
fixes: #48
fixes: #93