From 771ef63fc190e6b4525f7bad8cf58d392566620b Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Mon, 17 Nov 2025 20:54:22 +0100 Subject: [PATCH 01/24] updater: Include logger. --- pkg/reconciler/internal/updater/updater.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index 4c4bd31..07e28f0 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/pkg/reconciler/internal/updater/updater.go @@ -19,6 +19,7 @@ package updater import ( "context" + "github.com/go-logr/logr" "helm.sh/helm/v3/pkg/release" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -33,15 +34,18 @@ import ( "github.com/operator-framework/helm-operator-plugins/pkg/internal/status" ) -func New(client client.Client) Updater { +func New(client client.Client, logger logr.Logger) Updater { + logger = logger.WithName("updater") return Updater{ client: client, + logger: logger, } } type Updater struct { isCanceled bool client client.Client + logger logr.Logger updateFuncs []UpdateFunc updateStatusFuncs []UpdateStatusFunc } From dc56520b9b07ad615aa9436f62cf7c8cca93ae2c Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Mon, 17 Nov 2025 20:54:52 +0100 Subject: [PATCH 02/24] updater: Include new option: EnableAggressiveConflictResolution. --- pkg/reconciler/internal/updater/updater.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index 07e28f0..a15df42 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/pkg/reconciler/internal/updater/updater.go @@ -48,6 +48,12 @@ type Updater struct { logger logr.Logger updateFuncs []UpdateFunc updateStatusFuncs []UpdateStatusFunc + + enableAggressiveConflictResolution bool +} + +func (u *Updater) EnableAggressiveConflictResolution() { + u.enableAggressiveConflictResolution = true } type UpdateFunc func(*unstructured.Unstructured) bool From b677812c332034ef6233f127ef0f3849ff181586 Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Mon, 17 Nov 2025 21:02:33 +0100 Subject: [PATCH 03/24] updater: Implement aggressive conflict resolution. --- pkg/reconciler/internal/updater/updater.go | 133 +++++++++++++++++---- 1 file changed, 109 insertions(+), 24 deletions(-) diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index a15df42..59cc2b5 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/pkg/reconciler/internal/updater/updater.go @@ -18,6 +18,8 @@ package updater import ( "context" + "fmt" + "reflect" "github.com/go-logr/logr" "helm.sh/helm/v3/pkg/release" @@ -103,44 +105,127 @@ func retryOnRetryableUpdateError(backoff wait.Backoff, f func() error) error { return retry.OnError(backoff, isRetryableUpdateError, f) } -func (u *Updater) Apply(ctx context.Context, obj *unstructured.Unstructured) error { +func (u *Updater) Apply(ctx context.Context, baseObj *unstructured.Unstructured) error { if u.isCanceled { return nil } - backoff := retry.DefaultRetry - - st := statusFor(obj) - needsStatusUpdate := false - for _, f := range u.updateStatusFuncs { - needsStatusUpdate = f(st) || needsStatusUpdate - } - // Always update the status first. During uninstall, if // we remove the finalizer, updating the status will fail // because the object and its status will be garbage-collected. - if needsStatusUpdate { + backoff := retry.DefaultRetry + statusUpdateAttemptNumber := 0 + err := retryOnRetryableUpdateError(backoff, func() error { + // Note that st will also include all status conditions, also those not managed by helm-operator. + obj := baseObj.DeepCopy() + st := statusFor(obj) + needsStatusUpdate := false + for _, f := range u.updateStatusFuncs { + needsStatusUpdate = f(st) || needsStatusUpdate + } + + if !needsStatusUpdate { + return nil + } st.updateStatusObject() obj.Object["status"] = st.StatusObject - if err := retryOnRetryableUpdateError(backoff, func() error { - return u.client.Status().Update(ctx, obj) - }); err != nil { - return err + + if statusUpdateAttemptNumber > 0 { + u.logger.V(1).Info("Retrying status update", "attempt", statusUpdateAttemptNumber) + } + statusUpdateAttemptNumber++ + updateErr := u.client.Status().Update(ctx, obj) + if errors.IsConflict(updateErr) && u.enableAggressiveConflictResolution { + resolved, resolveErr := u.tryStatusUpdateConflictResolution(ctx, baseObj) + if resolveErr != nil { + return resolveErr + } + if !resolved { + return updateErr + } + return fmt.Errorf("status update conflict") // retriable error. + } else if updateErr != nil { + return updateErr } + baseObj.Object = obj.Object + return nil + }) + if err != nil { + return err } - needsUpdate := false - for _, f := range u.updateFuncs { - needsUpdate = f(obj) || needsUpdate - } - if needsUpdate { - if err := retryOnRetryableUpdateError(backoff, func() error { - return u.client.Update(ctx, obj) - }); err != nil { - return err + updateAttemptNumber := 0 + err = retryOnRetryableUpdateError(backoff, func() error { + obj := baseObj.DeepCopy() + needsUpdate := false + for _, f := range u.updateFuncs { + needsUpdate = f(obj) || needsUpdate + } + if !needsUpdate { + return nil } + if updateAttemptNumber > 0 { + u.logger.V(1).Info("Retrying update", "attempt", updateAttemptNumber) + } + updateAttemptNumber++ + updateErr := u.client.Update(ctx, obj) + if errors.IsConflict(updateErr) && u.enableAggressiveConflictResolution { + resolved, resolveErr := u.tryUpdateConflictResolution(ctx, baseObj) + if resolveErr != nil { + return resolveErr + } + if !resolved { + return updateErr + } + return fmt.Errorf("update conflict due to externally-managed status conditions") // retriable error. + } else if updateErr != nil { + return updateErr + } + baseObj.Object = obj.Object + return nil + }) + + return err +} + +func (u *Updater) tryStatusUpdateConflictResolution(ctx context.Context, obj *unstructured.Unstructured) (bool, error) { + u.logger.V(1).Info("Status update conflict detected") + // Retrieve current version from the cluster. + current := &unstructured.Unstructured{} + current.SetGroupVersionKind(obj.GroupVersionKind()) + objectKey := client.ObjectKeyFromObject(obj) + if err := u.client.Get(ctx, objectKey, current); err != nil { + err = fmt.Errorf("refreshing object %s/%s: %w", objectKey.Namespace, objectKey.Name, err) + return false, err } - return nil + + obj.Object = current.Object + u.logger.V(1).Info("Resolved status update conflict by fetching current in-cluster version") + return true, nil +} + +func (u *Updater) tryUpdateConflictResolution(ctx context.Context, obj *unstructured.Unstructured) (bool, error) { + u.logger.V(1).Info("Update conflict detected") + // Retrieve current version from the cluster. + current := &unstructured.Unstructured{} + current.SetGroupVersionKind(obj.GroupVersionKind()) + objectKey := client.ObjectKeyFromObject(obj) + if err := u.client.Get(ctx, objectKey, current); err != nil { + err = fmt.Errorf("refreshing object %s/%s: %w", objectKey.Namespace, objectKey.Name, err) + return false, err + } + + if !reflect.DeepEqual(obj.Object["spec"], current.Object["spec"]) { + // Diff in object spec. Nothing we can do about it -> Fail. + u.logger.V(1).Info("Cluster resource cannot be updated due to spec mismatch", + "namespace", objectKey.Namespace, "name", objectKey.Name, "gkv", obj.GroupVersionKind(), + ) + return false, nil + } + + obj.Object = current.Object + u.logger.V(1).Info("Resolved update conflict by fetching current in-cluster version") + return true, nil } func RemoveFinalizer(finalizer string) UpdateFunc { From 383acd45c5abaf6813187db632866c9cf57a85d1 Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Mon, 17 Nov 2025 21:13:02 +0100 Subject: [PATCH 04/24] reconciler: Prepare for aggressive conflict resolution --- pkg/reconciler/reconciler.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index db92150..34c85b5 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -100,6 +100,8 @@ type Reconciler struct { upgradeAnnotations map[string]annotation.Upgrade uninstallAnnotations map[string]annotation.Uninstall pauseReconcileAnnotation string + + enableAggressiveConflictResolution bool } // New creates a new Reconciler that reconciles custom resources that define a @@ -617,6 +619,13 @@ func WithControllerSetupFunc(f ControllerSetupFunc) Option { } } +func WithAggressiveConflictResolution(enabled bool) Option { + return func(r *Reconciler) error { + r.enableAggressiveConflictResolution = enabled + return nil + } +} + // ControllerSetup allows restricted access to the Controller using the WithControllerSetupFunc option. // Currently, the only supposed configuration is adding additional watchers do the controller. type ControllerSetup interface { From 6062872111f82318ce171c3ca99dbb7fce3e4a15 Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Mon, 17 Nov 2025 21:14:28 +0100 Subject: [PATCH 05/24] reconciler: use client.Patch() for setting uninstall finalizer --- pkg/reconciler/reconciler.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 34c85b5..e392661 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -691,8 +691,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re // This is a safety measure to ensure that the chart is fully uninstalled before the CR is deleted. if obj.GetDeletionTimestamp() == nil && !controllerutil.ContainsFinalizer(obj, uninstallFinalizer) { log.V(1).Info("Adding uninstall finalizer.") + patch := client.MergeFrom(obj.DeepCopy()) obj.SetFinalizers(append(obj.GetFinalizers(), uninstallFinalizer)) - if err := r.client.Update(ctx, obj); err != nil { + if err = r.client.Patch(ctx, obj, patch); err != nil { return ctrl.Result{}, errs.Wrapf(err, "failed to add uninstall finalizer to %s/%s", req.NamespacedName.Namespace, req.NamespacedName.Name) } } From cbbcb628e5e08da0c4944bf078b0dcad2775ab0f Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Mon, 17 Nov 2025 21:15:02 +0100 Subject: [PATCH 06/24] reconciler: configure new updater --- pkg/reconciler/reconciler.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index e392661..51baf3d 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -698,7 +698,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re } } - u := updater.New(r.client) + u := updater.New(r.client, log) + if r.enableAggressiveConflictResolution { + u.EnableAggressiveConflictResolution() + } defer func() { applyErr := u.Apply(ctx, obj) if err == nil && !apierrors.IsNotFound(applyErr) { @@ -886,7 +889,10 @@ func (r *Reconciler) handleDeletion(ctx context.Context, actionClient helmclient // and we need to be able to update the conditions on the CR to // indicate that the uninstall failed. if err := func() (err error) { - uninstallUpdater := updater.New(r.client) + uninstallUpdater := updater.New(r.client, log) + if r.enableAggressiveConflictResolution { + uninstallUpdater.EnableAggressiveConflictResolution() + } defer func() { applyErr := uninstallUpdater.Apply(ctx, obj) if err == nil { From 9eb202b1a54e718ab09e67d5266da97dc225d150 Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Mon, 17 Nov 2025 20:17:06 +0100 Subject: [PATCH 07/24] New function (for tests): conditions.go: FromUnstructured --- pkg/internal/status/conditions.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/pkg/internal/status/conditions.go b/pkg/internal/status/conditions.go index 0f9a442..d1739dc 100644 --- a/pkg/internal/status/conditions.go +++ b/pkg/internal/status/conditions.go @@ -22,6 +22,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" kubeclock "k8s.io/utils/clock" ) @@ -189,3 +190,19 @@ func (conditions Conditions) MarshalJSON() ([]byte, error) { }) return json.Marshal(conds) } + +func FromUnstructured(conditionsSlice []interface{}) (Conditions, error) { + conditions := make(Conditions, 0, len(conditionsSlice)) + for _, c := range conditionsSlice { + condMap, ok := c.(map[string]interface{}) + if !ok { + continue + } + cond := Condition{} + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(condMap, &cond); err != nil { + return nil, err + } + conditions = append(conditions, cond) + } + return conditions, nil +} From db5ff471ce97a6ead0cc281adc5ff379ce4248c0 Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Mon, 17 Nov 2025 21:10:43 +0100 Subject: [PATCH 08/24] Updater tests --- .../internal/updater/updater_test.go | 102 +++++++++++++++++- 1 file changed, 100 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/internal/updater/updater_test.go b/pkg/reconciler/internal/updater/updater_test.go index a4f8c3a..9713622 100644 --- a/pkg/reconciler/internal/updater/updater_test.go +++ b/pkg/reconciler/internal/updater/updater_test.go @@ -20,6 +20,7 @@ import ( "context" "errors" + "github.com/go-logr/logr" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -31,7 +32,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + pkgStatus "github.com/operator-framework/helm-operator-plugins/pkg/internal/status" "github.com/operator-framework/helm-operator-plugins/pkg/reconciler/internal/conditions" ) @@ -41,6 +44,10 @@ const ( replicasStatus = int64(5) ) +var ( + transientError = errors.New("transient error") +) + var _ = Describe("Updater", func() { var ( cl client.Client @@ -51,7 +58,7 @@ var _ = Describe("Updater", func() { JustBeforeEach(func() { cl = fake.NewClientBuilder().WithInterceptorFuncs(interceptorFuncs).Build() - u = New(cl) + u = New(cl, logr.Discard()) obj = &unstructured.Unstructured{Object: map[string]interface{}{ "apiVersion": "apps/v1", "kind": "Deployment", @@ -85,7 +92,7 @@ var _ = Describe("Updater", func() { interceptorFuncs.SubResourceUpdate = func(ctx context.Context, interceptorClient client.Client, subResourceName string, obj client.Object, opts ...client.SubResourceUpdateOption) error { updateCallCount++ if updateCallCount == 1 { - return errors.New("transient error") + return transientError } return interceptorClient.SubResource(subResourceName).Update(ctx, obj, opts...) } @@ -177,9 +184,85 @@ var _ = Describe("Updater", func() { Expect(found).To(BeTrue()) Expect(err).To(Succeed()) }) + + It("should add a finalizer", func() { + u.Update(func(u *unstructured.Unstructured) bool { + return controllerutil.AddFinalizer(u, testFinalizer) + }) + Expect(u.Apply(context.TODO(), obj)).To(Succeed()) + Expect(cl.Get(context.TODO(), types.NamespacedName{Namespace: "testNamespace", Name: "testDeployment"}, obj)).To(Succeed()) + Expect(obj.GetFinalizers()).To(ContainElement(testFinalizer)) + }) + + It("should remove a finalizer", func() { + obj.SetFinalizers([]string{testFinalizer}) + Expect(cl.Update(context.TODO(), obj)).To(Succeed()) + + u.Update(func(u *unstructured.Unstructured) bool { + return controllerutil.RemoveFinalizer(u, testFinalizer) + }) + Expect(u.Apply(context.TODO(), obj)).To(Succeed()) + Expect(cl.Get(context.TODO(), types.NamespacedName{Namespace: "testNamespace", Name: "testDeployment"}, obj)).To(Succeed()) + Expect(obj.GetFinalizers()).ToNot(ContainElement(testFinalizer)) + }) + + It("should preserve unknown status conditions", func() { + // Add external status condition on cluster. + clusterObj := obj.DeepCopy() + unknownCondition := map[string]interface{}{ + "type": "UnknownCondition", + "status": string(corev1.ConditionTrue), + "reason": "ExternallyManaged", + } + Expect(unstructured.SetNestedSlice(clusterObj.Object, []interface{}{unknownCondition}, "status", "conditions")).To(Succeed()) + Expect(retryOnTransientError(func() error { + return cl.Status().Update(context.TODO(), clusterObj) + })).ToNot(HaveOccurred()) + // Add status condition using updater. + u.UpdateStatus(EnsureCondition(conditions.Deployed(corev1.ConditionTrue, "", ""))) + u.EnableAggressiveConflictResolution() + Expect(u.Apply(context.TODO(), obj)).To(Succeed()) + // Retrieve object from cluster and extract status conditions. + Expect(cl.Get(context.TODO(), types.NamespacedName{Namespace: "testNamespace", Name: "testDeployment"}, obj)).To(Succeed()) + objConditionsSlice, _, err := unstructured.NestedSlice(obj.Object, "status", "conditions") + Expect(err).ToNot(HaveOccurred()) + objConditions, err := pkgStatus.FromUnstructured(objConditionsSlice) + Expect(err).ToNot(HaveOccurred()) + // Verify both status conditions are present. + Expect(objConditions.IsTrueFor(pkgStatus.ConditionType("UnknownCondition"))).To(BeTrue()) + Expect(objConditions.IsTrueFor(pkgStatus.ConditionType("Deployed"))).To(BeTrue()) + }) + + It("should fail on conflict without aggressive resolution", func() { + // Add external status condition on cluster. + clusterObj := obj.DeepCopy() + unknownCondition := map[string]interface{}{ + "type": "UnknownCondition", + "status": string(corev1.ConditionTrue), + "reason": "ExternallyManaged", + } + Expect(unstructured.SetNestedSlice(clusterObj.Object, []interface{}{unknownCondition}, "status", "conditions")).To(Succeed()) + Expect(retryOnTransientError(func() error { + return cl.Status().Update(context.TODO(), clusterObj) + })).ToNot(HaveOccurred()) + Expect(cl.Status().Update(context.TODO(), clusterObj)).To(Succeed()) + // Add status condition using updater. + u.UpdateStatus(EnsureCondition(conditions.Deployed(corev1.ConditionTrue, "", ""))) + err := u.Apply(context.TODO(), obj) + // Verify conflict error is returned. + Expect(apierrors.IsConflict(err)).To(BeTrue()) + }) }) }) +func retryOnTransientError(f func() error) error { + err := f() + if errors.Is(err, transientError) { + err = f() + } + return err +} + var _ = Describe("RemoveFinalizer", func() { var obj *unstructured.Unstructured @@ -325,4 +408,19 @@ var _ = Describe("statusFor", func() { obj.Object["status"] = "hello" Expect(statusFor(obj)).To(Equal(&helmAppStatus{})) }) + + It("should handle unknown status conditions", func() { + uSt := map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + "type": "UnknownCondition", + "status": string(corev1.ConditionTrue), + }, + }, + } + obj.Object["status"] = uSt + status := statusFor(obj) + Expect(status).ToNot(BeNil()) + Expect(status.Conditions.IsTrueFor(pkgStatus.ConditionType("UnknownCondition"))).To(BeTrue()) + }) }) From 71ab32cba26f2ea52ff443b62c86df5bb47a1464 Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Mon, 17 Nov 2025 21:20:01 +0100 Subject: [PATCH 09/24] transient --- pkg/reconciler/internal/updater/updater_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/internal/updater/updater_test.go b/pkg/reconciler/internal/updater/updater_test.go index 9713622..1489d14 100644 --- a/pkg/reconciler/internal/updater/updater_test.go +++ b/pkg/reconciler/internal/updater/updater_test.go @@ -45,7 +45,7 @@ const ( ) var ( - transientError = errors.New("transient error") + errTransient = errors.New("transient error") ) var _ = Describe("Updater", func() { @@ -92,7 +92,7 @@ var _ = Describe("Updater", func() { interceptorFuncs.SubResourceUpdate = func(ctx context.Context, interceptorClient client.Client, subResourceName string, obj client.Object, opts ...client.SubResourceUpdateOption) error { updateCallCount++ if updateCallCount == 1 { - return transientError + return errTransient } return interceptorClient.SubResource(subResourceName).Update(ctx, obj, opts...) } @@ -257,7 +257,7 @@ var _ = Describe("Updater", func() { func retryOnTransientError(f func() error) error { err := f() - if errors.Is(err, transientError) { + if errors.Is(err, errTransient) { err = f() } return err From bb2eb0c73a51f518b0bca6e059da9538f38adbdf Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Wed, 19 Nov 2025 09:16:32 +0100 Subject: [PATCH 10/24] updater.go: Move backoff definition for smaller diff. --- pkg/reconciler/internal/updater/updater.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index 59cc2b5..a877f04 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/pkg/reconciler/internal/updater/updater.go @@ -110,10 +110,11 @@ func (u *Updater) Apply(ctx context.Context, baseObj *unstructured.Unstructured) return nil } + backoff := retry.DefaultRetry + // Always update the status first. During uninstall, if // we remove the finalizer, updating the status will fail // because the object and its status will be garbage-collected. - backoff := retry.DefaultRetry statusUpdateAttemptNumber := 0 err := retryOnRetryableUpdateError(backoff, func() error { // Note that st will also include all status conditions, also those not managed by helm-operator. From 603f7838527285f6e7326b305b541cd6b701a8a1 Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Wed, 19 Nov 2025 09:26:02 +0100 Subject: [PATCH 11/24] Renamings of function names and log messages related to refreshing of the object --- pkg/reconciler/internal/updater/updater.go | 24 +++++++++++++--------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index a877f04..52c7b6a 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/pkg/reconciler/internal/updater/updater.go @@ -137,7 +137,7 @@ func (u *Updater) Apply(ctx context.Context, baseObj *unstructured.Unstructured) statusUpdateAttemptNumber++ updateErr := u.client.Status().Update(ctx, obj) if errors.IsConflict(updateErr) && u.enableAggressiveConflictResolution { - resolved, resolveErr := u.tryStatusUpdateConflictResolution(ctx, baseObj) + resolved, resolveErr := u.tryRefreshForStatusUpdate(ctx, baseObj) if resolveErr != nil { return resolveErr } @@ -171,7 +171,7 @@ func (u *Updater) Apply(ctx context.Context, baseObj *unstructured.Unstructured) updateAttemptNumber++ updateErr := u.client.Update(ctx, obj) if errors.IsConflict(updateErr) && u.enableAggressiveConflictResolution { - resolved, resolveErr := u.tryUpdateConflictResolution(ctx, baseObj) + resolved, resolveErr := u.tryRefreshForUpdate(ctx, baseObj) if resolveErr != nil { return resolveErr } @@ -189,9 +189,9 @@ func (u *Updater) Apply(ctx context.Context, baseObj *unstructured.Unstructured) return err } -func (u *Updater) tryStatusUpdateConflictResolution(ctx context.Context, obj *unstructured.Unstructured) (bool, error) { +func (u *Updater) tryRefreshForStatusUpdate(ctx context.Context, obj *unstructured.Unstructured) (bool, error) { u.logger.V(1).Info("Status update conflict detected") - // Retrieve current version from the cluster. + // Re-fetch object with client. current := &unstructured.Unstructured{} current.SetGroupVersionKind(obj.GroupVersionKind()) objectKey := client.ObjectKeyFromObject(obj) @@ -201,13 +201,14 @@ func (u *Updater) tryStatusUpdateConflictResolution(ctx context.Context, obj *un } obj.Object = current.Object - u.logger.V(1).Info("Resolved status update conflict by fetching current in-cluster version") + u.logger.V(1).Info("Refreshed object", "namespace", + objectKey.Namespace, "name", objectKey.Name, "gvk", obj.GroupVersionKind()) return true, nil } -func (u *Updater) tryUpdateConflictResolution(ctx context.Context, obj *unstructured.Unstructured) (bool, error) { +func (u *Updater) tryRefreshForUpdate(ctx context.Context, obj *unstructured.Unstructured) (bool, error) { u.logger.V(1).Info("Update conflict detected") - // Retrieve current version from the cluster. + // Re-fetch object with client. current := &unstructured.Unstructured{} current.SetGroupVersionKind(obj.GroupVersionKind()) objectKey := client.ObjectKeyFromObject(obj) @@ -218,14 +219,17 @@ func (u *Updater) tryUpdateConflictResolution(ctx context.Context, obj *unstruct if !reflect.DeepEqual(obj.Object["spec"], current.Object["spec"]) { // Diff in object spec. Nothing we can do about it -> Fail. - u.logger.V(1).Info("Cluster resource cannot be updated due to spec mismatch", - "namespace", objectKey.Namespace, "name", objectKey.Name, "gkv", obj.GroupVersionKind(), + u.logger.V(1).Info("Not refreshing object due to spec mismatch", + "namespace", objectKey.Namespace, + "name", objectKey.Name, + "gkv", obj.GroupVersionKind(), ) return false, nil } obj.Object = current.Object - u.logger.V(1).Info("Resolved update conflict by fetching current in-cluster version") + u.logger.V(1).Info("Refreshed object", + "namespace", objectKey.Namespace, "name", objectKey.Name, "gvk", obj.GroupVersionKind()) return true, nil } From 0d88dae6e6212712b2a300073fec83d63d4ce56b Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Wed, 19 Nov 2025 09:37:15 +0100 Subject: [PATCH 12/24] updater: Refactor attempt counting during retries --- pkg/reconciler/internal/updater/updater.go | 26 ++++++++++++---------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index 52c7b6a..7323d8d 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/pkg/reconciler/internal/updater/updater.go @@ -101,8 +101,14 @@ func isRetryableUpdateError(err error) bool { // // A NotFound error means that the object has been deleted, and the reconciliation loop // needs to be restarted anew as well. -func retryOnRetryableUpdateError(backoff wait.Backoff, f func() error) error { - return retry.OnError(backoff, isRetryableUpdateError, f) +func retryOnRetryableUpdateError(backoff wait.Backoff, f func(attemptNum uint) error) error { + var attemptNum uint = 1 + countingF := func() error { + err := f(attemptNum) + attemptNum++ + return err + } + return retry.OnError(backoff, isRetryableUpdateError, countingF) } func (u *Updater) Apply(ctx context.Context, baseObj *unstructured.Unstructured) error { @@ -115,8 +121,7 @@ func (u *Updater) Apply(ctx context.Context, baseObj *unstructured.Unstructured) // Always update the status first. During uninstall, if // we remove the finalizer, updating the status will fail // because the object and its status will be garbage-collected. - statusUpdateAttemptNumber := 0 - err := retryOnRetryableUpdateError(backoff, func() error { + err := retryOnRetryableUpdateError(backoff, func(attemptNumber uint) error { // Note that st will also include all status conditions, also those not managed by helm-operator. obj := baseObj.DeepCopy() st := statusFor(obj) @@ -131,10 +136,9 @@ func (u *Updater) Apply(ctx context.Context, baseObj *unstructured.Unstructured) st.updateStatusObject() obj.Object["status"] = st.StatusObject - if statusUpdateAttemptNumber > 0 { - u.logger.V(1).Info("Retrying status update", "attempt", statusUpdateAttemptNumber) + if attemptNumber > 1 { + u.logger.V(1).Info("Retrying status update", "attempt", attemptNumber) } - statusUpdateAttemptNumber++ updateErr := u.client.Status().Update(ctx, obj) if errors.IsConflict(updateErr) && u.enableAggressiveConflictResolution { resolved, resolveErr := u.tryRefreshForStatusUpdate(ctx, baseObj) @@ -155,8 +159,7 @@ func (u *Updater) Apply(ctx context.Context, baseObj *unstructured.Unstructured) return err } - updateAttemptNumber := 0 - err = retryOnRetryableUpdateError(backoff, func() error { + err = retryOnRetryableUpdateError(backoff, func(attemptNumber uint) error { obj := baseObj.DeepCopy() needsUpdate := false for _, f := range u.updateFuncs { @@ -165,10 +168,9 @@ func (u *Updater) Apply(ctx context.Context, baseObj *unstructured.Unstructured) if !needsUpdate { return nil } - if updateAttemptNumber > 0 { - u.logger.V(1).Info("Retrying update", "attempt", updateAttemptNumber) + if attemptNumber > 1 { + u.logger.V(1).Info("Retrying update", "attempt", attemptNumber) } - updateAttemptNumber++ updateErr := u.client.Update(ctx, obj) if errors.IsConflict(updateErr) && u.enableAggressiveConflictResolution { resolved, resolveErr := u.tryRefreshForUpdate(ctx, baseObj) From 8a6358369f39d35eb673f095a9e41d27c1e20fbb Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Wed, 19 Nov 2025 09:39:57 +0100 Subject: [PATCH 13/24] updater tests: seperate Expect() clause after calling retryOnTransientError --- pkg/reconciler/internal/updater/updater_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/internal/updater/updater_test.go b/pkg/reconciler/internal/updater/updater_test.go index 1489d14..8adc56a 100644 --- a/pkg/reconciler/internal/updater/updater_test.go +++ b/pkg/reconciler/internal/updater/updater_test.go @@ -215,9 +215,10 @@ var _ = Describe("Updater", func() { "reason": "ExternallyManaged", } Expect(unstructured.SetNestedSlice(clusterObj.Object, []interface{}{unknownCondition}, "status", "conditions")).To(Succeed()) - Expect(retryOnTransientError(func() error { + err := retryOnTransientError(func() error { return cl.Status().Update(context.TODO(), clusterObj) - })).ToNot(HaveOccurred()) + }) + Expect(err).ToNot(HaveOccurred()) // Add status condition using updater. u.UpdateStatus(EnsureCondition(conditions.Deployed(corev1.ConditionTrue, "", ""))) u.EnableAggressiveConflictResolution() From e85b41a91789ea83e07c443433e7ef0054ab7efe Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Wed, 19 Nov 2025 09:44:19 +0100 Subject: [PATCH 14/24] New reconciler method: newUpdater --- pkg/reconciler/reconciler.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 51baf3d..329fe1c 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -698,10 +698,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re } } - u := updater.New(r.client, log) - if r.enableAggressiveConflictResolution { - u.EnableAggressiveConflictResolution() - } + u := r.newUpdater(log) defer func() { applyErr := u.Apply(ctx, obj) if err == nil && !apierrors.IsNotFound(applyErr) { @@ -854,6 +851,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return ctrl.Result{RequeueAfter: r.reconcilePeriod}, nil } +func (r *Reconciler) newUpdater(log logr.Logger) updater.Updater { + u := updater.New(r.client, log) + if r.enableAggressiveConflictResolution { + u.EnableAggressiveConflictResolution() + } + return u +} + func (r *Reconciler) getValues(ctx context.Context, obj *unstructured.Unstructured) (chartutil.Values, error) { if err := internalvalues.ApplyOverrides(r.overrideValues, obj); err != nil { return chartutil.Values{}, err @@ -889,10 +894,7 @@ func (r *Reconciler) handleDeletion(ctx context.Context, actionClient helmclient // and we need to be able to update the conditions on the CR to // indicate that the uninstall failed. if err := func() (err error) { - uninstallUpdater := updater.New(r.client, log) - if r.enableAggressiveConflictResolution { - uninstallUpdater.EnableAggressiveConflictResolution() - } + uninstallUpdater := r.newUpdater(log) defer func() { applyErr := uninstallUpdater.Apply(ctx, obj) if err == nil { From 9e4a6508492f3e74cf6a0e22bf6fd163c9db7b3f Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Wed, 19 Nov 2025 10:00:28 +0100 Subject: [PATCH 15/24] updater tests: factor our coming init code into a shared JustBeforeEach. --- .../internal/updater/updater_test.go | 80 +++++++++---------- 1 file changed, 36 insertions(+), 44 deletions(-) diff --git a/pkg/reconciler/internal/updater/updater_test.go b/pkg/reconciler/internal/updater/updater_test.go index 8adc56a..1b7fa6d 100644 --- a/pkg/reconciler/internal/updater/updater_test.go +++ b/pkg/reconciler/internal/updater/updater_test.go @@ -205,53 +205,45 @@ var _ = Describe("Updater", func() { Expect(cl.Get(context.TODO(), types.NamespacedName{Namespace: "testNamespace", Name: "testDeployment"}, obj)).To(Succeed()) Expect(obj.GetFinalizers()).ToNot(ContainElement(testFinalizer)) }) + Context("when in-cluster object has been updated", func() { + JustBeforeEach(func() { + // Add external status condition on cluster. + clusterObj := obj.DeepCopy() + unknownCondition := map[string]interface{}{ + "type": "UnknownCondition", + "status": string(corev1.ConditionTrue), + "reason": "ExternallyManaged", + } + Expect(unstructured.SetNestedSlice(clusterObj.Object, []interface{}{unknownCondition}, "status", "conditions")).To(Succeed()) + err := retryOnTransientError(func() error { + return cl.Status().Update(context.TODO(), clusterObj) + }) + Expect(err).ToNot(HaveOccurred()) + }) - It("should preserve unknown status conditions", func() { - // Add external status condition on cluster. - clusterObj := obj.DeepCopy() - unknownCondition := map[string]interface{}{ - "type": "UnknownCondition", - "status": string(corev1.ConditionTrue), - "reason": "ExternallyManaged", - } - Expect(unstructured.SetNestedSlice(clusterObj.Object, []interface{}{unknownCondition}, "status", "conditions")).To(Succeed()) - err := retryOnTransientError(func() error { - return cl.Status().Update(context.TODO(), clusterObj) + It("should preserve unknown status conditions", func() { + // Add status condition using updater. + u.UpdateStatus(EnsureCondition(conditions.Deployed(corev1.ConditionTrue, "", ""))) + u.EnableAggressiveConflictResolution() + Expect(u.Apply(context.TODO(), obj)).To(Succeed()) + // Retrieve object from cluster and extract status conditions. + Expect(cl.Get(context.TODO(), types.NamespacedName{Namespace: "testNamespace", Name: "testDeployment"}, obj)).To(Succeed()) + objConditionsSlice, _, err := unstructured.NestedSlice(obj.Object, "status", "conditions") + Expect(err).ToNot(HaveOccurred()) + objConditions, err := pkgStatus.FromUnstructured(objConditionsSlice) + Expect(err).ToNot(HaveOccurred()) + // Verify both status conditions are present. + Expect(objConditions.IsTrueFor(pkgStatus.ConditionType("UnknownCondition"))).To(BeTrue()) + Expect(objConditions.IsTrueFor(pkgStatus.ConditionType("Deployed"))).To(BeTrue()) }) - Expect(err).ToNot(HaveOccurred()) - // Add status condition using updater. - u.UpdateStatus(EnsureCondition(conditions.Deployed(corev1.ConditionTrue, "", ""))) - u.EnableAggressiveConflictResolution() - Expect(u.Apply(context.TODO(), obj)).To(Succeed()) - // Retrieve object from cluster and extract status conditions. - Expect(cl.Get(context.TODO(), types.NamespacedName{Namespace: "testNamespace", Name: "testDeployment"}, obj)).To(Succeed()) - objConditionsSlice, _, err := unstructured.NestedSlice(obj.Object, "status", "conditions") - Expect(err).ToNot(HaveOccurred()) - objConditions, err := pkgStatus.FromUnstructured(objConditionsSlice) - Expect(err).ToNot(HaveOccurred()) - // Verify both status conditions are present. - Expect(objConditions.IsTrueFor(pkgStatus.ConditionType("UnknownCondition"))).To(BeTrue()) - Expect(objConditions.IsTrueFor(pkgStatus.ConditionType("Deployed"))).To(BeTrue()) - }) - It("should fail on conflict without aggressive resolution", func() { - // Add external status condition on cluster. - clusterObj := obj.DeepCopy() - unknownCondition := map[string]interface{}{ - "type": "UnknownCondition", - "status": string(corev1.ConditionTrue), - "reason": "ExternallyManaged", - } - Expect(unstructured.SetNestedSlice(clusterObj.Object, []interface{}{unknownCondition}, "status", "conditions")).To(Succeed()) - Expect(retryOnTransientError(func() error { - return cl.Status().Update(context.TODO(), clusterObj) - })).ToNot(HaveOccurred()) - Expect(cl.Status().Update(context.TODO(), clusterObj)).To(Succeed()) - // Add status condition using updater. - u.UpdateStatus(EnsureCondition(conditions.Deployed(corev1.ConditionTrue, "", ""))) - err := u.Apply(context.TODO(), obj) - // Verify conflict error is returned. - Expect(apierrors.IsConflict(err)).To(BeTrue()) + It("should fail on conflict without aggressive resolution", func() { + // Add status condition using updater. + u.UpdateStatus(EnsureCondition(conditions.Deployed(corev1.ConditionTrue, "", ""))) + err := u.Apply(context.TODO(), obj) + // Verify conflict error is returned. + Expect(apierrors.IsConflict(err)).To(BeTrue()) + }) }) }) }) From cc60c7de183123048dd543f2463b6bd4ecfb7e4e Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Wed, 19 Nov 2025 10:08:47 +0100 Subject: [PATCH 16/24] Moved the code for extracting conditions from unstructured the updater tests and use Ginkgo assertions instead of production error handling. --- pkg/internal/status/conditions.go | 17 ----------------- .../internal/updater/updater_test.go | 18 ++++++++++++++++-- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/pkg/internal/status/conditions.go b/pkg/internal/status/conditions.go index d1739dc..0f9a442 100644 --- a/pkg/internal/status/conditions.go +++ b/pkg/internal/status/conditions.go @@ -22,7 +22,6 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" kubeclock "k8s.io/utils/clock" ) @@ -190,19 +189,3 @@ func (conditions Conditions) MarshalJSON() ([]byte, error) { }) return json.Marshal(conds) } - -func FromUnstructured(conditionsSlice []interface{}) (Conditions, error) { - conditions := make(Conditions, 0, len(conditionsSlice)) - for _, c := range conditionsSlice { - condMap, ok := c.(map[string]interface{}) - if !ok { - continue - } - cond := Condition{} - if err := runtime.DefaultUnstructuredConverter.FromUnstructured(condMap, &cond); err != nil { - return nil, err - } - conditions = append(conditions, cond) - } - return conditions, nil -} diff --git a/pkg/reconciler/internal/updater/updater_test.go b/pkg/reconciler/internal/updater/updater_test.go index 1b7fa6d..93d9058 100644 --- a/pkg/reconciler/internal/updater/updater_test.go +++ b/pkg/reconciler/internal/updater/updater_test.go @@ -28,12 +28,14 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "github.com/operator-framework/helm-operator-plugins/pkg/internal/status" pkgStatus "github.com/operator-framework/helm-operator-plugins/pkg/internal/status" "github.com/operator-framework/helm-operator-plugins/pkg/reconciler/internal/conditions" ) @@ -230,8 +232,7 @@ var _ = Describe("Updater", func() { Expect(cl.Get(context.TODO(), types.NamespacedName{Namespace: "testNamespace", Name: "testDeployment"}, obj)).To(Succeed()) objConditionsSlice, _, err := unstructured.NestedSlice(obj.Object, "status", "conditions") Expect(err).ToNot(HaveOccurred()) - objConditions, err := pkgStatus.FromUnstructured(objConditionsSlice) - Expect(err).ToNot(HaveOccurred()) + objConditions := conditionsFromUnstructured(objConditionsSlice) // Verify both status conditions are present. Expect(objConditions.IsTrueFor(pkgStatus.ConditionType("UnknownCondition"))).To(BeTrue()) Expect(objConditions.IsTrueFor(pkgStatus.ConditionType("Deployed"))).To(BeTrue()) @@ -417,3 +418,16 @@ var _ = Describe("statusFor", func() { Expect(status.Conditions.IsTrueFor(pkgStatus.ConditionType("UnknownCondition"))).To(BeTrue()) }) }) + +func conditionsFromUnstructured(conditionsSlice []interface{}) status.Conditions { + conditions := make(status.Conditions, 0, len(conditionsSlice)) + for _, c := range conditionsSlice { + condMap, ok := c.(map[string]interface{}) + Expect(ok).To(BeTrue(), "condition is not a map[string]interface{}") + cond := status.Condition{} + err := runtime.DefaultUnstructuredConverter.FromUnstructured(condMap, &cond) + Expect(err).ToNot(HaveOccurred(), "failed to convert status condition from unstructured") + conditions = append(conditions, cond) + } + return conditions +} From 4b79554376f31091da69a7a75afe133bab9e70ce Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Wed, 19 Nov 2025 10:15:43 +0100 Subject: [PATCH 17/24] Clean up named imports --- pkg/reconciler/internal/updater/updater_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/reconciler/internal/updater/updater_test.go b/pkg/reconciler/internal/updater/updater_test.go index 93d9058..5c45b4c 100644 --- a/pkg/reconciler/internal/updater/updater_test.go +++ b/pkg/reconciler/internal/updater/updater_test.go @@ -35,7 +35,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "github.com/operator-framework/helm-operator-plugins/pkg/internal/status" pkgStatus "github.com/operator-framework/helm-operator-plugins/pkg/internal/status" "github.com/operator-framework/helm-operator-plugins/pkg/reconciler/internal/conditions" ) @@ -419,12 +418,12 @@ var _ = Describe("statusFor", func() { }) }) -func conditionsFromUnstructured(conditionsSlice []interface{}) status.Conditions { - conditions := make(status.Conditions, 0, len(conditionsSlice)) +func conditionsFromUnstructured(conditionsSlice []interface{}) pkgStatus.Conditions { + conditions := make(pkgStatus.Conditions, 0, len(conditionsSlice)) for _, c := range conditionsSlice { condMap, ok := c.(map[string]interface{}) Expect(ok).To(BeTrue(), "condition is not a map[string]interface{}") - cond := status.Condition{} + cond := pkgStatus.Condition{} err := runtime.DefaultUnstructuredConverter.FromUnstructured(condMap, &cond) Expect(err).ToNot(HaveOccurred(), "failed to convert status condition from unstructured") conditions = append(conditions, cond) From f85292afd11f3e6192a09e5ebf01f0fd2663d75b Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Fri, 21 Nov 2025 10:21:20 +0100 Subject: [PATCH 18/24] err scope --- pkg/reconciler/reconciler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 329fe1c..4ad5817 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -693,7 +693,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re log.V(1).Info("Adding uninstall finalizer.") patch := client.MergeFrom(obj.DeepCopy()) obj.SetFinalizers(append(obj.GetFinalizers(), uninstallFinalizer)) - if err = r.client.Patch(ctx, obj, patch); err != nil { + if err := r.client.Patch(ctx, obj, patch); err != nil { return ctrl.Result{}, errs.Wrapf(err, "failed to add uninstall finalizer to %s/%s", req.NamespacedName.Namespace, req.NamespacedName.Name) } } From ebf75a7b632741a24b2b2a06d44209b3bacb6d6e Mon Sep 17 00:00:00 2001 From: Marcin Owsiany Date: Fri, 21 Nov 2025 10:24:08 +0100 Subject: [PATCH 19/24] refactor tryRefresh* (#66) --- pkg/reconciler/internal/updater/updater.go | 49 ++++++++++------------ 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index 7323d8d..892977e 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/pkg/reconciler/internal/updater/updater.go @@ -141,7 +141,8 @@ func (u *Updater) Apply(ctx context.Context, baseObj *unstructured.Unstructured) } updateErr := u.client.Status().Update(ctx, obj) if errors.IsConflict(updateErr) && u.enableAggressiveConflictResolution { - resolved, resolveErr := u.tryRefreshForStatusUpdate(ctx, baseObj) + u.logger.V(1).Info("Status update conflict detected") + resolved, resolveErr := u.tryRefresh(ctx, baseObj, isSafeForStatusUpdate) if resolveErr != nil { return resolveErr } @@ -173,7 +174,8 @@ func (u *Updater) Apply(ctx context.Context, baseObj *unstructured.Unstructured) } updateErr := u.client.Update(ctx, obj) if errors.IsConflict(updateErr) && u.enableAggressiveConflictResolution { - resolved, resolveErr := u.tryRefreshForUpdate(ctx, baseObj) + u.logger.V(1).Info("Update conflict detected") + resolved, resolveErr := u.tryRefresh(ctx, baseObj, isSafeForUpdate) if resolveErr != nil { return resolveErr } @@ -191,25 +193,24 @@ func (u *Updater) Apply(ctx context.Context, baseObj *unstructured.Unstructured) return err } -func (u *Updater) tryRefreshForStatusUpdate(ctx context.Context, obj *unstructured.Unstructured) (bool, error) { - u.logger.V(1).Info("Status update conflict detected") - // Re-fetch object with client. - current := &unstructured.Unstructured{} - current.SetGroupVersionKind(obj.GroupVersionKind()) - objectKey := client.ObjectKeyFromObject(obj) - if err := u.client.Get(ctx, objectKey, current); err != nil { - err = fmt.Errorf("refreshing object %s/%s: %w", objectKey.Namespace, objectKey.Name, err) - return false, err - } +func isSafeForStatusUpdate(_ logr.Logger, _ *unstructured.Unstructured, _ *unstructured.Unstructured) bool { + return true +} - obj.Object = current.Object - u.logger.V(1).Info("Refreshed object", "namespace", - objectKey.Namespace, "name", objectKey.Name, "gvk", obj.GroupVersionKind()) - return true, nil +func isSafeForUpdate(logger logr.Logger, inMemory *unstructured.Unstructured, onCluster *unstructured.Unstructured) bool { + if !reflect.DeepEqual(inMemory.Object["spec"], onCluster.Object["spec"]) { + // Diff in object spec. Nothing we can do about it -> Fail. + logger.V(1).Info("Not refreshing object due to spec mismatch", + "namespace", inMemory.GetNamespace(), + "name", inMemory.GetName(), + "gkv", inMemory.GroupVersionKind(), + ) + return false + } + return true } -func (u *Updater) tryRefreshForUpdate(ctx context.Context, obj *unstructured.Unstructured) (bool, error) { - u.logger.V(1).Info("Update conflict detected") +func (u *Updater) tryRefresh(ctx context.Context, obj *unstructured.Unstructured, isSafe func(logger logr.Logger, inMemory *unstructured.Unstructured, onCluster *unstructured.Unstructured) bool) (bool, error) { // Re-fetch object with client. current := &unstructured.Unstructured{} current.SetGroupVersionKind(obj.GroupVersionKind()) @@ -219,19 +220,15 @@ func (u *Updater) tryRefreshForUpdate(ctx context.Context, obj *unstructured.Uns return false, err } - if !reflect.DeepEqual(obj.Object["spec"], current.Object["spec"]) { - // Diff in object spec. Nothing we can do about it -> Fail. - u.logger.V(1).Info("Not refreshing object due to spec mismatch", - "namespace", objectKey.Namespace, - "name", objectKey.Name, - "gkv", obj.GroupVersionKind(), - ) + if !isSafe(u.logger, obj, current) { return false, nil } obj.Object = current.Object u.logger.V(1).Info("Refreshed object", - "namespace", objectKey.Namespace, "name", objectKey.Name, "gvk", obj.GroupVersionKind()) + "namespace", objectKey.Namespace, + "name", objectKey.Name, + "gvk", obj.GroupVersionKind()) return true, nil } From fb30c679c22038d7f54e804974cbe3390d6422e1 Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Fri, 21 Nov 2025 10:28:29 +0100 Subject: [PATCH 20/24] Update comment about conflict retries --- pkg/reconciler/internal/updater/updater.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index 892977e..0908748 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/pkg/reconciler/internal/updater/updater.go @@ -95,9 +95,11 @@ func isRetryableUpdateError(err error) bool { // retryOnRetryableUpdateError retries the given function until it succeeds, // until the given backoff is exhausted, or until the error is not retryable. // -// In case of a Conflict error, the update cannot be retried because the underlying -// resource has been modified in the meantime, and the reconciliation loop needs -// to be restarted anew. +// In case of a Conflict error, the update is not retried by default because the +// underlying resource has been modified in the meantime, and the reconciliation loop +// needs to be restarted anew. However, when aggressive conflict resolution is enabled, +// the updater attempts to refresh the object from the cluster and retry if it's safe +// to do so (e.g., when only the status has changed). // // A NotFound error means that the object has been deleted, and the reconciliation loop // needs to be restarted anew as well. From 79610cdb3ff5a83c97017201710064ebd7e5f335 Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Fri, 21 Nov 2025 10:55:45 +0100 Subject: [PATCH 21/24] Add tests for preserving finalizers --- .../internal/updater/updater_test.go | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/pkg/reconciler/internal/updater/updater_test.go b/pkg/reconciler/internal/updater/updater_test.go index 5c45b4c..d9718ee 100644 --- a/pkg/reconciler/internal/updater/updater_test.go +++ b/pkg/reconciler/internal/updater/updater_test.go @@ -206,6 +206,35 @@ var _ = Describe("Updater", func() { Expect(cl.Get(context.TODO(), types.NamespacedName{Namespace: "testNamespace", Name: "testDeployment"}, obj)).To(Succeed()) Expect(obj.GetFinalizers()).ToNot(ContainElement(testFinalizer)) }) + It("should preserve a finalizer when removing a different one", func() { + otherFinalizer := "otherFinalizer" + obj.SetFinalizers([]string{testFinalizer, otherFinalizer}) + Expect(cl.Update(context.TODO(), obj)).To(Succeed()) + + u.Update(func(u *unstructured.Unstructured) bool { + return controllerutil.RemoveFinalizer(u, testFinalizer) + }) + Expect(u.Apply(context.TODO(), obj)).To(Succeed()) + Expect(cl.Get(context.TODO(), types.NamespacedName{Namespace: "testNamespace", Name: "testDeployment"}, obj)).To(Succeed()) + Expect(obj.GetFinalizers()).ToNot(ContainElement(testFinalizer)) + Expect(obj.GetFinalizers()).To(ContainElement(otherFinalizer)) + }) + Context("with aggressive conflict resolution enabled", func() { + It("should preserve a finalizer when removing a different one", func() { + otherFinalizer := "otherFinalizer" + obj.SetFinalizers([]string{testFinalizer, otherFinalizer}) + Expect(cl.Update(context.TODO(), obj)).To(Succeed()) + u.EnableAggressiveConflictResolution() + + u.Update(func(u *unstructured.Unstructured) bool { + return controllerutil.RemoveFinalizer(u, testFinalizer) + }) + Expect(u.Apply(context.TODO(), obj)).To(Succeed()) + Expect(cl.Get(context.TODO(), types.NamespacedName{Namespace: "testNamespace", Name: "testDeployment"}, obj)).To(Succeed()) + Expect(obj.GetFinalizers()).ToNot(ContainElement(testFinalizer)) + Expect(obj.GetFinalizers()).To(ContainElement(otherFinalizer)) + }) + }) Context("when in-cluster object has been updated", func() { JustBeforeEach(func() { // Add external status condition on cluster. From b80105f7d8e19a065f046f39dccbd2ec6bc2181c Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Fri, 21 Nov 2025 10:57:09 +0100 Subject: [PATCH 22/24] Rename function to retryOnceOnTransientError --- pkg/reconciler/internal/updater/updater_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/internal/updater/updater_test.go b/pkg/reconciler/internal/updater/updater_test.go index d9718ee..6fdfdc8 100644 --- a/pkg/reconciler/internal/updater/updater_test.go +++ b/pkg/reconciler/internal/updater/updater_test.go @@ -245,7 +245,7 @@ var _ = Describe("Updater", func() { "reason": "ExternallyManaged", } Expect(unstructured.SetNestedSlice(clusterObj.Object, []interface{}{unknownCondition}, "status", "conditions")).To(Succeed()) - err := retryOnTransientError(func() error { + err := retryOnceOnTransientError(func() error { return cl.Status().Update(context.TODO(), clusterObj) }) Expect(err).ToNot(HaveOccurred()) @@ -277,7 +277,7 @@ var _ = Describe("Updater", func() { }) }) -func retryOnTransientError(f func() error) error { +func retryOnceOnTransientError(f func() error) error { err := f() if errors.Is(err, errTransient) { err = f() From 319b0992d08a0c2cc6955eead8cab12083acbaca Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Fri, 21 Nov 2025 12:21:07 +0100 Subject: [PATCH 23/24] reconciler tests: Added tests for WithAggressiveConflictResolution() --- pkg/reconciler/reconciler_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 7c5af66..699f266 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -480,6 +480,16 @@ var _ = Describe("Reconciler", func() { Expect(r.selectorPredicate.Generic(event.GenericEvent{Object: objUnlabeled})).To(BeFalse()) }) }) + _ = Describe("WithAggressiveConflictResolution", func() { + It("should set aggressive conflict resolution to true", func() { + Expect(WithAggressiveConflictResolution(true)(r)).To(Succeed()) + Expect(r.enableAggressiveConflictResolution).To(BeTrue()) + }) + It("should set aggressive conflict resolution to false", func() { + Expect(WithAggressiveConflictResolution(false)(r)).To(Succeed()) + Expect(r.enableAggressiveConflictResolution).To(BeFalse()) + }) + }) }) _ = Describe("Reconcile", func() { From 4d79eb03b486755216d55a38d5cae8a942033e4f Mon Sep 17 00:00:00 2001 From: Moritz Clasmeier Date: Fri, 21 Nov 2025 13:13:52 +0100 Subject: [PATCH 24/24] Revert change to uninstall finalizer adding --- pkg/reconciler/reconciler.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 4ad5817..8d7597c 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -691,9 +691,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re // This is a safety measure to ensure that the chart is fully uninstalled before the CR is deleted. if obj.GetDeletionTimestamp() == nil && !controllerutil.ContainsFinalizer(obj, uninstallFinalizer) { log.V(1).Info("Adding uninstall finalizer.") - patch := client.MergeFrom(obj.DeepCopy()) obj.SetFinalizers(append(obj.GetFinalizers(), uninstallFinalizer)) - if err := r.client.Patch(ctx, obj, patch); err != nil { + if err := r.client.Update(ctx, obj); err != nil { return ctrl.Result{}, errs.Wrapf(err, "failed to add uninstall finalizer to %s/%s", req.NamespacedName.Namespace, req.NamespacedName.Name) } }