feat: allow RollingUpdate to be specified for k8s Deployment - #497
Conversation
a728390 to
baf447c
Compare
baf447c to
f171c37
Compare
Expose maxUnavailable/maxSurge on owned version Deployments for less disruptive in-place restarts. Co-authored-by: Cursor <cursoragent@cursor.com>
f171c37 to
e880e0e
Compare
|
@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. |
…ng-update-strategy
…ozen Co-authored-by: Cursor <cursoragent@cursor.com>
|
all good! |
…ng-update-strategy
9f11f30 to
7b3824e
Compare
|
all done |
Deep-copy strategy defaults so comparison does not mutate live Deployments, and move CR defaulting/validation onto the WorkerDeployment webhook.
7b3824e to
8672c08
Compare
| // DeploymentStrategy describes how to replace Pods for each versioned | ||
| // Deployment this controller owns. Mirrors apps/v1 Deployment.spec.strategy. | ||
| // When omitted, Kubernetes defaults apply (RollingUpdate with | ||
| // maxUnavailable/maxSurge 25%). This is distinct from spec.rollout.strategy, | ||
| // which controls Temporal traffic routing across worker versions. | ||
| // +optional | ||
| DeploymentStrategy *appsv1.DeploymentStrategy `json:"deploymentStrategy,omitempty"` |
There was a problem hiding this comment.
I have two concerns with this:
- Having both
deploymentStrategyandrolloutStrategymay be confusing. - 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.
There was a problem hiding this comment.
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
What
Adds an optional
spec.strategyfield onWorkerDeploymentthat mirrorsapps/v1Deployment.spec.strategy. The controller applies it to owned versioned Deployments on create/update and reconciles drift so settings likemaxUnavailable/maxSurgestick.This is distinct from
spec.rollout.strategy, which controls Temporal traffic routing across versions.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 spikeschedule_to_start. Users need a conservative strategy such asmaxUnavailable: 5%.Testing
make generate manifestsgo test ./internal/k8s/ ./internal/planner/ -run 'Strategy|ApplyDeploymentStrategyDefaults|ReplicasNilPreserved'Resolves #496