Use deterministic names for analysisruns created in experiments

experiments/analysisrun_test.go

@@ -39,7 +39,7 @@ func generateAnalysisTemplates(names ...string) []v1alpha1.AnalysisTemplate {
 func analysisTemplateToRun(name string, ex *v1alpha1.Experiment, spec *v1alpha1.AnalysisTemplateSpec) *v1alpha1.AnalysisRun {
 	ar := v1alpha1.AnalysisRun{
 		ObjectMeta: metav1.ObjectMeta{
-			GenerateName:    fmt.Sprintf("%s-%s-", ex.Name, name),
+			Name:            fmt.Sprintf("%s-%s", ex.Name, name),
 			Namespace:       ex.Namespace,
 			OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(ex, controllerKind)},
 		},
@@ -151,6 +151,34 @@ func TestAnalysisRunCreateError(t *testing.T) {
 	assert.Contains(t, patchedEx.Status.AnalysisRuns[0].Message, "intentional error")
 }
 
+// TestAnalysisRunCreateCollisionSemanticallyEqual verifies can claim an existing analysis run if it
+// is semantically equal.
+func TestAnalysisRunCreateCollisionSemanticallyEqual(t *testing.T) {
+	templates := generateTemplates("bar")
+	aTemplates := generateAnalysisTemplates("success-rate")
+	e := newExperiment("foo", templates, nil)
+	e.Spec.Analyses = []v1alpha1.ExperimentAnalysisTemplateRef{
+		{
+			Name:         "success-rate",
+			TemplateName: aTemplates[0].Name,
+		},
+	}
+	e.Status.Status = v1alpha1.AnalysisStatusRunning
+	e.Status.AvailableAt = now()
+	rs := templateToRS(e, templates[0], 1)
+	ar := analysisTemplateToRun("success-rate", e, &aTemplates[0].Spec)
+
+	f := newFixture(t, e, rs, &aTemplates[0], ar)
+	defer f.Close()
+
+	f.expectCreateAnalysisRunAction(ar) // fails do to AlreadyExists
+	f.expectGetAnalysisRunAction(ar)    // verifies it is semantically equal
+	patchIdx := f.expectPatchExperimentAction(e)
+	f.run(getKey(e, t))
+	patchedEx := f.getPatchedExperimentAsObj(patchIdx)
+	assert.Equal(t, v1alpha1.AnalysisStatusPending, patchedEx.Status.AnalysisRuns[0].Status)
+}
+
 func TestAnalysisRunSuccessful(t *testing.T) {
 	templates := generateTemplates("bar")
 	e := newExperiment("foo", templates, nil)
@@ -164,7 +192,6 @@ func TestAnalysisRunSuccessful(t *testing.T) {
 	e.Status.AvailableAt = now()
 	rs := templateToRS(e, templates[0], 1)
 	ar := analysisTemplateToRun("success-rate", e, &v1alpha1.AnalysisTemplateSpec{})
-	ar.Name = ar.GenerateName + "abc123"
 	ar.Status = v1alpha1.AnalysisRunStatus{
 		Status: v1alpha1.AnalysisStatusSuccessful,
 	}
@@ -204,9 +231,7 @@ func TestAssessAnalysisRunStatusesAfterTemplateSuccess(t *testing.T) {
 	rs := templateToRS(e, templates[0], 0)
 	rs.Spec.Replicas = new(int32)
 	ar1 := analysisTemplateToRun("success-rate", e, &v1alpha1.AnalysisTemplateSpec{})
-	ar1.Name = ar1.GenerateName + "abc123"
 	ar2 := analysisTemplateToRun("latency", e, &v1alpha1.AnalysisTemplateSpec{})
-	ar2.Name = ar2.GenerateName + "abc123"
 
 	e.Status.TemplateStatuses = []v1alpha1.TemplateStatus{
 		{
@@ -298,9 +323,7 @@ func TestFailExperimentWhenAnalysisFails(t *testing.T) {
 	e.Status.AvailableAt = secondsAgo(60)
 	rs := templateToRS(e, templates[0], 1)
 	ar1 := analysisTemplateToRun("success-rate", e, &v1alpha1.AnalysisTemplateSpec{})
-	ar1.Name = ar1.GenerateName + "abc123"
 	ar2 := analysisTemplateToRun("latency", e, &v1alpha1.AnalysisTemplateSpec{})
-	ar2.Name = ar2.GenerateName + "abc123"
 
 	e.Status.TemplateStatuses = []v1alpha1.TemplateStatus{
 		generateTemplatesStatus("bar", 1, 1, v1alpha1.TemplateStatusRunning, now()),
@@ -386,7 +409,6 @@ func TestTerminateAnalysisRuns(t *testing.T) {
 	rs := templateToRS(e, templates[0], 0)
 	rs.Spec.Replicas = new(int32)
 	ar := analysisTemplateToRun("success-rate", e, &v1alpha1.AnalysisTemplateSpec{})
-	ar.Name = ar.GenerateName + "abc123"
 	ar.Status = v1alpha1.AnalysisRunStatus{
 		Status: v1alpha1.AnalysisStatusRunning,
 	}

experiments/controller_test.go

@@ -517,6 +517,12 @@ func (f *fixture) expectCreateAnalysisRunAction(r *v1alpha1.AnalysisRun) int {
 	return len
 }
 
+func (f *fixture) expectGetAnalysisRunAction(r *v1alpha1.AnalysisRun) int {
+	len := len(f.actions)
+	f.actions = append(f.actions, core.NewGetAction(schema.GroupVersionResource{Resource: "analysisruns"}, r.Namespace, r.Name))
+	return len
+}
+
 func (f *fixture) expectPatchAnalysisRunAction(r *v1alpha1.AnalysisRun) int {
 	len := len(f.actions)
 	f.actions = append(f.actions, core.NewPatchAction(schema.GroupVersionResource{Resource: "analysisruns"}, r.Namespace, r.Name, types.MergePatchType, nil))

experiments/experiment.go

@@ -337,13 +337,31 @@ func (ec *experimentContext) reconcileAnalysisRun(analysis v1alpha1.ExperimentAn
 	newStatus.Message = run.Status.Message
 }
 
+// createAnalysisRun creates the analysis run. If an existing runs exists with same name, and is
+// semantically equal, returns the existing one, otherwise errors
 func (ec *experimentContext) createAnalysisRun(analysis v1alpha1.ExperimentAnalysisTemplateRef) (*v1alpha1.AnalysisRun, error) {
 	analysisRunIf := ec.argoProjClientset.ArgoprojV1alpha1().AnalysisRuns(ec.ex.Namespace)
 	run, err := ec.newAnalysisRun(analysis, analysis.Arguments)
 	if err != nil {
 		return nil, err
 	}
-	return analysisRunIf.Create(run)
+	newRun, createErr := analysisRunIf.Create(run)
+	if createErr != nil {
+		if !k8serrors.IsAlreadyExists(createErr) {
+			return nil, createErr
+		}
+		existingRun, err := analysisRunIf.Get(run.Name, metav1.GetOptions{})
+		if err != nil {
+			return nil, err
+		}
+		controllerRef := metav1.GetControllerOf(existingRun)
+		if ec.ex.UID == controllerRef.UID && analysisutil.IsSemanticallyEqual(run.Spec, existingRun.Spec) {
+			ec.log.Infof("Claimed existing analysisrun %s", existingRun.Name)
+			return existingRun, nil
+		}
+		return nil, createErr
+	}
+	return newRun, nil
 }
 
 func (ec *experimentContext) calculateStatus() *v1alpha1.ExperimentStatus {
@@ -458,7 +476,7 @@ func (ec *experimentContext) newAnalysisRun(analysis v1alpha1.ExperimentAnalysis
 
 	ar := v1alpha1.AnalysisRun{
 		ObjectMeta: metav1.ObjectMeta{
-			GenerateName:    fmt.Sprintf("%s-%s-", ec.ex.Name, analysis.Name),
+			Name:            fmt.Sprintf("%s-%s", ec.ex.Name, analysis.Name),
 			Namespace:       ec.ex.Namespace,
 			OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(ec.ex, controllerKind)},
 		},

rollout/controller_test.go

@@ -52,7 +52,6 @@ const (
 				"observedGeneration": ""
 			}
 	}`
-	MockGeneratedNameSuffix = "abc123"
 )
 
 type fixture struct {

rollout/experiment_test.go

@@ -192,7 +192,6 @@ func TestRolloutExperimentProcessingDoNothing(t *testing.T) {
 
 	r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 1, 0, 1, false)
 	ex, _ := GetExperimentFromTemplate(r2, rs1, rs2)
-	ex.Name = fmt.Sprintf("%s-%s", ex.GenerateName, MockGeneratedNameSuffix)
 	r2.Status.Canary.CurrentExperiment = ex.Name
 	progressingCondition, _ := newProgressingCondition(conditions.ReplicaSetUpdatedReason, rs2)
 	conditions.SetRolloutCondition(&r2.Status, progressingCondition)
@@ -317,7 +316,6 @@ func TestRolloutExperimentScaleDownExtraExperiment(t *testing.T) {
 
 	r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 1, 1, 1, false)
 	ex, _ := GetExperimentFromTemplate(r2, rs1, rs2)
-	ex.Name = fmt.Sprintf("%s-%s", ex.GenerateName, MockGeneratedNameSuffix)
 	r2.Status.Canary.CurrentExperiment = ex.Name
 	extraExp := &v1alpha1.Experiment{
 		ObjectMeta: metav1.ObjectMeta{
@@ -372,7 +370,6 @@ func TestRolloutExperimentFinishedIncrementStep(t *testing.T) {
 	ex.Status.Status = v1alpha1.AnalysisStatusSuccessful
 	now := metav1.Now()
 	ex.Status.AvailableAt = &now
-	ex.Name = fmt.Sprintf("%s-%s", ex.GenerateName, MockGeneratedNameSuffix)
 	r2.Status.Canary.CurrentExperiment = ex.Name
 
 	f.rolloutLister = append(f.rolloutLister, r2)
@@ -502,7 +499,6 @@ func TestDeleteExperimentWithNoMatchingRS(t *testing.T) {
 	rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
 
 	ex, _ := GetExperimentFromTemplate(r2, rs1, rs2)
-	ex.Name = fmt.Sprintf("%s-%s", ex.GenerateName, MockGeneratedNameSuffix)
 	ex.Status.Status = v1alpha1.AnalysisStatusSuccessful
 	r2.Status.Canary.CurrentExperiment = ex.Name
 	exWithNoMatchingPodHash := ex.DeepCopy()