Refine scheduling for exclusive, cluster and hw spec configs - #71
Conversation
Signed-off-by: avasilev <avasilev@redhat.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 |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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)
📝 WalkthroughWalkthroughDefaults and validation for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FournosController as Controller
participant Resolving
participant KueueClient as Kueue
participant KueueServer as KueueServer
User->>Controller: Create/Update FournosJob (spec.cluster?, spec.exclusive?, spec.hardware?)
Controller->>Resolving: reconcile_resolving(job.spec)
Resolving->>Resolving: _resolve_hardware(spec) -> (gpu_type, gpu_count) or (None,None) or (None,0)
alt validation failure
Resolving->>Controller: Patch job Failed (reason="NoHardware")
else success
Resolving->>Kueue: create_workload(..., exclusive=spec["exclusive"], slots=MAX or 1, gpu_type/count)
Kueue->>KueueServer: Submit workload to scheduler
KueueServer-->>Kueue: admission/queueing
Kueue-->>Controller: Workload created / status
Controller-->>User: Job transitions (Pending → Running → Succeeded/Failed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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.
🧹 Nitpick comments (1)
tests/test_scheduling.py (1)
175-180: Reduce pre-inspection race when asserting Workload fields.At Line 175, the pre-check allows
Phase.SUCCEEDED; if that happens first, Workload inspection at Lines 182-191 can become timing-sensitive.Suggested stabilization
- poll_phase( + phase = poll_phase( k8s, "test-shared-hw", - terminal={Phase.RUNNING, Phase.SUCCEEDED, Phase.FAILED}, + terminal={Phase.PENDING, Phase.ADMITTED, Phase.RUNNING, Phase.FAILED}, timeout=30, ) + assert phase != Phase.FAILED, job_status_summary(k8s, "test-shared-hw")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_scheduling.py` around lines 175 - 180, The pre-check call to poll_phase includes Phase.SUCCEEDED which can cause the test to observe a finished Workload before the subsequent Workload inspection block runs, making assertions flaky; change the poll_phase invocation to wait for the Workload to reach an active state (e.g., only Phase.RUNNING and Phase.FAILED, or exclusively Phase.RUNNING) instead of including Phase.SUCCEEDED so the test always inspects the Workload while it is running, then proceed with the existing Workload inspection/assertion code (the poll_phase call and the following Workload inspection block should be updated accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_scheduling.py`:
- Around line 175-180: The pre-check call to poll_phase includes Phase.SUCCEEDED
which can cause the test to observe a finished Workload before the subsequent
Workload inspection block runs, making assertions flaky; change the poll_phase
invocation to wait for the Workload to reach an active state (e.g., only
Phase.RUNNING and Phase.FAILED, or exclusively Phase.RUNNING) instead of
including Phase.SUCCEEDED so the test always inspects the Workload while it is
running, then proceed with the existing Workload inspection/assertion code (the
poll_phase call and the following Workload inspection block should be updated
accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21b7511f-72b4-4bb3-9fd0-601bfdc36b98
📒 Files selected for processing (13)
Fournos_Design_Document.mdREADME.mdfournos/core/kueue.pyfournos/handlers/lifecycle.pyfournos/handlers/resolving.pyfournos/handlers/status.pymanifests/crd.yamltests/test_exclusive.pytests/test_lifecycle.pytests/test_resolving.pytests/test_scheduling.pytests/test_shutdown.pytests/test_validation.py
Signed-off-by: avasilev <avasilev@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fournos/handlers/resolving.py`:
- Around line 128-140: Update the function docstring in resolving.py to reflect
the new exclusive-only path that skips GPU validation: change the sentence that
currently says the GPU type is "always validated against Kueue" to clarify that
GPU validation is performed except when the function returns the exclusive-only
sentinel (gpu_type is None and gpu_count is 0), in which case GPU validation is
intentionally skipped; keep the return description showing "(gpu_type,
gpu_count)" for success and "(None, None)" for validation failure and explicitly
document that "(None, 0)" represents the exclusive-only cluster-lock path that
bypasses Kueue validation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d232ae33-e77e-4e21-bcc6-1a9b323397d7
📒 Files selected for processing (1)
fournos/handlers/resolving.py
Signed-off-by: avasilev <avasilev@redhat.com>
|
/test deploy-fournos-wip |
|
🟢 Test of 'fournos_deploy --project-source' succeeded after 00 hours 09 minutes 52 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
|
thanks @avasilevskii , |
2 participants
Closes #67
Summary by CodeRabbit
New Features
exclusive: falseto allow shared cluster access.Bug Fixes / Behavior Changes
exclusive(omitting it may cause validation failures); non-exclusive jobs continue to require hardware.Documentation
Tests