Set root capability only when user not set it #4354

houyuting · 2025-06-09T08:58:24Z

Result:

UT- when I use old code, I got:

UT- when I use new code, I got:

JesseStutler · 2025-06-10T01:53:50Z

pkg/scheduler/plugins/capacity/capacity.go

@@ -598,7 +598,9 @@ func (cp *capacityPlugin) buildHierarchicalQueueAttrs(ssn *framework.Session) bo

 	// init root queue realCapability/capability/deserved as cp.totalResource
 	rootQueueAttr := cp.queueOpts[api.QueueID(cp.rootQueue)]
-	rootQueueAttr.capability = cp.totalResource
+	if rootQueueAttr.capability == nil || rootQueueAttr.capability.IsEmpty() {


I think rootQueueAttr.capability.IsEmpty() is enough because the capability of root queue will be initialized in newQueueAttr, and it won't be nil

Thank you for your suggestion, done.

JesseStutler

Please sqush to one commit, thanks :)

JesseStutler · 2025-06-10T03:06:45Z

@houyuting Please squash to one commit :)

JesseStutler · 2025-06-10T06:48:21Z

/ok-to-test

JesseStutler · 2025-06-10T07:24:37Z

/lgtm
Please also take a look @hwdef @lowang-bh @Monokaix I think it's reasonable when users want to pre-set the capability for the root queue

Xu-Wentao · 2025-06-12T07:53:08Z

pkg/scheduler/plugins/capacity/capacity.go

 	rootQueueAttr.realCapability = cp.totalResource
 	rootQueueAttr.deserved = cp.totalResource


should we handle realCapability and deseverd too? if realCapability and deseverd not empty and less than cp.totalResource may occurs error ?

I think realCapability and deseverd == cp.totalResource is suitable，because we need at least a field that identifies how many resources are actually available in the cluster and I haven't thought of any scenarios where these two fields need to be customized. Or do you have some ideas? We can discuss it together.

I think realCapability and deseverd == cp.totalResource is suitable，because we need at least a field that identifies how many resources are actually available in the cluster and I haven't thought of any scenarios where these two fields need to be customized. Or do you have some ideas? We can discuss it together.

yes, i think u r right, the realcapacity and deseverd will be caculated in checkHierarchicalQueue.

seems that if users previously set deserved fields of each sub queue, and then deserved field check may also fail when cluster resources reduced.

Yes, you are right, done, please review again, thanks.

Monokaix · 2025-06-18T02:54:19Z

That's a good catch, and @Xu-Wentao also has the same requirement, and I think that we can add root queue at helm chart, then user can set the spec of root queue when install volcano, and we can remove root queue creation operation in volcano scheduler.

houyuting · 2025-06-20T03:02:50Z

@Monokaix Good idea and we can open another feature issue and track it. If you have time，please give me approve, thanks.

JesseStutler · 2025-06-23T01:40:59Z

@Monokaix Good idea and we can open another feature issue and track it. If you have time，please give me approve, thanks.

OK @houyuting Would you like to raise this issue? Otherwise we may miss it :)

Monokaix · 2025-07-01T06:08:48Z

Please sign off your commit with git commit -s.

Monokaix · 2025-07-01T06:25:57Z

The root queue will also be updated when close session, so it's not enough to just modify plugin.

houyuting · 2025-07-01T06:33:36Z

The root queue will also be updated when close session, so it's not enough to just modify plugin.

got it

JesseStutler · 2025-07-14T01:49:47Z

The UT failed, please fix it thanks

JesseStutler · 2025-07-14T01:49:59Z

/gemini review

gemini-code-assist

Code Review

This pull request correctly implements the intended change to only set the root queue's capability and deserved resources if they are not already defined by the user. The changes are logical and the new test cases are a good addition.

I've pointed out a couple of areas for improvement. One is a minor comment update for maintainability. The other is a more significant concern regarding the removal of logic that caps a queue's deserved resources by its realCapability in a hierarchical setup. This could lead to inconsistencies and scheduling issues, so it's worth a second look. Overall, great work on addressing the issue.

gemini-code-assist · 2025-07-14T01:51:35Z

pkg/scheduler/plugins/capacity/capacity.go

@@ -598,9 +599,13 @@ func (cp *capacityPlugin) buildHierarchicalQueueAttrs(ssn *framework.Session) bo

 	// init root queue realCapability/capability/deserved as cp.totalResource


The comment on this line is now outdated due to the changes below. It states that capability and deserved are initialized as cp.totalResource, but this is now done conditionally. Please update the comment to reflect the new logic for better code maintainability.

Suggested change

// init root queue realCapability/capability/deserved as cp.totalResource

// init root queue: realCapability is set to total resource, and capability/deserved are also set if empty.

gemini-code-assist · 2025-07-14T01:51:35Z

pkg/scheduler/plugins/capacity/capacity_test.go

@@ -370,6 +370,7 @@ func Test_capacityPlugin_OnSessionOpenWithHierarchy(t *testing.T) {
 	pg1 := util.BuildPodGroup("pg1", "ns1", "q11", 1, nil, schedulingv1beta1.PodGroupInqueue)
 	// queue
 	root := buildQueueWithParents("root", "", nil, nil)
+	root1 := buildQueueWithParents("root", "", nil, api.BuildResourceList("16", "16Gi"))


It seems like root1 is overriding root queue, but root1 is not used anywhere in the test. Is this intentional?

gemini-code-assist · 2025-07-14T01:51:36Z

pkg/scheduler/plugins/capacity/capacity_test.go

@@ -468,6 +469,8 @@
 	pg13 := util.BuildPodGroup("pg13", "ns1", "q91", 1, nil, schedulingv1beta1.PodGroupInqueue)
 	// pod
 	p13 := util.BuildPod("ns1", "p13", "", corev1.PodPending, api.BuildResourceList("2", "2Gi", []api.ScalarResource{{Name: "nvidia.com/gpu", Value: "1"}}...), "pg13", make(map[string]string), make(map[string]string))
+  // queue
+	queue10 := buildQueueWithParents("q10", "root", nil, api.BuildResourceList("10", "4Gi", []api.ScalarResource{}...))


It seems like queue10 is not used anywhere in the test. Is this intentional?

JesseStutler

Please check my latest review comments and gemini's comments, thanks

JesseStutler · 2025-07-22T01:37:42Z

go.sum

I think go.mod and go.sum should not change in your pr

volcano-sh-bot · 2025-07-22T02:36:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign k82cn
You can assign the PR to them by writing /assign @k82cn in a comment when ready.

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

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

Signed-off-by: leona.hou <[email protected]>

Monokaix · 2025-07-22T03:40:25Z

pkg/scheduler/plugins/capacity/capacity.go

@@ -774,8 +780,14 @@ func (cp *capacityPlugin) checkHierarchicalQueue(attr *queueAttr) error {
 	}

 	if attr.name == cp.rootQueue {
-		attr.guarantee = totalGuarantee
-		cp.totalGuarantee = totalGuarantee
+		if attr.guarantee.IsEmpty() {


The root queue is created by yourself or auto created?

auto created

Then its guarantee and capability fields will be updated once created, so seems this can not take effect.

JesseStutler · 2025-07-22T06:09:26Z

/lgtm

volcano-sh-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 9, 2025

volcano-sh-bot requested review from shinytang6 and william-wang June 9, 2025 08:58

volcano-sh-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 9, 2025

houyuting force-pushed the fix_4350 branch from 9768573 to 23975ef Compare June 9, 2025 09:01

volcano-sh-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 9, 2025

houyuting marked this pull request as ready for review June 9, 2025 09:23

volcano-sh-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 9, 2025

JesseStutler reviewed Jun 10, 2025

View reviewed changes

houyuting requested a review from JesseStutler June 10, 2025 02:45

houyuting force-pushed the fix_4350 branch from 6f4407d to 5c85fbc Compare June 10, 2025 06:25

volcano-sh-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 10, 2025

volcano-sh-bot assigned JesseStutler Jun 10, 2025

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

Xu-Wentao reviewed Jun 12, 2025

View reviewed changes

volcano-sh-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 30, 2025

houyuting requested a review from Monokaix July 1, 2025 02:44

houyuting force-pushed the fix_4350 branch 2 times, most recently from 93f4a7b to e9e096a Compare July 9, 2025 02:53

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

houyuting force-pushed the fix_4350 branch from e9e096a to 49daeb4 Compare July 10, 2025 02:44

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

gemini-code-assist bot reviewed Jul 14, 2025

View reviewed changes

JesseStutler reviewed Jul 14, 2025

View reviewed changes

houyuting force-pushed the fix_4350 branch from 21d416a to 7d9c78b Compare July 19, 2025 07:01

volcano-sh-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 21, 2025

JesseStutler reviewed Jul 22, 2025

View reviewed changes

volcano-sh-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 22, 2025

houyuting force-pushed the fix_4350 branch from 180d76e to edd44ec Compare July 22, 2025 02:37

set root capability only when user not set it

157b064

Signed-off-by: leona.hou <[email protected]>

houyuting force-pushed the fix_4350 branch from edd44ec to 157b064 Compare July 22, 2025 03:27

Monokaix reviewed Jul 22, 2025

View reviewed changes

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

		rootQueueAttr.realCapability = cp.totalResource
		rootQueueAttr.deserved = cp.totalResource

		@@ -598,9 +599,13 @@ func (cp capacityPlugin) buildHierarchicalQueueAttrs(ssn framework.Session) bo

		// init root queue realCapability/capability/deserved as cp.totalResource

	// init root queue realCapability/capability/deserved as cp.totalResource
	// init root queue: realCapability is set to total resource, and capability/deserved are also set if empty.

Set root capability only when user not set it #4354

Are you sure you want to change the base?

Set root capability only when user not set it #4354

Uh oh!

Conversation

houyuting commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JesseStutler left a comment

Choose a reason for hiding this comment

Uh oh!

JesseStutler commented Jun 10, 2025

Uh oh!

JesseStutler commented Jun 10, 2025

Uh oh!

JesseStutler commented Jun 10, 2025

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!

Monokaix commented Jun 18, 2025

Uh oh!

houyuting commented Jun 20, 2025

Uh oh!

JesseStutler commented Jun 23, 2025

Uh oh!

Monokaix commented Jul 1, 2025

Uh oh!

Monokaix commented Jul 1, 2025

Uh oh!

houyuting commented Jul 1, 2025

Uh oh!

JesseStutler commented Jul 14, 2025

Uh oh!

JesseStutler commented Jul 14, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

JesseStutler left a comment

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!

volcano-sh-bot commented Jul 22, 2025

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 Jul 22, 2025

Uh oh!

Uh oh!

houyuting commented Jun 9, 2025 •

edited

Loading