fix: do not try to update actionset state when publishing artifacts (#…

…2883)

If there are multiple actions running in parallel, they will populate
artifacts while another action is still running.
This causes a conflict in update of actionset state.

Actionset state should be updated by separate function maybeSetActionSetStateComplete

Fixes #2882

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

pkg/controller/controller.go

@@ -469,7 +469,8 @@ func (c *Controller) runAction(ctx context.Context, t *tomb.Tomb, as *crv1alpha1
 			}
 			// render artifacts only if all the phases are run successfully
 			if deferErr == nil && coreErr == nil {
-				c.renderActionsetArtifacts(ctx, as, aIDX, as.Namespace, as.Name, action.Name, bp, tp, coreErr, deferErr)
+				c.renderActionsetArtifacts(ctx, as, aIDX, as.Namespace, as.Name, action.Name, bp, tp)
+				c.maybeSetActionSetStateComplete(ctx, as, aIDX, bp, coreErr, deferErr)
 				c.incrementActionSetResolutionCounterVec(ACTION_SET_COUNTER_VEC_LABEL_RES_SUCCESS)
 			} else {
 				c.incrementActionSetResolutionCounterVec(ACTION_SET_COUNTER_VEC_LABEL_RES_FAILURE)
@@ -639,23 +640,13 @@ func (c *Controller) renderActionsetArtifacts(ctx context.Context,
 	actionsetNS, actionsetName, actionName string,
 	bp *crv1alpha1.Blueprint,
 	tp *param.TemplateParams,
-	coreErr, deferErr error,
 ) {
 	// Check if output artifacts are present
 	artTpls := as.Status.Actions[aIDX].Artifacts
 	if len(artTpls) == 0 {
-		// No artifacts, set ActionSetStatus to complete
-		if rErr := reconcile.ActionSet(ctx, c.crClient.CrV1alpha1(), actionsetNS, actionsetName, func(ras *crv1alpha1.ActionSet) error {
-			// actionset execution is done, set the RunningPhase to empty string
-			ras.Status.Progress.RunningPhase = ""
-			if coreErr == nil && deferErr == nil {
-				ras.Status.State = crv1alpha1.StateComplete
-			} else {
-				ras.Status.State = crv1alpha1.StateFailed
-			}
-
-			return nil
-		}); rErr != nil {
+		// No artifacts, will only update action status if failed
+		af := updateActionSetArtifactsFun(aIDX, artTpls)
+		if rErr := reconcile.ActionSet(ctx, c.crClient.CrV1alpha1(), actionsetNS, actionsetName, af); rErr != nil {
 			reason := fmt.Sprintf("ActionSetFailed Action: %s", actionName)
 			msg := fmt.Sprintf("Failed to update ActionSet: %s", actionsetName)
 			c.logAndErrorEvent(ctx, msg, reason, rErr, as, bp)
@@ -675,19 +666,7 @@ func (c *Controller) renderActionsetArtifacts(ctx context.Context,
 			return nil
 		}
 	} else {
-		af = func(ras *crv1alpha1.ActionSet) error {
-			ras.Status.Actions[aIDX].Artifacts = arts
-			// actionset execution is done and artifacts are rendered, set the RunningPhase to empty string
-			ras.Status.Progress.RunningPhase = ""
-			// make sure that the core phases that were run also didnt return any error
-			// and then set actionset's state to be complete
-			if coreErr == nil && deferErr == nil {
-				ras.Status.State = crv1alpha1.StateComplete
-			} else {
-				ras.Status.State = crv1alpha1.StateFailed
-			}
-			return nil
-		}
+		af = updateActionSetArtifactsFun(aIDX, arts)
 	}
 	// Update ActionSet
 	if aErr := reconcile.ActionSet(ctx, c.crClient.CrV1alpha1(), actionsetNS, actionsetName, af); aErr != nil {
@@ -705,6 +684,53 @@ func (c *Controller) renderActionsetArtifacts(ctx context.Context,
 	}
 }
 
+func (c *Controller) maybeSetActionSetStateComplete(ctx context.Context,
+	as *crv1alpha1.ActionSet,
+	aIDX int,
+	bp *crv1alpha1.Blueprint,
+	coreErr, deferErr error,
+) {
+	af := func(ras *crv1alpha1.ActionSet) error {
+		// Running phase is in the current action, TODO: #2270 track multiple phases
+		if isPhaseInAction(ras.Status.Progress.RunningPhase, ras.Status.Actions[aIDX]) {
+			// set the RunningPhase to empty string
+			ras.Status.Progress.RunningPhase = ""
+		}
+
+		// make sure that the core phases that were run also didnt return any error
+		// and then set actionset's state to be complete
+		if coreErr != nil || deferErr != nil {
+			ras.Status.State = crv1alpha1.StateFailed
+			return nil
+		}
+
+		for _, as := range ras.Status.Actions {
+			for _, p := range as.Phases {
+				if p.State != crv1alpha1.StateComplete {
+					log.WithContext(ctx).Print(
+						"Finished action, but other action's phase is still running. Not setting state to complete.",
+						field.M{
+							"Status": ras.Status.State,
+							"Action": ras.Status.Actions[aIDX].Name,
+							"Phase":  fmt.Sprintf("%s->%s", p.Name, p.State)})
+					return nil
+				}
+			}
+		}
+
+		// Set state to complete if it wasn't failed already
+		if ras.Status.State != crv1alpha1.StateFailed {
+			ras.Status.State = crv1alpha1.StateComplete
+		}
+		return nil
+	}
+	if rErr := reconcile.ActionSet(ctx, c.crClient.CrV1alpha1(), as.Namespace, as.Name, af); rErr != nil {
+		reason := fmt.Sprintf("ActionSetFailed Action: %s", as.Status.Actions[aIDX].Name)
+		msg := fmt.Sprintf("Failed to update ActionSet: %s", as.Name)
+		c.logAndErrorEvent(ctx, msg, reason, rErr, as, bp)
+	}
+}
+
 func (c *Controller) logAndErrorEvent(ctx context.Context, msg, reason string, err error, objects ...runtime.Object) {
 	log.WithContext(ctx).WithError(err).Print(msg)
 	if len(objects) == 0 {
@@ -745,3 +771,19 @@ func setObjectKind(obj runtime.Object) {
 	}
 	ok.SetGroupVersionKind(gvk)
 }
+
+func updateActionSetArtifactsFun(aIDX int, arts map[string]crv1alpha1.Artifact) func(*crv1alpha1.ActionSet) error {
+	return func(ras *crv1alpha1.ActionSet) error {
+		ras.Status.Actions[aIDX].Artifacts = arts
+		return nil
+	}
+}
+
+func isPhaseInAction(phaseName string, actionStatus crv1alpha1.ActionStatus) bool {
+	for _, p := range actionStatus.Phases {
+		if p.Name == phaseName {
+			return true
+		}
+	}
+	return actionStatus.DeferPhase.Name == phaseName
+}