Refactor role validation logic in validator.go#879
Refactor role validation logic in validator.go#879nalan2012 wants to merge 4 commits intovolcano-sh:mainfrom
Conversation
Signed-off-by: shuguansheng <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors validateGangPolicy role lookup to avoid repeatedly scanning roles when validating minRoleReplicas, aiming to improve efficiency and simplify logic.
Changes:
- Build a role lookup map from
ms.Spec.Template.Roles. - Replace nested role-search loop with direct map lookup.
- Keep existing validations for unknown role names,
minReplicas > replicas, andminReplicas < 0.
Comments suppressed due to low confidence (1)
pkg/model-serving-controller/webhook/validator.go:1
roleis declared in theifinitializer, so its scope is limited to theifstatement; it is not accessible on lines 233–235. This will not compile. Fix by retrievingrolein a separate statement before theif !okcheck (e.g.,role, ok := roleMap[roleName]thenif !ok { ... continue }).
/*
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| roleMap := make(map[string]*workloadv1alpha1.Role) | ||
| for _, role := range ms.Spec.Template.Roles { | ||
| roleNames[role.Name] = true | ||
| roleMap[role.Name] = role | ||
| } |
There was a problem hiding this comment.
This assumes ms.Spec.Template.Roles iterates values that are *workloadv1alpha1.Role. If Roles is a slice of structs (common in Go APIs), this assignment won’t compile and/or may require taking addresses correctly (e.g., ranging by index and storing &ms.Spec.Template.Roles[i]). Align roleMap’s value type and population with the actual Roles element type to avoid type/scope pitfalls.
| replicas = *role.Replicas | ||
| } | ||
| // Find the role in the roleMap then check its actual replicas | ||
| /// Calculate total replicas for this role |
There was a problem hiding this comment.
Go convention uses // for comments; /// is non-idiomatic and can be confusing in Go codebases. Replace with a single // (and consider folding it into the existing comment on the next line to avoid redundancy).
| /// Calculate total replicas for this role | |
| // Calculate total replicas for this role |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes the validateGangPolicy function by replacing a nested loop with a map-based lookup for roles. While this improves efficiency, the current implementation introduces two critical issues: a potential loop variable address pitfall when populating the map and a scoping error where the role variable is declared within an if statement initialization but accessed later in the loop, which will result in compilation errors.
| for roleName, minReplicas := range minRoleReplicas { | ||
| // Check if the role exists | ||
| if !roleNames[roleName] { | ||
| if role, ok := roleMap[roleName]; !ok { |
There was a problem hiding this comment.
The variable role is declared within the scope of the if statement initialization. This means it is not accessible outside of the if block (e.g., at line 233), which will result in a compilation error. You should declare it before the if statement to ensure it remains in scope for the rest of the loop iteration.
role, ok := roleMap[roleName]
if !ok {…istently Signed-off-by: shuguansheng <[email protected]>
Signed-off-by: shuguansheng <[email protected]>
There was a problem hiding this comment.
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.
| for roleName, minReplicas := range minRoleReplicas { | ||
| // Check if the role exists | ||
| if !roleNames[roleName] { | ||
| roleElement, ok := roleMap[roleName]; |
There was a problem hiding this comment.
There’s a trailing semicolon after the map lookup (;) which is non-idiomatic in Go and will be removed by gofmt. Please remove the semicolon (and trailing whitespace) to match standard Go formatting.
| roleElement, ok := roleMap[roleName]; | |
| roleElement, ok := roleMap[roleName] |
| replicas = *role.Replicas | ||
| } | ||
| // Find the role in the roleMap then check its actual replicas | ||
| /// Calculate total replicas for this role |
There was a problem hiding this comment.
The triple-slash comment marker (///) is inconsistent with typical Go commenting style and reads like an accidental duplication. Please replace with a normal // comment (or remove if redundant) to keep comments consistent and avoid confusion.
| /// Calculate total replicas for this role | |
| // Calculate total replicas for this role |
Signed-off-by: shuguansheng <[email protected]>
LiZhenCheng9527
left a comment
LiZhenCheng9527
left a comment
There was a problem hiding this comment.
General LGTM
Please correct the errors in your comments.
|
https://github.com/volcano-sh/kthena/pull/885/changes |
What type of PR is this?
What this PR does / why we need it:
optimize the logic in the validation file
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
"None"