Use GenerateName when creating experiment (#194)

manifests/crds/rollout-crd.yaml

@@ -2642,6 +2642,8 @@ spec:
               properties:
                 currentBackgroundAnalysisRun:
                   type: string
+                currentExperiment:
+                  type: string
                 currentStepAnalysisRun:
                   type: string
                 experimentFailed:

manifests/install.yaml

@@ -7577,6 +7577,8 @@ spec:
               properties:
                 currentBackgroundAnalysisRun:
                   type: string
+                currentExperiment:
+                  type: string
                 currentStepAnalysisRun:
                   type: string
                 experimentFailed:

manifests/namespace-install.yaml

@@ -7577,6 +7577,8 @@ spec:
               properties:
                 currentBackgroundAnalysisRun:
                   type: string
+                currentExperiment:
+                  type: string
                 currentStepAnalysisRun:
                   type: string
                 experimentFailed:

pkg/apis/rollouts/v1alpha1/types.go

@@ -341,6 +341,8 @@ type CanaryStatus struct {
 	CurrentStepAnalysisRun string `json:"currentStepAnalysisRun,omitempty"`
 	// CurrentBackgroundAnalysisRun indicates the analysisRun for the Background step
 	CurrentBackgroundAnalysisRun string `json:"currentBackgroundAnalysisRun,omitempty"`
+	// CurrentExperiment indicates the running experiment
+	CurrentExperiment string `json:"currentExperiment,omitempty"`
 }
 
 // RolloutConditionType defines the conditions of Rollout

rollout/canary.go

@@ -59,7 +59,8 @@ func (c *RolloutController) rolloutCanary(rollout *v1alpha1.Rollout, rsList []*a
 	}
 
 	logCtx.Info("Reconciling Experiment step")
-	if err := c.reconcileExperiments(rollout, stableRS, newRS, currentEx, otherExs); err != nil {
+	currentEx, err = c.reconcileExperiments(rollout, stableRS, newRS, currentEx, otherExs)
+	if err != nil {
 		return err
 	}
 
@@ -303,8 +304,11 @@ func (c *RolloutController) syncRolloutStatusCanary(olderRSs []*appsv1.ReplicaSe
 			newStatus = c.calculateRolloutConditions(r, newStatus, allRSs, newRS, currExp, currArs)
 			return c.persistRolloutStatus(r, &newStatus, pointer.BoolPtr(false))
 		}
-		if currExp != nil && conditions.ExperimentTimeOut(currExp, currExp.Status) {
-			newStatus.Canary.ExperimentFailed = true
+		if currExp != nil {
+			newStatus.Canary.CurrentExperiment = currExp.Name
+			if conditions.ExperimentTimeOut(currExp, currExp.Status) {
+				newStatus.Canary.ExperimentFailed = true
+			}
 		}
 	}
 

rollout/controller_test.go

@@ -161,7 +161,8 @@ func newProgressingCondition(reason string, resourceObj runtime.Object) (v1alpha
 			msg = fmt.Sprintf(conditions.RolloutProgressingMessage, resource.Name)
 		}
 		if reason == conditions.RolloutExperimentFailedReason {
-			msg = fmt.Sprintf(conditions.RolloutExperimentFailedMessage, experimentutil.ExperimentNameFromRollout(resource), resource.Name)
+			exName := fmt.Sprintf("%s%s", experimentutil.ExperimentGeneratedNameFromRollout(resource), MockGeneratedNameSuffix)
+			msg = fmt.Sprintf(conditions.RolloutExperimentFailedMessage, exName, resource.Name)
 			status = corev1.ConditionFalse
 		}
 		if reason == conditions.RolloutAnalysisRunFailedReason {
@@ -418,6 +419,19 @@ func (f *fixture) newController(resync resyncFunc) (*RolloutController, informer
 		return true, ar.DeepCopyObject(), nil
 	})
 
+	f.client.PrependReactor("create", "experiments", func(action core.Action) (bool, runtime.Object, error) {
+		createAction, ok := action.(core.CreateAction)
+		if !ok {
+			assert.Fail(f.t, "Expected Created action, not %s", action.GetVerb())
+		}
+		ex := &v1alpha1.Experiment{}
+		converter := runtime.NewTestUnstructuredConverter(equality.Semantic)
+		objMap, _ := converter.ToUnstructured(createAction.GetObject())
+		runtime.NewTestUnstructuredConverter(equality.Semantic).FromUnstructured(objMap, ex)
+		ex.Name = ex.GenerateName + MockGeneratedNameSuffix
+		return true, ex.DeepCopyObject(), nil
+	})
+
 	return c, i, k8sI
 }
 
@@ -731,6 +745,20 @@ func (f *fixture) getCreatedAnalysisRun(index int) *v1alpha1.AnalysisRun {
 	return ar
 }
 
+func (f *fixture) getCreatedExperiment(index int) *v1alpha1.Experiment {
+	action := filterInformerActions(f.client.Actions())[index]
+	createAction, ok := action.(core.CreateAction)
+	if !ok {
+		f.t.Fatalf("Expected Patch action, not %s", action.GetVerb())
+	}
+	obj := createAction.GetObject()
+	ex := &v1alpha1.Experiment{}
+	converter := runtime.NewTestUnstructuredConverter(equality.Semantic)
+	objMap, _ := converter.ToUnstructured(obj)
+	runtime.NewTestUnstructuredConverter(equality.Semantic).FromUnstructured(objMap, ex)
+	return ex
+}
+
 func (f *fixture) getPatchedExperiment(index int) *v1alpha1.Experiment {
 	action := filterInformerActions(f.client.Actions())[index]
 	patchAction, ok := action.(core.PatchAction)

rollout/experiment.go

@@ -33,7 +33,7 @@ func GetExperimentFromTemplate(r *v1alpha1.Rollout, stableRS, newRS *appsv1.Repl
 	}
 	experiment := &v1alpha1.Experiment{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:            experimentutil.ExperimentNameFromRollout(r),
+			GenerateName:    experimentutil.ExperimentGeneratedNameFromRollout(r),
 			Namespace:       r.Namespace,
 			OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(r, controllerKind)},
 		},
@@ -91,7 +91,6 @@ func (c *RolloutController) getExperimentsForRollout(rollout *v1alpha1.Rollout)
 	if err != nil {
 		return nil, err
 	}
-	//TODO(dthomson) consider adoption
 	ownedByRollout := make([]*v1alpha1.Experiment, 0)
 	for i := range experiments {
 		e := experiments[i]
@@ -103,41 +102,41 @@ func (c *RolloutController) getExperimentsForRollout(rollout *v1alpha1.Rollout)
 	return ownedByRollout, nil
 }
 
-func (c *RolloutController) reconcileExperiments(rollout *v1alpha1.Rollout, stableRS, newRS *appsv1.ReplicaSet, currentEx *v1alpha1.Experiment, otherExs []*v1alpha1.Experiment) error {
+func (c *RolloutController) reconcileExperiments(rollout *v1alpha1.Rollout, stableRS, newRS *appsv1.ReplicaSet, currentEx *v1alpha1.Experiment, otherExs []*v1alpha1.Experiment) (*v1alpha1.Experiment, error) {
 	logCtx := logutil.WithRollout(rollout)
 	for i := range otherExs {
 		otherEx := otherExs[i]
 		if otherEx.Status.Running != nil && *otherEx.Status.Running {
 			logCtx.Infof("Canceling other running experiment '%s' owned by rollout", otherEx.Name)
 			_, err := c.argoprojclientset.ArgoprojV1alpha1().Experiments(otherEx.Namespace).Patch(otherEx.Name, patchtypes.MergePatchType, []byte(cancelExperimentPatch))
 			if err != nil {
-				return err
+				return nil, err
 			}
 		}
 	}
 
 	step, _ := replicasetutil.GetCurrentCanaryStep(rollout)
 	if step == nil || step.Experiment == nil {
-		return nil
+		return nil, nil
 	}
 	if currentEx == nil {
 		// An new experiment can not be created if the newRS or stableRS is not created yet
 		if newRS == nil || stableRS == nil {
 			logCtx.Infof("Cannot create experiment until newRS and stableRS both exist")
-			return nil
+			return nil, nil
 		}
 
 		newEx, err := GetExperimentFromTemplate(rollout, stableRS, newRS)
 		if err != nil {
-			return err
+			return nil, err
 		}
 
-		newEx, err = c.argoprojclientset.ArgoprojV1alpha1().Experiments(newEx.Namespace).Create(newEx)
+		currentEx, err = c.argoprojclientset.ArgoprojV1alpha1().Experiments(newEx.Namespace).Create(newEx)
 		if err != nil {
-			return err
+			return nil, err
 		}
 		msg := fmt.Sprintf("Created Experiment '%s'", newEx.Name)
 		c.recorder.Event(rollout, corev1.EventTypeNormal, "CreateExperiment", msg)
 	}
-	return nil
+	return currentEx, nil
 }

rollout/experiment_test.go

@@ -38,15 +38,28 @@ func TestRolloutCreateExperiment(t *testing.T) {
 	rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
 
 	ex, _ := GetExperimentFromTemplate(r2, rs1, rs2)
-	r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 1, 1, 1, false)
+	r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 1, 0, 1, false)
 
 	f.rolloutLister = append(f.rolloutLister, r2)
 	f.objects = append(f.objects, r2)
 
-	f.expectCreateExperimentAction(ex)
-	f.expectPatchRolloutAction(r1)
+	createExIndex := f.expectCreateExperimentAction(ex)
+	patchIndex := f.expectPatchRolloutAction(r1)
 
 	f.run(getKey(r2, t))
+	createdEx := f.getCreatedExperiment(createExIndex)
+	assert.Equal(t, createdEx.GenerateName, ex.GenerateName)
+	patch := f.getPatchedRollout(patchIndex)
+	expectedPatch := `{
+		"status": {
+			"canary": {
+				"currentExperiment": "%s%s"
+			},
+			"conditions": %s
+		}
+	}`
+	conds := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false)
+	assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, ex.GenerateName, MockGeneratedNameSuffix, conds)), patch)
 }
 
 func TestRolloutExperimentProcessingDoNothing(t *testing.T) {
@@ -68,13 +81,23 @@ 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)
+	availableCondition, _ := newAvailableCondition(true)
+	conditions.SetRolloutCondition(&r2.Status, availableCondition)
 
 	f.rolloutLister = append(f.rolloutLister, r2)
 	f.experimentLister = append(f.experimentLister, ex)
 	f.objects = append(f.objects, r2, ex)
 
-	f.expectPatchRolloutAction(r1)
+	patchIndex := f.expectPatchRolloutAction(r1)
 	f.run(getKey(r2, t))
+
+	patch := f.getPatchedRollout(patchIndex)
+	assert.Equal(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch)
+
 }
 
 func TestRolloutDegradedExperimentEnterDegraded(t *testing.T) {
@@ -100,6 +123,8 @@ func TestRolloutDegradedExperimentEnterDegraded(t *testing.T) {
 		Type:   v1alpha1.ExperimentProgressing,
 		Reason: conditions.TimedOutReason,
 	}}
+	ex.Name = fmt.Sprintf("%s%s", ex.GenerateName, MockGeneratedNameSuffix)
+	r2.Status.Canary.CurrentExperiment = ex.Name
 
 	f.rolloutLister = append(f.rolloutLister, r2)
 	f.experimentLister = append(f.experimentLister, ex)
@@ -140,6 +165,8 @@ 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{
 			Name:            "extraExp",
@@ -194,6 +221,8 @@ func TestRolloutExperimentFinishedIncrementStep(t *testing.T) {
 	ex.Status.Running = pointer.BoolPtr(false)
 	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)
 	f.experimentLister = append(f.experimentLister, ex)
@@ -204,6 +233,9 @@ func TestRolloutExperimentFinishedIncrementStep(t *testing.T) {
 	patch := f.getPatchedRollout(patchIndex)
 	expectedPatch := `{
 		"status": {
+			"canary": {
+				"currentExperiment":null
+			},
 			"currentStepIndex": 1,
 			"conditions": %s
 		}
@@ -283,43 +315,6 @@ func TestRolloutDoNotCreateExperimentWithoutStableRS(t *testing.T) {
 	f.run(getKey(r2, t))
 }
 
-// func TestExperimentCancelCurrentExperimentOnStepChange(t *testing.T) {
-// 	f := newFixture(t)
-// 	defer f.Close()
-
-// 	steps := []v1alpha1.CanaryStep{
-// 		{
-// 			Experiment: &v1alpha1.RolloutExperimentStep{},
-// 		},
-// 		{
-// 			SetWeight: pointer.Int32Ptr(30),
-// 		},
-// 	}
-
-// 	r1 := newCanaryRollout("foo", 1, nil, steps, pointer.Int32Ptr(1), intstr.FromInt(0), intstr.FromInt(1))
-// 	r2 := bumpVersion(r1)
-
-// 	rs1 := newReplicaSetWithStatus(r1, 1, 1)
-// 	rs2 := newReplicaSetWithStatus(r2, 0, 0)
-// 	f.kubeobjects = append(f.kubeobjects, rs1, rs2)
-// 	f.replicaSetLister = append(f.replicaSetLister, rs1, rs2)
-// 	rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
-
-// 	r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 1, 1, 1, false)
-// 	exToCancel, _ := GetExperimentFromTemplate(r2, rs1, rs2)
-
-// 	f.rolloutLister = append(f.rolloutLister, r2)
-// 	f.experimentLister = append(f.experimentLister, exToCancel)
-// 	f.objects = append(f.objects, r2, exToCancel)
-
-// 	exPatchIndex := f.expectPatchExperimentAction(exToCancel)
-// 	f.expectPatchRolloutAction(r2)
-// 	f.run(getKey(r2, t))
-// 	exPatch := f.getPatchedExperiment(exPatchIndex)
-// 	assert.NotNil(t, exPatch.Status.Running)
-// 	assert.False(t, *exPatch.Status.Running)
-// }
-
 func TestGetExperimentFromTemplate(t *testing.T) {
 	steps := []v1alpha1.CanaryStep{{
 		Experiment: &v1alpha1.RolloutExperimentStep{
@@ -369,6 +364,3 @@ func TestGetExperimentFromTemplate(t *testing.T) {
 	assert.Nil(t, err)
 
 }
-
-func TestRolloutExperimentNoCreateWithoutStableOrNewRs(t *testing.T) {
-}