feat: add lockOnly support for sentinel cluster-lock jobs - #89
Conversation
Hearth creates sentinel FournosJobs with lockOnly: true to hold cluster-slot quota for human-driven cluster ownership. Without this, the execution controller tries to create a PipelineRun and fails. - CRD: add lockOnly boolean field, remove executionEngine from required - lifecycle: validate lockOnly, create lock Workload skipping Forge - execution: early return in reconcile_admitted for lockOnly jobs
|
[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 |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR introduces a lock-only mode for FournosJob that reserves exclusive cluster capacity via Kueue without launching pipelines. The CRD schema adds a ChangesLock-Only Mode for FournosJob
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
fournos/handlers/lifecycle.py (2)
1-269:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix
ruff formatlint failure.CI is failing on
ruff format --check. Runruff format fournos/handlers/lifecycle.py(orruff format fournos/ tests/) before pushing so the lint job stays green.🤖 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 `@fournos/handlers/lifecycle.py` around lines 1 - 269, The file fails ruff format checks; run `ruff format fournos/handlers/lifecycle.py` (or the project-wide `ruff format`) and reformat the file, then re-run the linter before committing; ensure the formatted changes cover the top-level functions and helpers such as on_create, _create_lock_workload, _find_exclusive_locker, _pending_status, and reconcile_pending so imports, spacing, and string concatenations match ruff's style rules and the CI `ruff format --check` job passes.
53-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEdge case:
lockOnly: truecombined withexclusive: falseis silently inconsistent.When a user submits
lockOnly: truewithexclusive: falseexplicitly set, the validation at lines 55-63 passes, theLABEL_EXCLUSIVE_CLUSTERlabel at line 83-84 is not applied, but_create_lock_workloadunconditionally passesexclusive=Trueto Kueue (line 104). This produces:
- A Workload that exclusively locks the cluster slot, but
- No
LABEL_EXCLUSIVE_CLUSTERlabel on the FournosJob, so_find_exclusive_locker(line 144) cannot identify it as the locker for other pending jobs.The CRD description states
lockOnly"Impliesexclusive: true", so enforce that invariant explicitly in validation (or normalize it) so the label and the workload stay in sync.🛡️ Proposed fix — force exclusive=True when lockOnly is set
cluster = spec.get("cluster") exclusive = spec["exclusive"] lock_only = spec.get("lockOnly", False) if lock_only and not cluster: patch.status["phase"] = Phase.FAILED patch.status["message"] = "lockOnly: true requires 'cluster' to be set" return if not lock_only and not spec.get("executionEngine"): patch.status["phase"] = Phase.FAILED patch.status["message"] = "spec.executionEngine is required for non-lockOnly jobs" return + # lockOnly implies exclusive (per CRD contract). + if lock_only: + exclusive = True + if exclusive and not cluster:🤖 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 `@fournos/handlers/lifecycle.py` around lines 53 - 88, The validation allows lock_only True while exclusive is False, but _create_lock_workload always creates an exclusive Kueue Workload and LABEL_EXCLUSIVE_CLUSTER isn't applied, breaking _find_exclusive_locker; fix by normalizing/enforcing exclusive when lock_only is set: when lock_only (spec.get("lockOnly")) is True, set exclusive = True (or set spec["exclusive"]=True) and ensure patch.meta.setdefault("labels", {})[LABEL_EXCLUSIVE_CLUSTER] = cluster is applied (reuse LABEL_EXCLUSIVE_CLUSTER and the same branch that currently adds the label), so the label and created exclusive workload stay in sync with _create_lock_workload and _find_exclusive_locker.
🤖 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.
Outside diff comments:
In `@fournos/handlers/lifecycle.py`:
- Around line 1-269: The file fails ruff format checks; run `ruff format
fournos/handlers/lifecycle.py` (or the project-wide `ruff format`) and reformat
the file, then re-run the linter before committing; ensure the formatted changes
cover the top-level functions and helpers such as on_create,
_create_lock_workload, _find_exclusive_locker, _pending_status, and
reconcile_pending so imports, spacing, and string concatenations match ruff's
style rules and the CI `ruff format --check` job passes.
- Around line 53-88: The validation allows lock_only True while exclusive is
False, but _create_lock_workload always creates an exclusive Kueue Workload and
LABEL_EXCLUSIVE_CLUSTER isn't applied, breaking _find_exclusive_locker; fix by
normalizing/enforcing exclusive when lock_only is set: when lock_only
(spec.get("lockOnly")) is True, set exclusive = True (or set
spec["exclusive"]=True) and ensure patch.meta.setdefault("labels",
{})[LABEL_EXCLUSIVE_CLUSTER] = cluster is applied (reuse LABEL_EXCLUSIVE_CLUSTER
and the same branch that currently adds the label), so the label and created
exclusive workload stay in sync with _create_lock_workload and
_find_exclusive_locker.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 475f73ca-c672-415b-92e6-0bbcae958473
📒 Files selected for processing (3)
fournos/handlers/execution.pyfournos/handlers/lifecycle.pymanifests/crd.yaml
|
thanks @MML-coder , looks good. Please fix the linter and remove the |
|
@kpouget - this PR still needs lgtm :) Thanks |
when OCPCI is in charge of the labels, you can't get LGTM if there are pending changes 🙃 (the label would be removed with your push) /lgtm |
2 participants
Summary
lockOnlyboolean field to FournosJob CRD — sentinel jobs that hold cluster-slot quota without running a pipelinelifecycle.py(skips Forge/execution engine resolution)execution.py::reconcile_admitted()so lockOnly jobs stay Admitted indefinitelyexecutionEnginefrom CRD required fields (lockOnly jobs don't need one)Context: The hearth controller (cluster lifecycle operator) creates sentinel FournosJobs with
lockOnly: trueto implement human-driven cluster locking. Without this change, the execution controller tries to create a PipelineRun for the sentinel and fails with "Failed to fetch Pipeline".Test plan
oc patch fournoscluster <name> -n hearth --type=merge -p '{"spec":{"owner":"test"}}'oc patch fournoscluster <name> -n hearth --type=merge -p '{"spec":{"owner":""}}'Summary by CodeRabbit
New Features
lockOnlymode require cluster configuration and remain in Admitted phase until deletion.Chores