From ea7ec882911fa2d413b170c643251edad99b4912 Mon Sep 17 00:00:00 2001 From: Camila Macedo Date: Tue, 12 Nov 2024 02:09:46 +0000 Subject: [PATCH] Refactor ClusterExtensionReconciler to improve conflict resolution and data consistency The error `the object has been modified; please apply your changes to the latest version and try again` occurs when we attempt to update a resource that has changed since it was initially fetched. These changes ensure that we fetch the current state of the resource just before issuing updates, rather than using the version obtained at the start of reconciliation. Additionally: - Deep copy the ClusterExtension object before reconciliation to preserve the original state. - Re-fetch the latest resource version before updating status and finalizers to handle conflicts from concurrent updates. Closes: https://github.com/operator-framework/operator-controller/issues/1414 --- .../clusterextension_controller.go | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index 76ebd3932..c9c09aa6e 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -113,29 +113,28 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req reconciledExt := existingExt.DeepCopy() res, reconcileErr := r.reconcile(ctx, reconciledExt) - // Do checks before any Update()s, as Update() may modify the resource structure! - updateStatus := !equality.Semantic.DeepEqual(existingExt.Status, reconciledExt.Status) - updateFinalizers := !equality.Semantic.DeepEqual(existingExt.Finalizers, reconciledExt.Finalizers) - // If any unexpected fields have changed, panic before updating the resource unexpectedFieldsChanged := checkForUnexpectedFieldChange(*existingExt, *reconciledExt) if unexpectedFieldsChanged { panic("spec or metadata changed by reconciler") } - // Save the finalizers off to the side. If we update the status, the reconciledExt will be updated - // to contain the new state of the ClusterExtension, which contains the status update, but (critically) - // does not contain the finalizers. After the status update, we need to re-add the finalizers to the - // reconciledExt before updating the object. - finalizers := reconciledExt.Finalizers - if updateStatus { + // Re-fetch the object to get the latest resource version + // This is necessary because the object may have been updated by another controller + // between the time we fetched it and the time we finished reconciling it + if err := r.Client.Get(ctx, req.NamespacedName, existingExt); err != nil { + return ctrl.Result{}, client.IgnoreNotFound(err) + } + if !equality.Semantic.DeepEqual(existingExt.Status, reconciledExt.Status) { if err := r.Client.Status().Update(ctx, reconciledExt); err != nil { reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating status: %v", err)) } } - reconciledExt.Finalizers = finalizers - if updateFinalizers { + if err := r.Client.Get(ctx, req.NamespacedName, existingExt); err != nil { + return ctrl.Result{}, client.IgnoreNotFound(err) + } + if !equality.Semantic.DeepEqual(existingExt.Finalizers, reconciledExt.Finalizers) { if err := r.Client.Update(ctx, reconciledExt); err != nil { reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating finalizers: %v", err)) }