Address feedback

experiments/analysisrun_test.go

@@ -415,10 +415,9 @@ func TestCompleteExperimentOnSuccessfulRequiredAnalysisRun(t *testing.T) {
 	}
 	e.Status.AnalysisRuns = []v1alpha1.ExperimentAnalysisRunStatus{
 		{
-			Name:                  e.Spec.Analyses[0].Name,
-			Phase:                 v1alpha1.AnalysisPhaseRunning,
-			AnalysisRun:           ar.Name,
-			RequiredForCompletion: true,
+			Name:        e.Spec.Analyses[0].Name,
+			Phase:       v1alpha1.AnalysisPhaseRunning,
+			AnalysisRun: ar.Name,
 		},
 	}
 
@@ -432,6 +431,55 @@ func TestCompleteExperimentOnSuccessfulRequiredAnalysisRun(t *testing.T) {
 	assert.Equal(t, patchedEx.Status.Phase, v1alpha1.AnalysisPhaseSuccessful)
 }
 
+func TestDoNotCompleteExperimentWithRemainingRequiredAnalysisRun(t *testing.T) {
+	templates := generateTemplates("bar")
+	e := newExperiment("foo", templates, "")
+	e.Spec.Analyses = []v1alpha1.ExperimentAnalysisTemplateRef{
+		{
+			Name:                  "success-rate",
+			TemplateName:          "success-rate",
+			RequiredForCompletion: true,
+		},
+		{
+			Name:                  "success-rate-2",
+			TemplateName:          "success-rate",
+			RequiredForCompletion: true,
+		},
+	}
+	e.Status.Phase = v1alpha1.AnalysisPhaseRunning
+	e.Status.AvailableAt = secondsAgo(60)
+	rs := templateToRS(e, templates[0], 0)
+	rs.Spec.Replicas = new(int32)
+	ar := analysisTemplateToRun("success-rate", e, &v1alpha1.AnalysisTemplateSpec{})
+	ar.Status = v1alpha1.AnalysisRunStatus{
+		Phase: v1alpha1.AnalysisPhaseSuccessful,
+	}
+	ar2 := analysisTemplateToRun("success-rate-2", e, &v1alpha1.AnalysisTemplateSpec{})
+	ar2.Status = v1alpha1.AnalysisRunStatus{
+		Phase: v1alpha1.AnalysisPhaseRunning,
+	}
+	e.Status.AnalysisRuns = []v1alpha1.ExperimentAnalysisRunStatus{
+		{
+			Name:        e.Spec.Analyses[0].Name,
+			Phase:       v1alpha1.AnalysisPhaseSuccessful,
+			AnalysisRun: ar.Name,
+		},
+		{
+			Name:        e.Spec.Analyses[1].Name,
+			Phase:       v1alpha1.AnalysisPhaseRunning,
+			AnalysisRun: ar2.Name,
+		},
+	}
+
+	f := newFixture(t, e, rs, ar, ar2)
+	defer f.Close()
+	f.expectUpdateReplicaSetAction(rs)
+	patchIndex := f.expectPatchExperimentAction(e)
+	f.run(getKey(e, t))
+	patchedEx := f.getPatchedExperimentAsObj(patchIndex)
+	assert.NotEqual(t, patchedEx.Status.Phase, v1alpha1.AnalysisPhaseSuccessful)
+}
+
 // TestTerminateAnalysisRuns verifies we terminate analysis runs when experiment is terminating
 func TestTerminateAnalysisRuns(t *testing.T) {
 	templates := generateTemplates("bar")

experiments/experiment.go

@@ -24,7 +24,7 @@ import (
 )
 
 const (
-	requiredAnalysisCompletedMessage = "Required AnalysisRun completed"
+	requiredAnalysisCompletedMessage = "Required AnalysisRuns completed"
 )
 
 type experimentContext struct {
@@ -253,8 +253,7 @@ func (ec *experimentContext) reconcileAnalysisRun(analysis v1alpha1.ExperimentAn
 	prevStatus := experimentutil.GetAnalysisRunStatus(ec.ex.Status, analysis.Name)
 	if prevStatus == nil {
 		prevStatus = &v1alpha1.ExperimentAnalysisRunStatus{
-			Name:                  analysis.Name,
-			RequiredForCompletion: analysis.RequiredForCompletion,
+			Name: analysis.Name,
 		}
 	}
 	newStatus := prevStatus.DeepCopy()
@@ -385,7 +384,9 @@ func (ec *experimentContext) calculateStatus() *v1alpha1.ExperimentStatus {
 				// We will now fail the experiment.
 				ec.newStatus.Phase = analysesStatus
 				ec.newStatus.Message = analysesMessage
-			} else if analysesMessage == requiredAnalysisCompletedMessage {
+			} else if experimentutil.CompletedAllRequiredAnalysisRuns(ec.ex, ec.newStatus) {
+				// All the required analysis runs have completed successfully so we can conclude the experiment
+				// successfully.
 				ec.newStatus.Phase = analysesStatus
 				ec.newStatus.Message = analysesMessage
 			} else {
@@ -447,19 +448,15 @@ func (ec *experimentContext) assessAnalysisRuns() (v1alpha1.AnalysisPhase, strin
 	worstStatus := v1alpha1.AnalysisPhaseSuccessful
 	message := ""
 
-	requiredAnalysisRunCompleted := false
 	for _, a := range ec.ex.Spec.Analyses {
 		as := experimentutil.GetAnalysisRunStatus(*ec.newStatus, a.Name)
-		if as.RequiredForCompletion && as.Phase == v1alpha1.AnalysisPhaseSuccessful {
-			requiredAnalysisRunCompleted = true
-		}
 		if analysisutil.IsWorse(worstStatus, as.Phase) {
 			worstStatus = as.Phase
 			message = as.Message
 		}
 	}
 
-	if requiredAnalysisRunCompleted && analysisutil.IsWorse(worstStatus, v1alpha1.AnalysisPhaseRunning) {
+	if experimentutil.CompletedAllRequiredAnalysisRuns(ec.ex, ec.newStatus) && analysisutil.IsWorse(worstStatus, v1alpha1.AnalysisPhaseRunning) {
 		return v1alpha1.AnalysisPhaseSuccessful, requiredAnalysisCompletedMessage
 	}
 	if worstStatus == v1alpha1.AnalysisPhasePending {

manifests/crds/experiment-crd.yaml

@@ -2512,8 +2512,6 @@ spec:
                     type: string
                   phase:
                     type: string
-                  requiredForCompletion:
-                    type: boolean
                 required:
                 - analysisRun
                 - name

pkg/apis/rollouts/v1alpha1/experiment_types.go

@@ -203,6 +203,4 @@ type ExperimentAnalysisRunStatus struct {
 	Phase AnalysisPhase `json:"phase"`
 	// Message is a message explaining the current status
 	Message string `json:"message,omitempty"`
-	// RequiredForCompletion indicates that experiment should complete after analysis finishes
-	RequiredForCompletion bool `json:"requiredForCompletion,omitempty"`
 }

utils/experiment/experiment.go

@@ -45,11 +45,28 @@ func IsTerminating(experiment *v1alpha1.Experiment) bool {
 		case v1alpha1.AnalysisPhaseFailed, v1alpha1.AnalysisPhaseError, v1alpha1.AnalysisPhaseInconclusive:
 			return true
 		}
-		if run.Phase == v1alpha1.AnalysisPhaseSuccessful && run.RequiredForCompletion {
-			return true
+	}
+	return CompletedAllRequiredAnalysisRuns(experiment, &experiment.Status)
+}
+
+// CompletedAllRequiredAnalysisRuns has at least one required for completition analysis run
+// and it completed successfully
+func CompletedAllRequiredAnalysisRuns(ex *v1alpha1.Experiment, exStatus *v1alpha1.ExperimentStatus) bool {
+	if exStatus == nil {
+		return false
+	}
+	hasRequiredAnalysisRun := false
+	completedAllRequiredRuns := true
+	for _, analysis := range ex.Spec.Analyses {
+		if analysis.RequiredForCompletion {
+			hasRequiredAnalysisRun = true
+			analysisStatus := GetAnalysisRunStatus(*exStatus, analysis.Name)
+			if analysisStatus == nil || analysisStatus.Phase != v1alpha1.AnalysisPhaseSuccessful {
+				completedAllRequiredRuns = false
+			}
 		}
 	}
-	return false
+	return hasRequiredAnalysisRun && completedAllRequiredRuns
 }
 
 // PassedDurations indicates if the experiment has run longer than the duration

utils/experiment/experiment_test.go

@@ -5,14 +5,13 @@ import (
 	"testing"
 	"time"
 
+	"github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
+	"github.com/argoproj/argo-rollouts/pkg/client/clientset/versioned/fake"
 	"github.com/stretchr/testify/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	kubetesting "k8s.io/client-go/testing"
 	"k8s.io/utils/pointer"
-
-	"github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
-	"github.com/argoproj/argo-rollouts/pkg/client/clientset/versioned/fake"
 )
 
 func TestHasFinished(t *testing.T) {
@@ -196,12 +195,18 @@ func TestIsTeriminating(t *testing.T) {
 	}
 	{
 		e := &v1alpha1.Experiment{
+			Spec: v1alpha1.ExperimentSpec{
+				Analyses: []v1alpha1.ExperimentAnalysisTemplateRef{{
+					Name:                  "foo",
+					RequiredForCompletion: true,
+				}},
+			},
 			Status: v1alpha1.ExperimentStatus{
 				Phase: v1alpha1.AnalysisPhaseRunning,
 				AnalysisRuns: []v1alpha1.ExperimentAnalysisRunStatus{
 					{
-						Phase:                 v1alpha1.AnalysisPhaseSuccessful,
-						RequiredForCompletion: true,
+						Name:  "foo",
+						Phase: v1alpha1.AnalysisPhaseSuccessful,
 					},
 				},
 			},
@@ -362,3 +367,25 @@ func TestIsSemanticallyEqual(t *testing.T) {
 	right.Templates[0].Replicas = pointer.Int32Ptr(1)
 	assert.False(t, IsSemanticallyEqual(*left, *right))
 }
+
+func TestCompletedAllRequiredAnalysisRuns(t *testing.T) {
+	e := &v1alpha1.Experiment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "foo",
+		},
+	}
+	assert.False(t, CompletedAllRequiredAnalysisRuns(e, nil))
+	assert.False(t, CompletedAllRequiredAnalysisRuns(e, &e.Status))
+	e.Spec.Analyses = []v1alpha1.ExperimentAnalysisTemplateRef{{
+		Name: "foo",
+	}}
+	e.Status.AnalysisRuns = []v1alpha1.ExperimentAnalysisRunStatus{{
+		Name:  "foo",
+		Phase: v1alpha1.AnalysisPhaseRunning,
+	}}
+	assert.False(t, CompletedAllRequiredAnalysisRuns(e, &e.Status))
+	e.Spec.Analyses[0].RequiredForCompletion = true
+	assert.False(t, CompletedAllRequiredAnalysisRuns(e, &e.Status))
+	e.Status.AnalysisRuns[0].Phase = v1alpha1.AnalysisPhaseSuccessful
+	assert.True(t, CompletedAllRequiredAnalysisRuns(e, &e.Status))
+}