Skip to content

[draft] skip root level check for job enqueue #4680

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mtian29 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[draft] skip root level check for job enqueue #4680

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion pkg/scheduler/plugins/capacity/capacity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -886,7 +886,15 @@ func (cp *capacityPlugin) checkJobEnqueueableHierarchically(ssn *framework.Sessi
list := append(cp.queueOpts[queue.UID].ancestors, queue.UID)
// Check whether the job can be enqueued to the queue and all its ancestors.
for i := len(list) - 1; i >= 0; i-- {
if inqueue, resourceNames := cp.jobEnqueueable(ssn.Queues[list[i]], job); !inqueue {
currentQueue := ssn.Queues[list[i]]

// Skip capacity check for root queue - allow enqueue and let reclaim handle preemption
if cp.queueOpts[list[i]].name == cp.rootQueue {
klog.V(4).Infof("Skip capacity check for root queue, allow job <%s/%s> to enqueue for potential reclaim", job.Namespace, job.Name)
continue
Comment on lines +892 to +894
Copy link

@gemini-code-assist gemini-code-assist bot Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider extracting the condition cp.queueOpts[list[i]].name == cp.rootQueue into a well-named variable to improve readability. This will make the code easier to understand at a glance.

Also, consider adding a comment explaining why level 4 logging is used here.

currentQueueName := cp.queueOpts[list[i]].name

// Skip capacity check for root queue - allow enqueue and let reclaim handle preemption
if currentQueueName == cp.rootQueue {
	klog.V(4).Infof("Skip capacity check for root queue, allow job <%s/%s> to enqueue for potential reclaim", job.Namespace, job.Name)
	continue
}

}
Comment on lines +891 to +895
Copy link

@gemini-code-assist gemini-code-assist bot Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This change skips the capacity check for the root queue. While this might be intended to allow reclaim to handle preemption, it's crucial to ensure that this doesn't lead to over-commitment of resources at the root level, potentially starving other queues or causing unexpected behavior. It would be helpful to add more logging to track when and why this condition is being triggered, and to monitor the resource usage of the root queue more closely.

Consider adding a metric to track how often this condition is met, to ensure that it is not happening more often than expected.


if inqueue, resourceNames := cp.jobEnqueueable(currentQueue, job); !inqueue {
// If log level is 5, print the information of all queues from leaf to ancestor.
if klog.V(5).Enabled() {
for j := i - 1; j >= 0; j-- {
Expand Down