Refactor service status update logic to remove unnecessary context and client parameters (#1144) (#1152)

Signed-off-by: Daniel Fan <[email protected]>

Author: Daniel-Fan

api/v1alpha1/operandrequest_types.go

@@ -17,13 +17,10 @@
 package v1alpha1
 
 import (
-	"context"
 	"strings"
 	"sync"
 	"time"
 
-	client "sigs.k8s.io/controller-runtime/pkg/client"
-
 	gset "github.com/deckarep/golang-set"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -438,52 +435,40 @@ func (r *OperandRequest) RemoveOperandPhase(name string, mu sync.Locker) {
 	}
 }
 
-func (r *OperandRequest) SetServiceStatus(ctx context.Context, service ServiceStatus, updater client.StatusClient, mu sync.Locker) error {
+// SetServiceStatus updates the service status in the local object
+func (r *OperandRequest) SetServiceStatus(service ServiceStatus, mu sync.Locker) {
 	mu.Lock()
 	defer mu.Unlock()
+
 	pos, _ := getServiceStatus(&r.Status, service.OperatorName)
 	if pos != -1 {
 		if len(r.Status.Services[pos].Resources) == 0 {
+			// If no resources, replace the entire service
 			r.Status.Services[pos] = service
-			updateerr := updater.Status().Update(ctx, r)
-			if updateerr != nil {
-				return updateerr
-			}
 		} else {
+			// Update status if different
 			if r.Status.Services[pos].Status != service.Status {
 				r.Status.Services[pos].Status = service.Status
-				updateerr := updater.Status().Update(ctx, r)
-				if updateerr != nil {
-					return updateerr
-				}
 			}
+
+			// Process each resource
 			for i := range service.Resources {
 				if service.Resources[i].ObjectName != "" {
 					resourcePos, _ := getResourceStatus(r.Status.Services[pos].Resources, service.Resources[i].ObjectName)
 					if resourcePos != -1 {
+						// Update existing resource
 						r.Status.Services[pos].Resources[resourcePos] = service.Resources[i]
-						updateerr := updater.Status().Update(ctx, r)
-						if updateerr != nil {
-							return updateerr
-						}
 					} else {
+						// Add new resource
 						r.Status.Services[pos].Resources = append(r.Status.Services[pos].Resources, service.Resources[i])
-						updateerr := updater.Status().Update(ctx, r)
-						if updateerr != nil {
-							return updateerr
-						}
 					}
 				}
 			}
 		}
 	} else {
+		// Add new service
 		r.Status.Services = append(r.Status.Services, service)
-		updateerr := updater.Status().Update(ctx, r)
-		if updateerr != nil {
-			return updateerr
-		}
 	}
-	return nil
 }
 
 func (r *OperandRequest) setOperatorReadyCondition(operatorPhase OperatorPhase, name string) {

controllers/operandrequest/operandrequest_controller.go

@@ -18,7 +18,6 @@ package operandrequest
 
 import (
 	"context"
-	"fmt"
 	"reflect"
 	"regexp"
 	"strings"
@@ -91,24 +90,28 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 
 	// Always attempt to patch the status after each reconciliation.
 	defer func() {
-		// get the latest instance from the server and check if the status has changed
-		existingInstance := &operatorv1alpha1.OperandRequest{}
-		if err := r.Client.Get(ctx, req.NamespacedName, existingInstance); err != nil && !apierrors.IsNotFound(err) {
-			// Error reading the latest object - requeue the request.
-			reconcileErr = utilerrors.NewAggregate([]error{reconcileErr, fmt.Errorf("error while get latest OperandRequest.Status from server: %v", err)})
-		}
-
-		if reflect.DeepEqual(existingInstance.Status, requestInstance.Status) {
+		// Don't update status for deleted objects
+		if !requestInstance.DeletionTimestamp.IsZero() {
 			return
 		}
 
-		// Update requestInstance's resource version to avoid conflicts
-		requestInstance.ResourceVersion = existingInstance.ResourceVersion
-		if err := r.Client.Status().Patch(ctx, requestInstance, client.MergeFrom(existingInstance)); err != nil && !apierrors.IsNotFound(err) {
-			reconcileErr = utilerrors.NewAggregate([]error{reconcileErr, fmt.Errorf("error while patching OperandRequest.Status: %v", err)})
-		}
-		if reconcileErr != nil {
-			klog.Errorf("failed to patch status for OperandRequest %s: %v", req.NamespacedName.String(), reconcileErr)
+		// Only update if status has changed from the original
+		if !reflect.DeepEqual(originalInstance.Status, requestInstance.Status) {
+			// The controller-runtime client will handle retries with exponential backoff
+			if err := r.Client.Status().Update(ctx, requestInstance); err != nil {
+				if !apierrors.IsConflict(err) {
+					// Only aggregate non-conflict errors
+					reconcileErr = utilerrors.NewAggregate([]error{reconcileErr, err})
+					klog.Errorf("Failed to update status: %v", err)
+				} else {
+					// Log conflicts but don't return an error - the controller will retry anyway
+					klog.V(2).Infof("Status update conflict for %s/%s - will be retried",
+						requestInstance.Namespace, requestInstance.Name)
+				}
+			} else {
+				klog.V(2).Infof("Successfully updated status for %s/%s",
+					requestInstance.Namespace, requestInstance.Name)
+			}
 		}
 	}()
 

controllers/operandrequest/reconcile_operand.go

@@ -349,9 +349,7 @@ func (r *Reconciler) reconcileCRwithConfig(ctx context.Context, service *operato
 					var resources []operatorv1alpha1.OperandStatus
 					resources = append(resources, statusFromCR)
 					serviceStatus := newServiceStatus(operandName, operatorNamespace, resources)
-					if seterr := requestInstance.SetServiceStatus(ctx, serviceStatus, r.Client, mu); seterr != nil {
-						return seterr
-					}
+					requestInstance.SetServiceStatus(serviceStatus, mu)
 				}
 			} else {
 				klog.V(2).Info("Skip the custom resource not created by ODLM")
@@ -431,10 +429,7 @@ func (r *Reconciler) reconcileCRwithRequest(ctx context.Context, requestInstance
 				var resources []operatorv1alpha1.OperandStatus
 				resources = append(resources, statusFromCR)
 				serviceStatus := newServiceStatus(operand.Name, operatorNamespace, resources)
-				seterr := requestInstance.SetServiceStatus(ctx, serviceStatus, r.Client, mu)
-				if seterr != nil {
-					return seterr
-				}
+				requestInstance.SetServiceStatus(serviceStatus, mu)
 			}
 		} else {
 			klog.V(2).Info("Skip the custom resource not created by ODLM")