Ordinal - #1525
Conversation
…dified. Signed-off-by: LiZhenCheng9527 <lizhencheng6@huawei.com>
Signed-off-by: LiZhenCheng9527 <lizhencheng6@huawei.com>
|
[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
This PR updates the ModelServing controller to reuse missing ServingGroup/Role ordinals within [0, replicas) (instead of allocating maxOrdinal+1) and expands unit/E2E coverage to assert ordinal range correctness during scale/recovery/rolling-update flows.
Changes:
- Update ServingGroup/Role scale-up logic to fill missing ordinals within the replica range and create ControllerRevision snapshots lazily for new revisions.
- Adjust role scale-down / role rolling-update deletion logic around partition handling.
- Add/extend unit and E2E tests to validate ordinal reuse and contiguous ordinal coverage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pkg/model-serving-controller/controller/model_serving_controller.go |
Implements ordinal gap-filling for scale-up and updates partition handling logic for roles/rolling updates. |
pkg/model-serving-controller/controller/model_serving_controller_test.go |
Adds unit coverage for missing-ordinal computation and revised scale behaviors. |
test/e2e/controller-manager/model_serving_test.go |
Adds E2E assertions that ServingGroup/Role ordinals exactly cover [0, replicas). |
Suppressed comments (1)
pkg/model-serving-controller/controller/model_serving_controller.go:1468
- This protected-set computation is based on list position, not the role ordinal. That diverges from the API contract for Partition (protect ordinals [0, partition)) and can misclassify roles when there are gaps/out-of-range ordinals, leading to deleting/updating the wrong replicas during RoleRollingUpdate.
protected := sets.New[string]()
if partitionConfigured && partition > 0 {
for i := 0; i < partition && i < len(roleList); i++ {
protected.Insert(roleList[i].Name)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Partition protects the first N records in the ordinal-sorted roleList. | ||
| protectedRoleNames := sets.New[string]() | ||
| for i := 0; i < partition && i < len(roleList); i++ { | ||
| protectedRoleNames.Insert(roleList[i].Name) | ||
| } |
| existing := make([]bool, expectedCount) | ||
| for _, ordinal := range existingOrdinals { | ||
| if ordinal >= 0 && ordinal < expectedCount { | ||
| existing[ordinal] = true | ||
| } | ||
| } | ||
|
|
||
| missing := make([]int, 0, expectedCount) |
There was a problem hiding this comment.
When either top-level or Role replicas is very large, the webhook permits any nonnegative int32, but this helper allocates both a []bool and an []int sized to expectedCount before issuing an API operation. A valid value near the int32 limit therefore requests many gigabytes and can OOM the shared controller-manager;
There was a problem hiding this comment.
I remember that in our previous discussion, we only considered changes between ordinal numbers and did not take into account the impact of an increase in the number of copies.
What type of PR is this?
What this PR does / why we need it:
Fixes ServingGroup and Role ordinal allocation to reuse missing ordinals within [0, replicas) instead of allocating maxOrdinal + 1. This prevents out-of-range ordinals during recovery, scaling, and rolling updates.
Which issue(s) this PR fixes:
Fixes #1442
Bug evidence (required for bug-related PRs):
Special notes for your reviewer:
Adds unit and E2E coverage for ordinal reuse, partition handling, and recovery. AI assistance was used during implementation and test updates.
Does this PR introduce a user-facing change?: