fix: change logic of analysis run to better handle errors (#2695)

* change logic of analysis run to better haneld errors Signed-off-by: zachaller <[email protected]> * change logic to not call GetMetaData if not nil like the old behavior Signed-off-by: zachaller <[email protected]> * move code closer to usage Signed-off-by: zachaller <[email protected]> * change logic to not always call GetMetadata keeps original behavior Signed-off-by: zachaller <[email protected]> * fix logic Signed-off-by: zachaller <[email protected]> * cleanup Signed-off-by: zachaller <[email protected]> * add test Signed-off-by: zachaller <[email protected]> --------- Signed-off-by: zachaller <[email protected]>
argoproj · May 5, 2023 · a402042 · a402042
1 parent 73e7ffb
commit a402042
Show file tree

Hide file tree

Showing 2 changed files with 59 additions and 20 deletions.
diff --git a/analysis/analysis.go b/analysis/analysis.go
@@ -321,34 +321,18 @@ func (c *Controller) runMeasurements(run *v1alpha1.AnalysisRun, tasks []metricTa
 			//redact secret values from logs
 			logger := logutil.WithRedactor(*logutil.WithAnalysisRun(run).WithField("metric", t.metric.Name), secrets)
 
-			resultsLock.Lock()
-			metricResult := analysisutil.GetResult(run, t.metric.Name)
-			resultsLock.Unlock()
-
-			provider, err := c.newProvider(*logger, t.metric)
-			if err != nil {
-				log.Errorf("Error in getting metric provider :%v", err)
-				return err
-			}
-			if metricResult == nil {
-				metricResult = &v1alpha1.MetricResult{
-					Name:     t.metric.Name,
-					Phase:    v1alpha1.AnalysisPhaseRunning,
-					DryRun:   dryRunMetricsMap[t.metric.Name],
-					Metadata: provider.GetMetadata(t.metric),
-				}
-			}
-
 			var newMeasurement v1alpha1.Measurement
-			if err != nil {
+			provider, providerErr := c.newProvider(*logger, t.metric)
+			if providerErr != nil {
+				log.Errorf("Error in getting metric provider :%v", providerErr)
 				if t.incompleteMeasurement != nil {
 					newMeasurement = *t.incompleteMeasurement
 				} else {
 					startedAt := timeutil.MetaNow()
 					newMeasurement.StartedAt = &startedAt
 				}
 				newMeasurement.Phase = v1alpha1.AnalysisPhaseError
-				newMeasurement.Message = err.Error()
+				newMeasurement.Message = providerErr.Error()
 			} else {
 				if t.incompleteMeasurement == nil {
 					newMeasurement = provider.Run(run, t.metric)
@@ -366,12 +350,28 @@ func (c *Controller) runMeasurements(run *v1alpha1.AnalysisRun, tasks []metricTa
 				}
 			}
 
+			resultsLock.Lock()
+			metricResult := analysisutil.GetResult(run, t.metric.Name)
+			resultsLock.Unlock()
+			if metricResult == nil {
+				metricResult = &v1alpha1.MetricResult{
+					Name:   t.metric.Name,
+					Phase:  v1alpha1.AnalysisPhaseRunning,
+					DryRun: dryRunMetricsMap[t.metric.Name],
+				}
+
+				if provider != nil && providerErr == nil {
+					metricResult.Metadata = provider.GetMetadata(t.metric)
+				}
+			}
+
 			if newMeasurement.Phase.Completed() {
 				logger.Infof("Measurement Completed. Result: %s", newMeasurement.Phase)
 				if newMeasurement.FinishedAt == nil {
 					finishedAt := timeutil.MetaNow()
 					newMeasurement.FinishedAt = &finishedAt
 				}
+
 				switch newMeasurement.Phase {
 				case v1alpha1.AnalysisPhaseSuccessful:
 					metricResult.Successful++

diff --git a/analysis/controller_test.go b/analysis/controller_test.go
@@ -3,6 +3,7 @@ package analysis
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"reflect"
 	"testing"
 	"time"
@@ -317,6 +318,44 @@ func TestNoReconcileForAnalysisRunWithDeletionTimestamp(t *testing.T) {
 	f.run(getKey(ar, t))
 }
 
+func TestFailedToCreateProviderError(t *testing.T) {
+	f := newFixture(t)
+	defer f.Close()
+
+	ar := &v1alpha1.AnalysisRun{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "foo",
+			Namespace: metav1.NamespaceDefault,
+		},
+		Spec: v1alpha1.AnalysisRunSpec{
+			Metrics: []v1alpha1.Metric{
+				{
+					Name: "metric1",
+					Provider: v1alpha1.MetricProvider{
+						Plugin: map[string]json.RawMessage{"mypluginns/myplugin": []byte(`{"invalid": "json"}`)},
+					},
+				},
+			},
+		},
+	}
+	f.analysisRunLister = append(f.analysisRunLister, ar)
+	f.objects = append(f.objects, ar)
+
+	c, i, k8sI := f.newController(noResyncPeriodFunc)
+	c.newProvider = func(logCtx log.Entry, metric v1alpha1.Metric) (metric.Provider, error) {
+		return nil, fmt.Errorf("failed to create provider")
+	}
+
+	pi := f.expectPatchAnalysisRunAction(ar)
+
+	f.runController(getKey(ar, t), true, false, c, i, k8sI)
+
+	updatedAr := f.getPatchedAnalysisRun(pi)
+
+	assert.Equal(t, v1alpha1.AnalysisPhaseError, updatedAr.Status.MetricResults[0].Measurements[0].Phase)
+	assert.Equal(t, "failed to create provider", updatedAr.Status.MetricResults[0].Measurements[0].Message)
+}
+
 func TestRun(t *testing.T) {
 	f := newFixture(t)
 	defer f.Close()