diff --git a/controlplane/kubeadm/internal/controllers/inplace_trigger.go b/controlplane/kubeadm/internal/controllers/inplace_trigger.go index e16ea5230d90..8b1045270feb 100644 --- a/controlplane/kubeadm/internal/controllers/inplace_trigger.go +++ b/controlplane/kubeadm/internal/controllers/inplace_trigger.go @@ -129,7 +129,8 @@ func (r *KubeadmControlPlaneReconciler) triggerInPlaceUpdate(ctx context.Context // Note: Intentionally using client.Patch (via hooks.MarkAsPending + patchHelper) instead of SSA. Otherwise we would // have to ensure we preserve PendingHooksAnnotation on existing Machines in KCP and that would lead to race // conditions when the Machine controller tries to remove the annotation and KCP adds it back. - if err := hooks.MarkAsPending(ctx, r.Client, desiredMachine, runtimehooksv1.UpdateMachine); err != nil { + // Note: This call will update the resourceVersion on desiredMachine, so that WaitForCacheToBeUpToDate also considers this change. + if err := hooks.MarkAsPending(ctx, r.Client, desiredMachine, true, runtimehooksv1.UpdateMachine); err != nil { return errors.Wrapf(err, "failed to complete triggering in-place update for Machine %s", klog.KObj(machine)) } diff --git a/exp/topology/desiredstate/desired_state.go b/exp/topology/desiredstate/desired_state.go index 128c42c1bc3c..528b223814dc 100644 --- a/exp/topology/desiredstate/desired_state.go +++ b/exp/topology/desiredstate/desired_state.go @@ -649,7 +649,7 @@ func (g *generator) computeControlPlaneVersion(ctx context.Context, s *scope.Sco // After BeforeClusterUpgrade unblocked the upgrade, consider the upgrade started. // As a consequence, the system start tracking the intent of calling AfterClusterUpgrade once the upgrade is complete. // Note: this also prevent the BeforeClusterUpgrade to be called again (until after the upgrade is completed). - if err := hooks.MarkAsPending(ctx, g.Client, s.Current.Cluster, runtimehooksv1.AfterClusterUpgrade); err != nil { + if err := hooks.MarkAsPending(ctx, g.Client, s.Current.Cluster, false, runtimehooksv1.AfterClusterUpgrade); err != nil { return "", err } } @@ -685,7 +685,7 @@ func (g *generator) computeControlPlaneVersion(ctx context.Context, s *scope.Sco if machineDeploymentPendingUpgrade || machinePoolPendingUpgrade { hooksToBeCalled = append(hooksToBeCalled, runtimehooksv1.BeforeWorkersUpgrade, runtimehooksv1.AfterWorkersUpgrade) } - if err := hooks.MarkAsPending(ctx, g.Client, s.Current.Cluster, hooksToBeCalled...); err != nil { + if err := hooks.MarkAsPending(ctx, g.Client, s.Current.Cluster, false, hooksToBeCalled...); err != nil { return "", err } } diff --git a/exp/topology/desiredstate/lifecycle_hooks.go b/exp/topology/desiredstate/lifecycle_hooks.go index 91e125571541..fcddbe23ea7d 100644 --- a/exp/topology/desiredstate/lifecycle_hooks.go +++ b/exp/topology/desiredstate/lifecycle_hooks.go @@ -187,7 +187,7 @@ func (g *generator) callAfterControlPlaneUpgradeHook(ctx context.Context, s *sco return false, err } if len(extensionHandlers) == 0 { - if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, runtimehooksv1.AfterControlPlaneUpgrade); err != nil { + if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, false, runtimehooksv1.AfterControlPlaneUpgrade); err != nil { return false, err } return true, nil @@ -220,7 +220,7 @@ func (g *generator) callAfterControlPlaneUpgradeHook(ctx context.Context, s *sco ) return false, nil } - if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, runtimehooksv1.AfterControlPlaneUpgrade); err != nil { + if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, false, runtimehooksv1.AfterControlPlaneUpgrade); err != nil { return false, err } @@ -248,7 +248,7 @@ func (g *generator) callBeforeWorkersUpgradeHook(ctx context.Context, s *scope.S return false, err } if len(extensionHandlers) == 0 { - if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, runtimehooksv1.BeforeWorkersUpgrade); err != nil { + if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, false, runtimehooksv1.BeforeWorkersUpgrade); err != nil { return false, err } return true, nil @@ -282,7 +282,7 @@ func (g *generator) callBeforeWorkersUpgradeHook(ctx context.Context, s *scope.S ) return false, nil } - if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, runtimehooksv1.BeforeWorkersUpgrade); err != nil { + if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, false, runtimehooksv1.BeforeWorkersUpgrade); err != nil { return false, err } @@ -311,7 +311,7 @@ func (g *generator) callAfterWorkersUpgradeHook(ctx context.Context, s *scope.Sc return false, err } if len(extensionHandlers) == 0 { - if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, runtimehooksv1.AfterWorkersUpgrade); err != nil { + if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, false, runtimehooksv1.AfterWorkersUpgrade); err != nil { return false, err } return true, nil @@ -344,7 +344,7 @@ func (g *generator) callAfterWorkersUpgradeHook(ctx context.Context, s *scope.Sc ) return false, nil } - if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, runtimehooksv1.AfterWorkersUpgrade); err != nil { + if err := hooks.MarkAsDone(ctx, g.Client, s.Current.Cluster, false, runtimehooksv1.AfterWorkersUpgrade); err != nil { return false, err } diff --git a/internal/controllers/machine/machine_controller_inplace_update.go b/internal/controllers/machine/machine_controller_inplace_update.go index ca9654868a54..f499a5a22aae 100644 --- a/internal/controllers/machine/machine_controller_inplace_update.go +++ b/internal/controllers/machine/machine_controller_inplace_update.go @@ -204,7 +204,9 @@ func (r *Reconciler) completeInPlaceUpdate(ctx context.Context, s *scope) error } } - if err := hooks.MarkAsDone(ctx, r.Client, s.machine, runtimehooksv1.UpdateMachine); err != nil { + // Note: This call will not update the resourceVersion on machine, so that the patchHelper in the main + // Reconcile func won't get a conflict. + if err := hooks.MarkAsDone(ctx, r.Client, s.machine, false, runtimehooksv1.UpdateMachine); err != nil { return err } diff --git a/internal/controllers/topology/cluster/cluster_controller.go b/internal/controllers/topology/cluster/cluster_controller.go index dd45ac6a9f97..ebb6b9591e63 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -549,7 +549,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, s *scope.Scope) (ctrl. return ctrl.Result{}, err } if len(extensionHandlers) == 0 { - if err := hooks.MarkAsOkToDelete(ctx, r.Client, cluster); err != nil { + if err := hooks.MarkAsOkToDelete(ctx, r.Client, cluster, false); err != nil { return ctrl.Result{}, err } return ctrl.Result{}, nil @@ -577,7 +577,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, s *scope.Scope) (ctrl. } // The BeforeClusterDelete hook returned a non-blocking response. Now the cluster is ready to be deleted. // Lets mark the cluster as `ok-to-delete` - if err := hooks.MarkAsOkToDelete(ctx, r.Client, cluster); err != nil { + if err := hooks.MarkAsOkToDelete(ctx, r.Client, cluster, false); err != nil { return ctrl.Result{}, err } log.Info(fmt.Sprintf("Cluster deletion is unblocked by %s hook", runtimecatalog.HookName(runtimehooksv1.BeforeClusterDelete))) diff --git a/internal/controllers/topology/cluster/reconcile_state.go b/internal/controllers/topology/cluster/reconcile_state.go index 141826a5d022..29cfa04d6f00 100644 --- a/internal/controllers/topology/cluster/reconcile_state.go +++ b/internal/controllers/topology/cluster/reconcile_state.go @@ -187,7 +187,7 @@ func (r *Reconciler) callAfterHooks(ctx context.Context, s *scope.Scope) error { func (r *Reconciler) callAfterControlPlaneInitialized(ctx context.Context, s *scope.Scope) error { // If the cluster topology is being created then track to intent to call the AfterControlPlaneInitialized hook so that we can call it later. if !s.Current.Cluster.Spec.InfrastructureRef.IsDefined() && !s.Current.Cluster.Spec.ControlPlaneRef.IsDefined() { - if err := hooks.MarkAsPending(ctx, r.Client, s.Current.Cluster, runtimehooksv1.AfterControlPlaneInitialized); err != nil { + if err := hooks.MarkAsPending(ctx, r.Client, s.Current.Cluster, false, runtimehooksv1.AfterControlPlaneInitialized); err != nil { return err } } @@ -211,7 +211,7 @@ func (r *Reconciler) callAfterControlPlaneInitialized(ctx context.Context, s *sc return err } s.HookResponseTracker.Add(runtimehooksv1.AfterControlPlaneInitialized, hookResponse) - if err := hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, runtimehooksv1.AfterControlPlaneInitialized); err != nil { + if err := hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, false, runtimehooksv1.AfterControlPlaneInitialized); err != nil { return err } } @@ -259,7 +259,7 @@ func (r *Reconciler) callAfterClusterUpgrade(ctx context.Context, s *scope.Scope return err } if len(extensionHandlers) == 0 { - return hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, runtimehooksv1.AfterClusterUpgrade) + return hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, false, runtimehooksv1.AfterClusterUpgrade) } // DeepCopy cluster because ConvertFrom has side effects like adding the conversion annotation. @@ -285,7 +285,7 @@ func (r *Reconciler) callAfterClusterUpgrade(ctx context.Context, s *scope.Scope } // The hook is successfully called; we can remove this hook from the list of pending-hooks. - if err := hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, runtimehooksv1.AfterClusterUpgrade); err != nil { + if err := hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, false, runtimehooksv1.AfterClusterUpgrade); err != nil { return err } diff --git a/internal/hooks/tracking.go b/internal/hooks/tracking.go index b497cdfe6873..0df9a2c72da8 100644 --- a/internal/hooks/tracking.go +++ b/internal/hooks/tracking.go @@ -29,24 +29,28 @@ import ( runtimev1 "sigs.k8s.io/cluster-api/api/runtime/v1beta2" runtimecatalog "sigs.k8s.io/cluster-api/exp/runtime/catalog" - "sigs.k8s.io/cluster-api/util/patch" ) // MarkAsPending adds to the object's PendingHooksAnnotation the intent to execute a hook after an operation completes. // Usually this function is called when an operation is starting in order to track the intent to call an After hook later in the process. -func MarkAsPending(ctx context.Context, c client.Client, obj client.Object, hooks ...runtimecatalog.Hook) error { +func MarkAsPending(ctx context.Context, c client.Client, obj client.Object, updateResourceVersionOnObject bool, hooks ...runtimecatalog.Hook) error { hookNames := []string{} for _, hook := range hooks { hookNames = append(hookNames, runtimecatalog.HookName(hook)) } - patchHelper, err := patch.NewHelper(obj, c) - if err != nil { - return errors.Wrapf(err, "failed to mark %q hook(s) as pending", strings.Join(hookNames, ",")) + orig := obj.DeepCopyObject().(client.Object) + + if changed := MarkObjectAsPending(obj, hooks...); !changed { + return nil } - MarkObjectAsPending(obj, hooks...) - if err := patchHelper.Patch(ctx, obj); err != nil { + // In some cases it is preferred to not update resourceVersion in the input object, + // because this could lead to conflict errors e.g. when patching at the end of a reconcile loop. + if !updateResourceVersionOnObject { + obj = obj.DeepCopyObject().(client.Object) + } + if err := c.Patch(ctx, obj, client.MergeFrom(orig)); err != nil { return errors.Wrapf(err, "failed to mark %q hook(s) as pending", strings.Join(hookNames, ",")) } @@ -55,7 +59,7 @@ func MarkAsPending(ctx context.Context, c client.Client, obj client.Object, hook // MarkObjectAsPending adds to the object's PendingHooksAnnotation the intent to execute a hook after an operation completes. // Usually this function is called when an operation is starting in order to track the intent to call an After hook later in the process. -func MarkObjectAsPending(obj client.Object, hooks ...runtimecatalog.Hook) { +func MarkObjectAsPending(obj client.Object, hooks ...runtimecatalog.Hook) (changed bool) { hookNames := []string{} for _, hook := range hooks { hookNames = append(hookNames, runtimecatalog.HookName(hook)) @@ -66,8 +70,16 @@ func MarkObjectAsPending(obj client.Object, hooks ...runtimecatalog.Hook) { if annotations == nil { annotations = map[string]string{} } - annotations[runtimev1.PendingHooksAnnotation] = addToCommaSeparatedList(annotations[runtimev1.PendingHooksAnnotation], hookNames...) + + newAnnotationValue := addToCommaSeparatedList(annotations[runtimev1.PendingHooksAnnotation], hookNames...) + + if annotations[runtimev1.PendingHooksAnnotation] == newAnnotationValue { + return false + } + + annotations[runtimev1.PendingHooksAnnotation] = newAnnotationValue obj.SetAnnotations(annotations) + return true } // IsPending returns true if there is an intent to call a hook being tracked in the object's PendingHooksAnnotation. @@ -83,30 +95,33 @@ func IsPending(hook runtimecatalog.Hook, obj client.Object) bool { // MarkAsDone removes the intent to call a Hook from the object's PendingHooksAnnotation. // Usually this func is called after all the registered extensions for the Hook returned an answer without requests // to hold on to the object's lifecycle (retryAfterSeconds). -func MarkAsDone(ctx context.Context, c client.Client, obj client.Object, hooks ...runtimecatalog.Hook) error { - hookNames := []string{} - for _, hook := range hooks { - hookNames = append(hookNames, runtimecatalog.HookName(hook)) +func MarkAsDone(ctx context.Context, c client.Client, obj client.Object, updateResourceVersionOnObject bool, hook runtimecatalog.Hook) error { + if !IsPending(hook, obj) { + return nil } - patchHelper, err := patch.NewHelper(obj, c) - if err != nil { - return errors.Wrapf(err, "failed to mark %q hook(s) as done", strings.Join(hookNames, ",")) - } + hookName := runtimecatalog.HookName(hook) + + orig := obj.DeepCopyObject().(client.Object) // Read the annotation of the objects and add the hook to the comma separated list annotations := obj.GetAnnotations() if annotations == nil { annotations = map[string]string{} } - annotations[runtimev1.PendingHooksAnnotation] = removeFromCommaSeparatedList(annotations[runtimev1.PendingHooksAnnotation], hookNames...) + annotations[runtimev1.PendingHooksAnnotation] = removeFromCommaSeparatedList(annotations[runtimev1.PendingHooksAnnotation], hookName) if annotations[runtimev1.PendingHooksAnnotation] == "" { delete(annotations, runtimev1.PendingHooksAnnotation) } obj.SetAnnotations(annotations) - if err := patchHelper.Patch(ctx, obj); err != nil { - return errors.Wrapf(err, "failed to mark %q hook(s) as done", strings.Join(hookNames, ",")) + // In some cases it is preferred to not update resourceVersion in the input object, + // because this could lead to conflict errors e.g. when patching at the end of a reconcile loop. + if !updateResourceVersionOnObject { + obj = obj.DeepCopyObject().(client.Object) + } + if err := c.Patch(ctx, obj, client.MergeFrom(orig)); err != nil { + return errors.Wrapf(err, "failed to mark %q hook as done", hookName) } return nil @@ -125,16 +140,17 @@ func IsOkToDelete(obj client.Object) bool { } // MarkAsOkToDelete adds the OkToDeleteAnnotation annotation to the object and patches it. -func MarkAsOkToDelete(ctx context.Context, c client.Client, obj client.Object) error { +func MarkAsOkToDelete(ctx context.Context, c client.Client, obj client.Object, updateResourceVersionOnObject bool) error { + if _, ok := obj.GetAnnotations()[runtimev1.OkToDeleteAnnotation]; ok { + return nil + } + gvk, err := apiutil.GVKForObject(obj, c.Scheme()) if err != nil { return errors.Wrapf(err, "failed to mark %s as ok to delete: failed to get GVK for object", klog.KObj(obj)) } - patchHelper, err := patch.NewHelper(obj, c) - if err != nil { - return errors.Wrapf(err, "failed to mark %s %s as ok to delete", gvk.Kind, klog.KObj(obj)) - } + orig := obj.DeepCopyObject().(client.Object) annotations := obj.GetAnnotations() if annotations == nil { @@ -143,7 +159,12 @@ func MarkAsOkToDelete(ctx context.Context, c client.Client, obj client.Object) e annotations[runtimev1.OkToDeleteAnnotation] = "" obj.SetAnnotations(annotations) - if err := patchHelper.Patch(ctx, obj); err != nil { + // In some cases it is preferred to not update resourceVersion in the input object, + // because this could lead to conflict errors e.g. when patching at the end of a reconcile loop. + if !updateResourceVersionOnObject { + obj = obj.DeepCopyObject().(client.Object) + } + if err := c.Patch(ctx, obj, client.MergeFrom(orig)); err != nil { return errors.Wrapf(err, "failed to mark %s %s as ok to delete", gvk.Kind, klog.KObj(obj)) } diff --git a/internal/hooks/tracking_test.go b/internal/hooks/tracking_test.go index 2538459f9cb2..4e55b62e716f 100644 --- a/internal/hooks/tracking_test.go +++ b/internal/hooks/tracking_test.go @@ -107,6 +107,7 @@ func TestMarkAsPending(t *testing.T) { obj client.Object hook runtimecatalog.Hook expectedAnnotation string + expectNoOp bool }{ { name: "should add the marker if not already marked as pending", @@ -118,6 +119,7 @@ func TestMarkAsPending(t *testing.T) { }, hook: runtimehooksv1.AfterClusterUpgrade, expectedAnnotation: "AfterClusterUpgrade", + expectNoOp: false, }, { name: "should add the marker if not already marked as pending - other hooks are present", @@ -132,6 +134,7 @@ func TestMarkAsPending(t *testing.T) { }, hook: runtimehooksv1.AfterClusterUpgrade, expectedAnnotation: "AfterClusterUpgrade,AfterControlPlaneUpgrade", + expectNoOp: false, }, { name: "should pass if the marker is already marked as pending", @@ -146,6 +149,7 @@ func TestMarkAsPending(t *testing.T) { }, hook: runtimehooksv1.AfterClusterUpgrade, expectedAnnotation: "AfterClusterUpgrade", + expectNoOp: true, }, { name: "should pass if the marker is already marked as pending and remove empty string entry", @@ -160,18 +164,32 @@ func TestMarkAsPending(t *testing.T) { }, hook: runtimehooksv1.AfterClusterUpgrade, expectedAnnotation: "AfterClusterUpgrade", + expectNoOp: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - fakeClient := fake.NewClientBuilder().WithObjects(tt.obj).Build() - ctx := context.Background() - g.Expect(MarkAsPending(ctx, fakeClient, tt.obj, tt.hook)).To(Succeed()) - annotations := tt.obj.GetAnnotations() - g.Expect(annotations[runtimev1.PendingHooksAnnotation]).To(ContainSubstring(runtimecatalog.HookName(tt.hook))) - g.Expect(annotations[runtimev1.PendingHooksAnnotation]).To(Equal(tt.expectedAnnotation)) + + for _, updateResourceVersionOnObject := range []bool{true, false} { + obj := tt.obj.DeepCopyObject().(client.Object) + fakeClient := fake.NewClientBuilder().WithObjects(obj).Build() + + origResourceVersion := obj.GetResourceVersion() + + ctx := context.Background() + g.Expect(MarkAsPending(ctx, fakeClient, obj, updateResourceVersionOnObject, tt.hook)).To(Succeed()) + annotations := obj.GetAnnotations() + g.Expect(annotations[runtimev1.PendingHooksAnnotation]).To(ContainSubstring(runtimecatalog.HookName(tt.hook))) + g.Expect(annotations[runtimev1.PendingHooksAnnotation]).To(Equal(tt.expectedAnnotation)) + + if tt.expectNoOp || !updateResourceVersionOnObject { + g.Expect(obj.GetResourceVersion()).To(Equal(origResourceVersion)) + } else { + g.Expect(obj.GetResourceVersion()).ToNot(Equal(origResourceVersion)) + } + } }) } } @@ -182,6 +200,7 @@ func TestMarkAsDone(t *testing.T) { obj client.Object hook runtimecatalog.Hook expectedAnnotation string + expectNoOp bool }{ { name: "should pass if the marker is not already present", @@ -193,6 +212,7 @@ func TestMarkAsDone(t *testing.T) { }, hook: runtimehooksv1.AfterClusterUpgrade, expectedAnnotation: "", + expectNoOp: true, }, { name: "should remove if the marker is already present", @@ -207,6 +227,7 @@ func TestMarkAsDone(t *testing.T) { }, hook: runtimehooksv1.AfterClusterUpgrade, expectedAnnotation: "", + expectNoOp: false, }, { name: "should remove if the marker is already present among multiple hooks", @@ -221,6 +242,7 @@ func TestMarkAsDone(t *testing.T) { }, hook: runtimehooksv1.AfterClusterUpgrade, expectedAnnotation: "AfterControlPlaneUpgrade", + expectNoOp: false, }, { name: "should remove empty string entry", @@ -235,18 +257,32 @@ func TestMarkAsDone(t *testing.T) { }, hook: runtimehooksv1.AfterClusterUpgrade, expectedAnnotation: "AfterControlPlaneUpgrade", + expectNoOp: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - fakeClient := fake.NewClientBuilder().WithObjects(tt.obj).Build() - ctx := context.Background() - g.Expect(MarkAsDone(ctx, fakeClient, tt.obj, tt.hook)).To(Succeed()) - annotations := tt.obj.GetAnnotations() - g.Expect(annotations[runtimev1.PendingHooksAnnotation]).NotTo(ContainSubstring(runtimecatalog.HookName(tt.hook))) - g.Expect(annotations[runtimev1.PendingHooksAnnotation]).To(Equal(tt.expectedAnnotation)) + + for _, updateResourceVersionOnObject := range []bool{true, false} { + obj := tt.obj.DeepCopyObject().(client.Object) + fakeClient := fake.NewClientBuilder().WithObjects(obj).Build() + + origResourceVersion := obj.GetResourceVersion() + + ctx := context.Background() + g.Expect(MarkAsDone(ctx, fakeClient, obj, updateResourceVersionOnObject, tt.hook)).To(Succeed()) + annotations := obj.GetAnnotations() + g.Expect(annotations[runtimev1.PendingHooksAnnotation]).NotTo(ContainSubstring(runtimecatalog.HookName(tt.hook))) + g.Expect(annotations[runtimev1.PendingHooksAnnotation]).To(Equal(tt.expectedAnnotation)) + + if tt.expectNoOp || !updateResourceVersionOnObject { + g.Expect(obj.GetResourceVersion()).To(Equal(origResourceVersion)) + } else { + g.Expect(obj.GetResourceVersion()).ToNot(Equal(origResourceVersion)) + } + } }) } } @@ -292,8 +328,9 @@ func TestIsOkToDelete(t *testing.T) { func TestMarkAsOkToDelete(t *testing.T) { tests := []struct { - name string - obj client.Object + name string + obj client.Object + expectNoOp bool }{ { name: "should add the 'ok-to-delete' annotation on the object if not already present", @@ -303,6 +340,7 @@ func TestMarkAsOkToDelete(t *testing.T) { Namespace: "test-ns", }, }, + expectNoOp: false, }, { name: "should succeed if the 'ok-to-delete' annotation is already present", @@ -315,17 +353,31 @@ func TestMarkAsOkToDelete(t *testing.T) { }, }, }, + expectNoOp: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - fakeClient := fake.NewClientBuilder().WithObjects(tt.obj).Build() - ctx := context.Background() - g.Expect(MarkAsOkToDelete(ctx, fakeClient, tt.obj)).To(Succeed()) - annotations := tt.obj.GetAnnotations() - g.Expect(annotations).To(HaveKey(runtimev1.OkToDeleteAnnotation)) + + for _, updateResourceVersionOnObject := range []bool{true, false} { + obj := tt.obj.DeepCopyObject().(client.Object) + fakeClient := fake.NewClientBuilder().WithObjects(obj).Build() + + origResourceVersion := obj.GetResourceVersion() + + ctx := context.Background() + g.Expect(MarkAsOkToDelete(ctx, fakeClient, obj, updateResourceVersionOnObject)).To(Succeed()) + annotations := obj.GetAnnotations() + g.Expect(annotations).To(HaveKey(runtimev1.OkToDeleteAnnotation)) + + if tt.expectNoOp || !updateResourceVersionOnObject { + g.Expect(obj.GetResourceVersion()).To(Equal(origResourceVersion)) + } else { + g.Expect(obj.GetResourceVersion()).ToNot(Equal(origResourceVersion)) + } + } }) } }