Refactor reclaim action #4794

guoqinwill · 2025-12-10T12:45:55Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

origin pr：#4407

Which issue(s) this PR fixes:

Fixes #3738

Special notes for your reviewer:

Does this PR introduce a user-facing change?

volcano-sh-bot · 2025-12-10T12:46:01Z

@guoqinwill: The label(s) kind/improvement, kind//kind cannot be applied, because the repository doesn't have them.

Details

In response to this:

What type of PR is this?

/kind improvement /kind bug

What this PR does / why we need it:

origin pr：#4407

Which issue(s) this PR fixes:

Fixes #3738

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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.

volcano-sh-bot · 2025-12-10T12:46:02Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign shinytang6 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

pkg/scheduler/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gemini-code-assist · 2025-12-10T12:46:13Z

Summary of Changes

Hello @guoqinwill, I'm Gemini Code Assist¹! 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 refactors the reclaim action within the scheduler to enhance its robustness and correctness. The primary goal is to ensure that tasks explicitly marked with a PreemptNever policy do not inadvertently prevent other eligible tasks from reclaiming necessary resources. By restructuring the reclamation process and introducing a dedicated helper function, the scheduler can now gracefully handle non-preemptable tasks while continuing to evaluate and execute preemption for others, thereby improving overall resource management and addressing a known issue.

Highlights

Refactored Reclaim Action Logic: The core logic for reclaiming resources has been extracted into a new helper function, reclaimForTask, to improve modularity and readability within the Execute function.
Improved Handling of PreemptNever Policy: The refactoring addresses a bug where tasks with a PreemptNever policy could block subsequent tasks in the same job or queue from initiating reclamation. The updated logic now correctly skips such tasks, allowing other eligible tasks to proceed.
Enhanced Iteration for Reclamation: The main Execute function now features nested loops that iterate through queues, jobs, and their candidate tasks more systematically, ensuring that all potential reclamation opportunities are considered, even when some tasks are non-preemptable.
New Test Case for Preemption Policy: A new test case, Reclaim succeeds for second task when first task has PreemptionPolicy=Never, has been added to validate the correct behavior of the reclaim action when encountering tasks with PreemptNever policies.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request refactors the reclaim action to correctly handle tasks with a PreemptionPolicy of Never, preventing them from blocking other tasks. The overall change is an improvement and the new test case is well-suited to verify the fix. However, I've found a critical issue in the new loop structure that could lead to an infinite loop, and another bug in how task assignment is tracked within a job. I've also noted a leftover debug log that should be removed. My suggestions aim to fix these new issues while preserving the intended refactoring.

pkg/scheduler/actions/reclaim/reclaim.go

JesseStutler · 2025-12-11T01:52:38Z

/cc

JesseStutler · 2025-12-11T06:39:47Z

Also /assign @hajnalmt mate is also familiar with the reclaim logic

pkg/scheduler/actions/reclaim/reclaim.go

hajnalmt

This is a great contribution! Thank you @guoqinwill !
The logic behind the action looks flawless.

Following Jesse's comment, I also think the variable names for the two boolean variable assigned and the assignedInJob can be improved, to reflect the intention. (I don't like it in preempt action too.)
Could we rename them to taskPipelined and taskPipelinedInJob for clarity?
These names better reflect that the variables indicate whether a task was pipelined in the loop and within the job, respectively.

pkg/scheduler/actions/reclaim/reclaim.go

pkg/scheduler/actions/reclaim/reclaim_test.go

pkg/scheduler/actions/reclaim/reclaim.go

JesseStutler · 2025-12-12T02:18:12Z

pkg/scheduler/actions/reclaim/reclaim.go

+						task.UID, n.Name, ssn.UID, rollbackErr)
+				}
+			}
+			assigned = true


Just a reminder that here assigned should be renamed like reclaimed, but I still feel this variable is unuseful in reclaim scenario

We have a variable named reclaimed at line 213, that's why I suggested taskPipelined for this, but we can rename the other variable to reclaimedResources and name this to reclaimed maybe?
This variable indicates if there was a task pipelined in the function which is I think important to continue the reclaim in the other tasks in the job.

JesseStutler · 2025-12-12T07:10:08Z

pkg/scheduler/actions/reclaim/reclaim.go

+	predicateNodes, _ := predicateHelper.PredicateNodes(task, totalNodes, ssn.PredicateForPreemptAction, false)
+	assigned := false
+
+	for _, n := range predicateNodes {


#3997 Currently, reclaim is done by traversing nodes, instead of first filtering candidates by node and then sorting them. So, I'm wondering if we can fix this issue together? Imagine this worst-case scenario: each job's pods are scattered across different nodes, and then a job wants to reclaim, it might reclaim one pod from each of the other jobs. If we sort by priority, this would make it easier for pods from lower-priority jobs to be reclaimed first.

I see there wasn't any progress in this Issue.

How do you imagine the implementation for this?
Do we want to create a VictimNodeOrderFn implemented in the nodeOrder plugin?

I think the modifications to this issue can be split up for now, and we can focus on refactoring the reclaim process first.

hajnalmt

Please check my comments too 👍

pkg/scheduler/actions/reclaim/reclaim.go

hajnalmt · 2025-12-12T13:53:05Z

pkg/scheduler/actions/reclaim/reclaim.go

+						task.UID, n.Name, ssn.UID, rollbackErr)
+				}
+			}
+			assigned = true


We have a variable named reclaimed at line 213, that's why I suggested taskPipelined for this, but we can rename the other variable to reclaimedResources and name this to reclaimed maybe?
This variable indicates if there was a task pipelined in the function which is I think important to continue the reclaim in the other tasks in the job.

hajnalmt · 2025-12-12T13:54:38Z

pkg/scheduler/actions/reclaim/reclaim_test.go

 	"volcano.sh/volcano/pkg/scheduler/util"
 )

+func init() {


why did you leave the function definition here? Is it called somehwere?

Because predicateHelper.PredicateNodes was introduced, the unit test needs to initialize options.ServerOpts; otherwise, it will fail at runtime.

hajnalmt · 2025-12-12T13:56:10Z

pkg/scheduler/actions/reclaim/reclaim_test.go

 			},
 			PodGroups: []*schedulingv1beta1.PodGroup{
-				util.BuildPodGroupWithPrio("pg1", "c1", "q1", 1, nil, schedulingv1beta1.PodGroupInqueue, "mid-priority"),
+				util.BuildPodGroupWithPrio("pg1", "c1", "q1", 1, nil, schedulingv1beta1.PodGroupInqueue, "low-priority"),


This test case was fixed on master please rebase, and don't modify it in your commit directly.

hajnalmt · 2025-12-12T13:57:26Z

pkg/scheduler/actions/reclaim/reclaim.go

+	predicateNodes, _ := predicateHelper.PredicateNodes(task, totalNodes, ssn.PredicateForPreemptAction, false)
+	assigned := false
+
+	for _, n := range predicateNodes {


I see there wasn't any progress in this Issue.

How do you imagine the implementation for this?
Do we want to create a VictimNodeOrderFn implemented in the nodeOrder plugin?

Signed-off-by: GautamBytes <manchandanigautam@gmail.com> Signed-off-by: guoqinwill <gq411will@163.com>

Signed-off-by: guoqinwill <gq411will@163.com>

volcano-sh-bot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 10, 2025

volcano-sh-bot requested review from merryzhou and wangyang0616 December 10, 2025 12:46

volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 10, 2025

gemini-code-assist bot reviewed Dec 10, 2025

View reviewed changes

pkg/scheduler/actions/reclaim/reclaim.go Show resolved Hide resolved

pkg/scheduler/actions/reclaim/reclaim.go Outdated Show resolved Hide resolved

guoqinwill force-pushed the fix-reclaim-1 branch from 827a55d to be8390b Compare December 10, 2025 15:50

volcano-sh-bot requested a review from JesseStutler December 11, 2025 01:52

guoqinwill force-pushed the fix-reclaim-1 branch from be8390b to e3b68f4 Compare December 11, 2025 06:20

JesseStutler requested a review from hajnalmt December 11, 2025 06:39

JesseStutler reviewed Dec 11, 2025

View reviewed changes

pkg/scheduler/actions/reclaim/reclaim.go Outdated Show resolved Hide resolved

JesseStutler reviewed Dec 11, 2025

View reviewed changes

pkg/scheduler/actions/reclaim/reclaim.go Outdated Show resolved Hide resolved

JesseStutler reviewed Dec 11, 2025

View reviewed changes

pkg/scheduler/actions/reclaim/reclaim.go Outdated Show resolved Hide resolved

pkg/scheduler/actions/reclaim/reclaim.go Outdated Show resolved Hide resolved

guoqinwill force-pushed the fix-reclaim-1 branch 5 times, most recently from e4f55b6 to bcc4023 Compare December 11, 2025 12:28

hajnalmt suggested changes Dec 11, 2025

View reviewed changes

guoqinwill force-pushed the fix-reclaim-1 branch from bcc4023 to ec07b90 Compare December 11, 2025 15:16

JesseStutler reviewed Dec 12, 2025

View reviewed changes

JesseStutler mentioned this pull request Dec 12, 2025

capacity reclaim does not work #4808

Closed

JesseStutler reviewed Dec 12, 2025

View reviewed changes

hajnalmt suggested changes Dec 12, 2025

View reviewed changes

refactor reclaim.go

3057713

Signed-off-by: GautamBytes <manchandanigautam@gmail.com> Signed-off-by: guoqinwill <gq411will@163.com>

guoqinwill force-pushed the fix-reclaim-1 branch from ec07b90 to 01d921a Compare December 16, 2025 06:52

fix ci and update reclaim

cdaaa97

Signed-off-by: guoqinwill <gq411will@163.com>

guoqinwill force-pushed the fix-reclaim-1 branch from 01d921a to cdaaa97 Compare December 16, 2025 06:56

Refactor reclaim action #4794

Are you sure you want to change the base?

Refactor reclaim action #4794

Uh oh!

Conversation

guoqinwill commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

volcano-sh-bot commented Dec 10, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

volcano-sh-bot commented Dec 10, 2025

Uh oh!

gemini-code-assist bot commented Dec 10, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

JesseStutler commented Dec 11, 2025

Uh oh!

JesseStutler commented Dec 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hajnalmt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hajnalmt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hajnalmt Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guoqinwill commented Dec 10, 2025 •

edited

Loading

hajnalmt left a comment •

edited

Loading

hajnalmt Dec 12, 2025 •

edited

Loading