fix: move recorder event to after experiment reconcilation, fixes arg…

…oproj#4021 Signed-off-by: Roy Arnon <[email protected]>
Hellspam · Dec 22, 2024 · 9197bd5 · 9197bd5
1 parent 8a800a8
commit 9197bd5
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 10 deletions.
diff --git a/USERS.md b/USERS.md
@@ -61,3 +61,4 @@ Organizations below are **officially** using Argo Rollouts. Please send a PR wit
 1. [WeLab Bank](https://www.welab.bank/)
 1. [Yotpo](https://www.yotpo.com/)
 1. [VGS](https://www.vgs.io)
+1. [Taboola](https://www.taboola.com)
diff --git a/experiments/controller.go b/experiments/controller.go
@@ -2,6 +2,7 @@ package experiments
 
 import (
 	"context"
+	corev1 "k8s.io/api/core/v1"
 	"sync"
 	"time"
 
@@ -318,6 +319,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 +338,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
 }