fix(experiments): move recorder event to after experiment reconciliat…

…ion, fixes #4021 (#4022) * fix: move recorder event to after experiment reconcilation, fixes #4021 Signed-off-by: Roy Arnon <[email protected]> * Add Taboola to the user list --------- Signed-off-by: Roy Arnon <[email protected]> Co-authored-by: Zach Aller <[email protected]>
argoproj · Jan 8, 2025 · c0ca530 · c0ca530
1 parent 07c1028
commit c0ca530
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 10 deletions.
diff --git a/USERS.md b/USERS.md
@@ -52,6 +52,7 @@ Organizations below are **officially** using Argo Rollouts. Please send a PR wit
 1. [Skillz](https://www.skillz.com)
 1. [Spotify](https://www.spotify.com/)
 1. [Synamedia](https://www.synamedia.com)
+1. [Taboola](https://www.taboola.com)
 1. [TBC Bank](https://tbcbank.ge/)
 1. [Trustly](https://www.trustly.com/)
 1. [Tuhu](https://www.tuhu.cn/)

diff --git a/experiments/controller.go b/experiments/controller.go
@@ -5,6 +5,8 @@ import (
 	"sync"
 	"time"
 
+	corev1 "k8s.io/api/core/v1"
+
 	log "github.com/sirupsen/logrus"
 	appsv1 "k8s.io/api/apps/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
@@ -318,6 +320,7 @@ func (ec *Controller) syncHandler(ctx context.Context, key string) error {
 }
 
 func (ec *Controller) persistExperimentStatus(orig *v1alpha1.Experiment, newStatus *v1alpha1.ExperimentStatus) error {
+	prevStatus := orig.Status
 	ctx := context.TODO()
 	logCtx := logutil.WithExperiment(orig)
 	patch, modified, err := diff.CreateTwoWayMergePatch(
@@ -336,15 +339,27 @@ func (ec *Controller) persistExperimentStatus(orig *v1alpha1.Experiment, newStat
 		return nil
 	}
 	logCtx.Debugf("Experiment Patch: %s", patch)
-	_, err = ec.argoProjClientset.ArgoprojV1alpha1().Experiments(orig.Namespace).Patch(ctx, orig.Name, patchtypes.MergePatchType, patch, metav1.PatchOptions{})
+	patched, err := ec.argoProjClientset.ArgoprojV1alpha1().Experiments(orig.Namespace).Patch(ctx, orig.Name, patchtypes.MergePatchType, patch, metav1.PatchOptions{})
 	if err != nil {
 		logCtx.Warningf("Error updating experiment: %v", err)
 		return err
 	}
 	logCtx.Info("Patch status successfully")
+	ec.recordEvent(patched, prevStatus, newStatus)
 	return nil
 }
 
+func (ec *Controller) recordEvent(ex *v1alpha1.Experiment, prevStatus v1alpha1.ExperimentStatus, newStatus *v1alpha1.ExperimentStatus) {
+	if prevStatus.Phase != newStatus.Phase {
+		eventType := corev1.EventTypeNormal
+		switch newStatus.Phase {
+		case v1alpha1.AnalysisPhaseError, v1alpha1.AnalysisPhaseFailed, v1alpha1.AnalysisPhaseInconclusive:
+			eventType = corev1.EventTypeWarning
+		}
+		ec.recorder.Eventf(ex, record.EventOptions{EventType: eventType, EventReason: "Experiment" + string(newStatus.Phase)}, "Experiment transitioned from %s -> %s", prevStatus.Phase, newStatus.Phase)
+	}
+}
+
 // enqueueIfCompleted conditionally enqueues the AnalysisRun's Experiment if the run is complete
 func (ec *Controller) enqueueIfCompleted(obj any) {
 	run := unstructuredutil.ObjectToAnalysisRun(obj)

diff --git a/experiments/experiment.go b/experiments/experiment.go
@@ -523,7 +523,6 @@ func (ec *experimentContext) ResolveAnalysisRunArgs(args []v1alpha1.Argument) ([
 }
 
 func (ec *experimentContext) calculateStatus() *v1alpha1.ExperimentStatus {
-	prevStatus := ec.newStatus.DeepCopy()
 	switch ec.newStatus.Phase {
 	case "":
 		ec.newStatus.Phase = v1alpha1.AnalysisPhasePending
@@ -568,14 +567,6 @@ func (ec *experimentContext) calculateStatus() *v1alpha1.ExperimentStatus {
 		}
 	}
 	ec.newStatus = calculateExperimentConditions(ec.ex, *ec.newStatus)
-	if prevStatus.Phase != ec.newStatus.Phase {
-		eventType := corev1.EventTypeNormal
-		switch ec.newStatus.Phase {
-		case v1alpha1.AnalysisPhaseError, v1alpha1.AnalysisPhaseFailed, v1alpha1.AnalysisPhaseInconclusive:
-			eventType = corev1.EventTypeWarning
-		}
-		ec.recorder.Eventf(ec.ex, record.EventOptions{EventType: eventType, EventReason: "Experiment" + string(ec.newStatus.Phase)}, "Experiment transitioned from %s -> %s", prevStatus.Phase, ec.newStatus.Phase)
-	}
 	return ec.newStatus
 }