From 1ca9b9d9faeb2665c5207dd260ae3ea1e113e683 Mon Sep 17 00:00:00 2001 From: Pau Rosello Date: Thu, 16 Jul 2026 11:21:31 +0200 Subject: [PATCH 1/5] feat: make agent Deployment rollout strategy configurable 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 #2253 Signed-off-by: Pau Rosello --- .../config/crd/bases/kagent.dev_agents.yaml | 102 ++++++ .../crd/bases/kagent.dev_sandboxagents.yaml | 102 ++++++ go/api/v1alpha2/agent_types.go | 7 + go/api/v1alpha2/zz_generated.deepcopy.go | 6 + .../translator/agent/deployments.go | 4 + .../translator/agent/manifest_builder.go | 23 +- .../translator/agent/manifest_builder_test.go | 32 ++ .../agent_with_deployment_strategy.yaml | 36 +++ .../agent_with_deployment_strategy.json | 290 ++++++++++++++++++ .../templates/kagent.dev_agents.yaml | 102 ++++++ .../templates/kagent.dev_sandboxagents.yaml | 102 ++++++ 11 files changed, 799 insertions(+), 7 deletions(-) create mode 100644 go/core/internal/controller/translator/agent/testdata/inputs/agent_with_deployment_strategy.yaml create mode 100644 go/core/internal/controller/translator/agent/testdata/outputs/agent_with_deployment_strategy.json diff --git a/go/api/config/crd/bases/kagent.dev_agents.yaml b/go/api/config/crd/bases/kagent.dev_agents.yaml index 15b08f0ca8..d54746bd24 100644 --- a/go/api/config/crd/bases/kagent.dev_agents.yaml +++ b/go/api/config/crd/bases/kagent.dev_agents.yaml @@ -3415,6 +3415,57 @@ spec: description: Cmd overrides the container entrypoint (the container's command). type: string + deploymentStrategy: + description: |- + 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. + Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + properties: + rollingUpdate: + description: |- + Rolling update config params. Present only if DeploymentStrategyType = + RollingUpdate. + properties: + maxSurge: + anyOf: + - type: integer + - type: string + description: |- + The maximum number of pods that can be scheduled above the desired number of + pods. + Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). + This can not be 0 if MaxUnavailable is 0. + Absolute number is calculated from percentage by rounding up. + Defaults to 25%. + Example: when this is set to 30%, the new ReplicaSet can be scaled up immediately when + the rolling update starts, such that the total number of old and new pods do not exceed + 130% of desired pods. Once old pods have been killed, + new ReplicaSet can be scaled up further, ensuring that total number of pods running + at any time during the update is at most 130% of desired pods. + x-kubernetes-int-or-string: true + maxUnavailable: + anyOf: + - type: integer + - type: string + description: |- + The maximum number of pods that can be unavailable during the update. + Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). + Absolute number is calculated from percentage by rounding down. + This can not be 0 if MaxSurge is 0. + Defaults to 25%. + Example: when this is set to 30%, the old ReplicaSet can be scaled down to 70% of desired pods + immediately when the rolling update starts. Once new pods are ready, old ReplicaSet + can be scaled down further, followed by scaling up the new ReplicaSet, ensuring + that the total number of pods available at all times during the update is at + least 70% of desired pods. + x-kubernetes-int-or-string: true + type: object + type: + description: Type of deployment. Can be "Recreate" or + "RollingUpdate". Default is RollingUpdate. + type: string + type: object env: description: Env are additional environment variables set on the agent container. @@ -8760,6 +8811,57 @@ spec: description: Annotations are additional annotations added to the agent pods. type: object + deploymentStrategy: + description: |- + 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. + Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + properties: + rollingUpdate: + description: |- + Rolling update config params. Present only if DeploymentStrategyType = + RollingUpdate. + properties: + maxSurge: + anyOf: + - type: integer + - type: string + description: |- + The maximum number of pods that can be scheduled above the desired number of + pods. + Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). + This can not be 0 if MaxUnavailable is 0. + Absolute number is calculated from percentage by rounding up. + Defaults to 25%. + Example: when this is set to 30%, the new ReplicaSet can be scaled up immediately when + the rolling update starts, such that the total number of old and new pods do not exceed + 130% of desired pods. Once old pods have been killed, + new ReplicaSet can be scaled up further, ensuring that total number of pods running + at any time during the update is at most 130% of desired pods. + x-kubernetes-int-or-string: true + maxUnavailable: + anyOf: + - type: integer + - type: string + description: |- + The maximum number of pods that can be unavailable during the update. + Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). + Absolute number is calculated from percentage by rounding down. + This can not be 0 if MaxSurge is 0. + Defaults to 25%. + Example: when this is set to 30%, the old ReplicaSet can be scaled down to 70% of desired pods + immediately when the rolling update starts. Once new pods are ready, old ReplicaSet + can be scaled down further, followed by scaling up the new ReplicaSet, ensuring + that the total number of pods available at all times during the update is at + least 70% of desired pods. + x-kubernetes-int-or-string: true + type: object + type: + description: Type of deployment. Can be "Recreate" or + "RollingUpdate". Default is RollingUpdate. + type: string + type: object env: description: Env are additional environment variables set on the agent container. diff --git a/go/api/config/crd/bases/kagent.dev_sandboxagents.yaml b/go/api/config/crd/bases/kagent.dev_sandboxagents.yaml index 8dd8560b8c..d0e6f40d8c 100644 --- a/go/api/config/crd/bases/kagent.dev_sandboxagents.yaml +++ b/go/api/config/crd/bases/kagent.dev_sandboxagents.yaml @@ -1072,6 +1072,57 @@ spec: description: Cmd overrides the container entrypoint (the container's command). type: string + deploymentStrategy: + description: |- + 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. + Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + properties: + rollingUpdate: + description: |- + Rolling update config params. Present only if DeploymentStrategyType = + RollingUpdate. + properties: + maxSurge: + anyOf: + - type: integer + - type: string + description: |- + The maximum number of pods that can be scheduled above the desired number of + pods. + Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). + This can not be 0 if MaxUnavailable is 0. + Absolute number is calculated from percentage by rounding up. + Defaults to 25%. + Example: when this is set to 30%, the new ReplicaSet can be scaled up immediately when + the rolling update starts, such that the total number of old and new pods do not exceed + 130% of desired pods. Once old pods have been killed, + new ReplicaSet can be scaled up further, ensuring that total number of pods running + at any time during the update is at most 130% of desired pods. + x-kubernetes-int-or-string: true + maxUnavailable: + anyOf: + - type: integer + - type: string + description: |- + The maximum number of pods that can be unavailable during the update. + Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). + Absolute number is calculated from percentage by rounding down. + This can not be 0 if MaxSurge is 0. + Defaults to 25%. + Example: when this is set to 30%, the old ReplicaSet can be scaled down to 70% of desired pods + immediately when the rolling update starts. Once new pods are ready, old ReplicaSet + can be scaled down further, followed by scaling up the new ReplicaSet, ensuring + that the total number of pods available at all times during the update is at + least 70% of desired pods. + x-kubernetes-int-or-string: true + type: object + type: + description: Type of deployment. Can be "Recreate" or + "RollingUpdate". Default is RollingUpdate. + type: string + type: object env: description: Env are additional environment variables set on the agent container. @@ -6417,6 +6468,57 @@ spec: description: Annotations are additional annotations added to the agent pods. type: object + deploymentStrategy: + description: |- + 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. + Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + properties: + rollingUpdate: + description: |- + Rolling update config params. Present only if DeploymentStrategyType = + RollingUpdate. + properties: + maxSurge: + anyOf: + - type: integer + - type: string + description: |- + The maximum number of pods that can be scheduled above the desired number of + pods. + Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). + This can not be 0 if MaxUnavailable is 0. + Absolute number is calculated from percentage by rounding up. + Defaults to 25%. + Example: when this is set to 30%, the new ReplicaSet can be scaled up immediately when + the rolling update starts, such that the total number of old and new pods do not exceed + 130% of desired pods. Once old pods have been killed, + new ReplicaSet can be scaled up further, ensuring that total number of pods running + at any time during the update is at most 130% of desired pods. + x-kubernetes-int-or-string: true + maxUnavailable: + anyOf: + - type: integer + - type: string + description: |- + The maximum number of pods that can be unavailable during the update. + Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). + Absolute number is calculated from percentage by rounding down. + This can not be 0 if MaxSurge is 0. + Defaults to 25%. + Example: when this is set to 30%, the old ReplicaSet can be scaled down to 70% of desired pods + immediately when the rolling update starts. Once new pods are ready, old ReplicaSet + can be scaled down further, followed by scaling up the new ReplicaSet, ensuring + that the total number of pods available at all times during the update is at + least 70% of desired pods. + x-kubernetes-int-or-string: true + type: object + type: + description: Type of deployment. Can be "Recreate" or + "RollingUpdate". Default is RollingUpdate. + type: string + type: object env: description: Env are additional environment variables set on the agent container. diff --git a/go/api/v1alpha2/agent_types.go b/go/api/v1alpha2/agent_types.go index 2303687590..63c5a66f18 100644 --- a/go/api/v1alpha2/agent_types.go +++ b/go/api/v1alpha2/agent_types.go @@ -20,6 +20,7 @@ import ( "context" "fmt" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -484,6 +485,12 @@ type SharedDeploymentSpec struct { // Useful for sidecars such as token proxies, log shippers, or security agents. // +optional ExtraContainers []corev1.Container `json:"extraContainers,omitempty"` + // 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. + // Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + // +optional + DeploymentStrategy *appsv1.DeploymentStrategy `json:"deploymentStrategy,omitempty"` } type ServiceAccountConfig struct { diff --git a/go/api/v1alpha2/zz_generated.deepcopy.go b/go/api/v1alpha2/zz_generated.deepcopy.go index 98d89ec396..95ae996318 100644 --- a/go/api/v1alpha2/zz_generated.deepcopy.go +++ b/go/api/v1alpha2/zz_generated.deepcopy.go @@ -21,6 +21,7 @@ limitations under the License. package v1alpha2 import ( + appsv1 "k8s.io/api/apps/v1" "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -1822,6 +1823,11 @@ func (in *SharedDeploymentSpec) DeepCopyInto(out *SharedDeploymentSpec) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.DeploymentStrategy != nil { + in, out := &in.DeploymentStrategy, &out.DeploymentStrategy + *out = new(appsv1.DeploymentStrategy) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SharedDeploymentSpec. diff --git a/go/core/internal/controller/translator/agent/deployments.go b/go/core/internal/controller/translator/agent/deployments.go index 6b60a5692c..1bc70aa586 100644 --- a/go/core/internal/controller/translator/agent/deployments.go +++ b/go/core/internal/controller/translator/agent/deployments.go @@ -6,6 +6,7 @@ import ( "slices" "strings" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -48,6 +49,7 @@ type resolvedDeployment struct { ServiceAccountName *string ServiceAccountConfig *v1alpha2.ServiceAccountConfig ExtraContainers []corev1.Container + DeploymentStrategy *appsv1.DeploymentStrategy } // getDefaultResources sets default resource requirements if not specified @@ -274,6 +276,7 @@ func resolveInlineDeployment(agent v1alpha2.AgentObject, mdd *modelDeploymentDat ServiceAccountName: spec.ServiceAccountName, ServiceAccountConfig: spec.ServiceAccountConfig, ExtraContainers: slices.Clone(spec.ExtraContainers), + DeploymentStrategy: spec.DeploymentStrategy, } // Precedence: agent-level serviceAccountName > global default > auto-created SA (agent name) @@ -358,6 +361,7 @@ func resolveByoDeployment(agent v1alpha2.AgentObject) (*resolvedDeployment, erro ServiceAccountName: spec.ServiceAccountName, ServiceAccountConfig: spec.ServiceAccountConfig, ExtraContainers: slices.Clone(spec.ExtraContainers), + DeploymentStrategy: spec.DeploymentStrategy, } // Precedence: agent-level serviceAccountName > global default > auto-created SA (agent name) diff --git a/go/core/internal/controller/translator/agent/manifest_builder.go b/go/core/internal/controller/translator/agent/manifest_builder.go index 4003ae3906..48b61ebc33 100644 --- a/go/core/internal/controller/translator/agent/manifest_builder.go +++ b/go/core/internal/controller/translator/agent/manifest_builder.go @@ -596,13 +596,7 @@ func (a *adkApiTranslator) buildWorkloadObjects( ObjectMeta: manifestCtx.objectMeta(), Spec: appsv1.DeploymentSpec{ Replicas: manifestCtx.deployment.Replicas, - Strategy: appsv1.DeploymentStrategy{ - Type: appsv1.RollingUpdateDeploymentStrategyType, - RollingUpdate: &appsv1.RollingUpdateDeployment{ - MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: 0}, - MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: 1}, - }, - }, + Strategy: deploymentStrategy(manifestCtx.deployment.DeploymentStrategy), Selector: &metav1.LabelSelector{MatchLabels: manifestCtx.selectorLabels}, Template: podTemplate, }, @@ -619,6 +613,21 @@ func (a *adkApiTranslator) buildWorkloadObjects( }, nil } +// deploymentStrategy returns the user-provided Deployment update strategy, +// falling back to RollingUpdate{maxUnavailable: 0, maxSurge: 1} when unset. +func deploymentStrategy(strategy *appsv1.DeploymentStrategy) appsv1.DeploymentStrategy { + if strategy != nil { + return *strategy + } + return appsv1.DeploymentStrategy{ + Type: appsv1.RollingUpdateDeploymentStrategyType, + RollingUpdate: &appsv1.RollingUpdateDeployment{ + MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: 0}, + MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: 1}, + }, + } +} + func (a *adkApiTranslator) setManifestOwnerReferences( agent v1alpha2.AgentObject, manifest []client.Object, diff --git a/go/core/internal/controller/translator/agent/manifest_builder_test.go b/go/core/internal/controller/translator/agent/manifest_builder_test.go index 8c01a2af58..9845d2d351 100644 --- a/go/core/internal/controller/translator/agent/manifest_builder_test.go +++ b/go/core/internal/controller/translator/agent/manifest_builder_test.go @@ -2,10 +2,13 @@ package agent import ( "encoding/json" + "reflect" "testing" "github.com/kagent-dev/kagent/go/api/v1alpha2" + appsv1 "k8s.io/api/apps/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" ) func TestBuildSRTSettingsJSON_DefaultDenyConfig(t *testing.T) { @@ -117,3 +120,32 @@ func TestBuildConfigSecretData_IncludesSRTSettingsWhenPresent(t *testing.T) { t.Fatal("srt-settings.json should be present when non-empty") } } + +func TestDeploymentStrategy(t *testing.T) { + defaultStrategy := appsv1.DeploymentStrategy{ + Type: appsv1.RollingUpdateDeploymentStrategyType, + RollingUpdate: &appsv1.RollingUpdateDeployment{ + MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: 0}, + MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: 1}, + }, + } + recreate := appsv1.DeploymentStrategy{Type: appsv1.RecreateDeploymentStrategyType} + + tests := []struct { + name string + input *appsv1.DeploymentStrategy + want appsv1.DeploymentStrategy + }{ + {name: "nil falls back to default RollingUpdate", input: nil, want: defaultStrategy}, + {name: "override with Recreate is used", input: &recreate, want: recreate}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := deploymentStrategy(tt.input) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("deploymentStrategy() = %+v, want %+v", got, tt.want) + } + }) + } +} diff --git a/go/core/internal/controller/translator/agent/testdata/inputs/agent_with_deployment_strategy.yaml b/go/core/internal/controller/translator/agent/testdata/inputs/agent_with_deployment_strategy.yaml new file mode 100644 index 0000000000..d713e0aa30 --- /dev/null +++ b/go/core/internal/controller/translator/agent/testdata/inputs/agent_with_deployment_strategy.yaml @@ -0,0 +1,36 @@ +operation: translateAgent +targetObject: agent-with-deployment-strategy +namespace: test +objects: + - apiVersion: v1 + kind: Secret + metadata: + name: openai-secret + namespace: test + data: + api-key: c2stdGVzdC1hcGkta2V5 # base64 encoded "sk-test-api-key" + - apiVersion: kagent.dev/v1alpha2 + kind: ModelConfig + metadata: + name: basic-model + namespace: test + spec: + provider: OpenAI + model: gpt-4o + apiKeySecret: openai-secret + apiKeySecretKey: api-key + - apiVersion: kagent.dev/v1alpha2 + kind: Agent + metadata: + name: agent-with-deployment-strategy + namespace: test + spec: + type: Declarative + declarative: + description: A test agent with a Recreate deployment strategy + systemMessage: You are a helpful assistant. + modelConfig: basic-model + deployment: + deploymentStrategy: + type: Recreate + tools: [] diff --git a/go/core/internal/controller/translator/agent/testdata/outputs/agent_with_deployment_strategy.json b/go/core/internal/controller/translator/agent/testdata/outputs/agent_with_deployment_strategy.json new file mode 100644 index 0000000000..bff53dbf69 --- /dev/null +++ b/go/core/internal/controller/translator/agent/testdata/outputs/agent_with_deployment_strategy.json @@ -0,0 +1,290 @@ +{ + "agentCard": { + "capabilities": { + "streaming": true + }, + "defaultInputModes": [ + "text" + ], + "defaultOutputModes": [ + "text" + ], + "description": "", + "name": "agent_with_deployment_strategy", + "skills": null, + "supportedInterfaces": [ + { + "protocolBinding": "JSONRPC", + "protocolVersion": "0.3", + "url": "http://agent-with-deployment-strategy.test:8080" + }, + { + "protocolBinding": "JSONRPC", + "protocolVersion": "1.0", + "url": "http://agent-with-deployment-strategy.test:8080" + } + ], + "version": "" + }, + "config": { + "description": "", + "instruction": "You are a helpful assistant.", + "model": { + "base_url": "", + "model": "gpt-4o", + "type": "openai" + }, + "stream": false + }, + "manifest": [ + { + "apiVersion": "v1", + "kind": "Secret", + "metadata": { + "labels": { + "app": "kagent", + "app.kubernetes.io/managed-by": "kagent", + "app.kubernetes.io/name": "agent-with-deployment-strategy", + "app.kubernetes.io/part-of": "kagent", + "kagent": "agent-with-deployment-strategy" + }, + "name": "agent-with-deployment-strategy", + "namespace": "test", + "ownerReferences": [ + { + "apiVersion": "kagent.dev/v1alpha2", + "blockOwnerDeletion": true, + "controller": true, + "kind": "Agent", + "name": "agent-with-deployment-strategy", + "uid": "" + } + ] + }, + "stringData": { + "agent-card.json": "{\n \"defaultInputModes\": [\n \"text\"\n ],\n \"defaultOutputModes\": [\n \"text\"\n ],\n \"description\": \"\",\n \"name\": \"agent_with_deployment_strategy\",\n \"version\": \"\",\n \"skills\": [],\n \"capabilities\": {\n \"streaming\": true\n },\n \"supportedInterfaces\": [\n {\n \"url\": \"http://agent-with-deployment-strategy.test:8080\",\n \"protocolBinding\": \"JSONRPC\",\n \"protocolVersion\": \"0.3\"\n },\n {\n \"url\": \"http://agent-with-deployment-strategy.test:8080\",\n \"protocolBinding\": \"JSONRPC\",\n \"protocolVersion\": \"1.0\"\n }\n ],\n \"url\": \"http://agent-with-deployment-strategy.test:8080\",\n \"protocolVersion\": \"0.3\",\n \"preferredTransport\": \"JSONRPC\"\n}", + "config.json": "{\"model\":{\"type\":\"openai\",\"model\":\"gpt-4o\",\"base_url\":\"\"},\"description\":\"\",\"instruction\":\"You are a helpful assistant.\",\"stream\":false}" + } + }, + { + "apiVersion": "v1", + "kind": "ServiceAccount", + "metadata": { + "labels": { + "app": "kagent", + "app.kubernetes.io/managed-by": "kagent", + "app.kubernetes.io/name": "agent-with-deployment-strategy", + "app.kubernetes.io/part-of": "kagent", + "kagent": "agent-with-deployment-strategy" + }, + "name": "agent-with-deployment-strategy", + "namespace": "test", + "ownerReferences": [ + { + "apiVersion": "kagent.dev/v1alpha2", + "blockOwnerDeletion": true, + "controller": true, + "kind": "Agent", + "name": "agent-with-deployment-strategy", + "uid": "" + } + ] + } + }, + { + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": { + "labels": { + "app": "kagent", + "app.kubernetes.io/managed-by": "kagent", + "app.kubernetes.io/name": "agent-with-deployment-strategy", + "app.kubernetes.io/part-of": "kagent", + "kagent": "agent-with-deployment-strategy" + }, + "name": "agent-with-deployment-strategy", + "namespace": "test", + "ownerReferences": [ + { + "apiVersion": "kagent.dev/v1alpha2", + "blockOwnerDeletion": true, + "controller": true, + "kind": "Agent", + "name": "agent-with-deployment-strategy", + "uid": "" + } + ] + }, + "spec": { + "selector": { + "matchLabels": { + "app": "kagent", + "kagent": "agent-with-deployment-strategy" + } + }, + "strategy": { + "type": "Recreate" + }, + "template": { + "metadata": { + "annotations": { + "kagent.dev/config-hash": "17166104137763354010" + }, + "labels": { + "app": "kagent", + "app.kubernetes.io/managed-by": "kagent", + "app.kubernetes.io/name": "agent-with-deployment-strategy", + "app.kubernetes.io/part-of": "kagent", + "kagent": "agent-with-deployment-strategy" + } + }, + "spec": { + "containers": [ + { + "args": [ + "--host", + "0.0.0.0", + "--port", + "8080", + "--filepath", + "/config" + ], + "env": [ + { + "name": "OPENAI_API_KEY", + "valueFrom": { + "secretKeyRef": { + "key": "api-key", + "name": "openai-secret" + } + } + }, + { + "name": "KAGENT_NAMESPACE", + "valueFrom": { + "fieldRef": { + "fieldPath": "metadata.namespace" + } + } + }, + { + "name": "KAGENT_NAME", + "value": "agent-with-deployment-strategy" + }, + { + "name": "KAGENT_URL", + "value": "http://kagent-controller.kagent:8083" + } + ], + "image": "ghcr.io/kagent-dev/kagent/app:dev", + "imagePullPolicy": "IfNotPresent", + "name": "kagent", + "ports": [ + { + "containerPort": 8080, + "name": "http" + } + ], + "readinessProbe": { + "httpGet": { + "path": "/.well-known/agent-card.json", + "port": "http" + }, + "initialDelaySeconds": 15, + "periodSeconds": 15, + "timeoutSeconds": 15 + }, + "resources": { + "limits": { + "cpu": "2", + "memory": "1Gi" + }, + "requests": { + "cpu": "100m", + "memory": "384Mi" + } + }, + "volumeMounts": [ + { + "mountPath": "/config", + "name": "config" + }, + { + "mountPath": "/var/run/secrets/tokens", + "name": "kagent-token" + } + ] + } + ], + "serviceAccountName": "agent-with-deployment-strategy", + "volumes": [ + { + "name": "config", + "secret": { + "secretName": "agent-with-deployment-strategy" + } + }, + { + "name": "kagent-token", + "projected": { + "sources": [ + { + "serviceAccountToken": { + "audience": "kagent", + "expirationSeconds": 3600, + "path": "kagent-token" + } + } + ] + } + } + ] + } + } + }, + "status": {} + }, + { + "apiVersion": "v1", + "kind": "Service", + "metadata": { + "labels": { + "app": "kagent", + "app.kubernetes.io/managed-by": "kagent", + "app.kubernetes.io/name": "agent-with-deployment-strategy", + "app.kubernetes.io/part-of": "kagent", + "kagent": "agent-with-deployment-strategy" + }, + "name": "agent-with-deployment-strategy", + "namespace": "test", + "ownerReferences": [ + { + "apiVersion": "kagent.dev/v1alpha2", + "blockOwnerDeletion": true, + "controller": true, + "kind": "Agent", + "name": "agent-with-deployment-strategy", + "uid": "" + } + ] + }, + "spec": { + "ports": [ + { + "name": "http", + "port": 8080, + "targetPort": 8080 + } + ], + "selector": { + "app": "kagent", + "kagent": "agent-with-deployment-strategy" + }, + "type": "ClusterIP" + }, + "status": { + "loadBalancer": {} + } + } + ] +} \ No newline at end of file diff --git a/helm/kagent-crds/templates/kagent.dev_agents.yaml b/helm/kagent-crds/templates/kagent.dev_agents.yaml index 15b08f0ca8..d54746bd24 100644 --- a/helm/kagent-crds/templates/kagent.dev_agents.yaml +++ b/helm/kagent-crds/templates/kagent.dev_agents.yaml @@ -3415,6 +3415,57 @@ spec: description: Cmd overrides the container entrypoint (the container's command). type: string + deploymentStrategy: + description: |- + 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. + Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + properties: + rollingUpdate: + description: |- + Rolling update config params. Present only if DeploymentStrategyType = + RollingUpdate. + properties: + maxSurge: + anyOf: + - type: integer + - type: string + description: |- + The maximum number of pods that can be scheduled above the desired number of + pods. + Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). + This can not be 0 if MaxUnavailable is 0. + Absolute number is calculated from percentage by rounding up. + Defaults to 25%. + Example: when this is set to 30%, the new ReplicaSet can be scaled up immediately when + the rolling update starts, such that the total number of old and new pods do not exceed + 130% of desired pods. Once old pods have been killed, + new ReplicaSet can be scaled up further, ensuring that total number of pods running + at any time during the update is at most 130% of desired pods. + x-kubernetes-int-or-string: true + maxUnavailable: + anyOf: + - type: integer + - type: string + description: |- + The maximum number of pods that can be unavailable during the update. + Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). + Absolute number is calculated from percentage by rounding down. + This can not be 0 if MaxSurge is 0. + Defaults to 25%. + Example: when this is set to 30%, the old ReplicaSet can be scaled down to 70% of desired pods + immediately when the rolling update starts. Once new pods are ready, old ReplicaSet + can be scaled down further, followed by scaling up the new ReplicaSet, ensuring + that the total number of pods available at all times during the update is at + least 70% of desired pods. + x-kubernetes-int-or-string: true + type: object + type: + description: Type of deployment. Can be "Recreate" or + "RollingUpdate". Default is RollingUpdate. + type: string + type: object env: description: Env are additional environment variables set on the agent container. @@ -8760,6 +8811,57 @@ spec: description: Annotations are additional annotations added to the agent pods. type: object + deploymentStrategy: + description: |- + 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. + Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + properties: + rollingUpdate: + description: |- + Rolling update config params. Present only if DeploymentStrategyType = + RollingUpdate. + properties: + maxSurge: + anyOf: + - type: integer + - type: string + description: |- + The maximum number of pods that can be scheduled above the desired number of + pods. + Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). + This can not be 0 if MaxUnavailable is 0. + Absolute number is calculated from percentage by rounding up. + Defaults to 25%. + Example: when this is set to 30%, the new ReplicaSet can be scaled up immediately when + the rolling update starts, such that the total number of old and new pods do not exceed + 130% of desired pods. Once old pods have been killed, + new ReplicaSet can be scaled up further, ensuring that total number of pods running + at any time during the update is at most 130% of desired pods. + x-kubernetes-int-or-string: true + maxUnavailable: + anyOf: + - type: integer + - type: string + description: |- + The maximum number of pods that can be unavailable during the update. + Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). + Absolute number is calculated from percentage by rounding down. + This can not be 0 if MaxSurge is 0. + Defaults to 25%. + Example: when this is set to 30%, the old ReplicaSet can be scaled down to 70% of desired pods + immediately when the rolling update starts. Once new pods are ready, old ReplicaSet + can be scaled down further, followed by scaling up the new ReplicaSet, ensuring + that the total number of pods available at all times during the update is at + least 70% of desired pods. + x-kubernetes-int-or-string: true + type: object + type: + description: Type of deployment. Can be "Recreate" or + "RollingUpdate". Default is RollingUpdate. + type: string + type: object env: description: Env are additional environment variables set on the agent container. diff --git a/helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml b/helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml index 8dd8560b8c..d0e6f40d8c 100644 --- a/helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml +++ b/helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml @@ -1072,6 +1072,57 @@ spec: description: Cmd overrides the container entrypoint (the container's command). type: string + deploymentStrategy: + description: |- + 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. + Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + properties: + rollingUpdate: + description: |- + Rolling update config params. Present only if DeploymentStrategyType = + RollingUpdate. + properties: + maxSurge: + anyOf: + - type: integer + - type: string + description: |- + The maximum number of pods that can be scheduled above the desired number of + pods. + Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). + This can not be 0 if MaxUnavailable is 0. + Absolute number is calculated from percentage by rounding up. + Defaults to 25%. + Example: when this is set to 30%, the new ReplicaSet can be scaled up immediately when + the rolling update starts, such that the total number of old and new pods do not exceed + 130% of desired pods. Once old pods have been killed, + new ReplicaSet can be scaled up further, ensuring that total number of pods running + at any time during the update is at most 130% of desired pods. + x-kubernetes-int-or-string: true + maxUnavailable: + anyOf: + - type: integer + - type: string + description: |- + The maximum number of pods that can be unavailable during the update. + Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). + Absolute number is calculated from percentage by rounding down. + This can not be 0 if MaxSurge is 0. + Defaults to 25%. + Example: when this is set to 30%, the old ReplicaSet can be scaled down to 70% of desired pods + immediately when the rolling update starts. Once new pods are ready, old ReplicaSet + can be scaled down further, followed by scaling up the new ReplicaSet, ensuring + that the total number of pods available at all times during the update is at + least 70% of desired pods. + x-kubernetes-int-or-string: true + type: object + type: + description: Type of deployment. Can be "Recreate" or + "RollingUpdate". Default is RollingUpdate. + type: string + type: object env: description: Env are additional environment variables set on the agent container. @@ -6417,6 +6468,57 @@ spec: description: Annotations are additional annotations added to the agent pods. type: object + deploymentStrategy: + description: |- + 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. + Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + properties: + rollingUpdate: + description: |- + Rolling update config params. Present only if DeploymentStrategyType = + RollingUpdate. + properties: + maxSurge: + anyOf: + - type: integer + - type: string + description: |- + The maximum number of pods that can be scheduled above the desired number of + pods. + Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). + This can not be 0 if MaxUnavailable is 0. + Absolute number is calculated from percentage by rounding up. + Defaults to 25%. + Example: when this is set to 30%, the new ReplicaSet can be scaled up immediately when + the rolling update starts, such that the total number of old and new pods do not exceed + 130% of desired pods. Once old pods have been killed, + new ReplicaSet can be scaled up further, ensuring that total number of pods running + at any time during the update is at most 130% of desired pods. + x-kubernetes-int-or-string: true + maxUnavailable: + anyOf: + - type: integer + - type: string + description: |- + The maximum number of pods that can be unavailable during the update. + Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). + Absolute number is calculated from percentage by rounding down. + This can not be 0 if MaxSurge is 0. + Defaults to 25%. + Example: when this is set to 30%, the old ReplicaSet can be scaled down to 70% of desired pods + immediately when the rolling update starts. Once new pods are ready, old ReplicaSet + can be scaled down further, followed by scaling up the new ReplicaSet, ensuring + that the total number of pods available at all times during the update is at + least 70% of desired pods. + x-kubernetes-int-or-string: true + type: object + type: + description: Type of deployment. Can be "Recreate" or + "RollingUpdate". Default is RollingUpdate. + type: string + type: object env: description: Env are additional environment variables set on the agent container. From d5946cb3dea0807f946169f814c2db656af02695 Mon Sep 17 00:00:00 2001 From: Pau Rosello Date: Thu, 16 Jul 2026 12:32:30 +0200 Subject: [PATCH 2/5] fix: merge partial deploymentStrategy with defaults and validate at admission 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 --- .../config/crd/bases/kagent.dev_agents.yaml | 20 +- .../crd/bases/kagent.dev_sandboxagents.yaml | 24 ++- go/api/v1alpha2/agent_types.go | 6 +- .../v1alpha2/deploymentstrategy_cel_test.go | 190 ++++++++++++++++++ go/api/v1alpha2/sandboxagent_types.go | 1 + .../translator/agent/deployments.go | 41 +++- .../translator/agent/deployments_test.go | 72 +++++++ .../translator/agent/manifest_builder.go | 17 +- .../translator/agent/manifest_builder_test.go | 32 --- .../templates/kagent.dev_agents.yaml | 20 +- .../templates/kagent.dev_sandboxagents.yaml | 24 ++- 11 files changed, 387 insertions(+), 60 deletions(-) create mode 100644 go/api/v1alpha2/deploymentstrategy_cel_test.go diff --git a/go/api/config/crd/bases/kagent.dev_agents.yaml b/go/api/config/crd/bases/kagent.dev_agents.yaml index d54746bd24..745d303b16 100644 --- a/go/api/config/crd/bases/kagent.dev_agents.yaml +++ b/go/api/config/crd/bases/kagent.dev_agents.yaml @@ -3420,7 +3420,10 @@ spec: 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. - Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + 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. properties: rollingUpdate: description: |- @@ -3466,6 +3469,11 @@ spec: "RollingUpdate". Default is RollingUpdate. type: string type: object + x-kubernetes-validations: + - message: rollingUpdate may not be specified when strategy + type is Recreate + rule: '!(has(self.type) && self.type == ''Recreate'' && + has(self.rollingUpdate))' env: description: Env are additional environment variables set on the agent container. @@ -8816,7 +8824,10 @@ spec: 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. - Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + 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. properties: rollingUpdate: description: |- @@ -8862,6 +8873,11 @@ spec: "RollingUpdate". Default is RollingUpdate. type: string type: object + x-kubernetes-validations: + - message: rollingUpdate may not be specified when strategy + type is Recreate + rule: '!(has(self.type) && self.type == ''Recreate'' && + has(self.rollingUpdate))' env: description: Env are additional environment variables set on the agent container. diff --git a/go/api/config/crd/bases/kagent.dev_sandboxagents.yaml b/go/api/config/crd/bases/kagent.dev_sandboxagents.yaml index d0e6f40d8c..a2acf3ae1d 100644 --- a/go/api/config/crd/bases/kagent.dev_sandboxagents.yaml +++ b/go/api/config/crd/bases/kagent.dev_sandboxagents.yaml @@ -1077,7 +1077,10 @@ spec: 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. - Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + 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. properties: rollingUpdate: description: |- @@ -1123,6 +1126,11 @@ spec: "RollingUpdate". Default is RollingUpdate. type: string type: object + x-kubernetes-validations: + - message: rollingUpdate may not be specified when strategy + type is Recreate + rule: '!(has(self.type) && self.type == ''Recreate'' && + has(self.rollingUpdate))' env: description: Env are additional environment variables set on the agent container. @@ -6473,7 +6481,10 @@ spec: 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. - Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + 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. properties: rollingUpdate: description: |- @@ -6519,6 +6530,11 @@ spec: "RollingUpdate". Default is RollingUpdate. type: string type: object + x-kubernetes-validations: + - message: rollingUpdate may not be specified when strategy + type is Recreate + rule: '!(has(self.type) && self.type == ''Recreate'' && + has(self.rollingUpdate))' env: description: Env are additional environment variables set on the agent container. @@ -11471,6 +11487,10 @@ spec: x-kubernetes-validations: - message: spec.skills is not supported for sandbox agents rule: '!has(self.skills)' + - message: deploymentStrategy is not supported for sandbox agents + 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: type must be specified rule: has(self.type) - message: type must be either Declarative or BYO diff --git a/go/api/v1alpha2/agent_types.go b/go/api/v1alpha2/agent_types.go index 63c5a66f18..e9318c476c 100644 --- a/go/api/v1alpha2/agent_types.go +++ b/go/api/v1alpha2/agent_types.go @@ -488,7 +488,11 @@ type SharedDeploymentSpec struct { // 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. - // Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + // 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"` } diff --git a/go/api/v1alpha2/deploymentstrategy_cel_test.go b/go/api/v1alpha2/deploymentstrategy_cel_test.go new file mode 100644 index 0000000000..0c89caf933 --- /dev/null +++ b/go/api/v1alpha2/deploymentstrategy_cel_test.go @@ -0,0 +1,190 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha2 + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/intstr" + ctrl_client "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" +) + +// TestDeploymentStrategyCELValidation pins the deploymentStrategy CEL rules +// against a real kube-apiserver loaded with the shipped CRDs: +// - the field-level rule rejecting type Recreate combined with a +// rollingUpdate block (the apps/v1 API server rejects the same +// combination at Deployment-apply time; this surfaces it at admission) +// - the SandboxAgentSpec rule rejecting deploymentStrategy entirely, +// since a SandboxAgent's workload is an ActorTemplate, not a Deployment +func TestDeploymentStrategyCELValidation(t *testing.T) { + testEnv := &envtest.Environment{ + BinaryAssetsDirectory: envtestAssetsDir(t), + CRDDirectoryPaths: []string{crdBasesDir(t)}, + ErrorIfCRDPathMissing: true, + } + cfg, err := testEnv.Start() + require.NoError(t, err) + t.Cleanup(func() { _ = testEnv.Stop() }) + + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, AddToScheme(scheme)) + cl, err := ctrl_client.New(cfg, ctrl_client.Options{Scheme: scheme}) + require.NoError(t, err) + + ctx := context.Background() + const ns = "depstrategy-cel" + require.NoError(t, cl.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}})) + + declarativeSpec := func(strategy *appsv1.DeploymentStrategy) AgentSpec { + return AgentSpec{ + Type: AgentType_Declarative, + Declarative: &DeclarativeAgentSpec{ + Deployment: &DeclarativeDeploymentSpec{ + SharedDeploymentSpec: SharedDeploymentSpec{ + DeploymentStrategy: strategy, + }, + }, + }, + } + } + + maxUnavailable := intstr.FromInt32(0) + rollingUpdate := &appsv1.RollingUpdateDeployment{MaxUnavailable: &maxUnavailable} + + cases := []struct { + name string + build func() ctrl_client.Object + wantReject string // substring in admission error; empty means accept + }{ + { + name: "Agent: Recreate with rollingUpdate rejected", + build: func() ctrl_client.Object { + return &Agent{ + ObjectMeta: metav1.ObjectMeta{Name: "ag-recreate-ru", Namespace: ns}, + Spec: declarativeSpec(&appsv1.DeploymentStrategy{ + Type: appsv1.RecreateDeploymentStrategyType, + RollingUpdate: rollingUpdate, + }), + } + }, + wantReject: "rollingUpdate may not be specified when strategy type is Recreate", + }, + { + name: "Agent: Recreate without rollingUpdate accepted", + build: func() ctrl_client.Object { + return &Agent{ + ObjectMeta: metav1.ObjectMeta{Name: "ag-recreate", Namespace: ns}, + Spec: declarativeSpec(&appsv1.DeploymentStrategy{ + Type: appsv1.RecreateDeploymentStrategyType, + }), + } + }, + }, + { + name: "Agent: RollingUpdate with rollingUpdate accepted", + build: func() ctrl_client.Object { + return &Agent{ + ObjectMeta: metav1.ObjectMeta{Name: "ag-rollingupdate", Namespace: ns}, + Spec: declarativeSpec(&appsv1.DeploymentStrategy{ + Type: appsv1.RollingUpdateDeploymentStrategyType, + RollingUpdate: rollingUpdate, + }), + } + }, + }, + { + name: "Agent: type omitted with rollingUpdate accepted", + build: func() ctrl_client.Object { + return &Agent{ + ObjectMeta: metav1.ObjectMeta{Name: "ag-no-type-ru", Namespace: ns}, + Spec: declarativeSpec(&appsv1.DeploymentStrategy{ + RollingUpdate: rollingUpdate, + }), + } + }, + }, + { + name: "SandboxAgent: declarative deploymentStrategy rejected", + build: func() ctrl_client.Object { + return &SandboxAgent{ + ObjectMeta: metav1.ObjectMeta{Name: "sa-decl-strategy", Namespace: ns}, + Spec: SandboxAgentSpec{ + AgentSpec: declarativeSpec(&appsv1.DeploymentStrategy{ + Type: appsv1.RecreateDeploymentStrategyType, + }), + }, + } + }, + wantReject: "deploymentStrategy is not supported for sandbox agents", + }, + { + name: "SandboxAgent: byo deploymentStrategy rejected", + build: func() ctrl_client.Object { + return &SandboxAgent{ + ObjectMeta: metav1.ObjectMeta{Name: "sa-byo-strategy", Namespace: ns}, + Spec: SandboxAgentSpec{ + AgentSpec: AgentSpec{ + Type: AgentType_BYO, + BYO: &BYOAgentSpec{ + Deployment: &ByoDeploymentSpec{ + Image: "example.com/agent:latest", + SharedDeploymentSpec: SharedDeploymentSpec{ + DeploymentStrategy: &appsv1.DeploymentStrategy{ + Type: appsv1.RecreateDeploymentStrategyType, + }, + }, + }, + }, + }, + }, + } + }, + wantReject: "deploymentStrategy is not supported for sandbox agents", + }, + { + name: "SandboxAgent: no deploymentStrategy accepted", + build: func() ctrl_client.Object { + return &SandboxAgent{ + ObjectMeta: metav1.ObjectMeta{Name: "sa-no-strategy", Namespace: ns}, + Spec: SandboxAgentSpec{ + AgentSpec: declarativeSpec(nil), + }, + } + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + err := cl.Create(ctx, c.build()) + if c.wantReject == "" { + require.NoError(t, err) + return + } + require.Error(t, err) + require.Contains(t, err.Error(), c.wantReject) + }) + } +} diff --git a/go/api/v1alpha2/sandboxagent_types.go b/go/api/v1alpha2/sandboxagent_types.go index b1bceeeb1f..33caffbbab 100644 --- a/go/api/v1alpha2/sandboxagent_types.go +++ b/go/api/v1alpha2/sandboxagent_types.go @@ -38,6 +38,7 @@ type SandboxAgent struct { } // +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" type SandboxAgentSpec struct { AgentSpec `json:",inline"` diff --git a/go/core/internal/controller/translator/agent/deployments.go b/go/core/internal/controller/translator/agent/deployments.go index 1bc70aa586..d15ce820e3 100644 --- a/go/core/internal/controller/translator/agent/deployments.go +++ b/go/core/internal/controller/translator/agent/deployments.go @@ -9,6 +9,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" + "k8s.io/apimachinery/pkg/util/intstr" "github.com/kagent-dev/kagent/go/api/v1alpha2" "github.com/kagent-dev/kagent/go/core/internal/controller/translator/labels" @@ -49,7 +50,7 @@ type resolvedDeployment struct { ServiceAccountName *string ServiceAccountConfig *v1alpha2.ServiceAccountConfig ExtraContainers []corev1.Container - DeploymentStrategy *appsv1.DeploymentStrategy + DeploymentStrategy appsv1.DeploymentStrategy } // getDefaultResources sets default resource requirements if not specified @@ -69,6 +70,40 @@ func getDefaultResources(spec *corev1.ResourceRequirements) corev1.ResourceRequi return *spec } +// getDefaultDeploymentStrategy returns the Deployment update strategy to use, +// falling back to RollingUpdate{maxUnavailable: 0, maxSurge: 1} when unset. +// A partial RollingUpdate strategy is merged with those defaults so the emitted +// Deployment matches what the API server stores, avoiding reconcile hotloops. +// 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{ + Type: appsv1.RollingUpdateDeploymentStrategyType, + RollingUpdate: &appsv1.RollingUpdateDeployment{ + MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: 0}, + MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: 1}, + }, + } + } + + out := *strategy.DeepCopy() + if out.Type == "" { + out.Type = appsv1.RollingUpdateDeploymentStrategyType + } + if out.Type == appsv1.RollingUpdateDeploymentStrategyType { + if out.RollingUpdate == nil { + out.RollingUpdate = &appsv1.RollingUpdateDeployment{} + } + if out.RollingUpdate.MaxUnavailable == nil { + out.RollingUpdate.MaxUnavailable = &intstr.IntOrString{Type: intstr.Int, IntVal: 0} + } + if out.RollingUpdate.MaxSurge == nil { + out.RollingUpdate.MaxSurge = &intstr.IntOrString{Type: intstr.Int, IntVal: 1} + } + } + return out +} + func getDefaultLabels(agentName string, incoming map[string]string) map[string]string { defaultLabels := map[string]string{ labels.AppManagedBy: labels.ManagedByKagent, @@ -276,7 +311,7 @@ func resolveInlineDeployment(agent v1alpha2.AgentObject, mdd *modelDeploymentDat ServiceAccountName: spec.ServiceAccountName, ServiceAccountConfig: spec.ServiceAccountConfig, ExtraContainers: slices.Clone(spec.ExtraContainers), - DeploymentStrategy: spec.DeploymentStrategy, + DeploymentStrategy: getDefaultDeploymentStrategy(spec.DeploymentStrategy), } // Precedence: agent-level serviceAccountName > global default > auto-created SA (agent name) @@ -361,7 +396,7 @@ func resolveByoDeployment(agent v1alpha2.AgentObject) (*resolvedDeployment, erro ServiceAccountName: spec.ServiceAccountName, ServiceAccountConfig: spec.ServiceAccountConfig, ExtraContainers: slices.Clone(spec.ExtraContainers), - DeploymentStrategy: spec.DeploymentStrategy, + DeploymentStrategy: getDefaultDeploymentStrategy(spec.DeploymentStrategy), } // Precedence: agent-level serviceAccountName > global default > auto-created SA (agent name) diff --git a/go/core/internal/controller/translator/agent/deployments_test.go b/go/core/internal/controller/translator/agent/deployments_test.go index e937a20a01..3c5842f2ef 100644 --- a/go/core/internal/controller/translator/agent/deployments_test.go +++ b/go/core/internal/controller/translator/agent/deployments_test.go @@ -1,11 +1,14 @@ package agent import ( + "reflect" "testing" "github.com/kagent-dev/kagent/go/api/v1alpha2" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" ) func TestResolveByoDeployment_NilReplicasPreserved(t *testing.T) { @@ -125,3 +128,72 @@ func TestGetDefaultNodeSelector(t *testing.T) { } }) } + +func TestGetDefaultDeploymentStrategy(t *testing.T) { + intOrStr := func(v int32) *intstr.IntOrString { + return &intstr.IntOrString{Type: intstr.Int, IntVal: v} + } + defaultStrategy := appsv1.DeploymentStrategy{ + Type: appsv1.RollingUpdateDeploymentStrategyType, + RollingUpdate: &appsv1.RollingUpdateDeployment{ + MaxUnavailable: intOrStr(0), + MaxSurge: intOrStr(1), + }, + } + fromString := intstr.FromString("50%") + + tests := []struct { + name string + input *appsv1.DeploymentStrategy + want appsv1.DeploymentStrategy + }{ + { + name: "nil falls back to default RollingUpdate", + input: nil, + want: defaultStrategy, + }, + { + name: "Recreate passes through unchanged", + input: &appsv1.DeploymentStrategy{Type: appsv1.RecreateDeploymentStrategyType}, + want: appsv1.DeploymentStrategy{Type: appsv1.RecreateDeploymentStrategyType}, + }, + { + name: "empty type defaults to RollingUpdate with defaults filled", + input: &appsv1.DeploymentStrategy{}, + want: defaultStrategy, + }, + { + name: "RollingUpdate with nil rollingUpdate block gets defaults filled", + input: &appsv1.DeploymentStrategy{Type: appsv1.RollingUpdateDeploymentStrategyType}, + want: defaultStrategy, + }, + { + name: "partial rollingUpdate keeps user value and fills the rest", + input: &appsv1.DeploymentStrategy{ + Type: appsv1.RollingUpdateDeploymentStrategyType, + RollingUpdate: &appsv1.RollingUpdateDeployment{ + MaxSurge: &fromString, + }, + }, + want: appsv1.DeploymentStrategy{ + Type: appsv1.RollingUpdateDeploymentStrategyType, + RollingUpdate: &appsv1.RollingUpdateDeployment{ + MaxUnavailable: intOrStr(0), + MaxSurge: &fromString, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := getDefaultDeploymentStrategy(tt.input) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("getDefaultDeploymentStrategy() = %+v, want %+v", got, tt.want) + } + if tt.input != nil && tt.input.RollingUpdate != nil && got.RollingUpdate == tt.input.RollingUpdate { + t.Error("getDefaultDeploymentStrategy() must not alias the input's RollingUpdate pointer") + } + }) + } +} diff --git a/go/core/internal/controller/translator/agent/manifest_builder.go b/go/core/internal/controller/translator/agent/manifest_builder.go index 48b61ebc33..e1d88195c3 100644 --- a/go/core/internal/controller/translator/agent/manifest_builder.go +++ b/go/core/internal/controller/translator/agent/manifest_builder.go @@ -596,7 +596,7 @@ func (a *adkApiTranslator) buildWorkloadObjects( ObjectMeta: manifestCtx.objectMeta(), Spec: appsv1.DeploymentSpec{ Replicas: manifestCtx.deployment.Replicas, - Strategy: deploymentStrategy(manifestCtx.deployment.DeploymentStrategy), + Strategy: manifestCtx.deployment.DeploymentStrategy, Selector: &metav1.LabelSelector{MatchLabels: manifestCtx.selectorLabels}, Template: podTemplate, }, @@ -613,21 +613,6 @@ func (a *adkApiTranslator) buildWorkloadObjects( }, nil } -// deploymentStrategy returns the user-provided Deployment update strategy, -// falling back to RollingUpdate{maxUnavailable: 0, maxSurge: 1} when unset. -func deploymentStrategy(strategy *appsv1.DeploymentStrategy) appsv1.DeploymentStrategy { - if strategy != nil { - return *strategy - } - return appsv1.DeploymentStrategy{ - Type: appsv1.RollingUpdateDeploymentStrategyType, - RollingUpdate: &appsv1.RollingUpdateDeployment{ - MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: 0}, - MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: 1}, - }, - } -} - func (a *adkApiTranslator) setManifestOwnerReferences( agent v1alpha2.AgentObject, manifest []client.Object, diff --git a/go/core/internal/controller/translator/agent/manifest_builder_test.go b/go/core/internal/controller/translator/agent/manifest_builder_test.go index 9845d2d351..8c01a2af58 100644 --- a/go/core/internal/controller/translator/agent/manifest_builder_test.go +++ b/go/core/internal/controller/translator/agent/manifest_builder_test.go @@ -2,13 +2,10 @@ package agent import ( "encoding/json" - "reflect" "testing" "github.com/kagent-dev/kagent/go/api/v1alpha2" - appsv1 "k8s.io/api/apps/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/intstr" ) func TestBuildSRTSettingsJSON_DefaultDenyConfig(t *testing.T) { @@ -120,32 +117,3 @@ func TestBuildConfigSecretData_IncludesSRTSettingsWhenPresent(t *testing.T) { t.Fatal("srt-settings.json should be present when non-empty") } } - -func TestDeploymentStrategy(t *testing.T) { - defaultStrategy := appsv1.DeploymentStrategy{ - Type: appsv1.RollingUpdateDeploymentStrategyType, - RollingUpdate: &appsv1.RollingUpdateDeployment{ - MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: 0}, - MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: 1}, - }, - } - recreate := appsv1.DeploymentStrategy{Type: appsv1.RecreateDeploymentStrategyType} - - tests := []struct { - name string - input *appsv1.DeploymentStrategy - want appsv1.DeploymentStrategy - }{ - {name: "nil falls back to default RollingUpdate", input: nil, want: defaultStrategy}, - {name: "override with Recreate is used", input: &recreate, want: recreate}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := deploymentStrategy(tt.input) - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("deploymentStrategy() = %+v, want %+v", got, tt.want) - } - }) - } -} diff --git a/helm/kagent-crds/templates/kagent.dev_agents.yaml b/helm/kagent-crds/templates/kagent.dev_agents.yaml index d54746bd24..745d303b16 100644 --- a/helm/kagent-crds/templates/kagent.dev_agents.yaml +++ b/helm/kagent-crds/templates/kagent.dev_agents.yaml @@ -3420,7 +3420,10 @@ spec: 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. - Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + 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. properties: rollingUpdate: description: |- @@ -3466,6 +3469,11 @@ spec: "RollingUpdate". Default is RollingUpdate. type: string type: object + x-kubernetes-validations: + - message: rollingUpdate may not be specified when strategy + type is Recreate + rule: '!(has(self.type) && self.type == ''Recreate'' && + has(self.rollingUpdate))' env: description: Env are additional environment variables set on the agent container. @@ -8816,7 +8824,10 @@ spec: 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. - Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + 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. properties: rollingUpdate: description: |- @@ -8862,6 +8873,11 @@ spec: "RollingUpdate". Default is RollingUpdate. type: string type: object + x-kubernetes-validations: + - message: rollingUpdate may not be specified when strategy + type is Recreate + rule: '!(has(self.type) && self.type == ''Recreate'' && + has(self.rollingUpdate))' env: description: Env are additional environment variables set on the agent container. diff --git a/helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml b/helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml index d0e6f40d8c..a2acf3ae1d 100644 --- a/helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml +++ b/helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml @@ -1077,7 +1077,10 @@ spec: 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. - Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + 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. properties: rollingUpdate: description: |- @@ -1123,6 +1126,11 @@ spec: "RollingUpdate". Default is RollingUpdate. type: string type: object + x-kubernetes-validations: + - message: rollingUpdate may not be specified when strategy + type is Recreate + rule: '!(has(self.type) && self.type == ''Recreate'' && + has(self.rollingUpdate))' env: description: Env are additional environment variables set on the agent container. @@ -6473,7 +6481,10 @@ spec: 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. - Defaults to RollingUpdate with maxUnavailable 0 and maxSurge 1 when unset. + 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. properties: rollingUpdate: description: |- @@ -6519,6 +6530,11 @@ spec: "RollingUpdate". Default is RollingUpdate. type: string type: object + x-kubernetes-validations: + - message: rollingUpdate may not be specified when strategy + type is Recreate + rule: '!(has(self.type) && self.type == ''Recreate'' && + has(self.rollingUpdate))' env: description: Env are additional environment variables set on the agent container. @@ -11471,6 +11487,10 @@ spec: x-kubernetes-validations: - message: spec.skills is not supported for sandbox agents rule: '!has(self.skills)' + - message: deploymentStrategy is not supported for sandbox agents + 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: type must be specified rule: has(self.type) - message: type must be either Declarative or BYO From b32f68432d09644d5fddd3cca4dcd6c36d4a096d Mon Sep 17 00:00:00 2001 From: Pau Rosello Date: Thu, 16 Jul 2026 12:58:30 +0200 Subject: [PATCH 3/5] fix: restrict deploymentStrategy.type to RollingUpdate or Recreate 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 --- go/api/config/crd/bases/kagent.dev_agents.yaml | 6 ++++++ .../config/crd/bases/kagent.dev_sandboxagents.yaml | 6 ++++++ go/api/v1alpha2/agent_types.go | 1 + go/api/v1alpha2/deploymentstrategy_cel_test.go | 12 ++++++++++++ helm/kagent-crds/templates/kagent.dev_agents.yaml | 6 ++++++ .../templates/kagent.dev_sandboxagents.yaml | 6 ++++++ 6 files changed, 37 insertions(+) diff --git a/go/api/config/crd/bases/kagent.dev_agents.yaml b/go/api/config/crd/bases/kagent.dev_agents.yaml index 745d303b16..09374ce1ef 100644 --- a/go/api/config/crd/bases/kagent.dev_agents.yaml +++ b/go/api/config/crd/bases/kagent.dev_agents.yaml @@ -3470,6 +3470,9 @@ spec: type: string type: object x-kubernetes-validations: + - message: strategy type must be RollingUpdate or Recreate + rule: '!has(self.type) || self.type in [''RollingUpdate'', + ''Recreate'']' - message: rollingUpdate may not be specified when strategy type is Recreate rule: '!(has(self.type) && self.type == ''Recreate'' && @@ -8874,6 +8877,9 @@ spec: type: string type: object x-kubernetes-validations: + - message: strategy type must be RollingUpdate or Recreate + rule: '!has(self.type) || self.type in [''RollingUpdate'', + ''Recreate'']' - message: rollingUpdate may not be specified when strategy type is Recreate rule: '!(has(self.type) && self.type == ''Recreate'' && diff --git a/go/api/config/crd/bases/kagent.dev_sandboxagents.yaml b/go/api/config/crd/bases/kagent.dev_sandboxagents.yaml index a2acf3ae1d..f359326908 100644 --- a/go/api/config/crd/bases/kagent.dev_sandboxagents.yaml +++ b/go/api/config/crd/bases/kagent.dev_sandboxagents.yaml @@ -1127,6 +1127,9 @@ spec: type: string type: object x-kubernetes-validations: + - message: strategy type must be RollingUpdate or Recreate + rule: '!has(self.type) || self.type in [''RollingUpdate'', + ''Recreate'']' - message: rollingUpdate may not be specified when strategy type is Recreate rule: '!(has(self.type) && self.type == ''Recreate'' && @@ -6531,6 +6534,9 @@ spec: type: string type: object x-kubernetes-validations: + - message: strategy type must be RollingUpdate or Recreate + rule: '!has(self.type) || self.type in [''RollingUpdate'', + ''Recreate'']' - message: rollingUpdate may not be specified when strategy type is Recreate rule: '!(has(self.type) && self.type == ''Recreate'' && diff --git a/go/api/v1alpha2/agent_types.go b/go/api/v1alpha2/agent_types.go index e9318c476c..6e10b837b8 100644 --- a/go/api/v1alpha2/agent_types.go +++ b/go/api/v1alpha2/agent_types.go @@ -492,6 +492,7 @@ type SharedDeploymentSpec struct { // 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 in ['RollingUpdate', 'Recreate']",message="strategy type must be RollingUpdate or Recreate" // +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"` diff --git a/go/api/v1alpha2/deploymentstrategy_cel_test.go b/go/api/v1alpha2/deploymentstrategy_cel_test.go index 0c89caf933..67af640d77 100644 --- a/go/api/v1alpha2/deploymentstrategy_cel_test.go +++ b/go/api/v1alpha2/deploymentstrategy_cel_test.go @@ -125,6 +125,18 @@ func TestDeploymentStrategyCELValidation(t *testing.T) { } }, }, + { + name: "Agent: invalid strategy type rejected", + build: func() ctrl_client.Object { + return &Agent{ + ObjectMeta: metav1.ObjectMeta{Name: "ag-invalid-type", Namespace: ns}, + Spec: declarativeSpec(&appsv1.DeploymentStrategy{ + Type: appsv1.DeploymentStrategyType("Foo"), + }), + } + }, + wantReject: "strategy type must be RollingUpdate or Recreate", + }, { name: "SandboxAgent: declarative deploymentStrategy rejected", build: func() ctrl_client.Object { diff --git a/helm/kagent-crds/templates/kagent.dev_agents.yaml b/helm/kagent-crds/templates/kagent.dev_agents.yaml index 745d303b16..09374ce1ef 100644 --- a/helm/kagent-crds/templates/kagent.dev_agents.yaml +++ b/helm/kagent-crds/templates/kagent.dev_agents.yaml @@ -3470,6 +3470,9 @@ spec: type: string type: object x-kubernetes-validations: + - message: strategy type must be RollingUpdate or Recreate + rule: '!has(self.type) || self.type in [''RollingUpdate'', + ''Recreate'']' - message: rollingUpdate may not be specified when strategy type is Recreate rule: '!(has(self.type) && self.type == ''Recreate'' && @@ -8874,6 +8877,9 @@ spec: type: string type: object x-kubernetes-validations: + - message: strategy type must be RollingUpdate or Recreate + rule: '!has(self.type) || self.type in [''RollingUpdate'', + ''Recreate'']' - message: rollingUpdate may not be specified when strategy type is Recreate rule: '!(has(self.type) && self.type == ''Recreate'' && diff --git a/helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml b/helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml index a2acf3ae1d..f359326908 100644 --- a/helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml +++ b/helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml @@ -1127,6 +1127,9 @@ spec: type: string type: object x-kubernetes-validations: + - message: strategy type must be RollingUpdate or Recreate + rule: '!has(self.type) || self.type in [''RollingUpdate'', + ''Recreate'']' - message: rollingUpdate may not be specified when strategy type is Recreate rule: '!(has(self.type) && self.type == ''Recreate'' && @@ -6531,6 +6534,9 @@ spec: type: string type: object x-kubernetes-validations: + - message: strategy type must be RollingUpdate or Recreate + rule: '!has(self.type) || self.type in [''RollingUpdate'', + ''Recreate'']' - message: rollingUpdate may not be specified when strategy type is Recreate rule: '!(has(self.type) && self.type == ''Recreate'' && From a71a1c327f98352072b3c5ce1b2e59ce820296b0 Mon Sep 17 00:00:00 2001 From: Pau Rosello Date: Thu, 16 Jul 2026 14:44:05 +0200 Subject: [PATCH 4/5] refactor: rename getDefaultDeploymentStrategy to getDeploymentStrategyOrDefault Addresses PR review feedback. Signed-off-by: Pau Rosello --- .../internal/controller/translator/agent/deployments.go | 8 ++++---- .../controller/translator/agent/deployments_test.go | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/go/core/internal/controller/translator/agent/deployments.go b/go/core/internal/controller/translator/agent/deployments.go index d15ce820e3..c0f994f30e 100644 --- a/go/core/internal/controller/translator/agent/deployments.go +++ b/go/core/internal/controller/translator/agent/deployments.go @@ -70,12 +70,12 @@ func getDefaultResources(spec *corev1.ResourceRequirements) corev1.ResourceRequi return *spec } -// getDefaultDeploymentStrategy returns the Deployment update strategy to use, +// getDeploymentStrategyOrDefault returns the Deployment update strategy to use, // falling back to RollingUpdate{maxUnavailable: 0, maxSurge: 1} when unset. // A partial RollingUpdate strategy is merged with those defaults so the emitted // Deployment matches what the API server stores, avoiding reconcile hotloops. // The input is deep-copied so the result never aliases the Agent CR object. -func getDefaultDeploymentStrategy(strategy *appsv1.DeploymentStrategy) appsv1.DeploymentStrategy { +func getDeploymentStrategyOrDefault(strategy *appsv1.DeploymentStrategy) appsv1.DeploymentStrategy { if strategy == nil { return appsv1.DeploymentStrategy{ Type: appsv1.RollingUpdateDeploymentStrategyType, @@ -311,7 +311,7 @@ func resolveInlineDeployment(agent v1alpha2.AgentObject, mdd *modelDeploymentDat ServiceAccountName: spec.ServiceAccountName, ServiceAccountConfig: spec.ServiceAccountConfig, ExtraContainers: slices.Clone(spec.ExtraContainers), - DeploymentStrategy: getDefaultDeploymentStrategy(spec.DeploymentStrategy), + DeploymentStrategy: getDeploymentStrategyOrDefault(spec.DeploymentStrategy), } // Precedence: agent-level serviceAccountName > global default > auto-created SA (agent name) @@ -396,7 +396,7 @@ func resolveByoDeployment(agent v1alpha2.AgentObject) (*resolvedDeployment, erro ServiceAccountName: spec.ServiceAccountName, ServiceAccountConfig: spec.ServiceAccountConfig, ExtraContainers: slices.Clone(spec.ExtraContainers), - DeploymentStrategy: getDefaultDeploymentStrategy(spec.DeploymentStrategy), + DeploymentStrategy: getDeploymentStrategyOrDefault(spec.DeploymentStrategy), } // Precedence: agent-level serviceAccountName > global default > auto-created SA (agent name) diff --git a/go/core/internal/controller/translator/agent/deployments_test.go b/go/core/internal/controller/translator/agent/deployments_test.go index 3c5842f2ef..25b61d0c31 100644 --- a/go/core/internal/controller/translator/agent/deployments_test.go +++ b/go/core/internal/controller/translator/agent/deployments_test.go @@ -129,7 +129,7 @@ func TestGetDefaultNodeSelector(t *testing.T) { }) } -func TestGetDefaultDeploymentStrategy(t *testing.T) { +func TestGetDeploymentStrategyOrDefault(t *testing.T) { intOrStr := func(v int32) *intstr.IntOrString { return &intstr.IntOrString{Type: intstr.Int, IntVal: v} } @@ -187,12 +187,12 @@ func TestGetDefaultDeploymentStrategy(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := getDefaultDeploymentStrategy(tt.input) + got := getDeploymentStrategyOrDefault(tt.input) if !reflect.DeepEqual(got, tt.want) { - t.Errorf("getDefaultDeploymentStrategy() = %+v, want %+v", got, tt.want) + t.Errorf("getDeploymentStrategyOrDefault() = %+v, want %+v", got, tt.want) } if tt.input != nil && tt.input.RollingUpdate != nil && got.RollingUpdate == tt.input.RollingUpdate { - t.Error("getDefaultDeploymentStrategy() must not alias the input's RollingUpdate pointer") + t.Error("getDeploymentStrategyOrDefault() must not alias the input's RollingUpdate pointer") } }) } From 1592583ec5ed727e05de45541e84ecddc623bf44 Mon Sep 17 00:00:00 2001 From: Pau Rosello Date: Fri, 17 Jul 2026 09:25:18 +0200 Subject: [PATCH 5/5] refactor: simplify getDeploymentStrategyOrDefault 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 #2269. Signed-off-by: Pau Rosello --- .../translator/agent/deployments.go | 35 ++++++++----------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/go/core/internal/controller/translator/agent/deployments.go b/go/core/internal/controller/translator/agent/deployments.go index c0f994f30e..ff9971860c 100644 --- a/go/core/internal/controller/translator/agent/deployments.go +++ b/go/core/internal/controller/translator/agent/deployments.go @@ -76,30 +76,25 @@ func getDefaultResources(spec *corev1.ResourceRequirements) corev1.ResourceRequi // Deployment matches what the API server stores, avoiding reconcile hotloops. // The input is deep-copied so the result never aliases the Agent CR object. func getDeploymentStrategyOrDefault(strategy *appsv1.DeploymentStrategy) appsv1.DeploymentStrategy { - if strategy == nil { - return appsv1.DeploymentStrategy{ - Type: appsv1.RollingUpdateDeploymentStrategyType, - RollingUpdate: &appsv1.RollingUpdateDeployment{ - MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: 0}, - MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: 1}, - }, - } + out := appsv1.DeploymentStrategy{} + if strategy != nil { + out = *strategy.DeepCopy() } - - out := *strategy.DeepCopy() if out.Type == "" { out.Type = appsv1.RollingUpdateDeploymentStrategyType } - if out.Type == appsv1.RollingUpdateDeploymentStrategyType { - if out.RollingUpdate == nil { - out.RollingUpdate = &appsv1.RollingUpdateDeployment{} - } - if out.RollingUpdate.MaxUnavailable == nil { - out.RollingUpdate.MaxUnavailable = &intstr.IntOrString{Type: intstr.Int, IntVal: 0} - } - if out.RollingUpdate.MaxSurge == nil { - out.RollingUpdate.MaxSurge = &intstr.IntOrString{Type: intstr.Int, IntVal: 1} - } + // Recreate carries no rollingUpdate block; pass it through as-is. + if out.Type != appsv1.RollingUpdateDeploymentStrategyType { + return out + } + if out.RollingUpdate == nil { + out.RollingUpdate = &appsv1.RollingUpdateDeployment{} + } + if out.RollingUpdate.MaxUnavailable == nil { + out.RollingUpdate.MaxUnavailable = &intstr.IntOrString{Type: intstr.Int, IntVal: 0} + } + if out.RollingUpdate.MaxSurge == nil { + out.RollingUpdate.MaxSurge = &intstr.IntOrString{Type: intstr.Int, IntVal: 1} } return out }