From 1aca5300fce1e51666800b63d1b2603ec3e2ff53 Mon Sep 17 00:00:00 2001 From: zachaller Date: Wed, 14 Jun 2023 13:09:30 -0500 Subject: [PATCH 1/6] resolve args to metric in garbage collection function Signed-off-by: zachaller --- analysis/analysis.go | 7 ++- analysis/analysis_test.go | 125 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 1 deletion(-) diff --git a/analysis/analysis.go b/analysis/analysis.go index c826280d19..87129ceca1 100644 --- a/analysis/analysis.go +++ b/analysis/analysis.go @@ -711,8 +711,13 @@ func calculateNextReconcileTime(run *v1alpha1.AnalysisRun, metrics []v1alpha1.Me func (c *Controller) garbageCollectMeasurements(run *v1alpha1.AnalysisRun, measurementRetentionMetricNamesMap map[string]*v1alpha1.MeasurementRetention, limit int) error { var errors []error + resolvedArgsMetric, err := getResolvedMetricsWithoutSecrets(run.Spec.Metrics, run.Spec.Args) + if err != nil { + return err + } + metricsByName := make(map[string]v1alpha1.Metric) - for _, metric := range run.Spec.Metrics { + for _, metric := range resolvedArgsMetric { metricsByName[metric.Name] = metric } diff --git a/analysis/analysis_test.go b/analysis/analysis_test.go index ed12da2dca..f90b00eb7d 100644 --- a/analysis/analysis_test.go +++ b/analysis/analysis_test.go @@ -8,6 +8,8 @@ import ( "testing" "time" + "github.com/argoproj/argo-rollouts/metric" + log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -1097,6 +1099,129 @@ func TestTrimMeasurementHistory(t *testing.T) { assert.Equal(t, "2", run.Status.MetricResults[1].Measurements[0].Value) assert.Equal(t, "3", run.Status.MetricResults[1].Measurements[1].Value) } + { + run := newRun() + run.Spec.Args = append(run.Spec.Args, v1alpha1.Argument{ + Name: "port", + Value: pointer.String("port"), + }) + run.Spec.Metrics = []v1alpha1.Metric{ + { + Name: "metric-prom1", + Interval: "60s", + Provider: v1alpha1.MetricProvider{ + Prometheus: &v1alpha1.PrometheusMetric{ + Address: "https://prometheus.kubeaddons:{{args.port}}", + }, + }, + }, + } + var measurementRetentionMetricsMap = map[string]*v1alpha1.MeasurementRetention{} + measurementRetentionMetricsMap["metric2"] = &v1alpha1.MeasurementRetention{MetricName: "metric2", Limit: 2} + err := c.garbageCollectMeasurements(run, measurementRetentionMetricsMap, 1) + assert.Nil(t, err) + assert.Len(t, run.Status.MetricResults[0].Measurements, 1) + assert.Equal(t, "1", run.Status.MetricResults[0].Measurements[0].Value) + assert.Len(t, run.Status.MetricResults[1].Measurements, 2) + assert.Equal(t, "2", run.Status.MetricResults[1].Measurements[0].Value) + assert.Equal(t, "3", run.Status.MetricResults[1].Measurements[1].Value) + } +} + +func TestGarbageCollectArgResolution(t *testing.T) { + f := newFixture(t) + defer f.Close() + c, _, _ := f.newController(noResyncPeriodFunc) + + c.newProvider = func(logCtx log.Entry, metric v1alpha1.Metric) (metric.Provider, error) { + assert.Equal(t, "https://prometheus.kubeaddons:8080", metric.Provider.Prometheus.Address) + return f.provider, nil + } + + f.provider.On("GarbageCollect", mock.Anything, mock.Anything, mock.Anything).Return(nil) + run := &v1alpha1.AnalysisRun{ + Spec: v1alpha1.AnalysisRunSpec{ + Metrics: []v1alpha1.Metric{ + { + Name: "metric1", + Interval: "60s", + Provider: v1alpha1.MetricProvider{ + Prometheus: &v1alpha1.PrometheusMetric{ + Address: "https://prometheus.kubeaddons:{{args.port}}", + }, + }, + }, + { + Name: "metric2", + Interval: "60s", + Provider: v1alpha1.MetricProvider{ + Prometheus: &v1alpha1.PrometheusMetric{ + Address: "https://prometheus.kubeaddons:{{args.port}}", + }, + }, + }, + }, + }, + Status: v1alpha1.AnalysisRunStatus{ + Phase: v1alpha1.AnalysisPhaseRunning, + MetricResults: []v1alpha1.MetricResult{ + { + Name: "metric1", + Phase: v1alpha1.AnalysisPhaseRunning, + Measurements: []v1alpha1.Measurement{ + { + Value: "1", + Phase: v1alpha1.AnalysisPhaseSuccessful, + StartedAt: timePtr(metav1.NewTime(time.Now().Add(-60 * time.Second))), + FinishedAt: timePtr(metav1.NewTime(time.Now().Add(-60 * time.Second))), + }, + { + Value: "2", + Phase: v1alpha1.AnalysisPhaseSuccessful, + StartedAt: timePtr(metav1.NewTime(time.Now().Add(-60 * time.Second))), + FinishedAt: timePtr(metav1.NewTime(time.Now().Add(-60 * time.Second))), + }, + }, + }, + { + Name: "metric2", + Measurements: []v1alpha1.Measurement{ + { + Value: "2", + Phase: v1alpha1.AnalysisPhaseSuccessful, + StartedAt: timePtr(metav1.NewTime(time.Now().Add(-60 * time.Second))), + FinishedAt: timePtr(metav1.NewTime(time.Now().Add(-60 * time.Second))), + }, + { + Value: "3", + Phase: v1alpha1.AnalysisPhaseSuccessful, + StartedAt: timePtr(metav1.NewTime(time.Now().Add(-30 * time.Second))), + FinishedAt: timePtr(metav1.NewTime(time.Now().Add(-30 * time.Second))), + }, + { + Value: "4", + Phase: v1alpha1.AnalysisPhaseSuccessful, + StartedAt: timePtr(metav1.NewTime(time.Now().Add(-30 * time.Second))), + FinishedAt: timePtr(metav1.NewTime(time.Now().Add(-30 * time.Second))), + }, + }, + }, + }, + }, + } + run.Spec.Args = append(run.Spec.Args, v1alpha1.Argument{ + Name: "port", + Value: pointer.String("8080"), + }) + var measurementRetentionMetricsMap = map[string]*v1alpha1.MeasurementRetention{} + measurementRetentionMetricsMap["metric2"] = &v1alpha1.MeasurementRetention{MetricName: "metric2", Limit: 2} + err := c.garbageCollectMeasurements(run, measurementRetentionMetricsMap, 1) + assert.Nil(t, err) + assert.Len(t, run.Status.MetricResults[0].Measurements, 1) + assert.Equal(t, "2", run.Status.MetricResults[0].Measurements[0].Value) + assert.Len(t, run.Status.MetricResults[1].Measurements, 2) + assert.Equal(t, "3", run.Status.MetricResults[1].Measurements[0].Value) + assert.Equal(t, "4", run.Status.MetricResults[1].Measurements[1].Value) } func TestResolveMetricArgsUnableToSubstitute(t *testing.T) { From 6620c1bd175ab8eb09df634cc792e60b01094fd9 Mon Sep 17 00:00:00 2001 From: zachaller Date: Wed, 14 Jun 2023 13:12:47 -0500 Subject: [PATCH 2/6] remove incorrectly added test, this moved to a function Signed-off-by: zachaller --- analysis/analysis_test.go | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/analysis/analysis_test.go b/analysis/analysis_test.go index f90b00eb7d..cec3446b89 100644 --- a/analysis/analysis_test.go +++ b/analysis/analysis_test.go @@ -1099,33 +1099,6 @@ func TestTrimMeasurementHistory(t *testing.T) { assert.Equal(t, "2", run.Status.MetricResults[1].Measurements[0].Value) assert.Equal(t, "3", run.Status.MetricResults[1].Measurements[1].Value) } - { - run := newRun() - run.Spec.Args = append(run.Spec.Args, v1alpha1.Argument{ - Name: "port", - Value: pointer.String("port"), - }) - run.Spec.Metrics = []v1alpha1.Metric{ - { - Name: "metric-prom1", - Interval: "60s", - Provider: v1alpha1.MetricProvider{ - Prometheus: &v1alpha1.PrometheusMetric{ - Address: "https://prometheus.kubeaddons:{{args.port}}", - }, - }, - }, - } - var measurementRetentionMetricsMap = map[string]*v1alpha1.MeasurementRetention{} - measurementRetentionMetricsMap["metric2"] = &v1alpha1.MeasurementRetention{MetricName: "metric2", Limit: 2} - err := c.garbageCollectMeasurements(run, measurementRetentionMetricsMap, 1) - assert.Nil(t, err) - assert.Len(t, run.Status.MetricResults[0].Measurements, 1) - assert.Equal(t, "1", run.Status.MetricResults[0].Measurements[0].Value) - assert.Len(t, run.Status.MetricResults[1].Measurements, 2) - assert.Equal(t, "2", run.Status.MetricResults[1].Measurements[0].Value) - assert.Equal(t, "3", run.Status.MetricResults[1].Measurements[1].Value) - } } func TestGarbageCollectArgResolution(t *testing.T) { From 879462abdc634d2683e56bce9d4a66d20616d6ac Mon Sep 17 00:00:00 2001 From: zachaller Date: Wed, 14 Jun 2023 14:16:38 -0500 Subject: [PATCH 3/6] re-trigger Signed-off-by: zachaller From 50b8156a946c5af8887d1a872156e63c785128cf Mon Sep 17 00:00:00 2001 From: zachaller Date: Thu, 15 Jun 2023 10:45:26 -0500 Subject: [PATCH 4/6] add context to errors Signed-off-by: zachaller --- analysis/analysis.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analysis/analysis.go b/analysis/analysis.go index 87129ceca1..e46618a93f 100644 --- a/analysis/analysis.go +++ b/analysis/analysis.go @@ -713,7 +713,7 @@ func (c *Controller) garbageCollectMeasurements(run *v1alpha1.AnalysisRun, measu resolvedArgsMetric, err := getResolvedMetricsWithoutSecrets(run.Spec.Metrics, run.Spec.Args) if err != nil { - return err + return fmt.Errorf("failed to resolve metrics during garbage collection: %w", err) } metricsByName := make(map[string]v1alpha1.Metric) From 8f7ef965ea6b8d17f6348d718da4c0be62aadbcd Mon Sep 17 00:00:00 2001 From: zachaller Date: Thu, 15 Jun 2023 10:46:16 -0500 Subject: [PATCH 5/6] better error message Signed-off-by: zachaller --- analysis/analysis.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analysis/analysis.go b/analysis/analysis.go index e46618a93f..92ea36c983 100644 --- a/analysis/analysis.go +++ b/analysis/analysis.go @@ -713,7 +713,7 @@ func (c *Controller) garbageCollectMeasurements(run *v1alpha1.AnalysisRun, measu resolvedArgsMetric, err := getResolvedMetricsWithoutSecrets(run.Spec.Metrics, run.Spec.Args) if err != nil { - return fmt.Errorf("failed to resolve metrics during garbage collection: %w", err) + return fmt.Errorf("failed to resolve args on metric during garbage collection: %w", err) } metricsByName := make(map[string]v1alpha1.Metric) From c9dd7ea02f9dbc933ec706821b4530401ab9e6ba Mon Sep 17 00:00:00 2001 From: zachaller Date: Thu, 15 Jun 2023 10:46:30 -0500 Subject: [PATCH 6/6] better error message Signed-off-by: zachaller --- analysis/analysis.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analysis/analysis.go b/analysis/analysis.go index 92ea36c983..fb307e62e0 100644 --- a/analysis/analysis.go +++ b/analysis/analysis.go @@ -713,7 +713,7 @@ func (c *Controller) garbageCollectMeasurements(run *v1alpha1.AnalysisRun, measu resolvedArgsMetric, err := getResolvedMetricsWithoutSecrets(run.Spec.Metrics, run.Spec.Args) if err != nil { - return fmt.Errorf("failed to resolve args on metric during garbage collection: %w", err) + return fmt.Errorf("failed to resolve args on metrics during garbage collection: %w", err) } metricsByName := make(map[string]v1alpha1.Metric)