feat: make agent Deployment rollout strategy configurable by paurosello · Pull Request #2269 · kagent-dev/kagent

paurosello · 2026-07-16T10:33:28Z

Problem

The agent Deployment's update strategy is hardcoded to RollingUpdate{maxUnavailable: 0, maxSurge: 1} in the manifest builder. That default wedges any agent mounting a ReadWriteOnce volume: on upgrade the surge pod is created before the old pod terminates, and if it lands on a different node it stays Pending on Multi-Attach/FailedAttachVolume errors indefinitely. The generic Kubernetes answer — strategy.type: Recreate — cannot be expressed today. See #2253 for the full motivation.

Changes

Add an optional deploymentStrategy field (*appsv1.DeploymentStrategy) to SharedDeploymentSpec (v1alpha2), used by the manifest builder instead of the hardcoded literal.
Backward compatible defaulting: when unset, the strategy is exactly the previous RollingUpdate{maxUnavailable: 0, maxSurge: 1}. When set with type RollingUpdate (or type omitted), any unset rollingUpdate fields are merged with those defaults. This also means the emitted Deployment always matches what the API server stores — emitting nil fields verbatim would let the server default them to 25%, and the reconciler's DeepEqual comparison would then fire an Update on every reconcile, looping forever. Recreate is passed through as-is.
Defaulting lives in getDefaultDeploymentStrategy next to the other getDefault* helpers in the resolve functions, and deep-copies the input so the built Deployment never aliases the Agent object.
CEL validation: type: Recreate combined with a rollingUpdate block is rejected at Agent admission (the apps/v1 API server would otherwise reject the Deployment later, surfacing only as a reconcile failure). deploymentStrategy is rejected on SandboxAgent (both spec.declarative.deployment and spec.byo.deployment paths), following the existing spec.skills rejection pattern, since sandbox workloads are not backed by a Deployment.

Example

spec:
  declarative:
    deployment:
      deploymentStrategy:
        type: Recreate

Testing

Table-driven unit test for getDefaultDeploymentStrategy (nil fallback, Recreate passthrough, empty type, partial rollingUpdate merge, aliasing regression check).
Translator golden test agent_with_deployment_strategy (Recreate); existing goldens unchanged, confirming byte-identical default output.
Envtest CEL test (TestDeploymentStrategyCELValidation, 7 cases) exercising both new validation rules against a real kube-apiserver.
CRDs regenerated via make controller-manifests (including the helm/kagent-crds copies).

Add an optional DeploymentStrategy field to SharedDeploymentSpec and use it in the manifest builder instead of the hardcoded RollingUpdate{maxUnavailable: 0, maxSurge: 1} literal, falling back to that default when unset (fully backward compatible). This lets operators set strategy.type: Recreate for agents mounting ReadWriteOnce volumes, which otherwise wedge on Multi-Attach errors during a rolling update because the surge pod is created before the old pod releases the node-bound volume. Fixes kagent-dev#2253 Signed-off-by: Pau Rosello <pau@giantswarm.io>

…dmission A user-supplied RollingUpdate strategy is now merged with the maxUnavailable 0 / maxSurge 1 defaults for any unset rollingUpdate fields, so the emitted Deployment always matches what the API server stores. Emitting nil fields verbatim would let the server default them to 25%, and the reconciler's DeepEqual comparison would then trigger an Update on every reconcile, looping forever. The input is deep-copied so the built Deployment never aliases the Agent object, and the defaulting moves next to the other getDefault* helpers in the resolve functions. CEL validation rejects type=Recreate combined with a rollingUpdate block at admission time (the apps/v1 API server would otherwise reject the Deployment later, during reconcile), and rejects deploymentStrategy on SandboxAgents entirely since their workload is not backed by a Deployment. Signed-off-by: Pau Rosello <pau@giantswarm.io>

Copilot

Pull request overview

This PR makes the Agent workload’s apps/v1.Deployment rollout/update strategy configurable via the v1alpha2 API, so operators can choose Recreate (notably to avoid RWO multi-attach wedges) while preserving the existing default behavior and avoiding reconcile hot-loops caused by API-server defaulting.

Changes:

Adds deploymentStrategy (*appsv1.DeploymentStrategy) to SharedDeploymentSpec with defaulting/merging logic to keep emitted Deployments stable vs API-server defaults.
Wires the resolved/defaulted strategy into the Deployment manifest builder for both Declarative and BYO paths.
Adds CEL validations and tests (unit + envtest) plus translator golden coverage; regenerates CRDs (including Helm CRD chart copies).

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml	CRD schema updates for `deploymentStrategy` plus SandboxAgent-level CEL rejection rule.
helm/kagent-crds/templates/kagent.dev_agents.yaml	CRD schema updates for `deploymentStrategy` on Agent.
go/core/internal/controller/translator/agent/testdata/outputs/agent_with_deployment_strategy.json	New golden output covering `strategy.type: Recreate`.
go/core/internal/controller/translator/agent/testdata/inputs/agent_with_deployment_strategy.yaml	New golden input Agent specifying `deploymentStrategy`.
go/core/internal/controller/translator/agent/manifest_builder.go	Uses resolved `DeploymentStrategy` instead of hardcoded RollingUpdate literal.
go/core/internal/controller/translator/agent/deployments.go	Adds resolved deployment strategy plumbing + defaulting/merge helper.
go/core/internal/controller/translator/agent/deployments_test.go	Unit tests for deployment strategy defaulting/merging and aliasing.
go/api/v1alpha2/zz_generated.deepcopy.go	DeepCopy support for the new `DeploymentStrategy` field.
go/api/v1alpha2/sandboxagent_types.go	Adds SandboxAgent CEL rule rejecting `deploymentStrategy`.
go/api/v1alpha2/deploymentstrategy_cel_test.go	Envtest verifying new CEL validation behavior against real apiserver+CRDs.
go/api/v1alpha2/agent_types.go	API surface addition + field-level CEL for Recreate+rollingUpdate invalid combo.
go/api/config/crd/bases/kagent.dev_sandboxagents.yaml	Generated CRD base updates for `deploymentStrategy` + SandboxAgent CEL rule.
go/api/config/crd/bases/kagent.dev_agents.yaml	Generated CRD base updates for Agent `deploymentStrategy`.

Files not reviewed (1)

go/api/v1alpha2/zz_generated.deepcopy.go: Generated file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

paurosello · 2026-07-16T10:59:09Z

+	// DeploymentStrategy overrides the agent Deployment update strategy.
+	// Useful for agents mounting ReadWriteOnce volumes, which require the
+	// Recreate strategy to avoid multi-attach errors during rollouts.
+	// When unset, defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1.
+	// When set with type RollingUpdate (or with type omitted), any unset
+	// rollingUpdate fields default the same way: maxUnavailable to 0 and
+	// maxSurge to 1. Recreate is passed through as-is.
+	// +kubebuilder:validation:XValidation:rule="!(has(self.type) && self.type == 'Recreate' && has(self.rollingUpdate))",message="rollingUpdate may not be specified when strategy type is Recreate"
+	// +optional
+	DeploymentStrategy *appsv1.DeploymentStrategy `json:"deploymentStrategy,omitempty"`


Good catch — addressed in b32f684. Added a field-level CEL rule restricting type to RollingUpdate/Recreate (omitted still means RollingUpdate), regenerated the CRDs, and added an envtest case rejecting an invalid type.

should be fixed now

Reject invalid strategy types at Agent admission via CEL instead of letting them fail later when the apps/v1 Deployment is applied during reconcile. Omitted type still means RollingUpdate. Addresses PR review feedback. Signed-off-by: Pau Rosello <pau@giantswarm.io>

peterj · 2026-07-16T11:20:18Z

 }

 // +kubebuilder:validation:XValidation:rule="!has(self.skills)",message="spec.skills is not supported for sandbox agents"
+// +kubebuilder:validation:XValidation:rule="!(has(self.declarative) && has(self.declarative.deployment) && has(self.declarative.deployment.deploymentStrategy)) && !(has(self.byo) && has(self.byo.deployment) && has(self.byo.deployment.deploymentStrategy))",message="deploymentStrategy is not supported for sandbox agents"


since sandbox agents don't support deployment strategy, it might make sense to move the deploymentStrategy out of the shared spec and just have it in the declarative spec. what do you think?

makes sense

I just checked and this would then affect BYO agent and moving it to declarative doesn't remove it from the sandbox schema as SandboxAgentSpec inlines AgentSpec

Unfortunately this is more nuanced, we really need to remove all deployment related config from the SandboxAgent, but that’s a much larger change

peterj · 2026-07-16T11:36:09Z

+// The input is deep-copied so the result never aliases the Agent CR object.
+func getDefaultDeploymentStrategy(strategy *appsv1.DeploymentStrategy) appsv1.DeploymentStrategy {
+	if strategy == nil {
+		return appsv1.DeploymentStrategy{


you probably don't need this check for nil strategy here. you could create the out if strategy is not nil and then set the values like you're already setting them (if none of them are set, you're return the default values anyway). also, you can early return if the type is recreate.

Good call, simplified

peterj

added a couple of smaller comments

…yOrDefault Addresses PR review feedback. Signed-off-by: Pau Rosello <pau@giantswarm.io>

Drop the separate nil branch and early-return for non-RollingUpdate strategies. A nil strategy now starts from a zero DeploymentStrategy and flows through the same defaulting, producing the identical RollingUpdate{maxUnavailable: 0, maxSurge: 1}. Behavior unchanged. Addresses review feedback on kagent-dev#2269.

Drop the separate nil branch and early-return for non-RollingUpdate strategies. A nil strategy now starts from a zero DeploymentStrategy and flows through the same defaulting, producing the identical RollingUpdate{maxUnavailable: 0, maxSurge: 1}. Behavior unchanged. Addresses review feedback on kagent-dev#2269. Signed-off-by: Pau Rosello <pau@giantswarm.io>

paurosello added 2 commits July 16, 2026 11:21

paurosello requested a review from a team as a code owner July 16, 2026 10:33

Copilot AI review requested due to automatic review settings July 16, 2026 10:33

github-actions Bot added the enhancement New feature or request label Jul 16, 2026

paurosello mentioned this pull request Jul 16, 2026

[FEATURE] Make the agent Deployment rollout strategy configurable in SharedDeploymentSpec #2253

Open

Copilot started reviewing on behalf of paurosello July 16, 2026 10:33 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

peterj reviewed Jul 16, 2026

View reviewed changes

Comment thread go/core/internal/controller/translator/agent/deployments.go Outdated

peterj reviewed Jul 16, 2026

View reviewed changes

peterj requested changes Jul 16, 2026

View reviewed changes

refactor: rename getDefaultDeploymentStrategy to getDeploymentStrateg…

a71a1c3

…yOrDefault Addresses PR review feedback. Signed-off-by: Pau Rosello <pau@giantswarm.io>

paurosello requested a review from peterj July 17, 2026 07:24

paurosello force-pushed the fix/configurable-deployment-strategy branch from f1e5624 to 1592583 Compare July 17, 2026 07:27

Merge branch 'main' into fix/configurable-deployment-strategy

23acd20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: make agent Deployment rollout strategy configurable#2269

feat: make agent Deployment rollout strategy configurable#2269
paurosello wants to merge 6 commits into
kagent-dev:mainfrom
giantswarm:fix/configurable-deployment-strategy

paurosello commented Jul 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

paurosello Jul 16, 2026

Uh oh!

paurosello Jul 16, 2026

Uh oh!

peterj Jul 16, 2026

Uh oh!

paurosello Jul 16, 2026

Uh oh!

paurosello Jul 16, 2026

Uh oh!

EItanya Jul 16, 2026

Uh oh!

Uh oh!

peterj Jul 16, 2026

Uh oh!

paurosello Jul 17, 2026 •

edited

Loading

Uh oh!

peterj left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

paurosello commented Jul 16, 2026

Problem

Changes

Example

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

paurosello Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

paurosello Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

peterj Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

paurosello Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

paurosello Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

EItanya Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peterj Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

paurosello Jul 17, 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!

peterj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

paurosello Jul 17, 2026 •

edited

Loading