From 40959c060f890c34971ab003da7df135d03f0730 Mon Sep 17 00:00:00 2001 From: Ron Federman <73110295+RonFed@users.noreply.github.com> Date: Sun, 5 Jan 2025 09:51:00 +0200 Subject: [PATCH] Remove instrumentedApplication from instrumentor/instrumentationconfig (#2126) * When updating the `instrumentationConfig` spec (e.g. `SdkConfig`s) rely on the runtime detauls written in the status of the `instrumentationConfig`. * Create a common predicate to the instrumentor controller which passes events where the runtime details in the `instrumentationConfig` have been updated - currently only checking the length of the slice and this will be improved in the future. --- .../instrumentationconfig/common.go | 10 +- .../instrumentationconfig/common_test.go | 159 +++++------------- ...go => instrumentationconfig_controller.go} | 29 +--- .../instrumentationrule_controller.go | 37 ++-- .../instrumentationconfig/manager.go | 9 +- .../instrumentationconfig_controller.go | 49 ------ .../instrumentationdevice/manager.go | 11 +- .../instrumentation_rule.go} | 3 +- .../predicates/runtime_details_changed.go | 62 +++++++ 9 files changed, 143 insertions(+), 226 deletions(-) rename instrumentor/controllers/instrumentationconfig/{instrumentedapplication_controller.go => instrumentationconfig_controller.go} (65%) rename instrumentor/controllers/utils/{predicate.go => predicates/instrumentation_rule.go} (98%) create mode 100644 instrumentor/controllers/utils/predicates/runtime_details_changed.go diff --git a/instrumentor/controllers/instrumentationconfig/common.go b/instrumentor/controllers/instrumentationconfig/common.go index 22bed59d65..5a84a277ed 100644 --- a/instrumentor/controllers/instrumentationconfig/common.go +++ b/instrumentor/controllers/instrumentationconfig/common.go @@ -12,24 +12,24 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -func updateInstrumentationConfigForWorkload(ic *odigosv1alpha1.InstrumentationConfig, ia *odigosv1alpha1.InstrumentedApplication, rules *odigosv1alpha1.InstrumentationRuleList, serviceName string) error { +func updateInstrumentationConfigForWorkload(ic *odigosv1alpha1.InstrumentationConfig, rules *odigosv1alpha1.InstrumentationRuleList, serviceName string) error { - workloadName, workloadKind, err := workload.ExtractWorkloadInfoFromRuntimeObjectName(ia.Name) + workloadName, workloadKind, err := workload.ExtractWorkloadInfoFromRuntimeObjectName(ic.Name) if err != nil { return err } workload := workload.PodWorkload{ Name: workloadName, - Namespace: ia.Namespace, + Namespace: ic.Namespace, Kind: workloadKind, } ic.Spec.ServiceName = serviceName - sdkConfigs := make([]odigosv1alpha1.SdkConfig, 0, len(ia.Spec.RuntimeDetails)) + sdkConfigs := make([]odigosv1alpha1.SdkConfig, 0, len(ic.Status.RuntimeDetailsByContainer)) // create an empty sdk config for each detected programming language - for _, container := range ia.Spec.RuntimeDetails { + for _, container := range ic.Status.RuntimeDetailsByContainer { containerLanguage := container.Language if containerLanguage == common.IgnoredProgrammingLanguage || containerLanguage == common.UnknownProgrammingLanguage { continue diff --git a/instrumentor/controllers/instrumentationconfig/common_test.go b/instrumentor/controllers/instrumentationconfig/common_test.go index c5bede439d..1b4c591cbb 100644 --- a/instrumentor/controllers/instrumentationconfig/common_test.go +++ b/instrumentor/controllers/instrumentationconfig/common_test.go @@ -11,28 +11,23 @@ import ( ) func TestUpdateInstrumentationConfigForWorkload_SingleLanguage(t *testing.T) { - ic := odigosv1.InstrumentationConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "deployment-test", Namespace: "testns", }, Spec: odigosv1.InstrumentationConfigSpec{}, - } - ia := odigosv1.InstrumentedApplication{ - ObjectMeta: metav1.ObjectMeta{ - Name: "deployment-test", - Namespace: "testns", - }, - Spec: odigosv1.InstrumentedApplicationSpec{ - RuntimeDetails: []odigosv1.RuntimeDetailsByContainer{{ - ContainerName: "test-container", - Language: common.JavascriptProgrammingLanguage, - }}, + Status: odigosv1.InstrumentationConfigStatus{ + RuntimeDetailsByContainer: []odigosv1.RuntimeDetailsByContainer{ + { + ContainerName: "test-container", + Language: common.JavascriptProgrammingLanguage, + }, + }, }, } rules := &odigosv1.InstrumentationRuleList{} - err := updateInstrumentationConfigForWorkload(&ic, &ia, rules, "service-name") + err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name") if err != nil { t.Errorf("Expected nil error, got %v", err) } @@ -45,21 +40,14 @@ func TestUpdateInstrumentationConfigForWorkload_SingleLanguage(t *testing.T) { } func TestUpdateInstrumentationConfigForWorkload_MultipleLanguages(t *testing.T) { - ic := odigosv1.InstrumentationConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "deployment-test", Namespace: "testns", }, Spec: odigosv1.InstrumentationConfigSpec{}, - } - ia := odigosv1.InstrumentedApplication{ - ObjectMeta: metav1.ObjectMeta{ - Name: "deployment-test", - Namespace: "testns", - }, - Spec: odigosv1.InstrumentedApplicationSpec{ - RuntimeDetails: []odigosv1.RuntimeDetailsByContainer{ + Status: odigosv1.InstrumentationConfigStatus{ + RuntimeDetailsByContainer: []odigosv1.RuntimeDetailsByContainer{ { ContainerName: "test-container-1", Language: common.JavascriptProgrammingLanguage, @@ -72,7 +60,7 @@ func TestUpdateInstrumentationConfigForWorkload_MultipleLanguages(t *testing.T) }, } rules := &odigosv1.InstrumentationRuleList{} - err := updateInstrumentationConfigForWorkload(&ic, &ia, rules, "service-name") + err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name") if err != nil { t.Errorf("Expected nil error, got %v", err) } @@ -95,14 +83,8 @@ func TestUpdateInstrumentationConfigForWorkload_IgnoreUnknownLanguage(t *testing Namespace: "testns", }, Spec: odigosv1.InstrumentationConfigSpec{}, - } - ia := odigosv1.InstrumentedApplication{ - ObjectMeta: metav1.ObjectMeta{ - Name: "deployment-test", - Namespace: "testns", - }, - Spec: odigosv1.InstrumentedApplicationSpec{ - RuntimeDetails: []odigosv1.RuntimeDetailsByContainer{ + Status: odigosv1.InstrumentationConfigStatus{ + RuntimeDetailsByContainer: []odigosv1.RuntimeDetailsByContainer{ { ContainerName: "test-container-1", Language: common.JavascriptProgrammingLanguage, @@ -119,7 +101,7 @@ func TestUpdateInstrumentationConfigForWorkload_IgnoreUnknownLanguage(t *testing }, } rules := &odigosv1.InstrumentationRuleList{} - err := updateInstrumentationConfigForWorkload(&ic, &ia, rules, "service-name") + err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name") if err != nil { t.Errorf("Expected nil error, got %v", err) } @@ -139,18 +121,12 @@ func TestUpdateInstrumentationConfigForWorkload_NoLanguages(t *testing.T) { Namespace: "testns", }, Spec: odigosv1.InstrumentationConfigSpec{}, - } - ia := odigosv1.InstrumentedApplication{ - ObjectMeta: metav1.ObjectMeta{ - Name: "deployment-test", - Namespace: "testns", - }, - Spec: odigosv1.InstrumentedApplicationSpec{ - RuntimeDetails: []odigosv1.RuntimeDetailsByContainer{}, + Status: odigosv1.InstrumentationConfigStatus{ + RuntimeDetailsByContainer: []odigosv1.RuntimeDetailsByContainer{}, }, } rules := &odigosv1.InstrumentationRuleList{} - err := updateInstrumentationConfigForWorkload(&ic, &ia, rules, "service-name") + err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name") if err != nil { t.Errorf("Expected nil error, got %v", err) } @@ -160,21 +136,14 @@ func TestUpdateInstrumentationConfigForWorkload_NoLanguages(t *testing.T) { } func TestUpdateInstrumentationConfigForWorkload_SameLanguageMultipleContainers(t *testing.T) { - ic := odigosv1.InstrumentationConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "deployment-test", Namespace: "testns", }, Spec: odigosv1.InstrumentationConfigSpec{}, - } - ia := odigosv1.InstrumentedApplication{ - ObjectMeta: metav1.ObjectMeta{ - Name: "deployment-test", - Namespace: "testns", - }, - Spec: odigosv1.InstrumentedApplicationSpec{ - RuntimeDetails: []odigosv1.RuntimeDetailsByContainer{ + Status: odigosv1.InstrumentationConfigStatus{ + RuntimeDetailsByContainer: []odigosv1.RuntimeDetailsByContainer{ { ContainerName: "test-container-1", Language: common.JavascriptProgrammingLanguage, @@ -187,7 +156,7 @@ func TestUpdateInstrumentationConfigForWorkload_SameLanguageMultipleContainers(t }, } rules := &odigosv1.InstrumentationRuleList{} - err := updateInstrumentationConfigForWorkload(&ic, &ia, rules, "service-name") + err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name") if err != nil { t.Errorf("Expected nil error, got %v", err) } @@ -200,21 +169,14 @@ func TestUpdateInstrumentationConfigForWorkload_SameLanguageMultipleContainers(t } func TestUpdateInstrumentationConfigForWorkload_SingleMatchingRule(t *testing.T) { - ic := odigosv1.InstrumentationConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "deployment-test", Namespace: "testns", }, Spec: odigosv1.InstrumentationConfigSpec{}, - } - ia := odigosv1.InstrumentedApplication{ - ObjectMeta: metav1.ObjectMeta{ - Name: "deployment-test", - Namespace: "testns", - }, - Spec: odigosv1.InstrumentedApplicationSpec{ - RuntimeDetails: []odigosv1.RuntimeDetailsByContainer{ + Status: odigosv1.InstrumentationConfigStatus{ + RuntimeDetailsByContainer: []odigosv1.RuntimeDetailsByContainer{ { ContainerName: "test-container", Language: common.JavascriptProgrammingLanguage, @@ -237,7 +199,7 @@ func TestUpdateInstrumentationConfigForWorkload_SingleMatchingRule(t *testing.T) }, }, } - err := updateInstrumentationConfigForWorkload(&ic, &ia, rules, "service-name") + err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name") if err != nil { t.Errorf("Expected nil error, got %v", err) } @@ -266,14 +228,8 @@ func TestUpdateInstrumentationConfigForWorkload_InWorkloadList(t *testing.T) { Namespace: "testns", }, Spec: odigosv1.InstrumentationConfigSpec{}, - } - ia := odigosv1.InstrumentedApplication{ - ObjectMeta: metav1.ObjectMeta{ - Name: "deployment-test", - Namespace: "testns", - }, - Spec: odigosv1.InstrumentedApplicationSpec{ - RuntimeDetails: []odigosv1.RuntimeDetailsByContainer{ + Status: odigosv1.InstrumentationConfigStatus{ + RuntimeDetailsByContainer: []odigosv1.RuntimeDetailsByContainer{ { ContainerName: "test-container", Language: common.JavascriptProgrammingLanguage, @@ -303,7 +259,7 @@ func TestUpdateInstrumentationConfigForWorkload_InWorkloadList(t *testing.T) { }, } - err := updateInstrumentationConfigForWorkload(&ic, &ia, rules, "service-name") + err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name") if err != nil { t.Errorf("Expected nil error, got %v", err) } @@ -316,21 +272,14 @@ func TestUpdateInstrumentationConfigForWorkload_InWorkloadList(t *testing.T) { } func TestUpdateInstrumentationConfigForWorkload_NotInWorkloadList(t *testing.T) { - ic := odigosv1.InstrumentationConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "deployment-test", Namespace: "testns", }, Spec: odigosv1.InstrumentationConfigSpec{}, - } - ia := odigosv1.InstrumentedApplication{ - ObjectMeta: metav1.ObjectMeta{ - Name: "deployment-test", - Namespace: "testns", - }, - Spec: odigosv1.InstrumentedApplicationSpec{ - RuntimeDetails: []odigosv1.RuntimeDetailsByContainer{ + Status: odigosv1.InstrumentationConfigStatus{ + RuntimeDetailsByContainer: []odigosv1.RuntimeDetailsByContainer{ { ContainerName: "test-container", Language: common.JavascriptProgrammingLanguage, @@ -360,7 +309,7 @@ func TestUpdateInstrumentationConfigForWorkload_NotInWorkloadList(t *testing.T) }, } - err := updateInstrumentationConfigForWorkload(&ic, &ia, rules, "service-name") + err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name") if err != nil { t.Errorf("Expected nil error, got %v", err) } @@ -374,21 +323,14 @@ func TestUpdateInstrumentationConfigForWorkload_NotInWorkloadList(t *testing.T) } func TestUpdateInstrumentationConfigForWorkload_DisabledRule(t *testing.T) { - ic := odigosv1.InstrumentationConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "deployment-test", Namespace: "testns", }, Spec: odigosv1.InstrumentationConfigSpec{}, - } - ia := odigosv1.InstrumentedApplication{ - ObjectMeta: metav1.ObjectMeta{ - Name: "deployment-test", - Namespace: "testns", - }, - Spec: odigosv1.InstrumentedApplicationSpec{ - RuntimeDetails: []odigosv1.RuntimeDetailsByContainer{ + Status: odigosv1.InstrumentationConfigStatus{ + RuntimeDetailsByContainer: []odigosv1.RuntimeDetailsByContainer{ { ContainerName: "test-container", Language: common.JavascriptProgrammingLanguage, @@ -412,7 +354,7 @@ func TestUpdateInstrumentationConfigForWorkload_DisabledRule(t *testing.T) { }, } - err := updateInstrumentationConfigForWorkload(&ic, &ia, rules, "service-name") + err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name") if err != nil { t.Errorf("Expected nil error, got %v", err) } @@ -426,21 +368,14 @@ func TestUpdateInstrumentationConfigForWorkload_DisabledRule(t *testing.T) { } func TestUpdateInstrumentationConfigForWorkload_MultipleDefaultRules(t *testing.T) { - ic := odigosv1.InstrumentationConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "deployment-test", Namespace: "testns", }, Spec: odigosv1.InstrumentationConfigSpec{}, - } - ia := odigosv1.InstrumentedApplication{ - ObjectMeta: metav1.ObjectMeta{ - Name: "deployment-test", - Namespace: "testns", - }, - Spec: odigosv1.InstrumentedApplicationSpec{ - RuntimeDetails: []odigosv1.RuntimeDetailsByContainer{ + Status: odigosv1.InstrumentationConfigStatus{ + RuntimeDetailsByContainer: []odigosv1.RuntimeDetailsByContainer{ { ContainerName: "test-container", Language: common.JavascriptProgrammingLanguage, @@ -476,7 +411,7 @@ func TestUpdateInstrumentationConfigForWorkload_MultipleDefaultRules(t *testing. }, } - err := updateInstrumentationConfigForWorkload(&ic, &ia, rules, "service-name") + err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name") if err != nil { t.Errorf("Expected nil error, got %v", err) } @@ -531,14 +466,8 @@ func TestUpdateInstrumentationConfigForWorkload_RuleForLibrary(t *testing.T) { Namespace: "testns", }, Spec: odigosv1.InstrumentationConfigSpec{}, - } - ia := odigosv1.InstrumentedApplication{ - ObjectMeta: metav1.ObjectMeta{ - Name: "deployment-test", - Namespace: "testns", - }, - Spec: odigosv1.InstrumentedApplicationSpec{ - RuntimeDetails: []odigosv1.RuntimeDetailsByContainer{ + Status: odigosv1.InstrumentationConfigStatus{ + RuntimeDetailsByContainer: []odigosv1.RuntimeDetailsByContainer{ { ContainerName: "test-container", Language: common.JavascriptProgrammingLanguage, @@ -567,7 +496,7 @@ func TestUpdateInstrumentationConfigForWorkload_RuleForLibrary(t *testing.T) { }, } - err := updateInstrumentationConfigForWorkload(&ic, &ia, rules, "service-name") + err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name") if err != nil { t.Errorf("Expected nil error, got %v", err) } @@ -593,14 +522,8 @@ func TestUpdateInstrumentationConfigForWorkload_LibraryRuleOtherLanguage(t *test Namespace: "testns", }, Spec: odigosv1.InstrumentationConfigSpec{}, - } - ia := odigosv1.InstrumentedApplication{ - ObjectMeta: metav1.ObjectMeta{ - Name: "deployment-test", - Namespace: "testns", - }, - Spec: odigosv1.InstrumentedApplicationSpec{ - RuntimeDetails: []odigosv1.RuntimeDetailsByContainer{ + Status: odigosv1.InstrumentationConfigStatus{ + RuntimeDetailsByContainer: []odigosv1.RuntimeDetailsByContainer{ { ContainerName: "test-container", Language: common.JavascriptProgrammingLanguage, @@ -630,7 +553,7 @@ func TestUpdateInstrumentationConfigForWorkload_LibraryRuleOtherLanguage(t *test }, } - err := updateInstrumentationConfigForWorkload(&ic, &ia, rules, "service-name") + err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name") if err != nil { t.Errorf("Expected nil error, got %v", err) } diff --git a/instrumentor/controllers/instrumentationconfig/instrumentedapplication_controller.go b/instrumentor/controllers/instrumentationconfig/instrumentationconfig_controller.go similarity index 65% rename from instrumentor/controllers/instrumentationconfig/instrumentedapplication_controller.go rename to instrumentor/controllers/instrumentationconfig/instrumentationconfig_controller.go index 0090077059..6f6e063fb7 100644 --- a/instrumentor/controllers/instrumentationconfig/instrumentedapplication_controller.go +++ b/instrumentor/controllers/instrumentationconfig/instrumentationconfig_controller.go @@ -22,45 +22,32 @@ import ( 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" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" ) -type InstrumentedApplicationReconciler struct { +type InstrumentationConfigReconciler struct { client.Client Scheme *runtime.Scheme } -func (r *InstrumentedApplicationReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *InstrumentationConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { logger := log.FromContext(ctx) - var ia odigosv1.InstrumentedApplication - err := r.Client.Get(ctx, req.NamespacedName, &ia) - if err != nil { - return ctrl.Result{}, client.IgnoreNotFound(err) - } - var ic odigosv1.InstrumentationConfig - err = r.Client.Get(ctx, req.NamespacedName, &ic) + err := r.Client.Get(ctx, req.NamespacedName, &ic) if err != nil { - // each InstrumentedApplication should have a corresponding InstrumentationConfig - // but it might rarely happen that the InstrumentationConfig is deleted before the InstrumentedApplication - if apierrors.IsNotFound(err) { - logger.V(0).Info("Ignoring InstrumentedApplication without InstrumentationConfig", "runtime object name", ia.Name) - return ctrl.Result{}, nil - } - return ctrl.Result{}, err + return ctrl.Result{}, client.IgnoreNotFound(err) } - workloadName, workloadKind, err := workload.ExtractWorkloadInfoFromRuntimeObjectName(ia.Name) + workloadName, workloadKind, err := workload.ExtractWorkloadInfoFromRuntimeObjectName(ic.Name) if err != nil { return ctrl.Result{}, err } - serviceName, err := resolveServiceName(ctx, r.Client, workloadName, ia.Namespace, workloadKind) + serviceName, err := resolveServiceName(ctx, r.Client, workloadName, ic.Namespace, workloadKind) if err != nil { logger.Error(err, "Failed to resolve service name", "workload", workloadName, "kind", workloadKind) return ctrl.Result{}, err @@ -72,14 +59,14 @@ func (r *InstrumentedApplicationReconciler) Reconcile(ctx context.Context, req c return ctrl.Result{}, err } - err = updateInstrumentationConfigForWorkload(&ic, &ia, instrumentationRules, serviceName) + err = updateInstrumentationConfigForWorkload(&ic, instrumentationRules, serviceName) if err != nil { return ctrl.Result{}, err } err = r.Client.Update(ctx, &ic) if err == nil { - logger.V(0).Info("Updated instrumentation config", "workload", ia.Name) + logger.V(0).Info("Updated instrumentation config", "workload", ic.Name) } return utils.K8SUpdateErrorHandler(err) } diff --git a/instrumentor/controllers/instrumentationconfig/instrumentationrule_controller.go b/instrumentor/controllers/instrumentationconfig/instrumentationrule_controller.go index c2b0a7a473..32ec151dbb 100644 --- a/instrumentor/controllers/instrumentationconfig/instrumentationrule_controller.go +++ b/instrumentor/controllers/instrumentationconfig/instrumentationrule_controller.go @@ -5,7 +5,6 @@ import ( odigosv1alpha1 "github.com/odigos-io/odigos/api/odigos/v1alpha1" "github.com/odigos-io/odigos/k8sutils/pkg/workload" - 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" @@ -18,7 +17,6 @@ type InstrumentationRuleReconciler struct { } func (r *InstrumentationRuleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - logger := log.FromContext(ctx) instrumentationRules := &odigosv1alpha1.InstrumentationRuleList{} @@ -27,49 +25,40 @@ func (r *InstrumentationRuleReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, err } - instrumentedApplications := &odigosv1alpha1.InstrumentedApplicationList{} - err = r.Client.List(ctx, instrumentedApplications) + instrumentationConfigs := &odigosv1alpha1.InstrumentationConfigList{} + err = r.Client.List(ctx, instrumentationConfigs) if err != nil { return ctrl.Result{}, err } - for _, ia := range instrumentedApplications.Items { - workloadName, workloadKind, err := workload.ExtractWorkloadInfoFromRuntimeObjectName(ia.Name) + for _, ic := range instrumentationConfigs.Items { + currIc := ic + workloadName, workloadKind, err := workload.ExtractWorkloadInfoFromRuntimeObjectName(ic.Name) if err != nil { return ctrl.Result{}, err } - serviceName, err := resolveServiceName(ctx, r.Client, workloadName, ia.Namespace, workloadKind) + serviceName, err := resolveServiceName(ctx, r.Client, workloadName, ic.Namespace, workloadKind) if err != nil { - logger.Error(err, "error resolving service name", "workload", ia.Name) + logger.Error(err, "error resolving service name", "workload", ic.Name) continue } - ic := &odigosv1alpha1.InstrumentationConfig{} - err = r.Client.Get(ctx, client.ObjectKey{Name: ia.Name, Namespace: ia.Namespace}, ic) - if err != nil { - if apierrors.IsNotFound(err) { - continue - } else { - logger.Error(err, "error fetching instrumentation config", "workload", ia.Name) - return ctrl.Result{}, err - } - } - err = updateInstrumentationConfigForWorkload(ic, &ia, instrumentationRules, serviceName) + err = updateInstrumentationConfigForWorkload(&currIc, instrumentationRules, serviceName) if err != nil { - logger.Error(err, "error updating instrumentation config", "workload", ia.Name) + logger.Error(err, "error updating instrumentation config", "workload", ic.Name) continue } - err = r.Client.Update(ctx, ic) + err = r.Client.Update(ctx, &currIc) if client.IgnoreNotFound(err) != nil { - logger.Error(err, "error updating instrumentation config", "workload", ia.Name) + logger.Error(err, "error updating instrumentation config", "workload", ic.Name) return ctrl.Result{}, err } - logger.V(0).Info("Updated instrumentation config", "workload", ia.Name) + logger.V(0).Info("Updated instrumentation config", "workload", ic.Name) } - logger.V(0).Info("Payload Collection Rules changed, recalculating instrumentation configs", "number of instrumentation rules", len(instrumentationRules.Items), "number of instrumented workloads", len(instrumentedApplications.Items)) + logger.V(0).Info("Payload Collection Rules changed, recalculating instrumentation configs", "number of instrumentation rules", len(instrumentationRules.Items), "number of instrumented workloads", len(instrumentationConfigs.Items)) return ctrl.Result{}, nil } diff --git a/instrumentor/controllers/instrumentationconfig/manager.go b/instrumentor/controllers/instrumentationconfig/manager.go index 1a20b57188..c7e0a3d5e8 100644 --- a/instrumentor/controllers/instrumentationconfig/manager.go +++ b/instrumentor/controllers/instrumentationconfig/manager.go @@ -2,6 +2,7 @@ package instrumentationconfig import ( odigosv1alpha1 "github.com/odigos-io/odigos/api/odigos/v1alpha1" + instrumentorpredicate "github.com/odigos-io/odigos/instrumentor/controllers/utils/predicates" appsv1 "k8s.io/api/apps/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" @@ -21,12 +22,12 @@ func SetupWithManager(mgr ctrl.Manager) error { return err } - // Watch InstrumentedApplication err = builder. ControllerManagedBy(mgr). - Named("instrumentor-instrumentationconfig-instrumentedapplication"). - For(&odigosv1alpha1.InstrumentedApplication{}). - Complete(&InstrumentedApplicationReconciler{ + Named("instrumentor-instrumentationconfig-instrumentationconfig"). + For(&odigosv1alpha1.InstrumentationConfig{}). + WithEventFilter(&instrumentorpredicate.RuntimeDetailsChangedPredicate{}). + Complete(&InstrumentationConfigReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), }) diff --git a/instrumentor/controllers/instrumentationdevice/instrumentationconfig_controller.go b/instrumentor/controllers/instrumentationdevice/instrumentationconfig_controller.go index 724dd63b44..7bebf59493 100644 --- a/instrumentor/controllers/instrumentationdevice/instrumentationconfig_controller.go +++ b/instrumentor/controllers/instrumentationdevice/instrumentationconfig_controller.go @@ -26,9 +26,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/predicate" ) type InstrumentationConfigReconciler struct { @@ -36,53 +34,6 @@ type InstrumentationConfigReconciler struct { Scheme *runtime.Scheme } -type runtimeDetailsChangedPredicate struct{} - -func (o runtimeDetailsChangedPredicate) Create(e event.CreateEvent) bool { - if e.Object == nil { - return false - } - - ic, ok := e.Object.(*odigosv1.InstrumentationConfig) - if !ok { - return false - } - - return len(ic.Status.RuntimeDetailsByContainer) > 0 -} - -func (i runtimeDetailsChangedPredicate) Update(e event.UpdateEvent) bool { - if e.ObjectOld == nil || e.ObjectNew == nil { - return false - } - - oldIc, oldOk := e.ObjectOld.(*odigosv1.InstrumentationConfig) - newIc, newOk := e.ObjectNew.(*odigosv1.InstrumentationConfig) - - if !oldOk || !newOk { - return false - } - - // currently, we only check the lengths of the runtime details - // we should improve this once we support updating the runtime details more than once - if len(oldIc.Status.RuntimeDetailsByContainer) != len(newIc.Status.RuntimeDetailsByContainer) { - return true - } - - return false -} - -func (i runtimeDetailsChangedPredicate) Delete(e event.DeleteEvent) bool { - // when the instrumentation config is deleted we need to clean up the device - return true -} - -func (i runtimeDetailsChangedPredicate) Generic(e event.GenericEvent) bool { - return false -} - -var _ predicate.Predicate = &runtimeDetailsChangedPredicate{} - func (r *InstrumentationConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { logger := log.FromContext(ctx) diff --git a/instrumentor/controllers/instrumentationdevice/manager.go b/instrumentor/controllers/instrumentationdevice/manager.go index 7748820b1a..a8b5599e29 100644 --- a/instrumentor/controllers/instrumentationdevice/manager.go +++ b/instrumentor/controllers/instrumentationdevice/manager.go @@ -3,7 +3,7 @@ package instrumentationdevice import ( odigosv1 "github.com/odigos-io/odigos/api/odigos/v1alpha1" "github.com/odigos-io/odigos/common" - "github.com/odigos-io/odigos/instrumentor/controllers/utils" + instrumentorpredicate "github.com/odigos-io/odigos/instrumentor/controllers/utils/predicates" odigospredicate "github.com/odigos-io/odigos/k8sutils/pkg/predicate" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -102,9 +102,12 @@ func SetupWithManager(mgr ctrl.Manager) error { err = builder. ControllerManagedBy(mgr). - Named("instrumentationdevice-instrumentedapplication"). + Named("instrumentationdevice-instrumentationconfig"). For(&odigosv1.InstrumentationConfig{}). - WithEventFilter(&runtimeDetailsChangedPredicate{}). + // The following events are relevant to the device injection/removal from the instrumentation config: + // 1. When the runtime details are changed + // 2. When the instrumentation config is deleted + WithEventFilter(predicate.Or(&instrumentorpredicate.RuntimeDetailsChangedPredicate{}, &odigospredicate.DeletionPredicate{})). Complete(&InstrumentationConfigReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), @@ -152,7 +155,7 @@ func SetupWithManager(mgr ctrl.Manager) error { ControllerManagedBy(mgr). Named("instrumentationdevice-instrumentationrules"). For(&odigosv1.InstrumentationRule{}). - WithEventFilter(&utils.OtelSdkInstrumentationRulePredicate{}). + WithEventFilter(&instrumentorpredicate.OtelSdkInstrumentationRulePredicate{}). Complete(&InstrumentationRuleReconciler{ Client: mgr.GetClient(), }) diff --git a/instrumentor/controllers/utils/predicate.go b/instrumentor/controllers/utils/predicates/instrumentation_rule.go similarity index 98% rename from instrumentor/controllers/utils/predicate.go rename to instrumentor/controllers/utils/predicates/instrumentation_rule.go index 896c6ab529..b1da0900f9 100644 --- a/instrumentor/controllers/utils/predicate.go +++ b/instrumentor/controllers/utils/predicates/instrumentation_rule.go @@ -1,4 +1,4 @@ -package utils +package predicates import ( odigosv1alpha1 "github.com/odigos-io/odigos/api/odigos/v1alpha1" @@ -7,6 +7,7 @@ import ( type OtelSdkInstrumentationRulePredicate struct{} + func (o OtelSdkInstrumentationRulePredicate) Create(e event.CreateEvent) bool { // check if delete rule is for otel sdk instrumentationRule, ok := e.Object.(*odigosv1alpha1.InstrumentationRule) diff --git a/instrumentor/controllers/utils/predicates/runtime_details_changed.go b/instrumentor/controllers/utils/predicates/runtime_details_changed.go new file mode 100644 index 0000000000..d152d55c7e --- /dev/null +++ b/instrumentor/controllers/utils/predicates/runtime_details_changed.go @@ -0,0 +1,62 @@ +package predicates + +import ( + odigosv1 "github.com/odigos-io/odigos/api/odigos/v1alpha1" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" +) + +// RuntimeDetailsChangedPredicate is a predicate that checks if the runtime details of an InstrumentationConfig have changed. +// +// For Create events, it returns true if the InstrumentationConfig has any runtime details. +// For Update events, it returns true if the runtime details have changed (currently only checks the length of the runtime details). +// For Delete events, it returns false. +// +// TODO: once we support updating the runtime details more than once, we should improve this predicate to check the actual changes. +type RuntimeDetailsChangedPredicate struct{} + +var _ predicate.Predicate = &RuntimeDetailsChangedPredicate{} + +var InstrumentationConfigRuntimeDetailsChangedPredicate = RuntimeDetailsChangedPredicate{} + +func (o RuntimeDetailsChangedPredicate) Create(e event.CreateEvent) bool { + if e.Object == nil { + return false + } + + ic, ok := e.Object.(*odigosv1.InstrumentationConfig) + if !ok { + return false + } + + return len(ic.Status.RuntimeDetailsByContainer) > 0 +} + +func (i RuntimeDetailsChangedPredicate) Update(e event.UpdateEvent) bool { + if e.ObjectOld == nil || e.ObjectNew == nil { + return false + } + + oldIc, oldOk := e.ObjectOld.(*odigosv1.InstrumentationConfig) + newIc, newOk := e.ObjectNew.(*odigosv1.InstrumentationConfig) + + if !oldOk || !newOk { + return false + } + + // currently, we only check the lengths of the runtime details + // we should improve this once we support updating the runtime details more than once + if len(oldIc.Status.RuntimeDetailsByContainer) != len(newIc.Status.RuntimeDetailsByContainer) { + return true + } + + return false +} + +func (i RuntimeDetailsChangedPredicate) Delete(e event.DeleteEvent) bool { + return false +} + +func (i RuntimeDetailsChangedPredicate) Generic(e event.GenericEvent) bool { + return false +}