Don't delete explicitly instrumented sources when NS is uninstrumented

odigos-io · Jan 2, 2025 · c228058 · c228058
1 parent 57b61c5
commit c228058
Show file tree

Hide file tree

Showing 10 changed files with 346 additions and 24 deletions.
diff --git a/instrumentor/controllers/deleteinstrumentedapplication/common.go b/instrumentor/controllers/deleteinstrumentedapplication/common.go
@@ -27,7 +27,7 @@ func reconcileWorkloadObject(ctx context.Context, kubeClient client.Client, work
 		if err != nil {
 			return err
 		}
-		if len(sourceList.Items) == 0 {
+		if len(sourceList.Items) > 0 {
 			return nil
 		}
 	}

diff --git a/instrumentor/controllers/deleteinstrumentedapplication/namespace_controller.go b/instrumentor/controllers/deleteinstrumentedapplication/namespace_controller.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 
+	"github.com/odigos-io/odigos/api/odigos/v1alpha1"
 	"github.com/odigos-io/odigos/common/consts"
 	"github.com/odigos-io/odigos/k8sutils/pkg/workload"
 
@@ -150,11 +151,14 @@ func syncGenericWorkloadListToNs(ctx context.Context, c client.Client, kind work
 		}
 	}
 
-	if !isInheritingInstrumentationFromNs(freshWorkloadCopy) {
+	inheriting, err := isInheritingInstrumentationFromNs(ctx, c, freshWorkloadCopy)
+	if err != nil {
+		return err
+	}
+	if !inheriting {
 		return nil
 	}
 
-	var err error
 	err = errors.Join(err, deleteWorkloadInstrumentedApplication(ctx, c, freshWorkloadCopy))
 	err = errors.Join(err, removeReportedNameAnnotation(ctx, c, freshWorkloadCopy))
 	return err
@@ -164,11 +168,23 @@ func syncGenericWorkloadListToNs(ctx context.Context, c client.Client, kind work
 // when reconciling the namespace, the usecase is to delete instrumentation for workloads that were only
 // instrumented due to the label on the namespace. These are workloads with the label missing.
 // (they inherit the instrumentation from the namespace this way)
-func isInheritingInstrumentationFromNs(obj client.Object) bool {
+func isInheritingInstrumentationFromNs(ctx context.Context, c client.Client, obj client.Object) (bool, error) {
+	sourceList, err := v1alpha1.GetSourceListForWorkload(ctx, c, obj)
+	if err != nil {
+		return false, err
+	}
+	for _, source := range sourceList.Items {
+		// if a source exists for this workload specifically, then it is not inheriting from Namespace
+		if source.Spec.Workload.Name == obj.GetName() &&
+			source.Spec.Workload.Kind == workload.WorkloadKind(obj.GetObjectKind().GroupVersionKind().Kind) {
+			return false, nil
+		}
+	}
+
 	labels := obj.GetLabels()
 	if labels == nil {
-		return true
+		return true, nil
 	}
 	_, exists := labels[consts.OdigosInstrumentationLabel]
-	return !exists
+	return !exists, nil
 }
diff --git a/instrumentor/controllers/startlangdetection/source_controller.go b/instrumentor/controllers/startlangdetection/source_controller.go
@@ -47,6 +47,22 @@ func (r *SourceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 			return k8sutils.K8SUpdateErrorHandler(err)
 		}
 
+		// pre-process existing Sources for specific workloads so we don't have to make a bunch of API calls
+		// This is used to check if a workload already has an explicit Source, so we don't overwrite its InstrumentationConfig
+		sourceList := v1alpha1.SourceList{}
+		err := r.Client.List(ctx, &sourceList, client.InNamespace(source.Spec.Workload.Name))
+		if err != nil {
+			return ctrl.Result{}, err
+		}
+		namespaceKindSources := make(map[workload.WorkloadKind]map[string]struct{})
+		for _, source := range sourceList.Items {
+			if _, exists := namespaceKindSources[source.Spec.Workload.Kind]; !exists {
+				namespaceKindSources[source.Spec.Workload.Kind] = make(map[string]struct{})
+			}
+			// ex: map["Deployment"]["my-app"] = ...
+			namespaceKindSources[source.Spec.Workload.Kind][source.Spec.Workload.Name] = struct{}{}
+		}
+
 		if source.Spec.Workload.Kind == "Namespace" {
 			var deps appsv1.DeploymentList
 			err = r.Client.List(ctx, &deps, client.InNamespace(source.Spec.Workload.Name))
@@ -56,10 +72,12 @@ func (r *SourceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 			}
 
 			for _, dep := range deps.Items {
-				request := ctrl.Request{NamespacedName: client.ObjectKey{Name: dep.Name, Namespace: dep.Namespace}}
-				_, err = reconcileWorkload(ctx, r.Client, workload.WorkloadKindDeployment, request, r.Scheme)
-				if err != nil {
-					logger.Error(err, "error requesting runtime details from odiglets", "name", dep.Name, "namespace", dep.Namespace)
+				if _, exists := namespaceKindSources[workload.WorkloadKindDeployment][dep.Name]; !exists {
+					request := ctrl.Request{NamespacedName: client.ObjectKey{Name: dep.Name, Namespace: dep.Namespace}}
+					_, err = reconcileWorkload(ctx, r.Client, workload.WorkloadKindDeployment, request, r.Scheme)
+					if err != nil {
+						logger.Error(err, "error requesting runtime details from odiglets", "name", dep.Name, "namespace", dep.Namespace)
+					}
 				}
 			}
 
@@ -71,10 +89,12 @@ func (r *SourceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 			}
 
 			for _, st := range sts.Items {
-				request := ctrl.Request{NamespacedName: client.ObjectKey{Name: st.Name, Namespace: st.Namespace}}
-				_, err = reconcileWorkload(ctx, r.Client, workload.WorkloadKindStatefulSet, request, r.Scheme)
-				if err != nil {
-					logger.Error(err, "error requesting runtime details from odiglets", "name", st.Name, "namespace", st.Namespace)
+				if _, exists := namespaceKindSources[workload.WorkloadKindStatefulSet][st.Name]; !exists {
+					request := ctrl.Request{NamespacedName: client.ObjectKey{Name: st.Name, Namespace: st.Namespace}}
+					_, err = reconcileWorkload(ctx, r.Client, workload.WorkloadKindStatefulSet, request, r.Scheme)
+					if err != nil {
+						logger.Error(err, "error requesting runtime details from odiglets", "name", st.Name, "namespace", st.Namespace)
+					}
 				}
 			}
 
@@ -86,10 +106,12 @@ func (r *SourceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 			}
 
 			for _, ds := range dss.Items {
-				request := ctrl.Request{NamespacedName: client.ObjectKey{Name: ds.Name, Namespace: ds.Namespace}}
-				_, err = reconcileWorkload(ctx, r.Client, workload.WorkloadKindDaemonSet, request, r.Scheme)
-				if err != nil {
-					logger.Error(err, "error requesting runtime details from odiglets", "name", ds.Name, "namespace", ds.Namespace)
+				if _, exists := namespaceKindSources[workload.WorkloadKindDaemonSet][ds.Name]; !exists {
+					request := ctrl.Request{NamespacedName: client.ObjectKey{Name: ds.Name, Namespace: ds.Namespace}}
+					_, err = reconcileWorkload(ctx, r.Client, workload.WorkloadKindDaemonSet, request, r.Scheme)
+					if err != nil {
+						logger.Error(err, "error requesting runtime details from odiglets", "name", ds.Name, "namespace", ds.Namespace)
+					}
 				}
 			}
 		} else {

diff --git a/instrumentor/controllers/startlangdetection/workload_controllers.go b/instrumentor/controllers/startlangdetection/workload_controllers.go
@@ -10,7 +10,6 @@ import (
 
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
-	"github.com/odigos-io/odigos/api/odigos/v1alpha1"
 	odigosv1 "github.com/odigos-io/odigos/api/odigos/v1alpha1"
 	"github.com/odigos-io/odigos/common/consts"
 	"github.com/odigos-io/odigos/k8sutils/pkg/workload"
@@ -62,7 +61,7 @@ func reconcileWorkload(ctx context.Context, k8sClient client.Client, objKind wor
 
 	if !instrumented {
 		// Check if a Source object exists for this workload
-		sourceList, err := v1alpha1.GetSourceListForWorkload(ctx, k8sClient, obj)
+		sourceList, err := odigosv1.GetSourceListForWorkload(ctx, k8sClient, obj)
 		if err != nil {
 			return ctrl.Result{}, err
 		}

diff --git a/tests/e2e/source/05-assert-runtime-detected.yaml b/tests/e2e/source/05-assert-runtime-detected.yaml
@@ -0,0 +1,32 @@
+apiVersion: odigos.io/v1alpha1
+kind: InstrumentedApplication
+metadata:
+  name: deployment-frontend
+  namespace: default
+  ownerReferences:
+    - apiVersion: apps/v1
+      blockOwnerDeletion: true
+      controller: true
+      kind: Deployment
+      name: frontend
+spec:
+  runtimeDetails:
+    - containerName: frontend
+      language: java
+---
+apiVersion: odigos.io/v1alpha1
+kind: InstrumentationConfig
+metadata:
+  name: deployment-frontend
+  namespace: default
+  ownerReferences:
+    - apiVersion: apps/v1
+      blockOwnerDeletion: true
+      controller: true
+      kind: Deployment
+      name: frontend
+status:
+  runtimeDetailsByContainer:
+    - containerName: frontend
+      language: java
+      runtimeVersion: 17.0.11+9
diff --git a/tests/e2e/source/05-source.yaml b/tests/e2e/source/05-source.yaml
@@ -0,0 +1,12 @@
+apiVersion: odigos.io/v1alpha1
+kind: Source
+metadata:
+  name: frontend
+  namespace: default
+  labels:
+    odigos.io/e2e: source-single
+spec:
+  workload:
+    name: frontend
+    namespace: default
+    kind: Deployment
diff --git a/tests/e2e/source/05-workloads.yaml b/tests/e2e/source/05-workloads.yaml
@@ -0,0 +1,44 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    deployment.kubernetes.io/revision: "5"
+  generation: 5
+  name: coupon
+  namespace: default
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    deployment.kubernetes.io/revision: "6" # should be only one updated right now
+  generation: 6
+  name: frontend
+  namespace: default
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    deployment.kubernetes.io/revision: "5"
+  generation: 5
+  name: inventory
+  namespace: default
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    deployment.kubernetes.io/revision: "5"
+  generation: 5
+  name: membership
+  namespace: default
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    deployment.kubernetes.io/revision: "5"
+  generation: 5
+  name: pricing
+  namespace: default
diff --git a/tests/e2e/source/06-workloads.yaml b/tests/e2e/source/06-workloads.yaml
@@ -0,0 +1,44 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    deployment.kubernetes.io/revision: "6"
+  generation: 6
+  name: coupon
+  namespace: default
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    deployment.kubernetes.io/revision: "6"
+  generation: 6
+  name: frontend
+  namespace: default
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    deployment.kubernetes.io/revision: "6"
+  generation: 6
+  name: inventory
+  namespace: default
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    deployment.kubernetes.io/revision: "6"
+  generation: 6
+  name: membership
+  namespace: default
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    deployment.kubernetes.io/revision: "6"
+  generation: 6
+  name: pricing
+  namespace: default
diff --git a/tests/e2e/source/07-workloads.yaml b/tests/e2e/source/07-workloads.yaml
@@ -0,0 +1,44 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    deployment.kubernetes.io/revision: "7"
+  generation: 7
+  name: coupon
+  namespace: default
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    deployment.kubernetes.io/revision: "6"
+  generation: 6
+  name: frontend
+  namespace: default
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    deployment.kubernetes.io/revision: "7"
+  generation: 7
+  name: inventory
+  namespace: default
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    deployment.kubernetes.io/revision: "7"
+  generation: 7
+  name: membership
+  namespace: default
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    deployment.kubernetes.io/revision: "7"
+  generation: 7
+  name: pricing
+  namespace: default