fix: guard preemptive reclaim by realCapability#5053
fix: guard preemptive reclaim by realCapability#5053FAUST-BENCHOU wants to merge 1 commit intovolcano-sh:masterfrom
Conversation
Signed-off-by: zhoujinyu <2319109590@qq.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 |
Summary of ChangesHello @FAUST-BENCHOU, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a bug in the scheduler's capacity plugin where the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
volcano-sh-bot
added
the
size/M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a critical fix to the preemptiveFn in the capacity plugin, ensuring that preemptive reclaim operations respect the realCapability of a queue. Previously, the function only checked against deserved resources, potentially leading to over-reclamation beyond the queue's actual capacity. The change adds a LessEqualWithDimensionAndResourcesName check against realCapability before proceeding with the deserved check. A new test case TestPreemptiveFn_RespectsCapability has been added to validate this behavior. The changes are well-contained and directly address the identified bug.
| if allocatable, _ := futureUsed.LessEqualWithDimensionAndResourcesName(attr.realCapability, task.Resreq); !allocatable { | ||
| klog.V(3).Infof("Queue <%v> cannot reclaim for <%s/%s> because futureUsed <%v> exceeds realCapability <%v>.", | ||
| queue.Name, task.Namespace, task.Name, futureUsed, attr.realCapability) | ||
| metrics.UpdateQueueOverused(attr.name, true) | ||
| return false | ||
| } |
There was a problem hiding this comment.
This new check correctly guards against reclaiming beyond the realCapability. It's crucial for maintaining resource limits and preventing a queue from consuming more resources than it's allowed, even if its deserved resources are higher. This directly addresses the bug described in the PR description.
| func TestPreemptiveFn_RespectsCapability(t *testing.T) { | ||
| // Queue has deserved and capability both {cpu: 8, mem: 100Gi}, allocated {cpu: 7, mem: 90Gi}. | ||
| // A task requesting {cpu: 2, mem: 5Gi} would keep memory below deserved but push CPU above capability. | ||
| // preemptiveFn should reject preemption in this case. | ||
|
|
||
| deserved := api.NewResource(api.BuildResourceList("8", "100Gi")) | ||
| allocated := api.NewResource(api.BuildResourceList("7", "90Gi")) | ||
| capability := api.NewResource(api.BuildResourceList("8", "100Gi")) | ||
|
|
||
| attr := &queueAttr{ | ||
| queueID: "q-test", | ||
| name: "q-test", | ||
| deserved: deserved, | ||
| allocated: allocated, | ||
| capability: capability, | ||
| realCapability: capability.Clone(), | ||
| } | ||
|
|
||
| taskRes := api.NewResource(api.BuildResourceList("2", "5Gi")) | ||
| task := &api.TaskInfo{ | ||
| Resreq: taskRes, | ||
| InitResreq: taskRes, | ||
| } | ||
|
|
||
| queue := &api.QueueInfo{ | ||
| UID: "q-test", | ||
| Name: "q-test", | ||
| } | ||
|
|
||
| futureUsed := attr.allocated.Clone().Add(task.Resreq) | ||
| if allocatable, _ := futureUsed.LessEqualWithDimensionAndResourcesName(attr.realCapability, task.Resreq); allocatable { | ||
| t.Fatalf("precondition error: futureUsed %v should exceed capability %v", futureUsed, attr.realCapability) | ||
| } | ||
|
|
||
| // Simulate the logic in preemptiveFn after our fix: first capability, then deserved. | ||
| allocatable, _ := futureUsed.LessEqualWithDimensionAndResourcesName(attr.realCapability, task.Resreq) | ||
| if allocatable { | ||
| t.Fatalf("expected allocatable=false when futureUsed exceeds capability, got true") | ||
| } | ||
|
|
||
| // For completeness, check that even though memory is below deserved, we still don't allow reclaim | ||
| // because capability check fails. | ||
| isPreemptive, _ := futureUsed.LessEqualPartlyWithDimensionZeroFiltered(attr.deserved, task.Resreq) | ||
| if isPreemptive { | ||
| t.Logf("deserved check alone would allow reclaim, but capability check must block it") | ||
| } | ||
|
|
||
| _ = queue // queue is only needed to mirror function signature; logic above is independent of it. | ||
| } |
There was a problem hiding this comment.
The new test case TestPreemptiveFn_RespectsCapability effectively validates the fix. It sets up a scenario where the futureUsed resources would exceed realCapability but not deserved, and correctly asserts that preemption is rejected. This is a good, targeted test for the introduced change.
There was a problem hiding this comment.
Pull request overview
This PR fixes Bug #3 from issue #5048 by adding a capability check to the preemptiveFn in the capacity plugin. The bug allowed queues to reclaim resources above their capability/realCapability because the preemptiveFn only checked against the deserved value, not the capability limit.
Changes:
- Adds an early capability check in preemptiveFn that blocks preemption if futureUsed would exceed realCapability
- Adds a test that verifies the logic (though not the actual preemptiveFn callback, which is unexported)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/scheduler/plugins/capacity/capacity.go | Adds capability check before deserved check in preemptiveFn to prevent reclaim above capacity |
| pkg/scheduler/plugins/capacity/capacity_test.go | Adds test verifying the capability-first-then-deserved logic for preemption decisions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if allocatable, _ := futureUsed.LessEqualWithDimensionAndResourcesName(attr.realCapability, task.Resreq); !allocatable { | ||
| klog.V(3).Infof("Queue <%v> cannot reclaim for <%s/%s> because futureUsed <%v> exceeds realCapability <%v>.", | ||
| queue.Name, task.Namespace, task.Name, futureUsed, attr.realCapability) | ||
| metrics.UpdateQueueOverused(attr.name, true) | ||
| return false | ||
| } |
There was a problem hiding this comment.
The metrics.UpdateQueueOverused call at line 186 creates a duplicate metric update issue. When the capability check fails, this sets overused=true and returns. However, if the capability check passes but the deserved check fails, line 202 will update the same metric again with overused=!isPreemptive. This means the metric can be updated twice for different reasons in a single invocation.
Consider removing the metric update from line 186 and only keeping the one at line 202, or restructure the logic to ensure the metric is updated only once at the end of the function based on the final decision. The metric should reflect whether the queue is overused, not intermediate check failures.
|
/retest |
|
@FAUST-BENCHOU: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions 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/test-infra repository. |
|
/assign @hajnalmt |
3 participants
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
part of #5048
Bug3:preemptiveFn does not check queue capability which allows reclaim above capability/realCapability
Special notes for your reviewer:
preemptiveFnis a closure and not exported, so it cannot be tested; the test only reproduces the logic of "capability first, then deserved".