💬 ชาวเน็ตแห่แชร์: volcano sh/volcano/pull/

wangyang0616 · 2025-12-08T08:37:46Z

Fixes #4781
Related Apis #202

After optimizing SubGroupPolicy, update the grouping configuration and label configuration of Volcano Job during Pod creation to align with the policy;
Optimize the PodGroup grouping logic to match the updated SubGroupPolicy rules.

gemini-code-assist · 2025-12-08T08:38:17Z

Summary of Changes

Hello @wangyang0616, 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 significantly advances Volcano's Gang Scheduling capabilities by introducing a more sophisticated SubGroupPolicy mechanism. It transitions from basic label matching to a powerful LabelSelector and MatchLabelKeys system, enabling users to define intricate pod grouping rules for optimized resource allocation and network topology management. The changes encompass updates to API definitions, internal data structures, and controller logic, ensuring robust and seamless operation of the new subgroup-based scheduling. Additionally, it enhances scheduling flexibility by introducing a configurable dequeue strategy for queues.

Highlights

Enhanced SubGroupPolicy API: The PodGroup Custom Resource Definition (CRD) has been updated to replace the simpler MatchPolicy with a more robust LabelSelector and MatchLabelKeys. This provides greater flexibility and power in defining how pods are grouped for subgroup-based Gang Scheduling.
Optimized Pod Grouping Logic: The internal logic responsible for grouping pods within a PodGroup has been refactored to align with the new SubGroupPolicy definition. This ensures that network topology and Gang scheduling rules are applied correctly and efficiently based on the enhanced grouping criteria.
Updated Pod Labeling for Partitions: Pods are now consistently labeled with volcano.sh/task-name and volcano.sh/task-partition-id based on the PartitionPolicy and PartitionSize. This replaces older, less flexible labeling schemes and improves clarity and consistency in identifying pod partitions.
New Queue Dequeue Strategy: A dequeueStrategy field has been introduced to the Queue CRD, allowing users to specify either fifo (First-In, First-Out) or traverse for queue management. The default strategy is traverse, offering more control over how jobs are processed from the queue.
API Dependency Update: The project's dependency on volcano.sh/apis has been updated to incorporate the new API definitions related to SubGroupPolicy and Queue dequeue strategies, ensuring compatibility with the latest features.

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 significantly improves the SubGroupPolicy API by aligning it with standard Kubernetes practices using LabelSelector, which is a great enhancement. The refactoring of the test suite is also a major improvement in code quality and maintainability.

I've identified a few issues that need attention:

A critical issue with the go.mod file pointing to a personal fork, which must be addressed before merging.
A potential bug in getSubJobMatchValues where it returns an empty slice instead of nil for non-matching pods, which could break existing logic.
An inconsistency in the Clone method in job_info.go where Partitions are not correctly deep-copied.
A minor issue with a misleading comment in a test case.

Overall, this is a valuable contribution. Addressing these points will make it even better.

gemini-code-assist · 2025-12-08T08:41:05Z

+			name:          "partition size 0 (invalid), index 5",
+			partitionSize: 0,
+			index:         5,
+			expectedPart:  "-1", // Go returns 0 for division by zero


The comment // Go returns 0 for division by zero is incorrect. In Go, integer division by zero causes a runtime panic. The code in createJobPod correctly handles this edge case by checking if ts.PartitionPolicy.PartitionSize <= 0 and setting partitionID to -1, which is what this test case correctly asserts. The comment is misleading and should be removed or corrected to reflect the actual behavior.

Suggested change

expectedPart: "-1", // Go returns 0 for division by zero

expectedPart: "-1",

JesseStutler · 2025-12-11T06:16:38Z

+				MatchLabelKeys: []string{"any"},
+			},
+			pod: &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{}, // 无标签


Please delete the Chinese comment

JesseStutler · 2025-12-11T06:17:33Z

 						{
-							LabelKey: "key2",
+							Key:      "app",
+							Operator: "invalid", // 无效的操作符


Same as above. Please check all the Chinese comments if we don't need it

JesseStutler · 2025-12-11T06:25:15Z

Just want to verify that does this PR means that the pods must first match the labelSelector, and then be divided into different groups based on different keys? @wangyang0616

hzxuzhonghu

LGTM, not big problem, since this is mostly the logic updating subJob construction, while the scheduling has already been merged

hzxuzhonghu · 2025-12-11T06:23:45Z

 		job.Pods[key] = make(map[string]*v1.Pod, len(pods))
 		for pn, pod := range pods {
-			job.Pods[key][pn] = pod
+			clonedPod := pod.DeepCopy()


I donot see we mutate the pod. Deepcopy is a costy op, so i think maybe we can keep it as still

You're right. The change has been reverted.

wangyang0616 · 2025-12-11T07:38:48Z

Just want to verify that does this PR means that the pods must first match the labelSelector, and then be divided into different groups based on different keys? @wangyang0616

Yes, LabelSelector is an explicit condition that defines the valid scope for pod grouping. When the controller creates a PodGroup based on a VCJob, it automatically configures the LabelSelector information to match the corresponding tasks.

ouyangshengjia · 2025-12-11T09:51:32Z

-	return values
+
+	// Log when no matching rules are configured, using default group
+	klog.V(4).Infof("No MatchLabelKeys configured for policy, pod %s/%s uses default subjob group", pod.Namespace, pod.Name)


If a pod matches the LabelSelector but is missing some labels in MatchLabelKeys, then it will belongs to the default subjob, right?

I think it is better that if a SubGroupPolicy is configured with only LabelSelector and empty MatchLabelKeys, the pods matching the LabelSelector should be assigned to a separate SubJob.

For the definition of subgroup, it is recommended to use LabelSelector and matchLabelKey in combination, and express the subgroup logic through explicit declaration. This avoids excessive implicit default behaviors and improves maintainability.

wangyang0616 · 2025-12-13T02:04:33Z

/retest

Signed-off-by: wangyang0616 <wangyang8126@gmail.com>

… the YAML files Signed-off-by: wangyang0616 <wangyang8126@gmail.com>

Signed-off-by: wangyang0616 <wangyang8126@gmail.com>

hzxuzhonghu · 2025-12-15T11:12:48Z

/lgtm
/approve

volcano-sh-bot · 2025-12-15T11:12:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [hzxuzhonghu]

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

volcano-sh-bot requested review from hwdef, kingeasternsun and merryzhou December 8, 2025 08:37

volcano-sh-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 8, 2025

wangyang0616 changed the title ~~SubGroupPolicy enables optimized Kthena subgroup-based Gang Scheduling~~ [WIP]SubGroupPolicy enables optimized Kthena subgroup-based Gang Scheduling Dec 8, 2025

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 Dec 8, 2025

gemini-code-assist Bot reviewed Dec 8, 2025

View reviewed changes

wangyang0616 changed the title ~~[WIP]SubGroupPolicy enables optimized Kthena subgroup-based Gang Scheduling~~ SubGroupPolicy enables optimized Kthena subgroup-based Gang Scheduling Dec 9, 2025

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 Dec 9, 2025

wangyang0616 force-pushed the feature_subgroup_update branch 4 times, most recently from 30a1c2f to 87d0fd9 Compare December 11, 2025 02:52

JesseStutler reviewed Dec 11, 2025

View reviewed changes

hzxuzhonghu reviewed Dec 11, 2025

View reviewed changes

ouyangshengjia reviewed Dec 11, 2025

View reviewed changes

wangyang0616 added 5 commits December 13, 2025 15:18

Adapt volcano-controller module to SubGroupPolicy field updates

83a4868

Signed-off-by: wangyang0616 <wangyang8126@gmail.com>

Add UT for volcano-controller changes related to SubGroupPolicy

c40a3cc

Signed-off-by: wangyang0616 <wangyang8126@gmail.com>

Adapt volcano-scheduler module to SubGroupPolicy field updates

b0b2bc1

Signed-off-by: wangyang0616 <wangyang8126@gmail.com>

Add UT for subJobInfo of scheduler changes related to SubGroupPolicy

a3a43e9

Signed-off-by: wangyang0616 <wangyang8126@gmail.com>

Update the descriptions in the design document and the information in…

94ba664

… the YAML files Signed-off-by: wangyang0616 <wangyang8126@gmail.com>

wangyang0616 force-pushed the feature_subgroup_update branch from d4e39ca to 44f392e Compare December 13, 2025 07:19

Update invalid clone information to improve runtime efficiency

08a9b37

Signed-off-by: wangyang0616 <wangyang8126@gmail.com>

wangyang0616 force-pushed the feature_subgroup_update branch from 44f392e to 08a9b37 Compare December 15, 2025 08:20

volcano-sh-bot assigned hzxuzhonghu Dec 15, 2025

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

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

volcano-sh-bot merged commit 1be4c9d into volcano-sh:master Dec 15, 2025
22 checks passed

wangyang0616 mentioned this pull request Dec 16, 2025

Support network topology constrain with highestTierName #4795

Merged

	expectedPart: "-1", // Go returns 0 for division by zero
	expectedPart: "-1",

Uh oh!

Conversation

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

Uh oh!

Uh oh!

gemini-code-assist Bot commented Dec 8, 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!

Uh oh!

gemini-code-assist Bot Dec 8, 2025

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!

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

Uh oh!

Uh oh!

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangyang0616 Dec 11, 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!

wangyang0616 commented Dec 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouyangshengjia Dec 11, 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!

wangyang0616 commented Dec 13, 2025

Uh oh!

hzxuzhonghu commented Dec 15, 2025

Uh oh!

volcano-sh-bot commented Dec 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wangyang0616 commented Dec 8, 2025 •

edited

Loading

JesseStutler commented Dec 11, 2025 •

edited

Loading

wangyang0616 Dec 11, 2025 •

edited

Loading

ouyangshengjia Dec 11, 2025 •

edited

Loading