fix(instrumentor): reconcile device on restarts

common/consts/consts.go

@@ -5,18 +5,17 @@ import (
 )
 
 const (
-	CurrentNamespaceEnvVar        = "CURRENT_NS"
-	DefaultOdigosNamespace        = "odigos-system"
-	OdigosConfigurationName       = "odigos-config"
-	OdigosConfigurationFileName   = "config.yaml"
-	OTLPPort                      = 4317
-	OTLPHttpPort                  = 4318
-	PprofOdigosPort               = 6060
-	OdigosInstrumentationLabel    = "odigos-instrumentation"
-	InstrumentationEnabled        = "enabled"
-	InstrumentationDisabled       = "disabled"
-	OdigosReportedNameAnnotation  = "odigos.io/reported-name"
-	EbpfInstrumentationAnnotation = "instrumentation.odigos.io/ebpf" // deprecated.
+	CurrentNamespaceEnvVar       = "CURRENT_NS"
+	DefaultOdigosNamespace       = "odigos-system"
+	OdigosConfigurationName      = "odigos-config"
+	OdigosConfigurationFileName  = "config.yaml"
+	OTLPPort                     = 4317
+	OTLPHttpPort                 = 4318
+	PprofOdigosPort              = 6060
+	OdigosInstrumentationLabel   = "odigos-instrumentation"
+	InstrumentationEnabled       = "enabled"
+	InstrumentationDisabled      = "disabled"
+	OdigosReportedNameAnnotation = "odigos.io/reported-name"
 
 	// Used to store the original value of the environment variable in the pod manifest.
 	// This is used to restore the original value when an instrumentation is removed

instrumentor/controllers/instrumentationdevice/collectorsgroup_controller.go

@@ -50,6 +50,7 @@ func (r *CollectorsGroupReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	logger.V(0).Info("Reconciling instrumented applications after node collectors group became ready", "count", len(instApps.Items))
 
 	var reconcileErr error
+	var gotConflict bool
 
 	for _, runtimeDetails := range instApps.Items {
 		var currentInstApp odigosv1.InstrumentedApplication
@@ -67,9 +68,17 @@ func (r *CollectorsGroupReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 
 		err = reconcileSingleWorkload(ctx, r.Client, &currentInstApp, isDataCollectionReady)
 		if err != nil {
+			if apierrors.IsConflict(err) {
+				gotConflict = true
+			}
 			reconcileErr = errors.Join(reconcileErr, err)
 		}
 	}
 
+	if gotConflict && reconcileErr == nil {
+		// if we got a conflict and no other error, we will request a requeue and not return an error
+		// so we can retry the reconciliation but not have logs filled with errors
+		return ctrl.Result{Requeue: true}, nil
+	}
 	return ctrl.Result{}, reconcileErr
 }

instrumentor/controllers/instrumentationdevice/common.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 
 	odigosv1 "github.com/odigos-io/odigos/api/odigos/v1alpha1"
-	"github.com/odigos-io/odigos/common/consts"
 	"github.com/odigos-io/odigos/instrumentor/controllers/utils"
 	"github.com/odigos-io/odigos/instrumentor/controllers/utils/versionsupport"
 	"github.com/odigos-io/odigos/instrumentor/instrumentation"
@@ -42,15 +41,6 @@ var (
 	GetDefaultSDKs = sdks.GetDefaultSDKs
 )
 
-func clearInstrumentationEbpf(obj client.Object) {
-	annotations := obj.GetAnnotations()
-	if annotations == nil {
-		return
-	}
-
-	delete(annotations, consts.EbpfInstrumentationAnnotation)
-}
-
 func isDataCollectionReady(ctx context.Context, c client.Client) bool {
 	logger := log.FromContext(ctx)
 
@@ -165,36 +155,32 @@ func removeInstrumentationDeviceFromWorkload(ctx context.Context, kubeClient cli
 		return client.IgnoreNotFound(err)
 	}
 
-	result, err := controllerutil.CreateOrPatch(ctx, kubeClient, workloadObj, func() error {
-
-		// clear old ebpf instrumentation annotation, just in case it still exists
-		clearInstrumentationEbpf(workloadObj)
-		podSpec, err := getPodSpecFromObject(workloadObj)
-		if err != nil {
-			return err
-		}
-		// If instrumentation device is removed successfully, remove odigos.io/inject-instrumentation label to disable the webhook
-		instrumentation.RemoveInjectInstrumentationLabel(podSpec)
-
-		instrumentation.RevertInstrumentationDevices(podSpec)
-
-		err = instrumentation.RevertEnvOverwrites(workloadObj, podSpec)
-		if err != nil {
-			return err
-		}
+	podSpec, err := getPodSpecFromObject(workloadObj)
+	if err != nil {
+		return err
+	}
+	// If instrumentation device is removed successfully, remove odigos.io/inject-instrumentation label to disable the webhook
+	webhookLabelRemoved := instrumentation.RemoveInjectInstrumentationLabel(podSpec)
+	deviceRemoved := instrumentation.RevertInstrumentationDevices(podSpec)
+	envChanged, err := instrumentation.RevertEnvOverwrites(workloadObj, podSpec)
+	if err != nil {
+		return err
+	}
 
+	// if we didn't change anything, we don't need to update the object
+	// skip the api-server call, return no-op and skip the log message
+	if !webhookLabelRemoved && !deviceRemoved && !envChanged {
 		return nil
-	})
+	}
 
+	err = kubeClient.Update(ctx, workloadObj)
 	if err != nil {
+		// if the update fails due to a conflict, the controller will retry the operation
 		return err
 	}
 
-	modified := result != controllerutil.OperationResultNone
-	if modified {
-		logger := log.FromContext(ctx)
-		logger.V(0).Info("removed instrumentation device from workload", "namespace", workloadObj.GetNamespace(), "kind", workloadObj.GetObjectKind(), "name", workloadObj.GetName(), "reason", uninstrumentReason)
-	}
+	logger := log.FromContext(ctx)
+	logger.V(0).Info("removed instrumentation device from workload", "namespace", workloadObj.GetNamespace(), "kind", workloadObj.GetObjectKind(), "name", workloadObj.GetName(), "reason", uninstrumentReason)
 
 	return nil
 }

instrumentor/controllers/instrumentationdevice/instrumentationrule_controller.go

@@ -4,6 +4,7 @@ import (
 	"context"
 
 	odigosv1 "github.com/odigos-io/odigos/api/odigos/v1alpha1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -23,15 +24,24 @@ func (r *InstrumentationRuleReconciler) Reconcile(ctx context.Context, req ctrl.
 	}
 	isNodeCollectorReady := isDataCollectionReady(ctx, r.Client)
 
+	gotConflict := false
 	for _, runtimeDetails := range instApps.Items {
 		err := reconcileSingleWorkload(ctx, r.Client, &runtimeDetails, isNodeCollectorReady)
 		if err != nil {
-			return ctrl.Result{}, err
+			if apierrors.IsConflict(err) {
+				gotConflict = true
+			} else {
+				return ctrl.Result{}, err
+			}
 		}
 	}
 
 	logger := log.FromContext(ctx)
 	logger.V(0).Info("InstrumentationRule changed, recalculating instrumentation device for potential changes of otel sdks")
 
-	return ctrl.Result{}, nil
+	if gotConflict {
+		return ctrl.Result{Requeue: true}, nil
+	} else {
+		return ctrl.Result{}, nil
+	}
 }

instrumentor/controllers/instrumentationdevice/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"
 	"github.com/odigos-io/odigos/k8sutils/pkg/workload"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -52,15 +53,10 @@ func (r *InstrumentedApplicationReconciler) Reconcile(ctx context.Context, req c
 			return ctrl.Result{}, err
 		}
 		err = removeInstrumentationDeviceFromWorkload(ctx, r.Client, req.Namespace, workloadKind, workloadName, ApplyInstrumentationDeviceReasonNoRuntimeDetails)
-		if err != nil {
-			logger.Error(err, "error removing instrumentation")
-			return ctrl.Result{}, err
-		}
-
-		return ctrl.Result{}, nil
+		return utils.RetryOnConflict(err)
 	}
 
 	isNodeCollectorReady := isDataCollectionReady(ctx, r.Client)
 	err = reconcileSingleWorkload(ctx, r.Client, &runtimeDetails, isNodeCollectorReady)
-	return ctrl.Result{}, err
+	return utils.RetryOnConflict(err)
 }

instrumentor/controllers/instrumentationdevice/manager.go

@@ -28,7 +28,12 @@ type workloadPodTemplatePredicate struct {
 }
 
 func (w workloadPodTemplatePredicate) Create(e event.CreateEvent) bool {
-	return false
+	// when instrumentor restarts, this case will be triggered as workloads objects are being added to the cache.
+	// in this case, we need to reconcile the workload, and guarantee that the device is injected or removed
+	// based on the current state of the cluster.
+	// if the instrumented application is deleted but the device is not cleaned,
+	// the instrumented application controller will not be invoked after restart, which is why we need to handle this case here.
+	return true
 }
 
 func (w workloadPodTemplatePredicate) Update(e event.UpdateEvent) bool {

instrumentor/controllers/instrumentationdevice/workload_controllers.go

@@ -4,7 +4,9 @@ import (
 	"context"
 
 	odigosv1 "github.com/odigos-io/odigos/api/odigos/v1alpha1"
+	"github.com/odigos-io/odigos/k8sutils/pkg/utils"
 	"github.com/odigos-io/odigos/k8sutils/pkg/workload"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -17,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 ctrl.Result{}, err
+	return utils.RetryOnConflict(err)
 }
 
 type DaemonSetReconciler struct {
@@ -27,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 ctrl.Result{}, err
+	return utils.RetryOnConflict(err)
 }
 
 type StatefulSetReconciler struct {
@@ -37,16 +39,26 @@ 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 ctrl.Result{}, err
+	return utils.RetryOnConflict(err)
 }
 
 func reconcileSingleInstrumentedApplicationByName(ctx context.Context, k8sClient client.Client, instrumentedAppName string, namespace string) error {
 	var instrumentedApplication odigosv1.InstrumentedApplication
 	err := k8sClient.Get(ctx, types.NamespacedName{Name: instrumentedAppName, Namespace: namespace}, &instrumentedApplication)
 	if err != nil {
-		// changes in workload when there is no instrumented application is not interesting
-		return client.IgnoreNotFound(err)
+		if apierrors.IsNotFound(err) {
+			// if there is no instrumented application, make sure the device is removed from the workload pod template manifest
+			workloadName, workloadKind, err := workload.ExtractWorkloadInfoFromRuntimeObjectName(instrumentedAppName)
+			if err != nil {
+				return err
+			}
+			err = removeInstrumentationDeviceFromWorkload(ctx, k8sClient, namespace, workloadKind, workloadName, ApplyInstrumentationDeviceReasonNoRuntimeDetails)
+			return err
+		} else {
+			return err
+		}
 	}
 	isNodeCollectorReady := isDataCollectionReady(ctx, k8sClient)
+
 	return reconcileSingleWorkload(ctx, k8sClient, &instrumentedApplication, isNodeCollectorReady)
 }

instrumentor/instrumentation/instrumentation.go

@@ -81,12 +81,13 @@ func ApplyInstrumentationDevicesToPodTemplate(original *corev1.PodTemplateSpec,
 // this function restores a workload manifest env vars to their original values.
 // it is used when the instrumentation is removed from the workload.
 // the original values are read from the annotation which was saved when the instrumentation was applied.
-func RevertEnvOverwrites(obj client.Object, podSpec *corev1.PodTemplateSpec) error {
+func RevertEnvOverwrites(obj client.Object, podSpec *corev1.PodTemplateSpec) (bool, error) {
 	manifestEnvOriginal, err := envoverwrite.NewOrigWorkloadEnvValues(obj.GetAnnotations())
 	if err != nil {
-		return err
+		return false, err
 	}
 
+	changed := false
 	for iContainer, c := range podSpec.Spec.Containers {
 		containerOriginalEnv := manifestEnvOriginal.GetContainerStoredEnvs(c.Name)
 		newContainerEnvs := make([]corev1.EnvVar, 0, len(c.Env))
@@ -102,32 +103,37 @@ func RevertEnvOverwrites(obj client.Object, podSpec *corev1.PodTemplateSpec) err
 					// if the value is nil, the env var was not set by the user to begin with.
 					// we will simply not append it to the new envs to achieve the same effect.
 				}
+				changed = true
 			} else {
 				newContainerEnvs = append(newContainerEnvs, envVar)
 			}
 		}
 		podSpec.Spec.Containers[iContainer].Env = newContainerEnvs
 	}
 
-	manifestEnvOriginal.DeleteFromObj(obj)
+	annotationRemoved := manifestEnvOriginal.DeleteFromObj(obj)
 
-	return nil
+	return changed || annotationRemoved, nil
 }
 
-func RevertInstrumentationDevices(original *corev1.PodTemplateSpec) {
+func RevertInstrumentationDevices(original *corev1.PodTemplateSpec) bool {
+	changed := false
 	for _, container := range original.Spec.Containers {
 		for resourceName := range container.Resources.Limits {
 			if strings.HasPrefix(string(resourceName), common.OdigosResourceNamespace) {
 				delete(container.Resources.Limits, resourceName)
+				changed = true
 			}
 		}
 		// Is it needed?
 		for resourceName := range container.Resources.Requests {
 			if strings.HasPrefix(string(resourceName), common.OdigosResourceNamespace) {
 				delete(container.Resources.Requests, resourceName)
+				changed = true
 			}
 		}
 	}
+	return changed
 }
 
 func getLanguageOfContainer(instrumentation *odigosv1.InstrumentedApplication, containerName string) common.ProgrammingLanguage {
@@ -238,8 +244,12 @@ func SetInjectInstrumentationLabel(original *corev1.PodTemplateSpec) {
 }
 
 // RemoveInjectInstrumentationLabel removes the "odigos.io/inject-instrumentation" label if it exists.
-func RemoveInjectInstrumentationLabel(original *corev1.PodTemplateSpec) {
+func RemoveInjectInstrumentationLabel(original *corev1.PodTemplateSpec) bool {
 	if original.Labels != nil {
-		delete(original.Labels, "odigos.io/inject-instrumentation")
+		if _, ok := original.Labels["odigos.io/inject-instrumentation"]; ok {
+			delete(original.Labels, "odigos.io/inject-instrumentation")
+			return true
+		}
 	}
+	return false
 }

k8sutils/pkg/envoverwrite/origenv.go

@@ -95,11 +95,16 @@ func (o *OrigWorkloadEnvValues) SerializeToAnnotation(obj metav1.Object) error {
 	return nil
 }
 
-func (o *OrigWorkloadEnvValues) DeleteFromObj(obj metav1.Object) {
+func (o *OrigWorkloadEnvValues) DeleteFromObj(obj metav1.Object) bool {
 	currentAnnotations := obj.GetAnnotations()
 	if currentAnnotations == nil {
-		return
+		return false
+	}
+
+	if _, ok := currentAnnotations[consts.ManifestEnvOriginalValAnnotation]; !ok {
+		return false
 	}
 
 	delete(currentAnnotations, consts.ManifestEnvOriginalValAnnotation)
+	return true
 }