From b115281a4a8772a1f010c6eb19e03b46b074ea60 Mon Sep 17 00:00:00 2001 From: qingliu Date: Mon, 27 Jul 2026 21:59:10 +0800 Subject: [PATCH] feat(common): support rollout strategy in options The additional options transformer copies a fixed list of fields from the Deployment / StatefulSet given under `options` onto the manifest. The rollout strategy was not part of that list, so `spec.strategy` and its StatefulSet counterpart `spec.updateStrategy` were dropped without any error or warning. This blocks a configuration the operator otherwise supports: `options` already carries `replicas` and `affinity`, so a component can be pinned to one replica per node, but the default rollout then has nowhere to schedule its surge pod, and `maxUnavailable` rounds down to zero at three replicas, so the rollout can never complete. There is no supported way to set it outside of `options` either: the installer set treats `spec` as a reconcile field and copies it wholesale from the expected manifest, so editing the strategy on the live object is reverted the next time the installer set reconciles a differing spec. Copy both fields when a strategy type is set. The whole struct is replaced rather than merged field by field, because `rollingUpdate` may not be set when the type is `Recreate`, or `OnDelete` for StatefulSets, and a field-wise merge would leave the base manifest's rollingUpdate block behind, producing an object the API server rejects. An empty type keeps the strategy from the base manifest. A self-contradictory strategy is passed through as given rather than partially dropped, leaving validation to the API server. updateDeploymentHashValue() zeroes the strategy before hashing, so changing only the strategy leaves the operator-generated pod-template hash unchanged and does not trigger a rollout through that mechanism. Fixes #3812 Signed-off-by: qingliu Assisted-by: Claude Opus 5 (via Claude Code) --- docs/TektonConfig.md | 17 ++ ...onal-options-base-strategy-deployment.yaml | 24 ++ ...ons-base-strategy-recreate-deployment.yaml | 21 ++ ...tions-base-updatestrategy-statefulset.yaml | 23 ++ ...ons-test-strategy-recreate-deployment.yaml | 23 ++ ...-recreate-to-rollingupdate-deployment.yaml | 26 ++ ...ecreate-with-rollingupdate-deployment.yaml | 29 ++ ...est-strategy-rollingupdate-deployment.yaml | 26 ++ ...ptions-test-strategy-unset-deployment.yaml | 26 ++ ...t-updatestrategy-ondelete-statefulset.yaml | 25 ++ ...-updatestrategy-partition-statefulset.yaml | 27 ++ ...test-updatestrategy-unset-statefulset.yaml | 27 ++ .../common/transformer_additional_options.go | 18 ++ .../transformer_additional_options_test.go | 247 ++++++++++++++++++ 14 files changed, 559 insertions(+) create mode 100644 pkg/reconciler/common/testdata/test-additional-options-base-strategy-deployment.yaml create mode 100644 pkg/reconciler/common/testdata/test-additional-options-base-strategy-recreate-deployment.yaml create mode 100644 pkg/reconciler/common/testdata/test-additional-options-base-updatestrategy-statefulset.yaml create mode 100644 pkg/reconciler/common/testdata/test-additional-options-test-strategy-recreate-deployment.yaml create mode 100644 pkg/reconciler/common/testdata/test-additional-options-test-strategy-recreate-to-rollingupdate-deployment.yaml create mode 100644 pkg/reconciler/common/testdata/test-additional-options-test-strategy-recreate-with-rollingupdate-deployment.yaml create mode 100644 pkg/reconciler/common/testdata/test-additional-options-test-strategy-rollingupdate-deployment.yaml create mode 100644 pkg/reconciler/common/testdata/test-additional-options-test-strategy-unset-deployment.yaml create mode 100644 pkg/reconciler/common/testdata/test-additional-options-test-updatestrategy-ondelete-statefulset.yaml create mode 100644 pkg/reconciler/common/testdata/test-additional-options-test-updatestrategy-partition-statefulset.yaml create mode 100644 pkg/reconciler/common/testdata/test-additional-options-test-updatestrategy-unset-statefulset.yaml diff --git a/docs/TektonConfig.md b/docs/TektonConfig.md index cb19ad4dd8..f36009dd63 100644 --- a/docs/TektonConfig.md +++ b/docs/TektonConfig.md @@ -769,6 +769,12 @@ There is a field called `options` available in all the components.
> **NOTE:** There is a possibility to have two different values for a field.
> An example: with a pre-defined field you can set value and the same field may be defined under `options` as well. In that case value from `options` will be final. +> **NOTE:** The embedded objects are not merged as a whole. The operator copies a +> known set of fields onto the manifest, and the per-kind lists below are +> exhaustive - a field outside them is ignored silently, with no error or warning. +> If a field you set under `options` has no effect, it is most likely not +> supported yet - please open an issue. + A sample `options` field, ```yaml @@ -822,6 +828,11 @@ options: custom-annotation: "foo" spec: replicas: 2 + strategy: + type: RollingUpdate + rollingUpdate: + maxSurge: 0 + maxUnavailable: 1 template: spec: containers: @@ -838,6 +849,10 @@ options: custom-annotation: foo spec: replicas: 3 + updateStrategy: + type: RollingUpdate + rollingUpdate: + partition: 1 template: spec: containers: @@ -895,6 +910,7 @@ The following fields are supported in `deployment` - `annotations` - supports add and update - `spec` - `replicas` - updates deployment replicas count + - `strategy` - replaces the existing deployment strategy with this, if `type` is not empty - `template` - `metadata` - `labels` - supports add and update @@ -929,6 +945,7 @@ The following fields are supported in `StatefulSet` - `annotations` - supports add and update - `spec` - `replicas` - updates statefulSets replicas count + - `updateStrategy` - replaces the existing statefulSet update strategy with this, if `type` is not empty - `serviceName` - updates service name - `podManagementPolicy` - updates pod management policy - `volumeClaimTemplates` - updates volume claim templates diff --git a/pkg/reconciler/common/testdata/test-additional-options-base-strategy-deployment.yaml b/pkg/reconciler/common/testdata/test-additional-options-base-strategy-deployment.yaml new file mode 100644 index 0000000000..ef9062b7f2 --- /dev/null +++ b/pkg/reconciler/common/testdata/test-additional-options-base-strategy-deployment.yaml @@ -0,0 +1,24 @@ +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: tekton-pipelines-controller + namespace: tekton-pipelines +spec: + replicas: 1 + strategy: + type: RollingUpdate + rollingUpdate: + maxSurge: 25% + maxUnavailable: 25% + selector: + matchLabels: + app.kubernetes.io/name: controller + template: + metadata: + labels: + app.kubernetes.io/name: controller + spec: + containers: + - name: tekton-pipelines-controller + image: gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/controller:v0.50.1 diff --git a/pkg/reconciler/common/testdata/test-additional-options-base-strategy-recreate-deployment.yaml b/pkg/reconciler/common/testdata/test-additional-options-base-strategy-recreate-deployment.yaml new file mode 100644 index 0000000000..f01d752811 --- /dev/null +++ b/pkg/reconciler/common/testdata/test-additional-options-base-strategy-recreate-deployment.yaml @@ -0,0 +1,21 @@ +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: tekton-pipelines-controller + namespace: tekton-pipelines +spec: + replicas: 1 + strategy: + type: Recreate + selector: + matchLabels: + app.kubernetes.io/name: controller + template: + metadata: + labels: + app.kubernetes.io/name: controller + spec: + containers: + - name: tekton-pipelines-controller + image: gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/controller:v0.50.1 diff --git a/pkg/reconciler/common/testdata/test-additional-options-base-updatestrategy-statefulset.yaml b/pkg/reconciler/common/testdata/test-additional-options-base-updatestrategy-statefulset.yaml new file mode 100644 index 0000000000..335be24ecf --- /dev/null +++ b/pkg/reconciler/common/testdata/test-additional-options-base-updatestrategy-statefulset.yaml @@ -0,0 +1,23 @@ +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: web +spec: + serviceName: nginx + replicas: 2 + updateStrategy: + type: RollingUpdate + rollingUpdate: + partition: 0 + selector: + matchLabels: + app: nginx + template: + metadata: + labels: + app: nginx + spec: + containers: + - name: nginx + image: registry.k8s.io/nginx-slim:0.8 diff --git a/pkg/reconciler/common/testdata/test-additional-options-test-strategy-recreate-deployment.yaml b/pkg/reconciler/common/testdata/test-additional-options-test-strategy-recreate-deployment.yaml new file mode 100644 index 0000000000..e692e3f156 --- /dev/null +++ b/pkg/reconciler/common/testdata/test-additional-options-test-strategy-recreate-deployment.yaml @@ -0,0 +1,23 @@ +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: tekton-pipelines-controller + namespace: tekton-pipelines +status: {} +spec: + replicas: 1 + strategy: + type: Recreate + selector: + matchLabels: + app.kubernetes.io/name: controller + template: + metadata: + labels: + app.kubernetes.io/name: controller + spec: + containers: + - name: tekton-pipelines-controller + image: gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/controller:v0.50.1 + resources: {} diff --git a/pkg/reconciler/common/testdata/test-additional-options-test-strategy-recreate-to-rollingupdate-deployment.yaml b/pkg/reconciler/common/testdata/test-additional-options-test-strategy-recreate-to-rollingupdate-deployment.yaml new file mode 100644 index 0000000000..4f5c803473 --- /dev/null +++ b/pkg/reconciler/common/testdata/test-additional-options-test-strategy-recreate-to-rollingupdate-deployment.yaml @@ -0,0 +1,26 @@ +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: tekton-pipelines-controller + namespace: tekton-pipelines +status: {} +spec: + replicas: 1 + strategy: + type: RollingUpdate + rollingUpdate: + maxSurge: 0 + maxUnavailable: 1 + selector: + matchLabels: + app.kubernetes.io/name: controller + template: + metadata: + labels: + app.kubernetes.io/name: controller + spec: + containers: + - name: tekton-pipelines-controller + image: gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/controller:v0.50.1 + resources: {} diff --git a/pkg/reconciler/common/testdata/test-additional-options-test-strategy-recreate-with-rollingupdate-deployment.yaml b/pkg/reconciler/common/testdata/test-additional-options-test-strategy-recreate-with-rollingupdate-deployment.yaml new file mode 100644 index 0000000000..94d6e4c242 --- /dev/null +++ b/pkg/reconciler/common/testdata/test-additional-options-test-strategy-recreate-with-rollingupdate-deployment.yaml @@ -0,0 +1,29 @@ +--- +# The options asked for Recreate while also supplying a rollingUpdate block. +# The transformer passes both through untouched rather than silently dropping +# one of them; the API server is the component that rejects the combination. +apiVersion: apps/v1 +kind: Deployment +metadata: + name: tekton-pipelines-controller + namespace: tekton-pipelines +status: {} +spec: + replicas: 1 + strategy: + type: Recreate + rollingUpdate: + maxSurge: 0 + maxUnavailable: 1 + selector: + matchLabels: + app.kubernetes.io/name: controller + template: + metadata: + labels: + app.kubernetes.io/name: controller + spec: + containers: + - name: tekton-pipelines-controller + image: gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/controller:v0.50.1 + resources: {} diff --git a/pkg/reconciler/common/testdata/test-additional-options-test-strategy-rollingupdate-deployment.yaml b/pkg/reconciler/common/testdata/test-additional-options-test-strategy-rollingupdate-deployment.yaml new file mode 100644 index 0000000000..4f5c803473 --- /dev/null +++ b/pkg/reconciler/common/testdata/test-additional-options-test-strategy-rollingupdate-deployment.yaml @@ -0,0 +1,26 @@ +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: tekton-pipelines-controller + namespace: tekton-pipelines +status: {} +spec: + replicas: 1 + strategy: + type: RollingUpdate + rollingUpdate: + maxSurge: 0 + maxUnavailable: 1 + selector: + matchLabels: + app.kubernetes.io/name: controller + template: + metadata: + labels: + app.kubernetes.io/name: controller + spec: + containers: + - name: tekton-pipelines-controller + image: gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/controller:v0.50.1 + resources: {} diff --git a/pkg/reconciler/common/testdata/test-additional-options-test-strategy-unset-deployment.yaml b/pkg/reconciler/common/testdata/test-additional-options-test-strategy-unset-deployment.yaml new file mode 100644 index 0000000000..561900efc1 --- /dev/null +++ b/pkg/reconciler/common/testdata/test-additional-options-test-strategy-unset-deployment.yaml @@ -0,0 +1,26 @@ +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: tekton-pipelines-controller + namespace: tekton-pipelines +status: {} +spec: + replicas: 2 + strategy: + type: RollingUpdate + rollingUpdate: + maxSurge: 25% + maxUnavailable: 25% + selector: + matchLabels: + app.kubernetes.io/name: controller + template: + metadata: + labels: + app.kubernetes.io/name: controller + spec: + containers: + - name: tekton-pipelines-controller + image: gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/controller:v0.50.1 + resources: {} diff --git a/pkg/reconciler/common/testdata/test-additional-options-test-updatestrategy-ondelete-statefulset.yaml b/pkg/reconciler/common/testdata/test-additional-options-test-updatestrategy-ondelete-statefulset.yaml new file mode 100644 index 0000000000..847fbf55ac --- /dev/null +++ b/pkg/reconciler/common/testdata/test-additional-options-test-updatestrategy-ondelete-statefulset.yaml @@ -0,0 +1,25 @@ +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: web +spec: + serviceName: nginx + replicas: 2 + updateStrategy: + type: OnDelete + selector: + matchLabels: + app: nginx + template: + metadata: + labels: + app: nginx + spec: + containers: + - name: nginx + image: registry.k8s.io/nginx-slim:0.8 + resources: {} +status: + availableReplicas: 0 + replicas: 0 diff --git a/pkg/reconciler/common/testdata/test-additional-options-test-updatestrategy-partition-statefulset.yaml b/pkg/reconciler/common/testdata/test-additional-options-test-updatestrategy-partition-statefulset.yaml new file mode 100644 index 0000000000..b42d4f7b68 --- /dev/null +++ b/pkg/reconciler/common/testdata/test-additional-options-test-updatestrategy-partition-statefulset.yaml @@ -0,0 +1,27 @@ +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: web +spec: + serviceName: nginx + replicas: 2 + updateStrategy: + type: RollingUpdate + rollingUpdate: + partition: 2 + selector: + matchLabels: + app: nginx + template: + metadata: + labels: + app: nginx + spec: + containers: + - name: nginx + image: registry.k8s.io/nginx-slim:0.8 + resources: {} +status: + availableReplicas: 0 + replicas: 0 diff --git a/pkg/reconciler/common/testdata/test-additional-options-test-updatestrategy-unset-statefulset.yaml b/pkg/reconciler/common/testdata/test-additional-options-test-updatestrategy-unset-statefulset.yaml new file mode 100644 index 0000000000..78036f9995 --- /dev/null +++ b/pkg/reconciler/common/testdata/test-additional-options-test-updatestrategy-unset-statefulset.yaml @@ -0,0 +1,27 @@ +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: web +spec: + serviceName: nginx + replicas: 3 + updateStrategy: + type: RollingUpdate + rollingUpdate: + partition: 0 + selector: + matchLabels: + app: nginx + template: + metadata: + labels: + app: nginx + spec: + containers: + - name: nginx + image: registry.k8s.io/nginx-slim:0.8 + resources: {} +status: + availableReplicas: 0 + replicas: 0 diff --git a/pkg/reconciler/common/transformer_additional_options.go b/pkg/reconciler/common/transformer_additional_options.go index 9369518911..3a8468f652 100644 --- a/pkg/reconciler/common/transformer_additional_options.go +++ b/pkg/reconciler/common/transformer_additional_options.go @@ -287,6 +287,15 @@ func (ot *OptionsTransformer) updateDeployments(u *unstructured.Unstructured) er targetDeployment.Spec.Replicas = ptr.Int32(*deploymentOptions.Spec.Replicas) } + // update deployment strategy + // The whole struct is replaced instead of merged field by field: "rollingUpdate" + // may not be set when the type is "Recreate", so merging would leave the + // rollingUpdate block from the base manifest behind and the API server would + // reject the resulting deployment. + if deploymentOptions.Spec.Strategy.Type != "" { + targetDeployment.Spec.Strategy = deploymentOptions.Spec.Strategy + } + // update affinity if deploymentOptions.Spec.Template.Spec.Affinity != nil { targetDeployment.Spec.Template.Spec.Affinity = deploymentOptions.Spec.Template.Spec.Affinity @@ -589,6 +598,15 @@ func (ot *OptionsTransformer) updateStatefulSets(u *unstructured.Unstructured) e targetStatefulSet.Spec.Replicas = ptr.Int32(*statefulSetOptions.Spec.Replicas) } + // update statefulSet update strategy + // The whole struct is replaced instead of merged field by field: "rollingUpdate" + // may not be set when the type is "OnDelete", so merging would leave the + // rollingUpdate block from the base manifest behind and the API server would + // reject the resulting statefulSet. + if statefulSetOptions.Spec.UpdateStrategy.Type != "" { + targetStatefulSet.Spec.UpdateStrategy = statefulSetOptions.Spec.UpdateStrategy + } + // update affinity if statefulSetOptions.Spec.Template.Spec.Affinity != nil { targetStatefulSet.Spec.Template.Spec.Affinity = statefulSetOptions.Spec.Template.Spec.Affinity diff --git a/pkg/reconciler/common/transformer_additional_options_test.go b/pkg/reconciler/common/transformer_additional_options_test.go index 2d448af484..4c5600b830 100644 --- a/pkg/reconciler/common/transformer_additional_options_test.go +++ b/pkg/reconciler/common/transformer_additional_options_test.go @@ -31,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/util/intstr" "knative.dev/pkg/ptr" ) @@ -41,6 +42,8 @@ func TestExecuteAdditionalOptionsTransformer(t *testing.T) { ignorePolicy := admissionregistrationv1.Ignore failPolicy := admissionregistrationv1.Fail sideEffectUnknown := admissionregistrationv1.SideEffectClassUnknown + maxSurgeZero := intstr.FromInt32(0) + maxUnavailableOne := intstr.FromInt32(1) // verify the changes applied on the manifest @@ -575,6 +578,188 @@ func TestExecuteAdditionalOptionsTransformer(t *testing.T) { inputFilename: "./testdata/test-additional-options-base-webhook.yaml", expectedResultFilename: "./testdata/test-additional-options-test-webhook.yaml", }, + { + // switching to Recreate must drop the rollingUpdate block coming from + // the base manifest, otherwise the API server rejects the deployment + name: "test-strategy-recreate-for-deployments", + additionalOptions: v1alpha1.AdditionalOptions{ + Disabled: ptr.Bool(false), + Deployments: map[string]appsv1.Deployment{ + "tekton-pipelines-controller": { + Spec: appsv1.DeploymentSpec{ + Strategy: appsv1.DeploymentStrategy{ + Type: appsv1.RecreateDeploymentStrategyType, + }, + }, + }, + }, + }, + inputFilename: "./testdata/test-additional-options-base-strategy-deployment.yaml", + expectedResultFilename: "./testdata/test-additional-options-test-strategy-recreate-deployment.yaml", + }, + { + name: "test-strategy-rollingupdate-tuning-for-deployments", + additionalOptions: v1alpha1.AdditionalOptions{ + Disabled: ptr.Bool(false), + Deployments: map[string]appsv1.Deployment{ + "tekton-pipelines-controller": { + Spec: appsv1.DeploymentSpec{ + Strategy: appsv1.DeploymentStrategy{ + Type: appsv1.RollingUpdateDeploymentStrategyType, + RollingUpdate: &appsv1.RollingUpdateDeployment{ + MaxSurge: &maxSurgeZero, + MaxUnavailable: &maxUnavailableOne, + }, + }, + }, + }, + }, + }, + inputFilename: "./testdata/test-additional-options-base-strategy-deployment.yaml", + expectedResultFilename: "./testdata/test-additional-options-test-strategy-rollingupdate-deployment.yaml", + }, + { + // a deployment present in options but without a strategy must keep + // the strategy defined in the base manifest untouched + name: "test-strategy-not-set-for-deployments", + additionalOptions: v1alpha1.AdditionalOptions{ + Disabled: ptr.Bool(false), + Deployments: map[string]appsv1.Deployment{ + "tekton-pipelines-controller": { + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.Int32(2), + }, + }, + }, + }, + inputFilename: "./testdata/test-additional-options-base-strategy-deployment.yaml", + expectedResultFilename: "./testdata/test-additional-options-test-strategy-unset-deployment.yaml", + }, + { + // switching back from Recreate must bring the rollingUpdate block in, + // proving the replacement works in both directions + name: "test-strategy-recreate-to-rollingupdate-for-deployments", + additionalOptions: v1alpha1.AdditionalOptions{ + Disabled: ptr.Bool(false), + Deployments: map[string]appsv1.Deployment{ + "tekton-pipelines-controller": { + Spec: appsv1.DeploymentSpec{ + Strategy: appsv1.DeploymentStrategy{ + Type: appsv1.RollingUpdateDeploymentStrategyType, + RollingUpdate: &appsv1.RollingUpdateDeployment{ + MaxSurge: &maxSurgeZero, + MaxUnavailable: &maxUnavailableOne, + }, + }, + }, + }, + }, + }, + inputFilename: "./testdata/test-additional-options-base-strategy-recreate-deployment.yaml", + expectedResultFilename: "./testdata/test-additional-options-test-strategy-recreate-to-rollingupdate-deployment.yaml", + }, + { + // a rollingUpdate without a strategy type is deliberately ignored: + // the type drives the decision and an empty type means "keep the base" + name: "test-strategy-rollingupdate-without-type-is-ignored-for-deployments", + additionalOptions: v1alpha1.AdditionalOptions{ + Disabled: ptr.Bool(false), + Deployments: map[string]appsv1.Deployment{ + "tekton-pipelines-controller": { + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.Int32(2), + Strategy: appsv1.DeploymentStrategy{ + RollingUpdate: &appsv1.RollingUpdateDeployment{ + MaxSurge: &maxSurgeZero, + MaxUnavailable: &maxUnavailableOne, + }, + }, + }, + }, + }, + }, + inputFilename: "./testdata/test-additional-options-base-strategy-deployment.yaml", + expectedResultFilename: "./testdata/test-additional-options-test-strategy-unset-deployment.yaml", + }, + { + // a self-contradictory strategy from options is passed through as given + // rather than partially dropped; rejecting it is the API server's job + name: "test-strategy-recreate-with-rollingupdate-is-passed-through-for-deployments", + additionalOptions: v1alpha1.AdditionalOptions{ + Disabled: ptr.Bool(false), + Deployments: map[string]appsv1.Deployment{ + "tekton-pipelines-controller": { + Spec: appsv1.DeploymentSpec{ + Strategy: appsv1.DeploymentStrategy{ + Type: appsv1.RecreateDeploymentStrategyType, + RollingUpdate: &appsv1.RollingUpdateDeployment{ + MaxSurge: &maxSurgeZero, + MaxUnavailable: &maxUnavailableOne, + }, + }, + }, + }, + }, + }, + inputFilename: "./testdata/test-additional-options-base-strategy-deployment.yaml", + expectedResultFilename: "./testdata/test-additional-options-test-strategy-recreate-with-rollingupdate-deployment.yaml", + }, + { + name: "test-updatestrategy-partition-tuning-for-statefulsets", + additionalOptions: v1alpha1.AdditionalOptions{ + Disabled: ptr.Bool(false), + StatefulSets: map[string]appsv1.StatefulSet{ + "web": { + Spec: appsv1.StatefulSetSpec{ + UpdateStrategy: appsv1.StatefulSetUpdateStrategy{ + Type: appsv1.RollingUpdateStatefulSetStrategyType, + RollingUpdate: &appsv1.RollingUpdateStatefulSetStrategy{ + Partition: ptr.Int32(2), + }, + }, + }, + }, + }, + }, + inputFilename: "./testdata/test-additional-options-base-updatestrategy-statefulset.yaml", + expectedResultFilename: "./testdata/test-additional-options-test-updatestrategy-partition-statefulset.yaml", + }, + { + // a statefulSet present in options but without an update strategy must + // keep the strategy defined in the base manifest untouched + name: "test-updatestrategy-not-set-for-statefulsets", + additionalOptions: v1alpha1.AdditionalOptions{ + Disabled: ptr.Bool(false), + StatefulSets: map[string]appsv1.StatefulSet{ + "web": { + Spec: appsv1.StatefulSetSpec{ + Replicas: ptr.Int32(3), + }, + }, + }, + }, + inputFilename: "./testdata/test-additional-options-base-updatestrategy-statefulset.yaml", + expectedResultFilename: "./testdata/test-additional-options-test-updatestrategy-unset-statefulset.yaml", + }, + { + // switching to OnDelete must drop the rollingUpdate block coming from + // the base manifest, otherwise the API server rejects the statefulSet + name: "test-updatestrategy-ondelete-for-statefulsets", + additionalOptions: v1alpha1.AdditionalOptions{ + Disabled: ptr.Bool(false), + StatefulSets: map[string]appsv1.StatefulSet{ + "web": { + Spec: appsv1.StatefulSetSpec{ + UpdateStrategy: appsv1.StatefulSetUpdateStrategy{ + Type: appsv1.OnDeleteStatefulSetStrategyType, + }, + }, + }, + }, + }, + inputFilename: "./testdata/test-additional-options-base-updatestrategy-statefulset.yaml", + expectedResultFilename: "./testdata/test-additional-options-test-updatestrategy-ondelete-statefulset.yaml", + }, { name: "test-runtimeclassname-for-deployments", additionalOptions: v1alpha1.AdditionalOptions{ @@ -861,3 +1046,65 @@ func TestExecuteAdditionalOptionsTransformer(t *testing.T) { }) } } + +// The pod-template hash label drives pod recreation: it is recomputed from the +// deployment spec, but updateDeploymentHashValue() deliberately zeroes the +// strategy before hashing. Changing only the strategy must therefore leave the +// hash untouched, so switching the rollout strategy does not restart the pods. +// The table test above strips this label before comparing, so it is asserted here. +func TestDeploymentStrategyDoesNotAffectSpecHash(t *testing.T) { + ctx := context.TODO() + targetNamespace := "tekton-pipelines" + inputFile := "./testdata/test-additional-options-base-strategy-deployment.yaml" + + hashOf := func(t *testing.T, options v1alpha1.AdditionalOptions) string { + t.Helper() + manifest, err := Fetch(inputFile) + require.NoError(t, err) + require.NoError(t, ExecuteAdditionalOptionsTransformer(ctx, &manifest, targetNamespace, options)) + + for _, resource := range manifest.Resources() { + if resource.GetKind() != "Deployment" { + continue + } + labels, found, err := unstructured.NestedStringMap(resource.Object, "spec", "template", "metadata", "labels") + require.NoError(t, err) + require.True(t, found, "pod template labels not found") + hash, found := labels[v1alpha1.DeploymentSpecHashValueLabelKey] + require.True(t, found, "spec hash label not found") + return hash + } + t.Fatal("no deployment found in manifest") + return "" + } + + baseline := hashOf(t, v1alpha1.AdditionalOptions{Disabled: ptr.Bool(false)}) + + strategyOnly := hashOf(t, v1alpha1.AdditionalOptions{ + Disabled: ptr.Bool(false), + Deployments: map[string]appsv1.Deployment{ + "tekton-pipelines-controller": { + Spec: appsv1.DeploymentSpec{ + Strategy: appsv1.DeploymentStrategy{ + Type: appsv1.RecreateDeploymentStrategyType, + }, + }, + }, + }, + }) + require.Equal(t, baseline, strategyOnly, "changing the strategy must not change the spec hash") + + // a change outside the strategy must still be reflected in the hash, + // otherwise the assertion above would hold vacuously + replicasChanged := hashOf(t, v1alpha1.AdditionalOptions{ + Disabled: ptr.Bool(false), + Deployments: map[string]appsv1.Deployment{ + "tekton-pipelines-controller": { + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.Int32(5), + }, + }, + }, + }) + require.NotEqual(t, baseline, replicasChanged, "changing the replicas must change the spec hash") +}