Skip to content

Conversation

@LiZhenCheng9527
Copy link
Contributor

@LiZhenCheng9527 LiZhenCheng9527 commented Jan 31, 2026

What type of PR is this?
/kind documentation

What this PR does / why we need it:

The following changes have been made to the podGroup Crd for volcano:

type SubGroupPolicySpec struct {
        ..... exist code 
	// Name specifies the name of SubGroupPolicy
	Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"`

	// MatchPolicy defines matching strategies for different groups, where pods with the same labelKey value are grouped together.
	// The LabelKey in the list is unique.
	MatchPolicy []MatchPolicySpec `json:"matchPolicy,omitempty" protobuf:"bytes,2,opt,name=matchPolicy"`
        ..... exist code

to:

type SubGroupPolicySpec struct {
        ..... exist code
	// LabelSelector is used to find matching pods.
	// Pods that match this label selector are counted to determine the number of pods
	// in their corresponding topology domain.
	// +optional
	LabelSelector *metav1.LabelSelector `json:"labelSelector,omitempty" protobuf:"bytes,5,opt,name=labelSelector"`

	// MatchLabelKeys: A label-based grouping configuration field for Pods, defining filtering rules for grouping label keys
	// Core function: Refine grouping of Pods that meet LabelSelector criteria by label attributes, with the following rules and constraints:
	// 1. Scope: Only applies to Pods matching the predefined LabelSelector
	// 2. Grouping rule: Specify one or more label keys; Pods containing the target label keys with exactly the same corresponding label values are grouped together
	// 3. Policy constraint: Pods in the same group follow a unified NetworkTopology policy to achieve group-level network behavior governance
	// +listType=atomic
	// +optional
	MatchLabelKeys []string `json:"matchLabelKeys,omitempty" protobuf:"bytes,6,rep,name=matchLabelKeys"`
         ..... exist code
}

Modify the proposal for NetworkTopology to prevent the dissemination of inaccurate information.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


Copilot AI review requested due to automatic review settings January 31, 2026 08:35
@volcano-sh-bot volcano-sh-bot added the kind/documentation Improvements or additions to documentation label Jan 31, 2026
@volcano-sh-bot
Copy link
Contributor

[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 ask for approval from lizhencheng9527. 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:

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
Copy link

Summary of Changes

Hello @LiZhenCheng9527, I'm Gemini Code Assist1! 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 refines the podGroup Custom Resource Definition (CRD) in Volcano by enhancing the pod grouping mechanism for network topology policies. It replaces the previous matchPolicy with a more expressive combination of labelSelector and matchLabelKeys, allowing users to define precise criteria for selecting and grouping pods. Concurrently, the network-topology proposal documentation has been updated to align with these CRD changes, ensuring consistency and clarity.

Highlights

  • SubGroupPolicySpec CRD Update: The SubGroupPolicySpec within the podGroup Custom Resource Definition (CRD) has been updated to introduce more granular control over pod grouping.
  • matchPolicy Replacement: The matchPolicy field has been replaced by two new fields: labelSelector and matchLabelKeys. labelSelector is used for initial pod matching, while matchLabelKeys refines grouping based on specific label attributes.
  • Documentation Alignment: The network-topology.md proposal document has been modified to reflect these changes, ensuring the documentation accurately represents the updated podGroup CRD and prevents the dissemination of outdated information.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

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.

Footnotes

  1. 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.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the network topology proposal documentation to reflect changes in the podGroup CRD, specifically replacing matchPolicy with labelSelector and matchLabelKeys. The changes are generally correct, but I've found a few issues in the YAML examples and descriptive text that need to be addressed for correctness and consistency. I've left specific comments with suggestions. Also, there's a small typo in the pull request title ('uddate').

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the NetworkTopology proposal documentation to align PodGroup subGroupPolicy examples and explanations with the updated Volcano PodGroup CRD fields (labelSelector + matchLabelKeys) and avoid publishing outdated API usage.

Changes:

  • Replaced matchPolicy.labelKey in YAML examples with labelSelector and matchLabelKeys.
  • Updated field name mention in the explanatory bullet list (matchPolicymatchLabelKeys).
  • Adjusted the second YAML example similarly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 129 to 132
modelserving.volcano.sh/name: sample
modelserving.volcano.sh/role: prefill
matchLabelKeys:
modelserving.volcano.sh/role-id
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The YAML example doesn’t match the metav1.LabelSelector structure: labelSelector should use matchLabels: (or matchExpressions:) rather than putting label keys directly under labelSelector. Also matchLabelKeys is an array but the example shows a single scalar value; it should be a YAML list (e.g., - "modelserving.volcano.sh/role-id").

Suggested change
modelserving.volcano.sh/name: sample
modelserving.volcano.sh/role: prefill
matchLabelKeys:
modelserving.volcano.sh/role-id
matchLabels:
modelserving.volcano.sh/name: sample
modelserving.volcano.sh/role: prefill
matchLabelKeys:
- "modelserving.volcano.sh/role-id"

Copilot uses AI. Check for mistakes.
Comment on lines 176 to 179
modelserving.volcano.sh/name: sample
modelserving.volcano.sh/role: prefill
matchLabelKeys:
modelserving.volcano.sh/role-id
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue in this second example: labelSelector should be expressed as a Kubernetes LabelSelector (nested matchLabels:/matchExpressions:), and matchLabelKeys should be a YAML list (array of strings) instead of a single value.

Suggested change
modelserving.volcano.sh/name: sample
modelserving.volcano.sh/role: prefill
matchLabelKeys:
modelserving.volcano.sh/role-id
matchLabels:
modelserving.volcano.sh/name: sample
modelserving.volcano.sh/role: prefill
matchLabelKeys:
- modelserving.volcano.sh/role-id

Copilot uses AI. Check for mistakes.
Comment on lines 142 to 145
- `matchLabelKeys`: The label key used to match pods.
- `networkTopology`: The network topology of a subGroup.

This YAML specifies that the PodGroup requires six pods to be deployed together, and all six pods must be scheduled on nodes with a maximum network topology distance of 2. Among these, the six pods are divided into two subGroups of three based on the value of the `volcano.sh/task-subgroup-id` label, and each group must be strictly scheduled on nodes with a network topology distance of 1.
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explanatory bullets and paragraph below the YAML are now out of sync with the updated fields: it doesn’t mention labelSelector at all, describes matchLabelKeys as a single key (it’s a list), and still references grouping by volcano.sh/task-subgroup-id even though matchPolicy/labelKey were removed. Please update the description to reflect selection via labelSelector and grouping via matchLabelKeys.

Suggested change
- `matchLabelKeys`: The label key used to match pods.
- `networkTopology`: The network topology of a subGroup.
This YAML specifies that the PodGroup requires six pods to be deployed together, and all six pods must be scheduled on nodes with a maximum network topology distance of 2. Among these, the six pods are divided into two subGroups of three based on the value of the `volcano.sh/task-subgroup-id` label, and each group must be strictly scheduled on nodes with a network topology distance of 1.
- `labelSelector`: The labels used to select which pods are managed by this `subGroupPolicy`.
- `matchLabelKeys`: The list of label keys whose values are used to group the selected pods into subGroups.
- `networkTopology`: The network topology constraints applied to each subGroup.
This YAML specifies that the PodGroup requires six pods to be deployed together, and all six pods must be scheduled on nodes with a maximum network topology distance of 2. Among these, only pods matching the `labelSelector` (for example, pods with `modelserving.volcano.sh/name=sample` and `modelserving.volcano.sh/role=prefill`) are eligible for subGroup scheduling. The selected pods are then divided into two subGroups of three based on the value of the `modelserving.volcano.sh/role-id` label (listed in `matchLabelKeys`), and each subGroup must be strictly scheduled on nodes with a network topology distance of 1.

Copilot uses AI. Check for mistakes.
Signed-off-by: LiZhenCheng9527 <[email protected]>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- `subGroupSize`: The number of pods in a subGroup.
- `minSubGroups`: The minimum replicas of subGroups.
- `matchPolicy`: The label key used to match pods.
- `matchLabelKeys`: The label key used to match pods.
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The subGroupPolicy field list is missing labelSelector, even though the YAML and API description introduce it. Add a bullet explaining that labelSelector selects which pods are eligible for subgrouping, and clarify how it interacts with matchLabelKeys.

Suggested change
- `matchLabelKeys`: The label key used to match pods.
- `labelSelector`: The label selector that restricts which pods in the PodGroup are eligible to be considered for this `subGroupPolicy`; only pods whose labels match this selector will participate in subgrouping.
- `matchLabelKeys`: The label keys whose values are used to partition the pods selected by `labelSelector` into subGroups; pods that match the selector and share the same values for all listed keys belong to the same subGroup.

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good, need to clarify the matchLabelKeys

- `subGroupSize`: The number of pods in a subGroup.
- `minSubGroups`: The minimum replicas of subGroups.
- `matchPolicy`: The label key used to match pods.
- `matchLabelKeys`: The label key used to match pods.
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matchLabelKeys is documented as a single "label key" used to match pods, but the field is a list and (per the PR description) is meant to further group pods after applying labelSelector. Reword this bullet to reflect that it is an ordered/list of label keys used for grouping pods that already match labelSelector.

Suggested change
- `matchLabelKeys`: The label key used to match pods.
- `matchLabelKeys`: Ordered list of label keys used to group pods that already match `labelSelector`.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/documentation Improvements or additions to documentation size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants