feat: Support partition for Role in RoleRollingUpdate#1311
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
There was a problem hiding this comment.
Pull request overview
Adds per-Role partition support for RoleRollingUpdate in the ModelServing controller stack, enabling canary-style rollouts within a role by ordinal while keeping lower ordinals on the current revision until the partition is lowered.
Changes:
- Introduces
roles[].partition(IntOrString) to the workload API/CRD/docs and generated client/applyconfig/deepcopy code. - Adds webhook validation for role-level partition and controller logic to respect partition when selecting outdated role replicas to update and when recreating missing pods.
- Adds an e2e test covering role-level partition behavior during
RoleRollingUpdate.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/controller-manager/model_serving_test.go | Adds an e2e test for role-level partition behavior in RoleRollingUpdate. |
| pkg/model-serving-controller/webhook/validator.go | Validates roles[].partition and restricts it to RoleRollingUpdate. |
| pkg/model-serving-controller/utils/utils.go | Adds helpers to compute/interpret role partition and protection by ordinal. |
| pkg/model-serving-controller/utils/revision_util.go | Excludes partition from revision/template-hash shaping (like replicas/maxUnavailable). |
| pkg/model-serving-controller/controller/model_serving_controller.go | Applies partition logic when updating roles and when recreating missing pods during partitioned rollouts. |
| pkg/apis/workload/v1alpha1/zz_generated.deepcopy.go | DeepCopy support for the new Role.Partition field. |
| pkg/apis/workload/v1alpha1/servinggroup_types.go | Adds partition to the Role API type with kubebuilder markers and docs. |
| pkg/apis/workload/v1alpha1/model_serving_types.go | Updates RolloutStrategy docs to mention role partition alongside maxUnavailable. |
| docs/kthena/docs/reference/crd/workload.serving.volcano.sh.md | Updates generated CRD reference docs to include roles[].partition. |
| client-go/applyconfiguration/workload/v1alpha1/role.go | Adds declarative apply support (WithPartition). |
| charts/kthena/charts/workload/crds/workload.serving.volcano.sh_modelservings.yaml | Updates embedded CRD schema/docs for roles[].partition. |
Files not reviewed (2)
- client-go/applyconfiguration/workload/v1alpha1/role.go: Generated file
- pkg/apis/workload/v1alpha1/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d79b909 to
9b49e2c
Compare
| // This field is only valid when rolloutStrategy.type is RoleRollingUpdate. | ||
| // +kubebuilder:validation:XIntOrString | ||
| // +optional | ||
| Partition *intstr.IntOrString `json:"partition,omitempty"` |
There was a problem hiding this comment.
We can reuse the RollingUpdateConfiguration struct, make it inline
RollingUpdateConfiguration
There was a problem hiding this comment.
what about MaxUnavailable?I heard in future will also support maxSurge.I think its more reasonable to put all of them in RollingUpdateConfiguration
There was a problem hiding this comment.
YOu donot need to add that field, which is not related
| // "ServingGroupRollingUpdate" and "RoleRollingUpdate". If not specified, | ||
| // it defaults to "ServingGroupRollingUpdate". | ||
| // For `RoleRollingUpdate`, the `maxUnavailable` field in each Role will be used to determine the maximum number of role instances that can be unavailable during the update. | ||
| // For `RoleRollingUpdate`, the `maxUnavailable` and `partition` fields in each Role control update pacing and canary scope. |
There was a problem hiding this comment.
Based on the current implementation, during a RoleRollingUpdate, Partition takes effect, but maxUnavailable does not.
There was a problem hiding this comment.
inline RollingUpdateConfiguration can solve this
| MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"` | ||
|
|
||
| // Partition indicates the ordinal at which this Role should be partitioned for updates. | ||
| // During a RoleRollingUpdate, replicas with ordinal >= Partition are updated first. |
There was a problem hiding this comment.
The sequence numbers of roles in a cluster are not necessarily ordered starting from zero.
It would be better to describe it as the first Partition role replicas not being upgraded.
| roleToApply := targetRole | ||
| revisionToUse := newRevision | ||
| hashToUse := utils.CalRoleTemplateHash(targetRole) | ||
| if protected, err := utils.IsRoleReplicaPartitionProtected(targetRole, roleObj.Name); err != nil { | ||
| klog.Errorf("failed to evaluate role partition for %s: %v", roleObj.Name, err) | ||
| } else if protected { | ||
| revision := roleObj.Revision | ||
| if revision == "" { | ||
| revision = ms.Status.CurrentRevision | ||
| } | ||
| if revision != "" { | ||
| if cr, err := utils.GetControllerRevision(ctx, c.kubeClientSet, ms, revision); err != nil { | ||
| klog.Warningf("manageRoleReplicasPerGroup: failed to get ControllerRevision %s for partition-protected role %s: %v", revision, roleObj.Name, err) | ||
| } else if cr != nil { | ||
| if oldRoles, err := utils.GetRolesFromControllerRevision(cr); err != nil { | ||
| klog.Warningf("manageRoleReplicasPerGroup: failed to get roles from ControllerRevision %s for partition-protected role %s: %v", revision, roleObj.Name, err) | ||
| } else { | ||
| for _, oldRole := range oldRoles { | ||
| if oldRole.Name == targetRole.Name { | ||
| roleToApply = oldRole | ||
| revisionToUse = revision | ||
| hashToUse = roleObj.RoleTemplateHash | ||
| if hashToUse == "" { | ||
| hashToUse = utils.CalRoleTemplateHash(oldRole) | ||
| } | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I suggest abstracting it into a function.
| return false, err | ||
| } | ||
| _, ordinal := GetParentNameAndOrdinal(roleID) | ||
| return ordinal < partition, nil |
There was a problem hiding this comment.
I think this comparison is wrong.
Since partitions can be reconfigured later, the cluster might currently consist of r4, r5, r6, and so on. When the number of partitions is set to 2, r4 and r5 should be protected. However, the current comparison method cannot achieve this.
There was a problem hiding this comment.
done,also update the e2e test to test this case (partition=2 and r4,r5 protected)
| } | ||
|
|
||
| // IsRoleReplicaPartitionProtected reports whether a role replica should be kept on the current revision. | ||
| func IsRoleReplicaPartitionProtected(role workloadv1alpha1.Role, roleID string) (bool, error) { |
There was a problem hiding this comment.
I think that after sorting the roles by their serial ordinal, selecting the [0, partition) range for replica protection should suffice.
9b49e2c to
4e5b9b4
Compare
4e5b9b4 to
c17dcc8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 5 comments.
Files not reviewed (2)
- client-go/applyconfiguration/workload/v1alpha1/role.go: Generated file
- pkg/apis/workload/v1alpha1/zz_generated.deepcopy.go: Generated file
Comments suppressed due to low confidence (2)
charts/kthena/charts/workload/crds/workload.serving.volcano.sh_modelservings.yaml:1
- Setting an unconditional CRD default of
maxUnavailable: 1at the Role level will change behavior for users who previously omittedmaxUnavailable(they’ll now always get a configured value), and can also cause webhook rejections for non-RoleRollingUpdatestrategies because the field becomes present via defaulting. CRD schema defaults are not conditional; please remove this default from the CRD, or implement conditional defaulting in a mutating webhook (only whenrolloutStrategy.type == RoleRollingUpdate).
charts/kthena/charts/workload/crds/workload.serving.volcano.sh_modelservings.yaml:1 - This
partitiondescription refers to ServingGroup ordinals (and 'ModelServing should be partitioned'), but the field is being introduced for role-level rolling updates. Please update the schema description to accurately describe role replica partition semantics (and ensure the 'ordinal' wording matches the controller’s actual interpretation).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 7 comments.
Files not reviewed (2)
- client-go/applyconfiguration/workload/v1alpha1/role.go: Generated file
- pkg/apis/workload/v1alpha1/zz_generated.deepcopy.go: Generated file
Comments suppressed due to low confidence (1)
docs/kthena/docs/reference/crd/workload.serving.volcano.sh.md:940
- RollingUpdateConfiguration is now documented as appearing in Role, but its field docs still describe ServingGroup-level behavior (and maxUnavailable shows a default of 1). If RoleRollingUpdate uses different semantics (and/or maxUnavailable is intended to remain optional per-role), the generated CRD reference should be corrected by updating the Go type(s)/comments used for generation.
RollingUpdateConfiguration defines the parameters to be used for ServingGroupRollingUpdate.
_Appears in:_
- [Role](#role)
- [RolloutStrategy](#rolloutstrategy)
| Field | Description | Default | Validation |
| --- | --- | --- | --- |
| `maxUnavailable` _[IntOrString](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.33/#intorstring-intstr-util)_ | The maximum number of replicas that can be unavailable during the update.<br />Value can be an absolute number (ex: 5) or a percentage of total replicas at the start of update (ex: 10%).<br />Absolute number is calculated from percentage by rounding down.<br />This can not be 0.<br />By default, a fixed value of 1 is used. | 1 | XIntOrString: \{\} <br /> |
| `partition` _[IntOrString](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.33/#intorstring-intstr-util)_ | Partition indicates the ordinal at which the ModelServing should be partitioned<br />for updates. During a rolling update, all ServingGroups from ordinal Replicas-1 to<br />Partition are updated. All ServingGroups from ordinal Partition-1 to 0 remain untouched.<br />Value can be an absolute number (ex: 5) or a percentage of total replicas (ex: 10%).<br />Absolute number is calculated from percentage by rounding up.<br />The default value is 0. | | XIntOrString: \{\} <br /> |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 8 comments.
Files not reviewed (2)
- client-go/applyconfiguration/workload/v1alpha1/role.go: Generated file
- pkg/apis/workload/v1alpha1/zz_generated.deepcopy.go: Generated file
Comments suppressed due to low confidence (1)
pkg/model-serving-controller/controller/model_serving_controller.go:984
- manageRoleReplicasPerGroup uses the loop index from
range roleListto decide whether a replica is partition-protected. Because the loopcontinues overRoleDeletingentries, the index no longer represents the position among active replicas, so partition protection can shift unexpectedly (e.g., if the lowest ordinal is deleting, the next replica won't be treated as protected). Track a separate counter for non-deleting roles and basepartitionProtectedon that instead.
for index, roleObj := range roleList {
if roleObj.Status == datastore.RoleDeleting {
continue
}
roleIDValue := fmt.Sprintf("%s/%s/%s/%s", ms.Namespace, groupName, targetRole.Name, roleObj.Name)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 7 comments.
Files not reviewed (2)
- client-go/applyconfiguration/workload/v1alpha1/role.go: Generated file
- pkg/apis/workload/v1alpha1/zz_generated.deepcopy.go: Generated file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 10 comments.
Files not reviewed (2)
- client-go/applyconfiguration/workload/v1alpha1/role.go: Generated file
- pkg/apis/workload/v1alpha1/zz_generated.deepcopy.go: Generated file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 6 comments.
Files not reviewed (2)
- client-go/applyconfiguration/workload/v1alpha1/role.go: Generated file
- pkg/apis/workload/v1alpha1/zz_generated.deepcopy.go: Generated file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 9 comments.
Files not reviewed (2)
- client-go/applyconfiguration/workload/v1alpha1/role.go: Generated file
- pkg/apis/workload/v1alpha1/zz_generated.deepcopy.go: Generated file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 7 comments.
Files not reviewed (2)
- client-go/applyconfiguration/workload/v1alpha1/role.go: Generated file
- pkg/apis/workload/v1alpha1/zz_generated.deepcopy.go: Generated file
| if len(pods) < expectedPods { | ||
| klog.V(2).Infof("manageRoleReplicasPerGroup: role %s/%s in ServingGroup %s is missing pods (%d/%d), recreating", targetRole.Name, roleObj.Name, groupName, len(pods), expectedPods) | ||
| if roleTemplateHash == "" { | ||
| roleTemplateHash = utils.CalRoleTemplateHash(targetRole) | ||
| } | ||
| partitionProtected := partitionConfigured && partition > 0 && index < partition | ||
| roleToApply, revisionToUse, hashToUse := c.roleTemplateForReplica(ctx, ms, targetRole, roleObj, newRevision, partitionProtected) |
| protected := make(map[string]struct{}, partition) | ||
| for index, role := range roleList { | ||
| if index >= partition { | ||
| break | ||
| } | ||
| protected[role.Name] = struct{}{} | ||
| } |
| for index, role := range roleList { | ||
| if index >= partition { | ||
| break | ||
| } | ||
| if role.Status == datastore.RoleDeleting { | ||
| continue | ||
| } | ||
| observedHash, ok := c.resolveRoleTemplateHashForComparison(ms, sg, roleSpec.Name, role) | ||
| if ok && observedHash != expectedHash { | ||
| hasOutdatedRoles = true | ||
| break | ||
| } | ||
| } |
| default: 1 | ||
| description: |- | ||
| MaxUnavailable is the maximum number of replicas of this Role that can be | ||
| unavailable during a RoleRollingUpdate. Value can be an absolute number (ex: 2) | ||
| or a percentage of this Role's replicas (ex: 50%). Percentages are rounded down. | ||
| This field is only valid when rolloutStrategy.type is RoleRollingUpdate. | ||
| When unset, all outdated replicas of this Role are recreated at once. | ||
| The maximum number of replicas that can be unavailable during the update. | ||
| Value can be an absolute number (ex: 5) or a percentage of total replicas at the start of update (ex: 10%). | ||
| Absolute number is calculated from percentage by rounding down. | ||
| This can not be 0. | ||
| By default, a fixed value of 1 is used. |
| description: |- | ||
| Partition indicates the ordinal at which the ModelServing should be partitioned | ||
| for updates. During a rolling update, all ServingGroups from ordinal Replicas-1 to | ||
| Partition are updated. All ServingGroups from ordinal Partition-1 to 0 remain untouched. | ||
| Value can be an absolute number (ex: 5) or a percentage of total replicas (ex: 10%). | ||
| Absolute number is calculated from percentage by rounding up. | ||
| The default value is 0. |
| // TestModelServingRoleRollingUpdatePartition verifies RoleRollingUpdate respects role-level partition | ||
| // when role ordinals do not start from 0. Partition protects the first N replicas in the sorted role list | ||
| // (e.g. prefill-4 and prefill-5), not replicas whose ordinal is less than partition. |
| _Appears in:_ | ||
| - [Role](#role) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 8 comments.
Files not reviewed (2)
- client-go/applyconfiguration/workload/v1alpha1/role.go: Generated file
- pkg/apis/workload/v1alpha1/zz_generated.deepcopy.go: Generated file
| for index, roleObj := range roleList { | ||
| if roleObj.Status == datastore.RoleDeleting { | ||
| continue | ||
| } |
| protected := make(map[string]struct{}, partition) | ||
| for index, role := range roleList { | ||
| if index >= partition { | ||
| break | ||
| } | ||
| protected[role.Name] = struct{}{} | ||
| } |
| for index, role := range roleList { | ||
| if index >= partition { | ||
| break | ||
| } | ||
| if role.Status == datastore.RoleDeleting { | ||
| continue | ||
| } | ||
| observedHash, ok := c.resolveRoleTemplateHashForComparison(ms, sg, roleSpec.Name, role) | ||
| if ok && observedHash != expectedHash { | ||
| hasOutdatedRoles = true | ||
| break | ||
| } | ||
| } |
| description: |- | ||
| Partition indicates the ordinal at which the ModelServing should be partitioned | ||
| for updates. During a rolling update, all ServingGroups from ordinal Replicas-1 to | ||
| Partition are updated. All ServingGroups from ordinal Partition-1 to 0 remain untouched. | ||
| Value can be an absolute number (ex: 5) or a percentage of total replicas (ex: 10%). | ||
| Absolute number is calculated from percentage by rounding up. | ||
| The default value is 0. |
| // TestModelServingRoleRollingUpdatePartition verifies RoleRollingUpdate respects role-level partition | ||
| // when role ordinals do not start from 0. Partition protects the first N replicas in the sorted role list | ||
| // (e.g. prefill-4 and prefill-5), not replicas whose ordinal is less than partition. |
| type RoleApplyConfiguration struct { | ||
| Name *string `json:"name,omitempty"` | ||
| Replicas *int32 `json:"replicas,omitempty"` | ||
| EntryTemplate *PodTemplateSpecApplyConfiguration `json:"entryTemplate,omitempty"` | ||
| WorkerReplicas *int32 `json:"workerReplicas,omitempty"` | ||
| WorkerTemplate *PodTemplateSpecApplyConfiguration `json:"workerTemplate,omitempty"` | ||
| MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"` | ||
| Name *string `json:"name,omitempty"` | ||
| Replicas *int32 `json:"replicas,omitempty"` | ||
| EntryTemplate *PodTemplateSpecApplyConfiguration `json:"entryTemplate,omitempty"` | ||
| WorkerReplicas *int32 `json:"workerReplicas,omitempty"` |
| - [Role](#role) | ||
| - [RolloutStrategy](#rolloutstrategy) |
| description: |- | ||
| MaxUnavailable is the maximum number of replicas of this Role that can be | ||
| unavailable during a RoleRollingUpdate. Value can be an absolute number (ex: 2) | ||
| or a percentage of this Role's replicas (ex: 50%). Percentages are rounded down. | ||
| This field is only valid when rolloutStrategy.type is RoleRollingUpdate. | ||
| When unset, all outdated replicas of this Role are recreated at once. | ||
| The maximum number of replicas that can be unavailable during the update. | ||
| Value can be an absolute number (ex: 5) or a percentage of total replicas at the start of update (ex: 10%). | ||
| Absolute number is calculated from percentage by rounding down. | ||
| This can not be 0. |
| // For `partition`, the first N role replicas sorted by ordinal are protected from updates. | ||
| // +optional | ||
| MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"` | ||
| *RollingUpdateConfiguration `json:",inline,omitempty"` |
There was a problem hiding this comment.
prefer RollingUpdateConfigurationjson:",inline,omitempty"`` , we can still access by role.MaxUnavailable
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated 4 comments.
Files not reviewed (2)
- client-go/applyconfiguration/workload/v1alpha1/role.go: Generated file
- pkg/apis/workload/v1alpha1/zz_generated.deepcopy.go: Generated file
| partitionProtected := partitionConfigured && partition > 0 && index < partition | ||
| roleToApply, revisionToUse, hashToUse := c.roleTemplateForReplica(ctx, ms, targetRole, roleObj, newRevision, partitionProtected) | ||
| _, roleIndex := utils.GetParentNameAndOrdinal(roleObj.Name) | ||
| if err := c.CreatePodsByRole(ctx, *targetRole.DeepCopy(), ms, roleIndex, servingGroupOrdinal, newRevision, roleTemplateHash); err != nil { | ||
| if err := c.CreatePodsByRole(ctx, *roleToApply.DeepCopy(), ms, roleIndex, servingGroupOrdinal, revisionToUse, hashToUse); err != nil { |
| | `workerTemplate` _[PodTemplateSpec](#podtemplatespec)_ | WorkerTemplate defines the template for the worker pod of a role. | | | | ||
| | `maxUnavailable` _[IntOrString](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.33/#intorstring-intstr-util)_ | MaxUnavailable is the maximum number of replicas of this Role that can be<br />unavailable during a RoleRollingUpdate. Value can be an absolute number (ex: 2)<br />or a percentage of this Role's replicas (ex: 50%). Percentages are rounded down.<br />This field is only valid when rolloutStrategy.type is RoleRollingUpdate.<br />When unset, all outdated replicas of this Role are recreated at once. | | XIntOrString: \{\} <br /> | | ||
| | `maxUnavailable` _[IntOrString](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.33/#intorstring-intstr-util)_ | The maximum number of replicas that can be unavailable during the update.<br />Value can be an absolute number (ex: 5) or a percentage of total replicas at the start of update (ex: 10%).<br />Absolute number is calculated from percentage by rounding down.<br />This can not be 0.<br />By default, a fixed value of 1 is used. | 1 | XIntOrString: \{\} <br /> | | ||
| | `partition` _[IntOrString](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.33/#intorstring-intstr-util)_ | Partition indicates the ordinal at which the ModelServing should be partitioned<br />for updates. During a rolling update, all ServingGroups from ordinal Replicas-1 to<br />Partition are updated. All ServingGroups from ordinal Partition-1 to 0 remain untouched.<br />Value can be an absolute number (ex: 5) or a percentage of total replicas (ex: 10%).<br />Absolute number is calculated from percentage by rounding up.<br />The default value is 0. | | XIntOrString: \{\} <br /> | |
| description: |- | ||
| Partition indicates the ordinal at which the ModelServing should be partitioned | ||
| for updates. During a rolling update, all ServingGroups from ordinal Replicas-1 to | ||
| Partition are updated. All ServingGroups from ordinal Partition-1 to 0 remain untouched. | ||
| Value can be an absolute number (ex: 5) or a percentage of total replicas (ex: 10%). | ||
| Absolute number is calculated from percentage by rounding up. | ||
| The default value is 0. |
| // RollingUpdateConfiguration defines the parameters to be used for RoleRollingUpdate. | ||
| // It is inlined so `maxUnavailable` and `partition` can be set directly under a Role. | ||
| // For `partition`, the first N role replicas sorted by ordinal are protected from updates. | ||
| // +optional | ||
| MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"` | ||
| RollingUpdateConfiguration `json:",inline,omitempty"` |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #1310
Special notes for your reviewer:
Does this PR introduce a user-facing change?: