From 56daa4666ffa6f684d7fb9b004732b364f7e797a Mon Sep 17 00:00:00 2001 From: Nazih Ben Brahim Date: Sun, 1 Mar 2026 13:36:26 +0100 Subject: [PATCH 1/2] fix(deployment): migrate workload update calls to patch Signed-off-by: Nazih Ben Brahim --- .../deployment/deployment_controller.go | 18 ++- .../deployment/deployment_controller_test.go | 38 ++++++ .../deployment/status_patch_test.go | 113 ++++++++++++++++++ pkg/controller/deployment/sync.go | 29 +++-- pkg/util/workloads_utils.go | 11 +- pkg/util/workloads_utils_test.go | 88 ++++++++++++++ 6 files changed, 281 insertions(+), 16 deletions(-) create mode 100644 pkg/controller/deployment/status_patch_test.go diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index 9209508f..ffc0eace 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/pkg/controller/deployment/deployment_controller.go @@ -52,6 +52,13 @@ type DeploymentController struct { runtimeClient client.Client } +// patchDeploymentStatus patches the Deployment status subresource using a merge diff. +// Only changed fields are sent, which avoids overwriting unknown fields from newer API versions. +func (dc *DeploymentController) patchDeploymentStatus(ctx context.Context, oldD, newD *apps.Deployment) error { + patch := client.MergeFrom(oldD) + return dc.runtimeClient.Status().Patch(ctx, newD, patch) +} + // getReplicaSetsForDeployment uses ControllerRefManager to reconcile // ControllerRef by adopting and orphaning. // It returns the list of ReplicaSets that this Deployment should manage. @@ -100,10 +107,13 @@ func (dc *DeploymentController) syncDeployment(ctx context.Context, deployment * if reflect.DeepEqual(d.Spec.Selector, &everything) { dc.eventRecorder.Eventf(d, v1.EventTypeWarning, "SelectingAll", "This deployment is selecting all pods. A non-empty selector is required.") if d.Status.ObservedGeneration < d.Generation { - d.Status.ObservedGeneration = d.Generation - err := dc.runtimeClient.Status().Update(ctx, d) - if err != nil { - klog.Errorf("Failed to update deployment status: %v", err) + // Use Patch instead of Update to avoid erasing unknown status fields + // that a newer API server may have set (version-skew safety). + dCopy := d.DeepCopy() + dCopy.Status.ObservedGeneration = d.Generation + if err := dc.runtimeClient.Status().Patch(ctx, dCopy, client.MergeFrom(d)); err != nil { + + klog.Errorf("Failed to patch deployment status: %v", err) } } return nil diff --git a/pkg/controller/deployment/deployment_controller_test.go b/pkg/controller/deployment/deployment_controller_test.go index 20d48d04..75d4104a 100644 --- a/pkg/controller/deployment/deployment_controller_test.go +++ b/pkg/controller/deployment/deployment_controller_test.go @@ -26,6 +26,8 @@ import ( apps "k8s.io/api/apps/v1" "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + intstrutil "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/tools/record" "k8s.io/utils/pointer" @@ -236,3 +238,39 @@ func TestSyncDeployment(t *testing.T) { }) } } + +func TestSyncDeploymentSelectingAll(t *testing.T) { + deployment := generateDeployment("busybox") + deployment.Spec.Selector = &metav1.LabelSelector{} + deployment.Generation = 7 + deployment.Status.ObservedGeneration = 3 + deployment.Annotations = map[string]string{ + "test-unknown-field": "kept", + } + + fakeCtrlClient := ctrlfake.NewClientBuilder(). + WithStatusSubresource(&apps.Deployment{}). + WithObjects(&deployment). + Build() + + dc := &DeploymentController{ + eventRecorder: record.NewFakeRecorder(10), + runtimeClient: fakeCtrlClient, + } + + if err := dc.syncDeployment(context.TODO(), &deployment); err != nil { + t.Fatalf("syncDeployment returned unexpected error: %v", err) + } + + var result apps.Deployment + if err := fakeCtrlClient.Get(context.TODO(), ctrlclient.ObjectKeyFromObject(&deployment), &result); err != nil { + t.Fatalf("get deployment failed: %v", err) + } + + if result.Status.ObservedGeneration != result.Generation { + t.Fatalf("expected observedGeneration=%d, got %d", result.Generation, result.Status.ObservedGeneration) + } + if got := result.Annotations["test-unknown-field"]; got != "kept" { + t.Fatalf("expected annotation test-unknown-field=kept, got %q", got) + } +} diff --git a/pkg/controller/deployment/status_patch_test.go b/pkg/controller/deployment/status_patch_test.go new file mode 100644 index 00000000..7860cdda --- /dev/null +++ b/pkg/controller/deployment/status_patch_test.go @@ -0,0 +1,113 @@ +/* +Copyright 2022 The Kruise Authors. + +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 deployment + +import ( + "context" + "testing" + + apps "k8s.io/api/apps/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + ctrlfake "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +type statusCallCounter struct { + patchCalls int + updateCalls int +} + +type statusCountingClient struct { + ctrlclient.Client + counter statusCallCounter +} + +func (c *statusCountingClient) Status() ctrlclient.SubResourceWriter { + return &statusCountingWriter{ + SubResourceWriter: c.Client.Status(), + counter: &c.counter, + } +} + +type statusCountingWriter struct { + ctrlclient.SubResourceWriter + counter *statusCallCounter +} + +func (w *statusCountingWriter) Update(ctx context.Context, obj ctrlclient.Object, opts ...ctrlclient.SubResourceUpdateOption) error { + w.counter.updateCalls++ + return w.SubResourceWriter.Update(ctx, obj, opts...) +} + +func (w *statusCountingWriter) Patch(ctx context.Context, obj ctrlclient.Object, patch ctrlclient.Patch, opts ...ctrlclient.SubResourcePatchOption) error { + w.counter.patchCalls++ + return w.SubResourceWriter.Patch(ctx, obj, patch, opts...) +} + +func TestPatchDeploymentStatusUsesStatusPatch(t *testing.T) { + scheme := runtime.NewScheme() + if err := apps.AddToScheme(scheme); err != nil { + t.Fatalf("add apps/v1 scheme failed: %v", err) + } + + baseDeployment := &apps.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "demo", + Namespace: "default", + Generation: 5, + }, + Status: apps.DeploymentStatus{ + ObservedGeneration: 1, + }, + } + + baseClient := ctrlfake.NewClientBuilder(). + WithScheme(scheme). + WithStatusSubresource(&apps.Deployment{}). + WithObjects(baseDeployment). + Build() + countingClient := &statusCountingClient{Client: baseClient} + dc := &DeploymentController{runtimeClient: countingClient} + + current := &apps.Deployment{} + key := ctrlclient.ObjectKeyFromObject(baseDeployment) + if err := countingClient.Get(context.TODO(), key, current); err != nil { + t.Fatalf("get deployment failed: %v", err) + } + oldD := current.DeepCopy() + newD := current.DeepCopy() + newD.Status.ObservedGeneration = newD.Generation + + if err := dc.patchDeploymentStatus(context.TODO(), oldD, newD); err != nil { + t.Fatalf("patch deployment status failed: %v", err) + } + if countingClient.counter.patchCalls != 1 { + t.Fatalf("expected 1 status patch call, got %d", countingClient.counter.patchCalls) + } + if countingClient.counter.updateCalls != 0 { + t.Fatalf("expected 0 status update calls, got %d", countingClient.counter.updateCalls) + } + + result := &apps.Deployment{} + if err := countingClient.Get(context.TODO(), key, result); err != nil { + t.Fatalf("get patched deployment failed: %v", err) + } + if result.Status.ObservedGeneration != result.Generation { + t.Fatalf("expected observedGeneration=%d, got %d", result.Generation, result.Status.ObservedGeneration) + } +} diff --git a/pkg/controller/deployment/sync.go b/pkg/controller/deployment/sync.go index ee7779bb..d23c701c 100644 --- a/pkg/controller/deployment/sync.go +++ b/pkg/controller/deployment/sync.go @@ -133,6 +133,7 @@ func (dc *DeploymentController) getNewReplicaSet(ctx context.Context, d *apps.De } return rsCopy, nil } + oldD := d.DeepCopy() // Should use the revision in existingNewRS's annotation, since it set by before needsUpdate := deploymentutil.SetDeploymentRevision(d, rsCopy.Annotations[deploymentutil.RevisionAnnotation]) @@ -148,8 +149,9 @@ func (dc *DeploymentController) getNewReplicaSet(ctx context.Context, d *apps.De } if needsUpdate { - var err error - if err = dc.runtimeClient.Status().Update(ctx, d); err != nil { + // Patch instead of Update: only send the status delta so unknown + // fields from newer API servers are not erased (version-skew safety). + if err := dc.runtimeClient.Status().Patch(ctx, d, client.MergeFrom(oldD)); err != nil { return nil, err } } @@ -234,15 +236,17 @@ func (dc *DeploymentController) getNewReplicaSet(ctx context.Context, d *apps.De if d.Status.CollisionCount == nil { d.Status.CollisionCount = new(int32) } + oldD := d.DeepCopy() + preCollisionCount := *d.Status.CollisionCount *d.Status.CollisionCount++ - // Update the collisionCount for the Deployment and let it requeue by returning the original - // error. - dErr := dc.runtimeClient.Status().Update(ctx, d) + // Patch instead of Update: send only the collisionCount delta. + dBase := oldD.DeepCopy() + dErr := dc.runtimeClient.Status().Patch(ctx, d, client.MergeFrom(dBase)) if dErr == nil { klog.V(2).Infof("Found a hash collision for deployment %q - bumping collisionCount (%d->%d) to resolve it", d.Name, preCollisionCount, *d.Status.CollisionCount) } else { - klog.Errorf("Failed to update deployment collision count: %v", dErr) + klog.Errorf("Failed to patch deployment collision count: %v", dErr) } return nil, err case errors.HasStatusCause(err, v1.NamespaceTerminatingCause): @@ -251,13 +255,15 @@ func (dc *DeploymentController) getNewReplicaSet(ctx context.Context, d *apps.De case err != nil: msg := fmt.Sprintf("Failed to create new replica set %q: %v", newRS.Name, err) if deploymentutil.HasProgressDeadline(d) { + oldD := d.DeepCopy() cond := deploymentutil.NewDeploymentCondition(apps.DeploymentProgressing, v1.ConditionFalse, deploymentutil.FailedRSCreateReason, msg) + // Patch instead of Update: only the new condition is sent deploymentutil.SetDeploymentCondition(&d.Status, *cond) // We don't really care about this error at this point, since we have a bigger issue to report. // TODO: Identify which errors are permanent and switch DeploymentIsFailed to take into account // these reasons as well. Related issue: https://github.com/kubernetes/kubernetes/issues/18568 - if updateErr := dc.runtimeClient.Status().Update(ctx, d); updateErr != nil { - klog.Errorf("Failed to update deployment status after RS creation failure: %v", updateErr) + if updateErr := dc.runtimeClient.Status().Patch(ctx, d, client.MergeFrom(oldD)); updateErr != nil { + klog.Errorf("Failed to patch deployment status after RS creation failure: %v", updateErr) } } dc.eventRecorder.Eventf(d, v1.EventTypeWarning, deploymentutil.FailedRSCreateReason, msg) @@ -266,7 +272,7 @@ func (dc *DeploymentController) getNewReplicaSet(ctx context.Context, d *apps.De if !alreadyExists && newReplicasCount > 0 { dc.eventRecorder.Eventf(d, v1.EventTypeNormal, "ScalingReplicaSet", "Scaled up replica set %s to %d", createdRS.Name, newReplicasCount) } - + oldD := d.DeepCopy() needsUpdate := deploymentutil.SetDeploymentRevision(d, newRevision) if !alreadyExists && deploymentutil.HasProgressDeadline(d) { msg := fmt.Sprintf("Created new replica set %q", createdRS.Name) @@ -275,8 +281,9 @@ func (dc *DeploymentController) getNewReplicaSet(ctx context.Context, d *apps.De needsUpdate = true } if needsUpdate { - if updateErr := dc.runtimeClient.Status().Update(ctx, d); updateErr != nil { - klog.Errorf("Failed to update deployment status: %v", updateErr) + // Patch instead of Update: only the revision / Progressing condition delta is sent. + if updateErr := dc.runtimeClient.Status().Patch(ctx, d, client.MergeFrom(oldD)); updateErr != nil { + klog.Errorf("Failed to patch deployment status: %v", updateErr) err = updateErr } } diff --git a/pkg/util/workloads_utils.go b/pkg/util/workloads_utils.go index b768bc90..d9041957 100644 --- a/pkg/util/workloads_utils.go +++ b/pkg/util/workloads_utils.go @@ -206,12 +206,14 @@ func UpdateFinalizer(c client.Client, object client.Object, op FinalizerOpType, return getErr } finalizers := fetchedObject.GetFinalizers() + switch op { case AddFinalizerOpType: if controllerutil.ContainsFinalizer(fetchedObject, finalizer) { return nil } finalizers = append(finalizers, finalizer) + case RemoveFinalizerOpType: finalizerSet := sets.NewString(finalizers...) if !finalizerSet.Has(finalizer) { @@ -219,8 +221,15 @@ func UpdateFinalizer(c client.Client, object client.Object, op FinalizerOpType, } finalizers = finalizerSet.Delete(finalizer).List() } + + // Take a snapshot before mutating so only the finalizer delta is sent. + // Using Patch instead of Update prevents unknown/new API fields from being + // erased when Rollout is built against an older k8s version than the cluster. + baseObject := fetchedObject.DeepCopyObject().(client.Object) + fetchedObject.SetFinalizers(finalizers) - return c.Update(context.TODO(), fetchedObject) + patch := client.MergeFromWithOptions(baseObject, client.MergeFromWithOptimisticLock{}) + return c.Patch(context.TODO(), fetchedObject, patch) }) } diff --git a/pkg/util/workloads_utils_test.go b/pkg/util/workloads_utils_test.go index 5310ed72..4da30b2e 100644 --- a/pkg/util/workloads_utils_test.go +++ b/pkg/util/workloads_utils_test.go @@ -17,6 +17,7 @@ limitations under the License. package util import ( + "context" "reflect" "testing" "time" @@ -642,3 +643,90 @@ func TestNativeDaemonSetIntegration(t *testing.T) { Expect(owner.GetUID()).Should(Equal(nativeDaemonSet.GetUID())) }) } + +type patchCountingClient struct { + client.Client + patchCalls int + updateCalls int +} + +func (c *patchCountingClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + c.patchCalls++ + return c.Client.Patch(ctx, obj, patch, opts...) +} + +func (c *patchCountingClient) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + c.updateCalls++ + return c.Client.Update(ctx, obj, opts...) +} + +func TestUpdateFinalizerUsePatchForAdd(t *testing.T) { + deployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "demo", + Namespace: "default", + Finalizers: []string{"existing"}, + }, + } + baseClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(deployment).Build() + c := &patchCountingClient{Client: baseClient} + + if err := UpdateFinalizer(c, deployment.DeepCopy(), AddFinalizerOpType, "rollout.finalizer"); err != nil { + t.Fatalf("add finalizer failed: %v", err) + } + if c.patchCalls != 1 { + t.Fatalf("expected 1 patch call, got %d", c.patchCalls) + } + if c.updateCalls != 0 { + t.Fatalf("expected 0 update calls, got %d", c.updateCalls) + } + + // The second add is idempotent and should not trigger a patch. + if err := UpdateFinalizer(c, deployment.DeepCopy(), AddFinalizerOpType, "rollout.finalizer"); err != nil { + t.Fatalf("idempotent add finalizer failed: %v", err) + } + if c.patchCalls != 1 { + t.Fatalf("expected patch call count to remain 1, got %d", c.patchCalls) + } + if c.updateCalls != 0 { + t.Fatalf("expected 0 update calls after idempotent add, got %d", c.updateCalls) + } + + var result appsv1.Deployment + if err := c.Get(context.TODO(), client.ObjectKeyFromObject(deployment), &result); err != nil { + t.Fatalf("get deployment failed: %v", err) + } + if len(result.Finalizers) != 2 || result.Finalizers[0] != "existing" || result.Finalizers[1] != "rollout.finalizer" { + t.Fatalf("unexpected finalizers: %v", result.Finalizers) + } +} + +func TestUpdateFinalizerUsePatchForRemove(t *testing.T) { + deployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "demo", + Namespace: "default", + Finalizers: []string{"existing", "rollout.finalizer"}, + }, + } + baseClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(deployment).Build() + c := &patchCountingClient{Client: baseClient} + + if err := UpdateFinalizer(c, deployment.DeepCopy(), RemoveFinalizerOpType, "rollout.finalizer"); err != nil { + t.Fatalf("remove finalizer failed: %v", err) + } + if c.patchCalls != 1 { + t.Fatalf("expected 1 patch call, got %d", c.patchCalls) + } + if c.updateCalls != 0 { + t.Fatalf("expected 0 update calls, got %d", c.updateCalls) + } + + var result appsv1.Deployment + if err := c.Get(context.TODO(), client.ObjectKeyFromObject(deployment), &result); err != nil { + t.Fatalf("get deployment failed: %v", err) + } + if len(result.Finalizers) != 1 || result.Finalizers[0] != "existing" { + t.Fatalf("unexpected finalizers: %v", result.Finalizers) + } +} From 6ac351f58c887a0c2327620cb5d022d22e506c80 Mon Sep 17 00:00:00 2001 From: Nazih Ben Brahim Date: Sun, 1 Mar 2026 15:52:50 +0100 Subject: [PATCH 2/2] test(deployment): cover status patch paths Signed-off-by: Nazih Ben Brahim --- .../deployment/status_patch_test.go | 254 +++++++++++++++++- 1 file changed, 240 insertions(+), 14 deletions(-) diff --git a/pkg/controller/deployment/status_patch_test.go b/pkg/controller/deployment/status_patch_test.go index 7860cdda..4daac67b 100644 --- a/pkg/controller/deployment/status_patch_test.go +++ b/pkg/controller/deployment/status_patch_test.go @@ -18,13 +18,20 @@ package deployment import ( "context" + "errors" "testing" apps "k8s.io/api/apps/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/tools/record" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" ctrlfake "sigs.k8s.io/controller-runtime/pkg/client/fake" + + deploymentutil "github.com/openkruise/rollouts/pkg/controller/deployment/util" + rolloututil "github.com/openkruise/rollouts/pkg/util" ) type statusCallCounter struct { @@ -34,19 +41,32 @@ type statusCallCounter struct { type statusCountingClient struct { ctrlclient.Client - counter statusCallCounter + counter statusCallCounter + patchErr error + createErr error } func (c *statusCountingClient) Status() ctrlclient.SubResourceWriter { return &statusCountingWriter{ SubResourceWriter: c.Client.Status(), counter: &c.counter, + patchErr: c.patchErr, + } +} + +func (c *statusCountingClient) Create(ctx context.Context, obj ctrlclient.Object, opts ...ctrlclient.CreateOption) error { + if c.createErr != nil { + if _, ok := obj.(*apps.ReplicaSet); ok { + return c.createErr + } } + return c.Client.Create(ctx, obj, opts...) } type statusCountingWriter struct { ctrlclient.SubResourceWriter - counter *statusCallCounter + counter *statusCallCounter + patchErr error } func (w *statusCountingWriter) Update(ctx context.Context, obj ctrlclient.Object, opts ...ctrlclient.SubResourceUpdateOption) error { @@ -56,15 +76,54 @@ func (w *statusCountingWriter) Update(ctx context.Context, obj ctrlclient.Object func (w *statusCountingWriter) Patch(ctx context.Context, obj ctrlclient.Object, patch ctrlclient.Patch, opts ...ctrlclient.SubResourcePatchOption) error { w.counter.patchCalls++ + if w.patchErr != nil { + return w.patchErr + } return w.SubResourceWriter.Patch(ctx, obj, patch, opts...) } -func TestPatchDeploymentStatusUsesStatusPatch(t *testing.T) { +func newStatusPatchTestController(t *testing.T, objects ...ctrlclient.Object) (*statusCountingClient, *DeploymentController) { + t.Helper() + scheme := runtime.NewScheme() if err := apps.AddToScheme(scheme); err != nil { t.Fatalf("add apps/v1 scheme failed: %v", err) } + baseClient := ctrlfake.NewClientBuilder(). + WithScheme(scheme). + WithStatusSubresource(&apps.Deployment{}). + WithObjects(objects...). + Build() + countingClient := &statusCountingClient{Client: baseClient} + dc := &DeploymentController{ + eventRecorder: record.NewFakeRecorder(10), + runtimeClient: countingClient, + } + return countingClient, dc +} + +func mustGetDeployment(t *testing.T, c ctrlclient.Client, deployment *apps.Deployment) *apps.Deployment { + t.Helper() + + current := &apps.Deployment{} + if err := c.Get(context.TODO(), ctrlclient.ObjectKeyFromObject(deployment), current); err != nil { + t.Fatalf("get deployment failed: %v", err) + } + return current +} + +func mustGetReplicaSet(t *testing.T, c ctrlclient.Client, rs *apps.ReplicaSet) *apps.ReplicaSet { + t.Helper() + + current := &apps.ReplicaSet{} + if err := c.Get(context.TODO(), ctrlclient.ObjectKeyFromObject(rs), current); err != nil { + t.Fatalf("get replicaSet failed: %v", err) + } + return current +} + +func TestPatchDeploymentStatusUsesStatusPatch(t *testing.T) { baseDeployment := &apps.Deployment{ ObjectMeta: metav1.ObjectMeta{ Name: "demo", @@ -76,19 +135,10 @@ func TestPatchDeploymentStatusUsesStatusPatch(t *testing.T) { }, } - baseClient := ctrlfake.NewClientBuilder(). - WithScheme(scheme). - WithStatusSubresource(&apps.Deployment{}). - WithObjects(baseDeployment). - Build() - countingClient := &statusCountingClient{Client: baseClient} - dc := &DeploymentController{runtimeClient: countingClient} + countingClient, dc := newStatusPatchTestController(t, baseDeployment) - current := &apps.Deployment{} + current := mustGetDeployment(t, countingClient, baseDeployment) key := ctrlclient.ObjectKeyFromObject(baseDeployment) - if err := countingClient.Get(context.TODO(), key, current); err != nil { - t.Fatalf("get deployment failed: %v", err) - } oldD := current.DeepCopy() newD := current.DeepCopy() newD.Status.ObservedGeneration = newD.Generation @@ -111,3 +161,179 @@ func TestPatchDeploymentStatusUsesStatusPatch(t *testing.T) { t.Fatalf("expected observedGeneration=%d, got %d", result.Generation, result.Status.ObservedGeneration) } } + +func TestSyncDeploymentSelectingAllIgnoresStatusPatchError(t *testing.T) { + deployment := generateDeployment("busybox") + deployment.Namespace = "default" + deployment.Spec.Selector = &metav1.LabelSelector{} + deployment.Generation = 7 + deployment.Status.ObservedGeneration = 3 + + countingClient, dc := newStatusPatchTestController(t, &deployment) + countingClient.patchErr = errors.New("status patch failed") + + if err := dc.syncDeployment(context.TODO(), &deployment); err != nil { + t.Fatalf("syncDeployment returned unexpected error: %v", err) + } + if countingClient.counter.patchCalls != 1 { + t.Fatalf("expected 1 status patch call, got %d", countingClient.counter.patchCalls) + } + if countingClient.counter.updateCalls != 0 { + t.Fatalf("expected 0 status update calls, got %d", countingClient.counter.updateCalls) + } + + result := mustGetDeployment(t, countingClient, &deployment) + if result.Status.ObservedGeneration != 3 { + t.Fatalf("expected observedGeneration to remain 3 after patch error, got %d", result.Status.ObservedGeneration) + } +} + +func TestGetNewReplicaSetUsesStatusPatchForExistingNewRS(t *testing.T) { + deployment := generateDeployment("busybox") + deployment.Namespace = "default" + progressDeadlineSeconds := int32(30) + deployment.Spec.ProgressDeadlineSeconds = &progressDeadlineSeconds + + existingNewRS := generateRS(deployment) + existingNewRS.Name = "busybox-existing-rs" + existingNewRS.Namespace = deployment.Namespace + existingNewRS.Annotations = map[string]string{ + deploymentutil.RevisionAnnotation: "1", + } + + countingClient, dc := newStatusPatchTestController(t, &deployment, &existingNewRS) + currentDeployment := mustGetDeployment(t, countingClient, &deployment) + currentNewRS := mustGetReplicaSet(t, countingClient, &existingNewRS) + + newRS, err := dc.getNewReplicaSet(context.TODO(), currentDeployment, []*apps.ReplicaSet{currentNewRS}, nil, false) + if err != nil { + t.Fatalf("getNewReplicaSet returned unexpected error: %v", err) + } + if newRS == nil || newRS.Name != currentNewRS.Name { + t.Fatalf("expected existing newRS %q, got %#v", currentNewRS.Name, newRS) + } + if countingClient.counter.patchCalls != 1 { + t.Fatalf("expected 1 status patch call, got %d", countingClient.counter.patchCalls) + } + if countingClient.counter.updateCalls != 0 { + t.Fatalf("expected 0 status update calls, got %d", countingClient.counter.updateCalls) + } + + result := mustGetDeployment(t, countingClient, &deployment) + condition := deploymentutil.GetDeploymentCondition(result.Status, apps.DeploymentProgressing) + if condition == nil { + t.Fatalf("expected progressing condition to be set") + } + if condition.Reason != deploymentutil.FoundNewRSReason { + t.Fatalf("expected progressing reason %q, got %q", deploymentutil.FoundNewRSReason, condition.Reason) + } +} + +func TestGetNewReplicaSetUsesStatusPatchWhenCreatingNewRS(t *testing.T) { + deployment := generateDeployment("busybox") + deployment.Namespace = "default" + progressDeadlineSeconds := int32(30) + deployment.Spec.ProgressDeadlineSeconds = &progressDeadlineSeconds + + countingClient, dc := newStatusPatchTestController(t, &deployment) + currentDeployment := mustGetDeployment(t, countingClient, &deployment) + + newRS, err := dc.getNewReplicaSet(context.TODO(), currentDeployment, nil, nil, true) + if err != nil { + t.Fatalf("getNewReplicaSet returned unexpected error: %v", err) + } + if newRS == nil { + t.Fatalf("expected a new ReplicaSet to be created") + } + if countingClient.counter.patchCalls != 1 { + t.Fatalf("expected 1 status patch call, got %d", countingClient.counter.patchCalls) + } + if countingClient.counter.updateCalls != 0 { + t.Fatalf("expected 0 status update calls, got %d", countingClient.counter.updateCalls) + } + + result := mustGetDeployment(t, countingClient, &deployment) + condition := deploymentutil.GetDeploymentCondition(result.Status, apps.DeploymentProgressing) + if condition == nil { + t.Fatalf("expected progressing condition to be set") + } + if condition.Reason != deploymentutil.NewReplicaSetReason { + t.Fatalf("expected progressing reason %q, got %q", deploymentutil.NewReplicaSetReason, condition.Reason) + } + + storedNewRS := &apps.ReplicaSet{} + if err := countingClient.Get(context.TODO(), ctrlclient.ObjectKeyFromObject(newRS), storedNewRS); err != nil { + t.Fatalf("get created ReplicaSet failed: %v", err) + } +} + +func TestGetNewReplicaSetUsesStatusPatchWhenCreateFails(t *testing.T) { + deployment := generateDeployment("busybox") + deployment.Namespace = "default" + progressDeadlineSeconds := int32(30) + deployment.Spec.ProgressDeadlineSeconds = &progressDeadlineSeconds + + countingClient, dc := newStatusPatchTestController(t, &deployment) + countingClient.createErr = errors.New("create replicaSet failed") + currentDeployment := mustGetDeployment(t, countingClient, &deployment) + + newRS, err := dc.getNewReplicaSet(context.TODO(), currentDeployment, nil, nil, true) + if err == nil { + t.Fatalf("expected getNewReplicaSet to return an error") + } + if newRS != nil { + t.Fatalf("expected no ReplicaSet to be returned on create failure, got %q", newRS.Name) + } + if countingClient.counter.patchCalls != 1 { + t.Fatalf("expected 1 status patch call, got %d", countingClient.counter.patchCalls) + } + if countingClient.counter.updateCalls != 0 { + t.Fatalf("expected 0 status update calls, got %d", countingClient.counter.updateCalls) + } + + result := mustGetDeployment(t, countingClient, &deployment) + condition := deploymentutil.GetDeploymentCondition(result.Status, apps.DeploymentProgressing) + if condition == nil { + t.Fatalf("expected progressing condition to be set on create failure") + } + if condition.Reason != deploymentutil.FailedRSCreateReason { + t.Fatalf("expected progressing reason %q, got %q", deploymentutil.FailedRSCreateReason, condition.Reason) + } +} + +func TestGetNewReplicaSetUsesStatusPatchOnHashCollision(t *testing.T) { + deployment := generateDeployment("busybox") + deployment.Namespace = "default" + deployment.UID = "deployment-uid" + + newRSTemplate := *deployment.Spec.Template.DeepCopy() + hash := rolloututil.ComputeHash(&newRSTemplate, deployment.Status.CollisionCount) + conflictingRS := generateRS(deployment) + conflictingRS.Name = deployment.Name + "-" + hash + conflictingRS.Namespace = deployment.Namespace + conflictingRS.OwnerReferences = nil + conflictingRS.Spec.Template.Spec.Containers[0].Image = "other-image" + + countingClient, dc := newStatusPatchTestController(t, &deployment, &conflictingRS) + countingClient.createErr = apierrors.NewAlreadyExists(schema.GroupResource{Group: "apps", Resource: "replicasets"}, conflictingRS.Name) + currentDeployment := mustGetDeployment(t, countingClient, &deployment) + + newRS, err := dc.getNewReplicaSet(context.TODO(), currentDeployment, nil, nil, true) + if !apierrors.IsAlreadyExists(err) { + t.Fatalf("expected already exists error, got %v", err) + } + if newRS != nil { + t.Fatalf("expected no ReplicaSet to be returned on hash collision, got %q", newRS.Name) + } + if countingClient.counter.patchCalls != 1 { + t.Fatalf("expected 1 status patch call, got %d", countingClient.counter.patchCalls) + } + if countingClient.counter.updateCalls != 0 { + t.Fatalf("expected 0 status update calls, got %d", countingClient.counter.updateCalls) + } + + result := mustGetDeployment(t, countingClient, &deployment) + if result.Status.CollisionCount == nil || *result.Status.CollisionCount != 1 { + t.Fatalf("expected collisionCount=1, got %#v", result.Status.CollisionCount) + } +}