handle conflict error in instrumentation config updates (#1737)

A typical error we see when trying to update the instrumentation config
CR is:

> 2024-11-12T08:43:28Z ERROR error updating instrumentation config
{"controller":
"instrumentor-instrumentationconfig-instrumentedapplication",
"controllerGroup": "odigos.io", "controllerKind":
"InstrumentedApplication", "InstrumentedApplication":
{"name":"deployment-pricing","namespace":"simple-demo19"}, "namespace":
"simple-demo19", "name": "deployment-pricing", "reconcileID":
"8c980fc2-ead9-47c7-b2ac-69be6e880002", "workload":
"deployment-pricing", "error": "Operation cannot be fulfilled on
instrumentationconfigs.odigos.io \"deployment-pricing\": the object has
been modified; please apply your changes to the latest version and try
again"}

This is due to the Get and Update pattern we use. Following #1710 , use
the update error util for this case as well.
Adding to the error handling util - a check for `IsNotFound` error and
ignoring it.

---------

Co-authored-by: Amir Blum <[email protected]>

instrumentor/controllers/instrumentationconfig/instrumentedapplication_controller.go

@@ -20,6 +20,7 @@ import (
 	"context"
 
 	odigosv1 "github.com/odigos-io/odigos/api/odigos/v1alpha1"
+	"github.com/odigos-io/odigos/k8sutils/pkg/utils"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -55,19 +56,18 @@ func (r *InstrumentedApplicationReconciler) Reconcile(ctx context.Context, req c
 
 	instrumentationRules := &odigosv1.InstrumentationRuleList{}
 	err = r.Client.List(ctx, instrumentationRules)
+	if client.IgnoreNotFound(err) != nil {
+		return ctrl.Result{}, err
+	}
 
 	err = updateInstrumentationConfigForWorkload(&ic, &ia, instrumentationRules)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
 	err = r.Client.Update(ctx, &ic)
-	if client.IgnoreNotFound(err) != nil {
-		logger.Error(err, "error updating instrumentation config", "workload", ia.Name)
-		return ctrl.Result{}, err
+	if err == nil {
+		logger.V(0).Info("Updated instrumentation config", "workload", ia.Name)
 	}
-
-	logger.V(0).Info("Updated instrumentation config", "workload", ia.Name)
-
-	return ctrl.Result{}, nil
+	return utils.K8SUpdateErrorHandler(err)
 }

instrumentor/controllers/instrumentationdevice/instrumentedapplication_controller.go

@@ -52,10 +52,10 @@ func (r *InstrumentedApplicationReconciler) Reconcile(ctx context.Context, req c
 			return ctrl.Result{}, err
 		}
 		err = removeInstrumentationDeviceFromWorkload(ctx, r.Client, req.Namespace, workloadKind, workloadName, ApplyInstrumentationDeviceReasonNoRuntimeDetails)
-		return utils.RetryOnConflict(err)
+		return utils.K8SUpdateErrorHandler(err)
 	}
 
 	isNodeCollectorReady := isDataCollectionReady(ctx, r.Client)
 	err = reconcileSingleWorkload(ctx, r.Client, &runtimeDetails, isNodeCollectorReady)
-	return utils.RetryOnConflict(err)
+	return utils.K8SUpdateErrorHandler(err)
 }

instrumentor/controllers/instrumentationdevice/workload_controllers.go

@@ -19,7 +19,7 @@ type DeploymentReconciler struct {
 func (r *DeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	instrumentedAppName := workload.CalculateWorkloadRuntimeObjectName(req.Name, workload.WorkloadKindDeployment)
 	err := reconcileSingleInstrumentedApplicationByName(ctx, r.Client, instrumentedAppName, req.Namespace)
-	return utils.RetryOnConflict(err)
+	return utils.K8SUpdateErrorHandler(err)
 }
 
 type DaemonSetReconciler struct {
@@ -29,7 +29,7 @@ type DaemonSetReconciler struct {
 func (r *DaemonSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	instrumentedAppName := workload.CalculateWorkloadRuntimeObjectName(req.Name, workload.WorkloadKindDaemonSet)
 	err := reconcileSingleInstrumentedApplicationByName(ctx, r.Client, instrumentedAppName, req.Namespace)
-	return utils.RetryOnConflict(err)
+	return utils.K8SUpdateErrorHandler(err)
 }
 
 type StatefulSetReconciler struct {
@@ -39,7 +39,7 @@ type StatefulSetReconciler struct {
 func (r *StatefulSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	instrumentedAppName := workload.CalculateWorkloadRuntimeObjectName(req.Name, workload.WorkloadKindStatefulSet)
 	err := reconcileSingleInstrumentedApplicationByName(ctx, r.Client, instrumentedAppName, req.Namespace)
-	return utils.RetryOnConflict(err)
+	return utils.K8SUpdateErrorHandler(err)
 }
 
 func reconcileSingleInstrumentedApplicationByName(ctx context.Context, k8sClient client.Client, instrumentedAppName string, namespace string) error {

k8sutils/pkg/utils/retryonconflict.go

@@ -5,12 +5,21 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
-func RetryOnConflict(err error) (reconcile.Result, error) {
+// K8SUpdateErrorHandler is a helper function to handle k8s update errors.
+// It returns a reconcile.Result and an error
+// If the error is a conflict error, it returns a requeue result without an error
+// If the error is a not found error, it returns an empty result without an error
+// For other errors, it returns the error as is - which will cause a requeue
+func K8SUpdateErrorHandler(err error) (reconcile.Result, error) {
 	if errors.IsConflict(err) {
 		// For conflict errors, requeue without returning an error.
 		// this is so that we don't have errors and stack traces in the logs for valid scenario
 		return reconcile.Result{Requeue: true}, nil
 	}
+	if errors.IsNotFound(err) {
+		// For not found errors, ignore
+		return reconcile.Result{}, nil
+	}
 	// For other errors, return as is (will log the stack trace)
 	return reconcile.Result{}, err
 }