From 38c8332b3f6d59170cf2de65ab1461bac9f6f742 Mon Sep 17 00:00:00 2001 From: odubajDT <93584209+odubajDT@users.noreply.github.com> Date: Mon, 18 Sep 2023 14:46:51 +0200 Subject: [PATCH] chore(metrics-operator): refactor fetching resouce namespaces during analysis (#2105) Signed-off-by: odubajDT --- metrics-operator/api/v1alpha3/common.go | 12 +++ metrics-operator/api/v1alpha3/common_test.go | 27 ++++++ .../controllers/analysis/controller.go | 11 +-- .../controllers/analysis/controller_test.go | 55 ++++++++++++ .../controllers/analysis/provider_selector.go | 16 ++-- .../analysis/provider_selector_test.go | 90 ++++++++++++++++++- .../common/analysis/objective_evaluator.go | 2 +- metrics-operator/main.go | 1 - .../00-teststep-template.yaml | 6 +- .../analysis-controller/install.yaml | 7 -- .../analysis-controller/mock-server.yaml | 3 - 11 files changed, 192 insertions(+), 38 deletions(-) create mode 100644 metrics-operator/api/v1alpha3/common_test.go diff --git a/metrics-operator/api/v1alpha3/common.go b/metrics-operator/api/v1alpha3/common.go index 149c8f4d3c..fe4a459b51 100644 --- a/metrics-operator/api/v1alpha3/common.go +++ b/metrics-operator/api/v1alpha3/common.go @@ -6,3 +6,15 @@ type ObjectReference struct { // Namespace defines the namespace of the referenced object Namespace string `json:"namespace,omitempty"` } + +func (o *ObjectReference) IsNamespaceSet() bool { + return o.Namespace != "" +} + +func (o *ObjectReference) GetNamespace(defaultNamespace string) string { + if o.IsNamespaceSet() { + return o.Namespace + } + + return defaultNamespace +} diff --git a/metrics-operator/api/v1alpha3/common_test.go b/metrics-operator/api/v1alpha3/common_test.go new file mode 100644 index 0000000000..ef2798a0ca --- /dev/null +++ b/metrics-operator/api/v1alpha3/common_test.go @@ -0,0 +1,27 @@ +package v1alpha3 + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestObjectReference_IsNamespaceSet(t *testing.T) { + o := ObjectReference{} + + require.False(t, o.IsNamespaceSet()) + + o.Namespace = "ns" + + require.True(t, o.IsNamespaceSet()) +} + +func TestObjectReference_GetNamespace(t *testing.T) { + o := ObjectReference{} + + require.Equal(t, "default", o.GetNamespace("default")) + + o.Namespace = "ns" + + require.Equal(t, "ns", o.GetNamespace("default")) +} diff --git a/metrics-operator/controllers/analysis/controller.go b/metrics-operator/controllers/analysis/controller.go index 0b7357db48..703e1a714c 100644 --- a/metrics-operator/controllers/analysis/controller.go +++ b/metrics-operator/controllers/analysis/controller.go @@ -41,7 +41,6 @@ type AnalysisReconciler struct { Scheme *runtime.Scheme Log logr.Logger MaxWorkers int //maybe 2 or 4 as def - Namespace string NewWorkersPoolFactory common.IAnalysisEvaluator } @@ -73,14 +72,12 @@ func (a *AnalysisReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } //find AnalysisDefinition to have the collection of Objectives + analysisDefNamespace := analysis.Spec.AnalysisDefinition.GetNamespace(analysis.Namespace) analysisDef := &metricsapi.AnalysisDefinition{} - if analysis.Spec.AnalysisDefinition.Namespace == "" { - analysis.Spec.AnalysisDefinition.Namespace = a.Namespace - } err := a.Client.Get(ctx, types.NamespacedName{ Name: analysis.Spec.AnalysisDefinition.Name, - Namespace: analysis.Spec.AnalysisDefinition.Namespace}, + Namespace: analysisDefNamespace}, analysisDef, ) @@ -89,7 +86,7 @@ func (a *AnalysisReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c a.Log.Info( fmt.Sprintf("AnalysisDefinition '%s' in namespace '%s' not found, requeue", analysis.Spec.AnalysisDefinition.Name, - analysis.Spec.AnalysisDefinition.Namespace), + analysisDefNamespace), ) return ctrl.Result{Requeue: true, RequeueAfter: 10 * time.Second}, nil } @@ -107,7 +104,7 @@ func (a *AnalysisReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } //create multiple workers handling the Objectives - childCtx, wp := a.NewWorkersPoolFactory(ctx, analysis, todo, a.MaxWorkers, a.Client, a.Log, a.Namespace) + childCtx, wp := a.NewWorkersPoolFactory(ctx, analysis, todo, a.MaxWorkers, a.Client, a.Log, analysisDefNamespace) res, err := wp.DispatchAndCollect(childCtx) if err != nil { diff --git a/metrics-operator/controllers/analysis/controller_test.go b/metrics-operator/controllers/analysis/controller_test.go index be7d072354..3dfc947567 100644 --- a/metrics-operator/controllers/analysis/controller_test.go +++ b/metrics-operator/controllers/analysis/controller_test.go @@ -25,6 +25,54 @@ func TestAnalysisReconciler_Reconcile_BasicControlLoop(t *testing.T) { analysis, analysisDef, template, _ := getTestCRDs() + analysis2 := metricsapi.Analysis{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-analysis", + Namespace: "default", + }, + Spec: metricsapi.AnalysisSpec{ + Timeframe: metricsapi.Timeframe{ + From: metav1.Time{ + Time: time.Now(), + }, + To: metav1.Time{ + Time: time.Now(), + }, + }, + Args: map[string]string{ + "good": "good", + "dot": ".", + }, + AnalysisDefinition: metricsapi.ObjectReference{ + Name: "my-analysis-def", + Namespace: "default2", + }, + }, + } + + analysisDef2 := metricsapi.AnalysisDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-analysis-def", + Namespace: "default2", + }, + Spec: metricsapi.AnalysisDefinitionSpec{ + Objectives: []metricsapi.Objective{ + { + AnalysisValueTemplateRef: metricsapi.ObjectReference{ + Name: "my-template", + Namespace: "default", + }, + Weight: 1, + KeyObjective: false, + }, + }, + TotalScore: metricsapi.TotalScore{ + PassPercentage: 0, + WarningPercentage: 0, + }, + }, + } + tests := []struct { name string client client.Client @@ -55,6 +103,13 @@ func TestAnalysisReconciler_Reconcile_BasicControlLoop(t *testing.T) { wantErr: false, status: &metricsapi.AnalysisStatus{Raw: "{\"objectiveResults\":null,\"totalScore\":0,\"maximumScore\":0,\"pass\":true,\"warning\":false}", Pass: true}, res: metricstypes.AnalysisResult{Pass: true}, + }, { + name: "succeeded - analysis in different namespace, status updated", + client: fake2.NewClient(&analysis2, &analysisDef2, &template), + want: controllerruntime.Result{}, + wantErr: false, + status: &metricsapi.AnalysisStatus{Raw: "{\"objectiveResults\":null,\"totalScore\":0,\"maximumScore\":0,\"pass\":true,\"warning\":false}", Pass: true}, + res: metricstypes.AnalysisResult{Pass: true}, }, } diff --git a/metrics-operator/controllers/analysis/provider_selector.go b/metrics-operator/controllers/analysis/provider_selector.go index 1ec5e6e607..2bb9833738 100644 --- a/metrics-operator/controllers/analysis/provider_selector.go +++ b/metrics-operator/controllers/analysis/provider_selector.go @@ -54,36 +54,30 @@ func (ps ProvidersPool) DispatchToProviders(ctx context.Context, id int) { default: ps.log.Info("worker", "id:", id, "started job:", j.AnalysisValueTemplateRef.Name) templ := &metricsapi.AnalysisValueTemplate{} - if j.AnalysisValueTemplateRef.Namespace == "" { - j.AnalysisValueTemplateRef.Namespace = ps.Namespace - } err := ps.Client.Get(ctx, types.NamespacedName{ Name: j.AnalysisValueTemplateRef.Name, - Namespace: j.AnalysisValueTemplateRef.Namespace}, + Namespace: j.AnalysisValueTemplateRef.GetNamespace(ps.Namespace)}, templ, ) if err != nil { - ps.log.Error(err, "Failed to get the correct Provider") + ps.log.Error(err, "Failed to get AnalysisValueTemplate") ps.results <- metricsapi.ProviderResult{Objective: j.AnalysisValueTemplateRef, ErrMsg: err.Error()} ps.cancel() return } providerRef := &metricsapi.KeptnMetricsProvider{} - if templ.Spec.Provider.Namespace == "" { - templ.Spec.Provider.Namespace = ps.Namespace - } err = ps.Client.Get(ctx, types.NamespacedName{ Name: templ.Spec.Provider.Name, - Namespace: templ.Spec.Provider.Namespace}, + Namespace: templ.Spec.Provider.GetNamespace(ps.Namespace)}, providerRef, ) if err != nil { - ps.log.Error(err, "Failed to get Provider") + ps.log.Error(err, "Failed to get KeptnMetricsProvider") ps.results <- metricsapi.ProviderResult{Objective: j.AnalysisValueTemplateRef, ErrMsg: err.Error()} ps.cancel() return @@ -91,7 +85,7 @@ func (ps ProvidersPool) DispatchToProviders(ctx context.Context, id int) { templatedQuery, err := generateQuery(templ.Spec.Query, ps.Analysis.Spec.Args) if err != nil { - ps.log.Error(err, "Failed to substitute args in templ") + ps.log.Error(err, "Failed to substitute args in AnalysisValueTemplate") ps.results <- metricsapi.ProviderResult{Objective: j.AnalysisValueTemplateRef, ErrMsg: err.Error()} ps.cancel() return diff --git a/metrics-operator/controllers/analysis/provider_selector_test.go b/metrics-operator/controllers/analysis/provider_selector_test.go index 9716cef663..1b41e1997f 100644 --- a/metrics-operator/controllers/analysis/provider_selector_test.go +++ b/metrics-operator/controllers/analysis/provider_selector_test.go @@ -11,6 +11,7 @@ import ( metricstypes "github.com/keptn/lifecycle-toolkit/metrics-operator/controllers/common/analysis/types" fake2 "github.com/keptn/lifecycle-toolkit/metrics-operator/controllers/common/fake" "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -75,28 +76,109 @@ func TestProvidersPool(t *testing.T) { analysis, analysisDef, template, provider := getTestCRDs() + provider2 := metricsapi.KeptnMetricsProvider{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-provider", + Namespace: "default2", + }, + Spec: metricsapi.KeptnMetricsProviderSpec{ + Type: "prometheus", + TargetServer: "localhost:2000", + }, + } + + template2 := metricsapi.AnalysisValueTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-template", + Namespace: "default", + }, + Spec: metricsapi.AnalysisValueTemplateSpec{ + Provider: metricsapi.ObjectReference{ + Name: "my-provider", + }, + Query: "this is a {{.good}} query{{.dot}}", + }, + } + + analysisDef2 := metricsapi.AnalysisDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-analysis-def", + Namespace: "default", + }, + Spec: metricsapi.AnalysisDefinitionSpec{ + Objectives: []metricsapi.Objective{ + { + AnalysisValueTemplateRef: metricsapi.ObjectReference{ + Name: "my-template", + }, + Weight: 1, + KeyObjective: false, + }, + }, + TotalScore: metricsapi.TotalScore{ + PassPercentage: 0, + WarningPercentage: 0, + }, + }, + } + + template3 := metricsapi.AnalysisValueTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-template", + Namespace: "default2", + }, + Spec: metricsapi.AnalysisValueTemplateSpec{ + Provider: metricsapi.ObjectReference{ + Name: "my-provider", + Namespace: "default", + }, + Query: "this is a {{.good}} query{{.dot}}", + }, + } + provider.Spec.Type = "mock-provider" + provider2.Spec.Type = "mock-provider" testCases := []struct { name string expectedErr string mockClient client.Client + analysisDef metricsapi.AnalysisDefinition providerResult *metricstypes.ProviderRequest }{ { name: "MissingTemplate", expectedErr: "analysisvaluetemplates.metrics.keptn.sh \"my-template\" not found", + analysisDef: analysisDef, mockClient: fake2.NewClient(&analysis, &analysisDef), }, { name: "MissingProvider", expectedErr: "keptnmetricsproviders.metrics.keptn.sh \"my-provider\" not found", + analysisDef: analysisDef, mockClient: fake2.NewClient(&analysis, &analysisDef, &template), }, { - name: "Success", - mockClient: fake2.NewClient(&analysis, &analysisDef, &template, &provider), + name: "Success", + mockClient: fake2.NewClient(&analysis, &analysisDef, &template, &provider), + analysisDef: analysisDef, + providerResult: &metricstypes.ProviderRequest{ + Query: "this is a good query.", + }, + }, + { + name: "Success - provider in same namespace", + mockClient: fake2.NewClient(&analysis, &analysisDef, &template2, &provider2), + analysisDef: analysisDef, + providerResult: &metricstypes.ProviderRequest{ + Query: "this is a good query.", + }, + }, + { + name: "Success - analysisValueTemplate in same namespace", + mockClient: fake2.NewClient(&analysis, &analysisDef2, &template3, &provider), + analysisDef: analysisDef2, providerResult: &metricstypes.ProviderRequest{ Query: "this is a good query.", }, @@ -118,9 +200,9 @@ func TestProvidersPool(t *testing.T) { IObjectivesEvaluator: mockEvaluator, Client: tc.mockClient, log: mockLogger, - Namespace: "default", + Namespace: "default2", Objectives: map[int][]metricsapi.Objective{ - 1: analysisDef.Spec.Objectives, + 1: tc.analysisDef.Spec.Objectives, }, Analysis: &analysis, results: resultChan, diff --git a/metrics-operator/controllers/common/analysis/objective_evaluator.go b/metrics-operator/controllers/common/analysis/objective_evaluator.go index c958918bbc..0e050ca185 100644 --- a/metrics-operator/controllers/common/analysis/objective_evaluator.go +++ b/metrics-operator/controllers/common/analysis/objective_evaluator.go @@ -63,7 +63,7 @@ func getValueFromMap(values map[string]v1alpha3.ProviderResult, key string) (flo } func ComputeKey(obj v1alpha3.ObjectReference) string { - if obj.Namespace == "" { + if !obj.IsNamespaceSet() { return obj.Name } return obj.Name + "-" + obj.Namespace diff --git a/metrics-operator/main.go b/metrics-operator/main.go index 8d8f3a4fa4..d00fa1ea27 100644 --- a/metrics-operator/main.go +++ b/metrics-operator/main.go @@ -194,7 +194,6 @@ func main() { Scheme: mgr.GetScheme(), Log: analysisLogger.V(env.AnalysisControllerLogLevel), MaxWorkers: 2, - Namespace: env.PodNamespace, NewWorkersPoolFactory: analysiscontroller.NewWorkersPool, IAnalysisEvaluator: &analysisEval, }).SetupWithManager(mgr); err != nil { diff --git a/test/integration/analysis-controller/00-teststep-template.yaml b/test/integration/analysis-controller/00-teststep-template.yaml index ea3f3168fa..519913ae4f 100644 --- a/test/integration/analysis-controller/00-teststep-template.yaml +++ b/test/integration/analysis-controller/00-teststep-template.yaml @@ -2,8 +2,6 @@ apiVersion: kuttl.dev/v1beta1 kind: TestStep commands: - script: | - envsubst < mock-server.yaml | kubectl apply -f - - # substitutes current time and namespace, making sure they are changed to env var first - # to prevent bad files in case of a test interrupt + kubectl apply -f mock-server.yaml -n $NAMESPACE - script: | - envsubst < install.yaml | kubectl apply -f - + envsubst < install.yaml | kubectl apply -f - -n $NAMESPACE diff --git a/test/integration/analysis-controller/install.yaml b/test/integration/analysis-controller/install.yaml index 9b8dd7049c..7b1986fcb4 100644 --- a/test/integration/analysis-controller/install.yaml +++ b/test/integration/analysis-controller/install.yaml @@ -2,23 +2,19 @@ apiVersion: metrics.keptn.sh/v1alpha3 kind: AnalysisValueTemplate metadata: name: ready - namespace: $NAMESPACE spec: provider: name: my-mocked-provider - namespace: $NAMESPACE query: 'sum(kube_pod_container_status_ready{namespace="{{.ns}}"})' --- apiVersion: metrics.keptn.sh/v1alpha3 kind: AnalysisDefinition metadata: name: ed-my-proj-dev-svc1 - namespace: $NAMESPACE spec: objectives: - analysisValueTemplateRef: name: ready - namespace: $NAMESPACE target: failure: lessThan: @@ -36,7 +32,6 @@ apiVersion: metrics.keptn.sh/v1alpha3 kind: Analysis metadata: name: analysis-sample - namespace: $NAMESPACE spec: timeframe: from: 2023-09-14T07:33:19Z @@ -45,13 +40,11 @@ spec: "ns": "keptn-lifecycle-toolkit-system" analysisDefinition: name: ed-my-proj-dev-svc1 - namespace: $NAMESPACE --- apiVersion: metrics.keptn.sh/v1alpha3 kind: KeptnMetricsProvider metadata: name: my-mocked-provider - namespace: $NAMESPACE spec: type: prometheus targetServer: "http://mockserver.$NAMESPACE.svc.cluster.local:1080" diff --git a/test/integration/analysis-controller/mock-server.yaml b/test/integration/analysis-controller/mock-server.yaml index 44ddf38d5b..b01cda4fb3 100644 --- a/test/integration/analysis-controller/mock-server.yaml +++ b/test/integration/analysis-controller/mock-server.yaml @@ -2,7 +2,6 @@ apiVersion: v1 kind: Service metadata: name: mockserver - namespace: $NAMESPACE spec: ports: - name: serviceport @@ -20,7 +19,6 @@ metadata: labels: app: mockserver name: mockserver - namespace: $NAMESPACE spec: replicas: 1 selector: @@ -83,7 +81,6 @@ kind: ConfigMap apiVersion: v1 metadata: name: mockserver-config - namespace: $NAMESPACE data: initializerJson.json: |- [