add nodegroup nonstrict to fit 0nodes queue #4602

wuyueandrew · 2025-09-07T13:53:48Z

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #4528

Special notes for your reviewer:

Does this PR introduce a user-facing change?

gemini-code-assist

Summary of Changes

Hello @wuyueandrew, 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 enhances the nodegroup scheduler plugin by introducing a configurable "nonstrict" mode. This mode modifies the behavior of queue-group affinity checks and scoring, making the scheduler more resilient to scenarios where queues might not have strictly defined node group affiliations, particularly for queues that might temporarily or permanently have zero associated nodes.

Highlights

New Configuration Option: Introduced a "nonstrict" boolean configuration option for the nodegroup plugin, allowing for more flexible handling of node group affinities.
Predicate Logic Adjustment: Modified the predicate function within queueGroupAffinity to bypass strict group requirements when nonstrict mode is enabled, preventing tasks from being unschedulable if a queue has no associated groups.
Scoring Behavior Modification: Adjusted the score function to assign a specific negative score (-2) when nonstrict mode is active and a queue has no associated nodes, influencing scheduling decisions.

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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

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 issue 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 introduces a nonstrict mode to the nodegroup plugin, allowing queues to schedule on nodes without explicit affinity, which is useful for queues that might not have dedicated nodes. The implementation is straightforward, but I've identified a significant pre-existing bug in the predicate logic that is exacerbated by this change. Additionally, I've provided some suggestions to improve code clarity. A crucial omission is the lack of unit tests for the new nonstrict functionality; adding tests would be essential to ensure correctness and prevent future regressions.

pkg/scheduler/plugins/nodegroup/nodegroup.go

gemini-code-assist · 2025-09-07T13:55:17Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

 	flag := false
+	if nonstrict {
+		flag = true
+	}


This initialization can be simplified for better readability.

Suggested change

flag := false

if nonstrict {

flag = true

}

flag := nonstrict

gemini-code-assist · 2025-09-07T13:55:17Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

 		return nodeScore
 	}
+	if nonstrict {
+		nodeScore = -2


The value -2 is a magic number. It would be better to define it as a constant at the top of the file (e.g., const NonStrictPenalty = -2) to improve readability and maintainability.

pkg/scheduler/plugins/nodegroup/nodegroup.go

kingeasternsun · 2025-09-12T03:08:04Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

 	if len(queue) == 0 {
 		return nodeScore
 	}
+	if nonstrict {


kingeasternsun · 2025-09-19T02:13:13Z

/lgtm

Monokaix · 2025-09-19T06:42:09Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

 type nodeGroupPlugin struct {
 	// Arguments given for the plugin
 	pluginArguments framework.Arguments
+	nonstrict       bool


I prefer to set a strict flag instead of nonstricet, which is more understandable, and it's defaut value is true.

Monokaix · 2025-09-19T06:43:21Z

And please also update the ndoegroup design doc to include this parameter and merge to one commit is OK.

volcano-sh-bot · 2025-09-22T10:11:06Z

New changes are detected. LGTM label has been removed.

wuyueandrew · 2025-09-22T10:11:37Z

And please also update the ndoegroup design doc to include this parameter and merge to one commit is OK.

done

Monokaix · 2025-09-26T09:02:02Z

/approve

JesseStutler · 2025-09-26T10:00:17Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

 // New function returns prioritize plugin object.
 func New(arguments framework.Arguments) framework.Plugin {
-	return &nodeGroupPlugin{pluginArguments: arguments}
+	nodeGroupPlugin := &nodeGroupPlugin{pluginArguments: arguments, strict: true}


I think we should also update nodegroup plugin user guide? But we will release v1.13.0 soon, if you have limit time to update it, you can also seperate in the other PR @wuyueandrew

Signed-off-by: wuyue <[email protected]>

volcano-sh-bot · 2025-09-26T10:18:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Monokaix

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Monokaix]

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

JesseStutler · 2025-09-27T02:34:22Z

docs/user-guide/how_to_use_nodegroup_plugin.md

+  - name: predicates
+  - name: proportion
+  - name: nodegroup
+    strict: false


I think it's

- name: nodegroup arguments: strict: false

JesseStutler · 2025-09-27T02:36:04Z

docs/user-guide/how_to_use_nodegroup_plugin.md


+### Nodegroup Plugin Strict Configuration
+
+Strict is a switch to specific behavior of unset affinity queue. If `strict: false`, workload assigned to a unset affinity queue could be scheduled to Node with empty label `volcano.sh/nodegroup-name`. Otherwise, workload cannot be scheduled.


Seems that it's if the node doesn't have the "volcano.sh/nodegroup-name" label or the queue doesn't set affinity ?

kingeasternsun · 2025-09-28T03:10:07Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+			if np.strict || !unsetAffinity {
+				return newFitErr(task, node, errNodeGroupLabelNotFound)
+			} else {
+				return nil
+			}


Suggested change

if np.strict || !unsetAffinity {

return newFitErr(task, node, errNodeGroupLabelNotFound)

} else {

return nil

}

if np.strict || !unsetAffinity {

return newFitErr(task, node, errNodeGroupLabelNotFound)

}

return nil

kingeasternsun · 2025-09-28T03:11:10Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+			} else {
+				return nil
+			}


Suggested change

} else {

return nil

}

}

return nil

kingeasternsun · 2025-09-28T03:11:37Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+			} else {
+				return nil
+			}


Suggested change

} else {

return nil

}

return nil

JesseStutler · 2025-09-28T08:43:12Z

@kingeasternsun @Monokaix Consider we need to release new version asap and @wuyueandrew has no time currently, I have made a new PR and we can merge that #4652

volcano-sh-bot requested review from Thor-wl and k82cn September 7, 2025 13:53

gemini-code-assist bot reviewed Sep 7, 2025

View reviewed changes

volcano-sh-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 7, 2025

gemini-code-assist bot reviewed Sep 7, 2025

View reviewed changes

kingeasternsun reviewed Sep 15, 2025

View reviewed changes

wuyueandrew force-pushed the issue4528 branch from 1a29cc5 to d9a22ce Compare September 15, 2025 06:02

volcano-sh-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 18, 2025

volcano-sh-bot assigned kingeasternsun Sep 19, 2025

volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2025

Monokaix reviewed Sep 19, 2025

View reviewed changes

wuyueandrew force-pushed the issue4528 branch from 4abb45d to e731d50 Compare September 22, 2025 10:11

volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2025

volcano-sh-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2025

wuyueandrew force-pushed the issue4528 branch from e731d50 to fe24f30 Compare September 24, 2025 11:08

volcano-sh-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2025

wuyueandrew force-pushed the issue4528 branch 3 times, most recently from 2356df9 to d33ea69 Compare September 26, 2025 08:57

volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2025

JesseStutler reviewed Sep 26, 2025

View reviewed changes

add strict to fit unset affinity queue

dfd2b80

Signed-off-by: wuyue <[email protected]>

wuyueandrew force-pushed the issue4528 branch from d33ea69 to dfd2b80 Compare September 26, 2025 10:18

JesseStutler reviewed Sep 27, 2025

View reviewed changes

kingeasternsun reviewed Sep 28, 2025

View reviewed changes

JesseStutler mentioned this pull request Sep 28, 2025

Add nodegroup nonstrict mode #4652

Merged

volcano-sh-bot merged commit dfd2b80 into volcano-sh:master Sep 28, 2025
17 of 18 checks passed


		### Nodegroup Plugin Strict Configuration

		Strict is a switch to specific behavior of unset affinity queue. If `strict: false`, workload assigned to a unset affinity queue could be scheduled to Node with empty label `volcano.sh/nodegroup-name`. Otherwise, workload cannot be scheduled.

add nodegroup nonstrict to fit 0nodes queue #4602

add nodegroup nonstrict to fit 0nodes queue #4602

Uh oh!

Conversation

wuyueandrew commented Sep 7, 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!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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!

gemini-code-assist bot Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 7, 2025

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!

kingeasternsun commented Sep 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Monokaix commented Sep 19, 2025

Uh oh!

volcano-sh-bot commented Sep 22, 2025

Uh oh!

wuyueandrew commented Sep 22, 2025

Uh oh!

Monokaix commented Sep 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

volcano-sh-bot commented Sep 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JesseStutler Sep 27, 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!

Choose a reason for hiding this comment

Uh oh!

JesseStutler commented Sep 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JesseStutler Sep 27, 2025 •

edited

Loading