model-serving: add rolloutStrategy.updateMethod and in-place eligibility checks by kube-gopher · Pull Request #1882 · volcano-sh/kthena

kube-gopher · 2026-10-19T08:53:08Z

What type of PR is this?
/kind feature

What this PR does / why we need it
API (RolloutStrategy):

updateMethod: Recreate (default) or InPlace. Under InPlace, the controller patches container images on existing Pods when only regular-container images change. Otherwise it recreates the ServingGroup, or the Role instance under RoleRollingUpdate, and an InPlaceUpdateFallback event reports why.
progressDeadlineSeconds: minimum 60. The controller applies the 1800s default. It is the maximum time for a ServingGroup or Role instance updated in place to become ready on the target image. After it passes, Pods that are not ready follow the RecoveryPolicy. It only applies to in-place updates.

Eligibility (pkg/model-serving-controller/inplace/eligibility.go):

CheckEligibility(old, new) lists every change from one revision to another that an image patch cannot apply. Only regular-container images may change. Changes to schedulerName, plugins, gangPolicy, networkTopology, Role names, workerReplicas, container names or order, or any other Pod template field are reported.
CheckRoleEligibility(old, new, role) checks one Role, for RoleRollingUpdate.
restartPolicy must resolve to Always, and no regular container may set restartPolicy or restartPolicyRules, because the update relies on kubelet restarting the patched container.
An omitted imagePullPolicy is resolved the way the API server defaults it. A tag change that would change the effective pull policy is reported, so users can set it explicitly.

Which issue(s) this PR fixes:
Part of #1829

Bug evidence (required for bug-related PRs):

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kube-gopher · 2026-10-19T09:00:59Z

+	// It includes any wait before the images are patched.
+	// When the deadline passes, the update of the unit has failed, and its Pods that are not ready on the target image follow the RecoveryPolicy.
+	// It only applies when updateMethod is InPlace, and is ignored otherwise.
+	// The controller defaults it to 1800 seconds.


If it is set to +kubebuilder:default=1800, it will take effect under the rolloutStrategy field and updateMethod will be ignored; therefore, it will also be applied to Recreate. progressDeadlineSeconds itself only applies to Inplace.

kube-gopher · 2026-10-19T09:03:27Z

/cc @acsoto

acsoto · 2026-10-19T09:17:30Z

+	if inplace.UpdateMethodOf(oldMS) != workloadv1alpha1.InPlaceUpdateMethod {
+		allErrs = append(allErrs, validateRolloutComplete(oldMS)...)
+	} else if revisionChanged(oldMS, ms) {
+		allErrs = append(allErrs, validateNoUnitInUpdate(oldMS)...)


Is blocking all image changes during an unfinished rollout temporary? This also rejects reverting a bad image, while #1829 relies on retargeting failed units for in-place rollback.

acsoto · 2026-10-19T09:21:23Z

+	if err != nil {
+		return false
+	}
+	return !bytes.Equal(oldData, newData)


How will this check align with #1799's semantic revision comparison so admission and the controller agree on what constitutes a new revision?

LiZhenCheng9527 · 2026-10-19T11:28:11Z

+	// The controller defaults it to 1800 seconds.
+	// +kubebuilder:validation:Minimum=60
+	// +optional
+	ProgressDeadlineSeconds *int32 `json:"progressDeadlineSeconds,omitempty"`


@VanderChen
I believe we still need a time interval that is different from RestartGracePeriodSeconds. This is because the cold start time for an LLM pod and the pod error tolerance period are two distinct concepts, and the time required for each is difficult to measure using a single time interval.

LiZhenCheng9527

I think your webhook is too redundant. Moreover, many of the restrictions are unreasonable.

You just need to specify in reconcile that when updateMethod is set to Inplace, you should use the Inplace upgrade only when the pod image has been modified. If any of the above conditions are not met, fall back to the recreate upgrade.

LiZhenCheng9527 · 2026-10-19T11:42:43Z

+	strategyPath := field.NewPath("spec", "rolloutStrategy")
+	if strategy.Type == workloadv1alpha1.RoleRollingUpdate {
+		allErrs = append(allErrs, field.Forbidden(strategyPath.Child("type"),
+			"RoleRollingUpdate cannot be combined with rolloutStrategy.updateMethod InPlace"))


why?
If you're saying that your current implementation doesn't work, you need to explain why sg can use inplace but Role cannot.

Role is also supported, it’s just not in the current plan.

When will that plan be implemented?
Also, I'm curious about the difference between SG and Role in an in-place scenario—what makes it necessary to implement them separately? And this results in the need to add a validator.

I want to add support for Role now too. I’ll go make the changes.

When will that plan be implemented? Also, I'm curious about the difference between SG and Role in an in-place scenario—what makes it necessary to implement them separately? And this results in the need to add a validator.

Actually, the difference isn't that big—it's just the order in which they're implemented. hh

LiZhenCheng9527 · 2026-10-19T11:43:24Z

+	allErrs = append(allErrs, validateInPlaceBudget(ms)...)
+	if len(ms.Spec.Plugins) > 0 {
+		allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "plugins"),
+			"plugins cannot be used with rolloutStrategy.updateMethod InPlace"))


Why is that?

LiZhenCheng9527 · 2026-10-19T11:56:33Z

+		return nil
+	}
+	var allErrs field.ErrorList
+	if inplace.UpdateMethodOf(oldMS) != workloadv1alpha1.InPlaceUpdateMethod {


How does this relate to the UpdateMethod in oldModelserving?

LiZhenCheng9527 · 2026-10-19T12:01:02Z

+
+// validateNoUnitInUpdate rejects a new revision until every ServingGroup outside the partition is updated.
+// The target of an unfinished in-place update cannot change yet.
+func validateNoUnitInUpdate(oldMS *workloadv1alpha1.ModelServing) field.ErrorList {


Why do this?
If this restriction is implemented, and an inplace update operation never completes due to an error, will ModelServing be unable to modify the configuration forever?

kube-gopher · 2026-10-19T12:55:38Z

I think your webhook is too redundant. Moreover, many of the restrictions are unreasonable.

You just need to specify in reconcile that when updateMethod is set to Inplace, you should use the Inplace upgrade only when the pod image has been modified. If any of the above conditions are not met, fall back to the recreate upgrade.

okay, i will fix it.

kube-gopher · 2026-10-20T04:19:42Z

@LiZhenCheng9527 @acsoto PTAL.

acsoto · 2026-10-20T07:00:38Z

+	// It only applies to in-place updates, and defaults to 1800 seconds.
+	// +kubebuilder:validation:Minimum=60
+	// +optional
+	ProgressDeadlineSeconds *int32 `json:"progressDeadlineSeconds,omitempty"`


What should happen when this deadline expires, and why do we need a separate timeout rather than leaving a stalled update for the user to resolve?

After the timeout, a Warning event named ProgressDeadlineExceeded is emitted first, and then the RecoverPolicy is applied.

@VanderChen
I believe we still need a time interval that is different from RestartGracePeriodSeconds. This is because the cold start time for an LLM pod and the pod error tolerance period are two distinct concepts, and the time required for each is difficult to measure using a single time interval.

This is the explanation.
Our previous consideration was that we only need to wait for a RestartGracePeriodSeconds timeout and then proceed with the RecoveryPolicy.

acsoto · 2026-10-20T08:01:23Z

/lgtm

LiZhenCheng9527

I have an idea to verify whether ModelServing modifies only the pod.image. I would replace the old pod.Spec.image with the new one, calculate the hash value, and compare it with the hash value of the new ModelServing. If they match, it means only the pod.Spec.image was modified.

Let's discuss it at tomorrow's community meeting.

kube-gopher · 2026-10-21T11:28:01Z

I have an idea to verify whether ModelServing modifies only the pod.image. I would replace the old pod.Spec.image with the new one, calculate the hash value, and compare it with the hash value of the new ModelServing. If they match, it means only the pod.Spec.image was modified.

Let's discuss it at tomorrow's community meeting.

The current implementation of checkTemplate already copies the old image into the new container and then runs DeepEqual on the entire template. The redundant part of the check simply involves many additional checks, so I removed that part of the check.

RBG's approach only accepts replacement operations whose paths match the /spec/containers/N/image regex. Our revision data, however, has two layouts: the old one is a wrapper list of complete Roles containing replicas, while the new one has entryTemplate and workerTemplate for each Role. Therefore, we need to write our own regex for each layout and add a replicas filter, which increases complexity.

…ity checks Signed-off-by: kube-gopher <jzlyy68@gmail.com>

LiZhenCheng9527

It looks like everything's fine now.
/lgtm

volcano-sh-bot · 2026-10-27T01:33:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LiZhenCheng9527

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~charts/OWNERS~~ [LiZhenCheng9527]
~~client-go/OWNERS~~ [LiZhenCheng9527]
~~docs/OWNERS~~ [LiZhenCheng9527]
~~pkg/OWNERS~~ [LiZhenCheng9527]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

github-project-automation Bot added this to Kthena Release Tracking Oct 19, 2026

volcano-sh-bot added the kind/feature label Oct 19, 2026

github-project-automation Bot moved this to Accepted in Kthena Release Tracking Oct 19, 2026

volcano-sh-bot requested review from FAUST-BENCHOU, LiZhenCheng9527, YaoZengzeng, git-malu and hzxuzhonghu October 19, 2026 08:53

volcano-sh-bot added the size/XXL label Oct 19, 2026

kube-gopher commented Oct 19, 2026

View reviewed changes

volcano-sh-bot requested a review from acsoto October 19, 2026 09:03

acsoto reviewed Oct 19, 2026

View reviewed changes

LiZhenCheng9527 reviewed Oct 19, 2026

View reviewed changes

kube-gopher force-pushed the inplace-api-admission branch from 89788bc to 3be54cf Compare October 20, 2026 02:47

volcano-sh-bot added size/XL and removed size/XXL labels Oct 20, 2026

kube-gopher marked this pull request as draft October 20, 2026 02:58

volcano-sh-bot added the do-not-merge/work-in-progress label Oct 20, 2026

kube-gopher changed the title ~~model-serving: add rolloutStrategy.updateMethod and in-place admission~~ model-serving: add rolloutStrategy.updateMethod and in-place eligibility checks Oct 20, 2026

kube-gopher force-pushed the inplace-api-admission branch from 3be54cf to 73bf24e Compare October 20, 2026 04:08

kube-gopher marked this pull request as ready for review October 20, 2026 04:09

volcano-sh-bot removed the do-not-merge/work-in-progress label Oct 20, 2026

volcano-sh-bot requested a review from LiZhenCheng9527 October 20, 2026 04:09

kube-gopher mentioned this pull request Oct 20, 2026

model-serving: add the in-place rolling update controller #1883

Merged

acsoto reviewed Oct 20, 2026

View reviewed changes

volcano-sh-bot assigned acsoto Oct 20, 2026

volcano-sh-bot added the lgtm label Oct 20, 2026

LiZhenCheng9527 reviewed Oct 20, 2026

View reviewed changes

model-serving: add rolloutStrategy.updateMethod and in-place eligibil…

44b1ddd

…ity checks Signed-off-by: kube-gopher <jzlyy68@gmail.com>

kube-gopher force-pushed the inplace-api-admission branch from 73bf24e to 44b1ddd Compare October 21, 2026 11:40

volcano-sh-bot added size/L and removed lgtm size/XL labels Oct 21, 2026

LiZhenCheng9527 approved these changes Oct 27, 2026

View reviewed changes

volcano-sh-bot assigned LiZhenCheng9527 Oct 27, 2026

volcano-sh-bot added the lgtm label Oct 27, 2026

volcano-sh-bot added the approved label Oct 27, 2026

volcano-sh-bot merged commit 84bacdd into volcano-sh:main Oct 27, 2026
19 checks passed

github-project-automation Bot moved this from Accepted to Done in Kthena Release Tracking Oct 27, 2026

Conversation

kube-gopher commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kube-gopher commented Oct 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LiZhenCheng9527 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kube-gopher commented Oct 19, 2026

Uh oh!

kube-gopher commented Oct 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kube-gopher Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acsoto commented Oct 20, 2026

Uh oh!

LiZhenCheng9527 left a comment

Choose a reason for hiding this comment

Uh oh!

kube-gopher commented Oct 21, 2026

Uh oh!

LiZhenCheng9527 left a comment

Choose a reason for hiding this comment

Uh oh!

volcano-sh-bot commented Oct 27, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kube-gopher commented Oct 19, 2026 •

edited

Loading

kube-gopher Oct 20, 2026 •

edited

Loading