Add metric knob for maxInconclusive (#181)

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 {

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",

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:

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:

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"

utils/analysis/analysis.go

@@ -29,12 +29,18 @@ 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")
 	}
 	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")
 	}
@@ -78,18 +84,19 @@ 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 {
-	for _, res := range run.Status.MetricResults {
-		switch res.Status {
-		case v1alpha1.AnalysisStatusFailed, v1alpha1.AnalysisStatusError:
-			return true
+	if run.Spec.Terminate {
+		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

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{
@@ -151,7 +182,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 +199,20 @@ 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))
+	run.Status.MetricResults = nil
+	assert.False(t, IsTerminating(run))
+	run.Status = nil
+	assert.False(t, IsTerminating(run))
 }
 
 func TestGetResult(t *testing.T) {