From 431996aa710c6a077694e770aa896bc565d59c96 Mon Sep 17 00:00:00 2001 From: Jesse Suen Date: Wed, 2 Oct 2019 14:40:00 -0700 Subject: [PATCH 1/2] Add metric knob for maxInconclusive --- analysis/analysis.go | 23 ++++++-------- analysis/analysis_test.go | 31 +++++++++++++++++++ manifests/crds/analysis-run-crd.yaml | 3 ++ manifests/crds/analysis-template-crd.yaml | 3 ++ pkg/apis/rollouts/v1alpha1/analysis_types.go | 9 ++++-- .../rollouts/v1alpha1/openapi_generated.go | 11 +++++-- utils/analysis/analysis.go | 18 ++++++----- utils/analysis/analysis_test.go | 11 ++++--- 8 files changed, 78 insertions(+), 31 deletions(-) diff --git a/analysis/analysis.go b/analysis/analysis.go index 0bf7dd527d..de5b5bf8a6 100644 --- a/analysis/analysis.go +++ b/analysis/analysis.go @@ -286,6 +286,10 @@ func assessMetricStatus(metric v1alpha1.Metric, result v1alpha1.MetricResult, te log.Infof("metric assessed %s: failed (%d) > maxFailures (%d)", v1alpha1.AnalysisStatusFailed, result.Failed, metric.MaxFailures) return v1alpha1.AnalysisStatusFailed } + if result.Inconclusive > metric.MaxInconclusive { + log.Infof("metric assessed %s: inconclusive (%d) > maxInconclusive (%d)", v1alpha1.AnalysisStatusInconclusive, result.Inconclusive, metric.MaxInconclusive) + return v1alpha1.AnalysisStatusInconclusive + } consecutiveErrors := analysisutil.ConsecutiveErrors(result) maxConsecutiveErrors := DefaultMaxConsecutiveErrors if metric.MaxConsecutiveErrors != nil { @@ -295,22 +299,13 @@ func assessMetricStatus(metric v1alpha1.Metric, result v1alpha1.MetricResult, te log.Infof("metric assessed %s: consecutiveErrors (%d) > maxConsecutiveErrors (%d)", v1alpha1.AnalysisStatusError, consecutiveErrors, maxConsecutiveErrors) return v1alpha1.AnalysisStatusError } - // If a count was specified, and we reached that count, then we assess the status based on - // the greater of the Successful & Inconclusive status counters. - // Error and Failed counters are ignored because those checks have already been taken into - // consideration above, and we do not want to fail the metric if failures < maxFailures. - // TODO(jessesuen): may need to tweak this logic + // If a count was specified, and we reached that count, then metric is considered Successful. + // The Error, Failed, Inconclusive counters are ignored because those checks have already been + // taken into consideration above, and we do not want to fail if failures < maxFailures. effectiveCount := metric.EffectiveCount() if effectiveCount != nil && result.Count >= *effectiveCount { - var status v1alpha1.AnalysisStatus - if result.Successful > result.Inconclusive { - status = v1alpha1.AnalysisStatusSuccessful - } else { - status = v1alpha1.AnalysisStatusInconclusive - } - log.Infof("metric assessed %s: count %d reached, successful: %d, inconclusive: %d, errors: %d, failures: %d", - status, result.Count, result.Successful, result.Inconclusive, result.Error, result.Failed) - return status + log.Infof("metric assessed %s: count (%d) reached", v1alpha1.AnalysisStatusSuccessful, *effectiveCount) + return v1alpha1.AnalysisStatusSuccessful } // if we get here, this metric runs indefinitely if terminating { diff --git a/analysis/analysis_test.go b/analysis/analysis_test.go index dea9643a85..8885df2d93 100644 --- a/analysis/analysis_test.go +++ b/analysis/analysis_test.go @@ -312,9 +312,11 @@ func TestAssessMetricStatusMaxFailures(t *testing.T) { // max failures metric := v1alpha1.Metric{ Name: "success-rate", MaxFailures: 2, + Interval: pointer.Int32Ptr(60), } result := v1alpha1.MetricResult{ Failed: 3, + Count: 3, Measurements: []v1alpha1.Measurement{ { Value: "99", @@ -326,7 +328,36 @@ func TestAssessMetricStatusMaxFailures(t *testing.T) { // max failures } assert.Equal(t, v1alpha1.AnalysisStatusFailed, assessMetricStatus(metric, result, false)) assert.Equal(t, v1alpha1.AnalysisStatusFailed, assessMetricStatus(metric, result, true)) + metric.MaxFailures = 3 + assert.Equal(t, v1alpha1.AnalysisStatusRunning, assessMetricStatus(metric, result, false)) + assert.Equal(t, v1alpha1.AnalysisStatusSuccessful, assessMetricStatus(metric, result, true)) } + +func TestAssessMetricStatusMaxInconclusive(t *testing.T) { // max failures + metric := v1alpha1.Metric{ + Name: "success-rate", + MaxInconclusive: 2, + Interval: pointer.Int32Ptr(60), + } + result := v1alpha1.MetricResult{ + Inconclusive: 3, + Count: 3, + Measurements: []v1alpha1.Measurement{ + { + Value: "99", + Status: v1alpha1.AnalysisStatusInconclusive, + StartedAt: timePtr(metav1.NewTime(time.Now().Add(-60 * time.Second))), + FinishedAt: timePtr(metav1.NewTime(time.Now().Add(-60 * time.Second))), + }, + }, + } + assert.Equal(t, v1alpha1.AnalysisStatusInconclusive, assessMetricStatus(metric, result, false)) + assert.Equal(t, v1alpha1.AnalysisStatusInconclusive, assessMetricStatus(metric, result, true)) + metric.MaxInconclusive = 3 + assert.Equal(t, v1alpha1.AnalysisStatusRunning, assessMetricStatus(metric, result, false)) + assert.Equal(t, v1alpha1.AnalysisStatusSuccessful, assessMetricStatus(metric, result, true)) +} + func TestAssessMetricStatusConsecutiveErrors(t *testing.T) { metric := v1alpha1.Metric{ Name: "success-rate", diff --git a/manifests/crds/analysis-run-crd.yaml b/manifests/crds/analysis-run-crd.yaml index a6488c6e18..32d8e24069 100644 --- a/manifests/crds/analysis-run-crd.yaml +++ b/manifests/crds/analysis-run-crd.yaml @@ -42,6 +42,9 @@ spec: maxFailures: format: int32 type: integer + maxInconclusive: + format: int32 + type: integer name: type: string provider: diff --git a/manifests/crds/analysis-template-crd.yaml b/manifests/crds/analysis-template-crd.yaml index 28485ff019..5a2b096bf8 100644 --- a/manifests/crds/analysis-template-crd.yaml +++ b/manifests/crds/analysis-template-crd.yaml @@ -40,6 +40,9 @@ spec: maxFailures: format: int32 type: integer + maxInconclusive: + format: int32 + type: integer name: type: string provider: diff --git a/pkg/apis/rollouts/v1alpha1/analysis_types.go b/pkg/apis/rollouts/v1alpha1/analysis_types.go index 2843a891d4..11de0cafff 100644 --- a/pkg/apis/rollouts/v1alpha1/analysis_types.go +++ b/pkg/apis/rollouts/v1alpha1/analysis_types.go @@ -52,8 +52,11 @@ type Metric struct { // either condition, the measurement is considered Inconclusive FailureCondition string `json:"failureCondition,omitempty"` // MaxFailures is the maximum number of times the measurement is allowed to fail, before the - // entire metric is considered failed (default: 0) + // entire metric is considered Failed (default: 0) MaxFailures int32 `json:"maxFailures,omitempty"` + // MaxInconclusive is the maximum number of times the measurement is allowed to measure + // Inconclusive, before the entire metric is considered Inconclusive (default: 0) + MaxInconclusive int32 `json:"maxInconclusive,omitempty"` // MaxConsecutiveErrors is the maximum number of times the measurement is allowed to error in // succession, before the metric is considered error (default: 4) MaxConsecutiveErrors *int32 `json:"maxConsecutiveErrors,omitempty"` @@ -85,10 +88,10 @@ type AnalysisProvider struct { Prometheus *PrometheusMetric `json:"prometheus,omitempty"` } -// AnalysisStatus is the overall status of the AnalysisRun, MetricResults, or Measurement +// AnalysisStatus is the overall status of an AnalysisRun, MetricResult, or Measurement type AnalysisStatus string -// AnalysisStatus is the overall status of the AnalysisRun, MetricResults +// Possible AnalysisStatus values const ( AnalysisStatusPending AnalysisStatus = "Pending" AnalysisStatusRunning AnalysisStatus = "Running" diff --git a/pkg/apis/rollouts/v1alpha1/openapi_generated.go b/pkg/apis/rollouts/v1alpha1/openapi_generated.go index 785cb0a4f8..72d769e327 100644 --- a/pkg/apis/rollouts/v1alpha1/openapi_generated.go +++ b/pkg/apis/rollouts/v1alpha1/openapi_generated.go @@ -254,7 +254,7 @@ func schema_pkg_apis_rollouts_v1alpha1_AnalysisRunStatus(ref common.ReferenceCal }, "metricResults": { SchemaProps: spec.SchemaProps{ - Description: "Metrics contains the metrics collected during the run", + Description: "MetricResults contains the metrics collected during the run", Type: []string{"array"}, Items: &spec.SchemaOrArray{ Schema: &spec.Schema{ @@ -976,7 +976,14 @@ func schema_pkg_apis_rollouts_v1alpha1_Metric(ref common.ReferenceCallback) comm }, "maxFailures": { SchemaProps: spec.SchemaProps{ - Description: "MaxFailures is the maximum number of times the measurement is allowed to fail, before the entire metric is considered failed (default: 0)", + Description: "MaxFailures is the maximum number of times the measurement is allowed to fail, before the entire metric is considered Failed (default: 0)", + Type: []string{"integer"}, + Format: "int32", + }, + }, + "maxInconclusive": { + SchemaProps: spec.SchemaProps{ + Description: "MaxInconclusive is the maximum number of times the measurement is allowed to measure Inconclusive, before the entire metric is considered Inconclusive (default: 0)", Type: []string{"integer"}, Format: "int32", }, diff --git a/utils/analysis/analysis.go b/utils/analysis/analysis.go index 57edf207f4..d02230df7f 100644 --- a/utils/analysis/analysis.go +++ b/utils/analysis/analysis.go @@ -29,6 +29,9 @@ func ValidateMetric(metric v1alpha1.Metric) error { if metric.Count < metric.MaxFailures { return fmt.Errorf("count must be >= maxFailures") } + if metric.Count < metric.MaxInconclusive { + return fmt.Errorf("count must be >= maxInconclusive") + } if metric.Count > 1 && metric.Interval == nil { return fmt.Errorf("interval must be specified when count > 1") } @@ -78,17 +81,16 @@ func IsWorse(current, new v1alpha1.AnalysisStatus) bool { return newIndex > currentIndex } -// IsTerminating returns whether or not the analysis run is terminating +// IsTerminating returns whether or not the analysis run is terminating, either because a terminate +// was requested explicitly, or because a metric has already measured Failed, Error, or Inconclusive +// which causes the run to end prematurely. func IsTerminating(run *v1alpha1.AnalysisRun) bool { - return run.Spec.Terminate || IsFailing(run) -} - -// IsFailing returns whether or not any metric has measured Failed or Error, which will eventually -// cause the entire run to fail. -func IsFailing(run *v1alpha1.AnalysisRun) bool { + if run.Spec.Terminate { + return true + } for _, res := range run.Status.MetricResults { switch res.Status { - case v1alpha1.AnalysisStatusFailed, v1alpha1.AnalysisStatusError: + case v1alpha1.AnalysisStatusFailed, v1alpha1.AnalysisStatusError, v1alpha1.AnalysisStatusInconclusive: return true } } diff --git a/utils/analysis/analysis_test.go b/utils/analysis/analysis_test.go index 3301d2dbf8..9dfe95f5ce 100644 --- a/utils/analysis/analysis_test.go +++ b/utils/analysis/analysis_test.go @@ -151,7 +151,7 @@ func TestIsWorst(t *testing.T) { assert.False(t, IsWorse(v1alpha1.AnalysisStatusFailed, v1alpha1.AnalysisStatusFailed)) } -func TestIsFailing(t *testing.T) { +func TestIsFastFailTerminating(t *testing.T) { run := &v1alpha1.AnalysisRun{ Status: &v1alpha1.AnalysisRunStatus{ Status: v1alpha1.AnalysisStatusRunning, @@ -168,13 +168,16 @@ func TestIsFailing(t *testing.T) { }, } successRate := run.Status.MetricResults[1] - assert.False(t, IsFailing(run)) + assert.False(t, IsTerminating(run)) successRate.Status = v1alpha1.AnalysisStatusError run.Status.MetricResults[1] = successRate - assert.True(t, IsFailing(run)) + assert.True(t, IsTerminating(run)) successRate.Status = v1alpha1.AnalysisStatusFailed run.Status.MetricResults[1] = successRate - assert.True(t, IsFailing(run)) + assert.True(t, IsTerminating(run)) + successRate.Status = v1alpha1.AnalysisStatusInconclusive + run.Status.MetricResults[1] = successRate + assert.True(t, IsTerminating(run)) } func TestGetResult(t *testing.T) { From 1955bd13c617ce2bc230ecb0e7a0cfc9d6205d4e Mon Sep 17 00:00:00 2001 From: Jesse Suen Date: Mon, 7 Oct 2019 12:59:42 -0700 Subject: [PATCH 2/2] Address review comments --- utils/analysis/analysis.go | 13 ++++++++---- utils/analysis/analysis_test.go | 35 +++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/utils/analysis/analysis.go b/utils/analysis/analysis.go index d02230df7f..22eaca2f4b 100644 --- a/utils/analysis/analysis.go +++ b/utils/analysis/analysis.go @@ -38,6 +38,9 @@ func ValidateMetric(metric v1alpha1.Metric) error { if metric.MaxFailures < 0 { return fmt.Errorf("maxFailures must be >= 0") } + if metric.MaxInconclusive < 0 { + return fmt.Errorf("maxInconclusive must be >= 0") + } if metric.MaxConsecutiveErrors != nil && *metric.MaxConsecutiveErrors < 0 { return fmt.Errorf("maxConsecutiveErrors must be >= 0") } @@ -88,10 +91,12 @@ func IsTerminating(run *v1alpha1.AnalysisRun) bool { if run.Spec.Terminate { return true } - for _, res := range run.Status.MetricResults { - switch res.Status { - case v1alpha1.AnalysisStatusFailed, v1alpha1.AnalysisStatusError, v1alpha1.AnalysisStatusInconclusive: - return true + if run.Status != nil { + for _, res := range run.Status.MetricResults { + switch res.Status { + case v1alpha1.AnalysisStatusFailed, v1alpha1.AnalysisStatusError, v1alpha1.AnalysisStatusInconclusive: + return true + } } } return false diff --git a/utils/analysis/analysis_test.go b/utils/analysis/analysis_test.go index 9dfe95f5ce..605940f153 100644 --- a/utils/analysis/analysis_test.go +++ b/utils/analysis/analysis_test.go @@ -26,6 +26,22 @@ func TestValidateMetrics(t *testing.T) { err := ValidateAnalysisTemplateSpec(spec) assert.EqualError(t, err, "metrics[0]: count must be >= maxFailures") } + { + spec := v1alpha1.AnalysisTemplateSpec{ + Metrics: []v1alpha1.Metric{ + { + Name: "success-rate", + Count: 1, + MaxInconclusive: 2, + Provider: v1alpha1.AnalysisProvider{ + Prometheus: &v1alpha1.PrometheusMetric{}, + }, + }, + }, + } + err := ValidateAnalysisTemplateSpec(spec) + assert.EqualError(t, err, "metrics[0]: count must be >= maxInconclusive") + } { spec := v1alpha1.AnalysisTemplateSpec{ Metrics: []v1alpha1.Metric{ @@ -100,6 +116,21 @@ func TestValidateMetrics(t *testing.T) { err := ValidateAnalysisTemplateSpec(spec) assert.EqualError(t, err, "metrics[0]: maxFailures must be >= 0") } + { + spec := v1alpha1.AnalysisTemplateSpec{ + Metrics: []v1alpha1.Metric{ + { + Name: "success-rate", + MaxInconclusive: -1, + Provider: v1alpha1.AnalysisProvider{ + Prometheus: &v1alpha1.PrometheusMetric{}, + }, + }, + }, + } + err := ValidateAnalysisTemplateSpec(spec) + assert.EqualError(t, err, "metrics[0]: maxInconclusive must be >= 0") + } { spec := v1alpha1.AnalysisTemplateSpec{ Metrics: []v1alpha1.Metric{ @@ -178,6 +209,10 @@ func TestIsFastFailTerminating(t *testing.T) { successRate.Status = v1alpha1.AnalysisStatusInconclusive run.Status.MetricResults[1] = successRate assert.True(t, IsTerminating(run)) + run.Status.MetricResults = nil + assert.False(t, IsTerminating(run)) + run.Status = nil + assert.False(t, IsTerminating(run)) } func TestGetResult(t *testing.T) {