Skip to content

fix: reject empty parent in serving group names#1041

Open
pm-ju wants to merge 1 commit into
volcano-sh:mainfrom
pm-ju:fix-servinggroup-ordinal-parsing
Open

fix: reject empty parent in serving group names#1041
pm-ju wants to merge 1 commit into
volcano-sh:mainfrom
pm-ju:fix-servinggroup-ordinal-parsing

Conversation

@pm-ju
Copy link
Copy Markdown
Contributor

@pm-ju pm-ju commented May 13, 2026

/kind bug

What this PR does / why we need it:

GetParentNameAndOrdinal currently accepts names like -1 and returns an empty parent with ordinal 1.

This tightens the generated ServingGroup/role name parser so it only accepts names with a non-empty parent before the final numeric ordinal, for example vllm-sample-0.

This also adds unit coverage for valid generated names and malformed inputs.

Verification:

go test ./pkg/model-serving-controller/utils
image

Copilot AI review requested due to automatic review settings May 13, 2026 16:20
@volcano-sh-bot
Copy link
Copy Markdown
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 assign git-malu for approval. 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

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

Tightens the servingGroupRegex so names with an empty parent (e.g. -1) are rejected, and adds unit tests covering the parser.

Changes:

  • Anchor the regex with ^ and require a non-empty parent (.+ instead of .*).
  • Minor docstring grammar fix.
  • Add TestGetParentNameAndOrdinal covering valid and malformed inputs.

Reviewed changes

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

File Description
pkg/model-serving-controller/utils/utils.go Updated regex to require non-empty parent and anchored start.
pkg/model-serving-controller/utils/utils_test.go Added unit tests for GetParentNameAndOrdinal.

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

Copy link
Copy Markdown
Contributor

@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 servingGroupRegex in utils.go to include a start-of-string anchor and require a non-empty parent name. It also adds a comprehensive test suite TestGetParentNameAndOrdinal in utils_test.go to verify the regex behavior across various edge cases. I have no feedback to provide.

Signed-off-by: pm-ju <pmdevops29@gmail.com>
@hzxuzhonghu hzxuzhonghu force-pushed the fix-servinggroup-ordinal-parsing branch from 76bf296 to ea301fd Compare May 15, 2026 09:38
Copy link
Copy Markdown
Contributor

@FAUST-BENCHOU FAUST-BENCHOU left a comment

Choose a reason for hiding this comment

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

low priority i think

func GenerateServingGroupName(miName string, idx int) string {
return miName + "-" + strconv.Itoa(idx)
}

@pm-ju
Copy link
Copy Markdown
Contributor Author

pm-ju commented May 16, 2026

low priority i think

func GenerateServingGroupName(miName string, idx int) string {
return miName + "-" + strconv.Itoa(idx)
}

Fair, agreed this is a small hardening fix rather than a high-priority issue. I opened it because the parser currently accepts malformed generated names like -1, but I’m fine keeping this low priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants