Fix queue not counting volcano.sh/vgpu-memory and volcano.sh/vgpu-number properly #4520

archlitchi · 2025-08-08T03:38:22Z

/BUG

related issue: #4168

gemini-code-assist

Summary of Changes

Hello @archlitchi, 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!

I've addressed a bug where the Volcano scheduler's queue was not accurately counting volcano.sh/vgpu-memory and volcano.sh/vgpu-number resources. This fix ensures that vGPU resources are properly accounted for when updating queue allocated resources, resolving the related issue #4168.

Highlights

Enhanced Resource Accounting: I've introduced an AddQueueResource method to the Devices interface and implemented it for vGPU devices. This method calculates the actual vGPU memory and core usage from pod annotations, ensuring these resources are correctly added to the queue's allocated resources.
Dynamic Resource Name Resolution: I've refactored the vGPU utility functions to use a new getConfig() helper. This change replaces hardcoded vGPU resource names with dynamically retrieved ones, improving flexibility and maintainability.
Corrected Queue Status Updates: The updateQueueStatus function in the scheduler framework now leverages the new AddQueueResource method. This ensures that when a pod's resources are added to the queue's allocated pool, vGPU memory and number are accurately reflected.

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 or fill out our survey to provide feedback.

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 fixes an issue with queue resource accounting for vGPU memory and number. It introduces a new interface method AddQueueResource to calculate the actual resources consumed by a pod with vGPU, and updates the queue status accordingly. The changes look mostly correct, but I've identified a potential issue with resource unit conversion that could lead to incorrect accounting, a code duplication issue, and a minor performance improvement suggestion.

pkg/scheduler/api/devices/nvidia/vgpu/device_info.go

pkg/scheduler/api/devices/nvidia/vgpu/utils.go

archlitchi · 2025-08-08T06:12:30Z

CC @JesseStutler

JesseStutler · 2025-08-08T06:19:07Z

/kind bug

JesseStutler · 2025-08-11T01:50:04Z

/priority high
/cc

archlitchi · 2025-08-11T02:19:51Z

The result can be seen as follows:

JesseStutler · 2025-08-11T03:08:16Z

pkg/scheduler/framework/session.go

 		for status, tasks := range job.TaskStatusIndex {
 			if api.AllocatedStatus(status) {
 				for _, task := range tasks {
+					node, ok1 := ssn.Nodes[task.NodeName]


Should not name variable as ok1. ok1 --> ok

JesseStutler · 2025-08-11T03:13:50Z

pkg/scheduler/framework/session.go

+					taskReq := task.Resreq
+					if ok1 {
+						for _, sharedDevices := range node.Others {
+							if sharedDevices.(api.Devices).HasDeviceRequest(task.Pod) {


I'm wondering that why Others field in NodeInfo is a map[string]interface{} type rather than map[string]Devices ? Will Others have other types of interface in the future? We cannot control other contributors who may put other interface implementations into Others. If other interfaces do not have HasDeviceRequest methods, there will be problems here.

haha, the nodeInfo.Others field exists before i implement deviceshare API. It's william-wang's idea to put devices related logics here, so we don't need to change the node_info struct

If we don't modify NodeInfo, there should be an assertion here

JesseStutler · 2025-08-11T03:19:41Z

pkg/scheduler/api/devices/nvidia/vgpu/device_info.go

+		for _, deviceused := range val {
+			for _, gsdevice := range gs.Device {
+				if strings.Contains(deviceused.UUID, gsdevice.UUID) {
+					res[getConfig().ResourceMemoryName] += float64(deviceused.Usedmem * 1000)


Sorry not so familiar with the previous codes, where is VolcanoVGPUNumber calculated? Can it be correctly recorded in queue allocated field?

volcanoVGPUNumber doesn't need to be calculated here, it is correctly counted in default behavior

Monokaix · 2025-08-19T06:58:27Z

pkg/scheduler/api/devices/nvidia/vgpu/device_info.go

+func (gs *GPUDevices) AddQueueResource(pod *v1.Pod) map[string]float64 {
+	klog.Infoln("AddQueueResource:Name=", pod.Name)
+	res := map[string]float64{}
+	if gs == nil {


Better move this to top.

Monokaix · 2025-08-19T06:59:48Z

pkg/scheduler/api/devices/nvidia/vgpu/device_info.go

+}
+
+func (gs *GPUDevices) AddQueueResource(pod *v1.Pod) map[string]float64 {
+	klog.Infoln("AddQueueResource:Name=", pod.Name)


klog level 3？

Monokaix · 2025-08-19T07:16:02Z

Can Allocateable and Enqueueable be aware of vgpu resources? If queue.status has vgpu resources, we should also check the vgpu resource quota when allocate.

archlitchi · 2025-08-19T13:24:37Z

Can Allocateable and Enqueueable be aware of vgpu resources? If queue.status has vgpu resources, we should also check the vgpu resource quota when allocate.

currently no, because you can't know exactly how much device memory a task uses before scheduling, for example, i task allocate volcano.sh/vgpu-number:1, it allocates a whole card, but different card has different device memory, which can't be decided before scheduling.

Monokaix · 2025-08-20T01:11:42Z

Can Allocateable and Enqueueable be aware of vgpu resources? If queue.status has vgpu resources, we should also check the vgpu resource quota when allocate.

currently no, because you can't know exactly how much device memory a task uses before scheduling, for example, i task allocate volcano.sh/vgpu-number:1, it allocates a whole card, but different card has different device memory, which can't be decided before scheduling.

deviceshare plugin can be aware of that, right? device memory quota check in queue is just like task's memory request check in resource filter.

archlitchi · 2025-08-20T01:54:08Z

Can Allocateable and Enqueueable be aware of vgpu resources? If queue.status has vgpu resources, we should also check the vgpu resource quota when allocate.

currently no, because you can't know exactly how much device memory a task uses before scheduling, for example, i task allocate volcano.sh/vgpu-number:1, it allocates a whole card, but different card has different device memory, which can't be decided before scheduling.

deviceshare plugin can be aware of that, right? device memory quota check in queue is just like task's memory request check in resource filter.

yes, but that's in the process of scheduling, so that task is still in the wrong state, because such task shouldn't be enqueue

Monokaix · 2025-08-20T03:23:22Z

Can Allocateable and Enqueueable be aware of vgpu resources? If queue.status has vgpu resources, we should also check the vgpu resource quota when allocate.

currently no, because you can't know exactly how much device memory a task uses before scheduling, for example, i task allocate volcano.sh/vgpu-number:1, it allocates a whole card, but different card has different device memory, which can't be decided before scheduling.

deviceshare plugin can be aware of that, right? device memory quota check in queue is just like task's memory request check in resource filter.

yes, but that's in the process of scheduling, so that task is still in the wrong state, because such task shouldn't be enqueue

But enqueueable is also a callback, and plugin can also implement it.

archlitchi · 2025-08-20T03:41:52Z

@Monokaix The problem is not we can't implement, it is we don't know the exact device memory a certain task uses before scheduling

Monokaix · 2025-09-10T06:34:33Z

pkg/scheduler/framework/session.go

+					node, ok := ssn.Nodes[task.NodeName]
+					taskReq := task.Resreq
+					if ok {
+						for _, sharedDevices := range node.Others {


sharedDevices is nil when node has shared gpu resource, will it panic here?

In current version , either node.Others is empty and this for loop is not entered, or sharedDevices is not nil because we have initialized it. so panic won't happen

Monokaix · 2025-09-10T06:34:58Z

pkg/scheduler/framework/session.go

 		for status, tasks := range job.TaskStatusIndex {
 			if api.AllocatedStatus(status) {
 				for _, task := range tasks {
+					node, ok := ssn.Nodes[task.NodeName]


We can wrap this as a func.

Signed-off-by: limengxuan <mengxuan.li@dynamia.ai>

Monokaix · 2025-09-11T02:43:13Z

/approve

volcano-sh-bot · 2025-09-11T02:43:21Z

[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:

~~pkg/scheduler/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

/lgtm
Thanks

volcano-sh-bot requested review from hwdef and k82cn August 8, 2025 03:38

gemini-code-assist bot reviewed Aug 8, 2025

View reviewed changes

volcano-sh-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 8, 2025

gemini-code-assist bot reviewed Aug 8, 2025

View reviewed changes

pkg/scheduler/api/devices/nvidia/vgpu/device_info.go Show resolved Hide resolved

pkg/scheduler/api/devices/nvidia/vgpu/device_info.go Show resolved Hide resolved

pkg/scheduler/api/devices/nvidia/vgpu/utils.go Show resolved Hide resolved

archlitchi force-pushed the master branch from 12c9c99 to c7b2a7b Compare August 8, 2025 04:05

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

volcano-sh-bot requested a review from JesseStutler August 11, 2025 01:50

volcano-sh-bot added the priority/high label Aug 11, 2025

JesseStutler reviewed Aug 11, 2025

View reviewed changes

archlitchi force-pushed the master branch from 2ddcd89 to bde2534 Compare August 19, 2025 02:15

Monokaix reviewed Aug 19, 2025

View reviewed changes

archlitchi force-pushed the master branch from e001648 to f835f4d Compare August 19, 2025 13:14

Monokaix reviewed Sep 10, 2025

View reviewed changes

update queue

226cad0

Signed-off-by: limengxuan <mengxuan.li@dynamia.ai>

archlitchi force-pushed the master branch from 120d6d1 to 226cad0 Compare September 10, 2025 07:41

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 Sep 10, 2025

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

JesseStutler reviewed Sep 11, 2025

View reviewed changes

volcano-sh-bot assigned JesseStutler Sep 11, 2025

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

volcano-sh-bot merged commit 5007c6b into volcano-sh:master Sep 11, 2025
20 checks passed

This was referenced Sep 22, 2025

[BUG] When use volcano-vgpu-device-plugin with volcano queue, allocated vgpu-memory and vgpu-cores cannot report normally Project-HAMi/volcano-vgpu-device-plugin#80

Closed

volcano vgpu插件怎么使用队列 Project-HAMi/volcano-vgpu-device-plugin#54

Closed

Fix queue not counting volcano.sh/vgpu-memory and volcano.sh/vgpu-number properly #4520

Fix queue not counting volcano.sh/vgpu-memory and volcano.sh/vgpu-number properly #4520

Uh oh!

Conversation

archlitchi commented Aug 8, 2025

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!

Uh oh!

Uh oh!

archlitchi commented Aug 8, 2025

Uh oh!

JesseStutler commented Aug 8, 2025

Uh oh!

JesseStutler commented Aug 11, 2025

Uh oh!

archlitchi commented Aug 11, 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!

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 Aug 19, 2025

Uh oh!

archlitchi commented Aug 19, 2025

Uh oh!

Monokaix commented Aug 20, 2025

Uh oh!

archlitchi commented Aug 20, 2025

Uh oh!

Monokaix commented Aug 20, 2025

Uh oh!

archlitchi commented Aug 20, 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!

Monokaix commented Sep 11, 2025

Uh oh!

volcano-sh-bot commented Sep 11, 2025

Uh oh!

JesseStutler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants