add MaxUnavailable/MaxSurge to RolloutStrategy by jaypipes · Pull Request #564 · temporalio/temporal-worker-controller

jaypipes · 2026-09-01T11:14:01Z

Instead of adding a deploymentStrategy field to WorkerDeploymentSpec (which was seen as confusing due to the existing rolloutStrategy field), we add a MaxUnavailable and MaxSurge field to the RolloutStrategy struct. The DeploymentStrategyType for the Kubernetes Deployments created by the controller are always appsv1.RollingUpdateDeploymentStrategyType and the appsv1.DeploymentStrategy struct created for the Kubernetes Deployment now defaults the RollingUpdate.MaxUnavailable and RollingUpdate.MaxSurge fields to the matching values in WorkerDeploymentSpec.rolloutStrategy.maxUnavailable and WorkerDeploymentSpeec.rolloutStrategy.maxSurge.

Co-authored-by: @otan

Issue #496

Instead of adding a `deploymentStrategy` field to `WorkerDeploymentSpec` (which was seen as confusing due to the existing `rolloutStrategy` field), we add a `MaxUnavailable` and `MaxSurge` field to the `RolloutStrategy` struct. The DeploymentStrategyType for the Kubernetes Deployments created by the controller are *always* `appsv1.RollingUpdateDeploymentStrategyType` and the `appsv1.DeploymentStrategy` struct created for the Kubernetes Deployment now defaults the `RollingUpdate.MaxUnavailable` and `RollingUpdate.MaxSurge` fields to the matching values in `WorkerDeploymentSpec.rolloutStrategy.maxUnavailable` and `WorkerDeploymentSpeec.rolloutStrategy.maxSurge`. Issue temporalio#496 Co-authored-by: Oliver Tan <otan@cursor.com> Signed-off-by: Jay Pipes <jay.pipes@temporal.io>

CLAassistant · 2026-09-01T11:14:12Z

All committers have signed the CLA.

CLAassistant · 2026-09-01T11:14:13Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

jaypipes · 2026-09-03T15:21:14Z

@otan Hi Oliver! We've had some weird issues with the CLA assistant thing... any chance you can re-sign the CLA with your Github account? Thank ou!

otan · 2026-09-03T15:50:38Z

you have my sword

Shivs11

just one comment

Shivs11 · 2026-09-03T16:45:01Z

+	}
+
+	desired := appsv1.DeploymentStrategy{
+		Type: appsv1.RollingUpdateDeploymentStrategyType,


maybe my understanding of this is flawed, but here goes: I see that the default type here, for the strategy, is a rolling update. However, say tomorrow some user wanted to have this set to Recreate.

Would it be possible for them to set this? Are we saying that for worker-controller managed versions, we would never have this strat at all?

Moreover, without this PR, if someone today were to manually update their deployments and set this "Recreate" strategy, the worker-controller would not care. However, after this PR goes in, the controller would overwrite this field. Do we want this?

maybe my understanding of this is flawed, but here goes: I see that the default type here, for the strategy, is a rolling update. However, say tomorrow some user wanted to have this set to Recreate.

Not the default type... it's the only strategy type supported.

Would it be possible for them to set this? Are we saying that for worker-controller managed versions, we would never have this strat at all?

I don't really see a use case for Recreate strategy when it comes to TWC.

The Kubernetes Deployment's spec.strategy.type only influences how the Kubernetes Deployment controller handles mutations to the Kubernetes Deployment's spec.template.spec field.

TWC is the only thing that creates the Kubernetes Deployments associated with each individual WorkerDeploymentVersion that comprise the "rainbow deployment" of the Temporal WorkerDeployment. TWC is the thing that manages the setting of those Kubernetes Deployments' spec.template.spec field.

The only code path in TWC that ever updates the the Kubernetes Deployment's spec.template.spec field restricts the update to only the Kubernetes Deployment representing the current/target build ID. TWC never updates non-current Kubernetes Deployment spec.template.spec field contents. This is deliberate, I believe: we did not want a change to, say, the cpu/memory resource in the WorkerDeployment's spec.template.spec..resources field to impact drained or draining build IDs, only the current build ID (and future target versions).

The original feature request for this PR describes how @otan's team occasionally performs manual kubectl rollout restart commands on the Kubernetes Deployment associated with the current buildID for two primary reasons:

They have made non-breaking changes to the worker image referenced in the Kubernetes Deployment's spec.template.spec.containers[*].image field and pushed a new image with the same tag and want to have the Pods in the Kubernetes Deployment restart to pick up that new image.

The rollout may have gotten "stuck" due to some reason and they want to "kick" the rollout into gear after making a change to a field in the Kubernetes Deployment's spec.template.spec to fix whatever caused the rollout to get stuck.

In both cases, the desire is to be able to have more fine-grained control over the Kubernetes Deployment's restart of the rollout in order to force Kubernetes Deployment controller to be more conservative with its strategy of killing existing Pods and starting new Pods. I don't see how the Recreate rollout deployment strategy would be something anyone would be looking for...

Shivs11 · 2026-09-03T17:00:44Z

 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/intstr"


I think each of the files that have been touched have unit tests, but not seeing an e2e integration test which could maybe test out that updating a WD would update it's existing child deployment with the intended numbers

Shivs11 · 2026-09-03T17:01:37Z

@@ -71,6 +79,9 @@ spec:
            x-kubernetes-validations:
            - message: Only one of mutualTLSSecretRef or apiKeySecretRef may be set
              rule: '!(has(self.mutualTLSSecretRef) && has(self.apiKeySecretRef))'
+            - message: tls.caCertSecretRef cannot be combined with mutualTLSSecretRef;
+                bundle the CA into that secret's own ca.crt key instead
+              rule: '!(has(self.mutualTLSSecretRef) && has(self.tls) && has(self.tls.caCertSecretRef))'


isn't this an unrelated change to the crds?

Yeah, I'm actually not sure where this came from. I will undo it.

carlydf

Other than the placement of the new fields, the semantics of how the fields are used looks good to me! I think it's fine to say that only rolling updates within a version's replicas are allowed, and not leave room for other Deployment update strategies

carlydf · 2026-09-03T17:39:02Z

 // +kubebuilder:validation:XValidation:rule="!has(self.gate) || !has(self.gate.inputFrom) || (has(self.gate.inputFrom.configMapKeyRef) != has(self.gate.inputFrom.secretKeyRef))",message="exactly one of configMapKeyRef or secretKeyRef must be set"
 // +kubebuilder:validation:XValidation:rule="!has(self.gate) || !has(self.gate.encoding) || self.gate.encoding != 'binary/protobuf' || has(self.gate.messageType)",message="gate.messageType is required when gate.encoding is binary/protobuf"
 // +kubebuilder:validation:XValidation:rule="!has(self.gate) || !has(self.gate.messageType) || (has(self.gate.encoding) && (self.gate.encoding == 'json/protobuf' || self.gate.encoding == 'binary/protobuf'))",message="gate.messageType may only be set when gate.encoding is json/protobuf or binary/protobuf"
 type RolloutStrategy struct {


I think the behavior and semantics of these fields make sense, but I’m concerned about putting them under RolloutStrategy (we discussed this in yesterday's meeting and @jlegrone shares this concern).

spec.rollout describes how to roll out a target version (how to direct Temporal traffic over the rollout time period). These new fields instead describe availability and temporary capacity while replacing pods belonging to one version, which is independent: a pod replacement can happen without a Temporal version rollout. I think putting these settings under RolloutStrategy conflates the too in a way that isn't helpful to users (doesn't add simplicity to their lives).

Could we model the user-visible behavior instead? something like

spec: workerVersionSettings: podReplacement: maxUnavailable: 5% maxSurge: 0 rollout: strategy: Progressive

workerVersionSettings avoids the term perVersion while describing the scope: these settings apply to each WDV individually. podReplacement describes the behavior users care about without committing us to k8s Deployment as the backing k8s resource.

This also leaves a reasonable place for other version-scoped settings in the future (ie. replica-count or build-ID/version-status-specific overrides for certain settings) without putting unrelated settings under Temporal rollout configuration. Not saying that we add any of those now (or ever unless there is compelling reason), but I think we should put these new version-specific pod-replacement settings in a place where we would be comfortable adding similarly scoped settings in the future.

Other than the placement of the new fields, the semantics of how the fields are used looks good to me! I think it's fine to say that only rolling updates within a version's replicas are allowed, and not leave room for other Deployment update strategies

I think I'd prefer keeping these options under rollout. My reasoning is that workerVersionSettings and podReplacement are things users would have to look up (even if the naming is intuitive) to understand the exact meaning. If the options stay under rollout, I feel like it's more "obvious" what it refers to since those options are related to deployment strategy.

I guess my point is that the pod replacement strategy of a specific k8s Deployment is actually totally orthogonal to how you roll out a new target version. So it should not be under rollout.strategy.

jaypipes requested review from a team, eniko-dif and jlegrone as code owners September 1, 2026 11:14

Merge branch 'main' into rework-otan-deploy-strategy

9e47457

Shivs11 reviewed Sep 3, 2026

View reviewed changes

Comment thread api/v1alpha1/workerdeployment_webhook.go

Shivs11 reviewed Sep 3, 2026

View reviewed changes

carlydf reviewed Sep 3, 2026

View reviewed changes

Uh oh!

Conversation

jaypipes commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Sep 1, 2026

Uh oh!

jaypipes commented Sep 3, 2026

Uh oh!

otan commented Sep 3, 2026

Uh oh!

Shivs11 left a comment

Choose a reason for hiding this comment

Uh oh!

Shivs11 Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

jaypipes Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Shivs11 Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

Shivs11 Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

jaypipes Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

carlydf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlydf Sep 3, 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!

eniko-dif Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

carlydf Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jaypipes commented Sep 1, 2026 •

edited

Loading

CLAassistant commented Sep 1, 2026 •

edited

Loading

carlydf Sep 3, 2026 •

edited

Loading