feat: allow RollingUpdate to be specified for k8s Deployment

[otan]

What

Adds an optional spec.deploymentStrategy field on WorkerDeployment that mirrors apps/v1 Deployment.spec.strategy. The controller sets it on owned versioned Deployments on create/update and reconciles drift so settings like maxUnavailable / maxSurge stick.

This is distinct from spec.rollout.strategy, which controls Temporal traffic routing across versions.

RollingUpdate settings mainly matter for in-place pod rolls on an existing version Deployment (for example kubectl rollout restart, or a pod-template change while unsafeCustomBuildID stays the same). The common new-build-ID path creates a fresh Deployment scaled up from zero, so those settings have little practical effect there even though the field is still written onto the object.

Why

Owned Deployments currently inherit the Kubernetes default rolling update (25% / 25%). On large fleets, in-place restarts of a Current version (same build ID) can take too many pollers offline at once and spike schedule_to_start. Users need a conservative strategy such as maxUnavailable: 5%.

Testing

• make generate manifests
• go test ./internal/k8s/ ./internal/planner/ -run 'Strategy|ApplyDeploymentStrategyDefaults|ReplicasNilPreserved'

Resolves #496

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ jaypipes
❌ otan
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[jaypipes]

@otan thanks so much for this contribution! I'll review a little later today or tomorrow morning. Been looking into some bug fixes earlier this week.

[jaypipes]

@otan thanks for this! :) Code looks good to me except for a couple requests (see inline). Address those small issues and I'll aim to get this into the v1.9.0 release (set to be cut on August 18th)

[otan]

all good!

[jaypipes]

Awesomesauce, thank you so much @otan :)

[otan]

all done

[jaypipes]

Thank you @otan!

[jlegrone]

I have two concerns with this:

1. Having both deploymentStrategy and rolloutStrategy may be confusing.
2. So far we haven't committed to making the underlying deployment resource type part of the public API. Ie. we could switch to statefulsets or replicasets instead in the future. Ideally we should keep the worker spec agnostic to the underlying k8s resource types.

I also think it needs to be clarified in the documentation when deploymentStrategy would apply, since the controller computes a new build ID (and thus creates a fresh k8s deployment object) for each new worker version. For example does this only matter if you manually perform a rolling update via kubectl rollout restart for a specific version?

Do we actually need to support the Recreate deployment strategy? If not, what would it look like if we only exposed maxUnavailable as an option? I think my concerns would be resolved if the new config was named like spec.maxUnavailablePerVersion which doesn't tie us to deployments as the underlying resource type.

[otan]

Having both deploymentStrategy and rolloutStrategy may be confusing.

For example does this only matter if you manually perform a rolling update via kubectl rollout restart for a specific version?

yep. we just have no control over kubectl rollout restart at the moment (and we do a manual HPA patch to get around it). i can amend that - i originally wanted to keep it to just strategy to meet the original k8s Worker definition but was told to change it. i'm happy to rename it to whatever makes you more comfortable.

So far we haven't committed to making the underlying deployment resource type part of the public API. Ie. we could switch to statefulsets or replicasets instead in the future. Ideally we should keep the worker spec agnostic to the underlying k8s resource types.

AFAICT, this deployment.strategy is an option available to both statefulsets or replicasets

[carlydf]

AFAICT, this deployment.strategy is an option available to both statefulsets or replicasets

Could you share what gives you that impression? I did some reading on both, and it seems that ReplicaSets (https://kubernetes.io/docs/concepts/workloads/controllers/replicaset/) don't have such a strategy at all; if you update the pod template of a ReplicaSet, the ReplicaSet won't change or replace any of its Pods that are already running with a different template. If, later on, the ReplicaSet wants more replicas, it will use the new pod template to create new pods (still not touching the existing pods).

StatefulSets (https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#update-strategies) have a rolling update strategy option which goes one-by-one by default. In k8s v1.35, there is a maxUnavailable field, but it is in beta.

[carlydf]

we just have no control over kubectl rollout restart at the moment (and we do a manual HPA patch to get around it).

Could you explain more what you mean by these two things? Curious about your use case (so we can try to support it without re-packaging Deployment fields if we can)

[otan]

I purely want to be able to do a kubectl rollout restart deployment <name> without having it restart up to 25% pods at a time. I have to manually patch my deployment to do this at the moment.

we ... currently have to do restarts regularly to clean up some in-app-worker-state that can get borked because, e.g. node memory leaks. forcing a new deployment to do this means we have extra pinned worker versions for no reason.

[carlydf]

thanks for elaborating! I was hung up on trying to understand how a "manual HPA patch" would help you, but patching the Deployments makes sense. And I agree folks should be able to do a rolling restart with conservative settings.

[carlydf]

Maybe a crazy idea: what if instead of exposing a new knob here, we just make the rolling update settings for all worker-controller-managed deployments conservative by default (ie. maxUnavailable=1)?

That is what the default StatefulSet update strategy does, fwiw

[jaypipes]

AFAICT, this deployment.strategy is an option available to both statefulsets or replicasets

Could you share what gives you that impression? I did some reading on both, and it seems that ReplicaSets (https://kubernetes.io/docs/concepts/workloads/controllers/replicaset/) don't have such a strategy at all; if you update the pod template of a ReplicaSet, the ReplicaSet won't change or replace any of its Pods that are already running with a different template. If, later on, the ReplicaSet wants more replicas, it will use the new pod template to create new pods (still not touching the existing pods).

Yeah, because ReplicaSets are kind of a hidden behind-the-scenes resource grouping mechanism. The Deployment is the thing that has multiple ReplicaSets that correspond to the unique hash of the Deployment's pod template spec so when referring to deployment.strategy it's really just referring to the DeploymentSpec.strategy...

[jaypipes]

@otan we merged a couple fixes in internal/planner/planner.go that have caused some conflicts for this patch. would you mind pushing a quick conflict-resolving patch? If you want me to do it, let me know, I just didn't want to step on your toes. :)

[otan]

all done i think

[jaypipes]

@otan I think the PR summary might need to be updated to reflect the reality of the patch. See inline...

[jaypipes]

Same comment about the PR summary seemingly contradicting this:

and it does not apply when the controller creates a new version.

[otan]

hehe, sorry a bit of a kubernetes noob and this slid through me (deployment version vs temporal version so I thought they made sense) - updating accordingly.

[otan]

hmm, these comments are too verbose / detail oriented (AI justifying itself) - to cut the crap i'll just remove them

[otan]

i'm about to nod off (sydney time), feel free to amend accordingly!