Refactor ActionSet Reconciliation Mechanism

[ihcsim]

Following #1297 (comment), it is becoming harder to reason about the multiple places where the actionset Status.State are set and reconcile.ActionSet() are called, within the controller.runAction() method.

We should look into refactoring these code, with the following changes:

i. Call reconcile.ActionSet() just once. Right now we try to reconcile the actionset in every iteration of the for loop
i. Within the same for loop, we store only the state of each phase in memory. The state of the actionset should be set only once per the next bullet point
i. Set as.Status.State and as.Status.ErState.Error only once, based on whether coreErr and deferErr are nil or not. It feels like because we are setting them in different places, we need to have some "final overrides" in the controll.errenderActionsetArtifacts() method.

How to test:

1. All CI tests should pass
2. Manual testing with blueprints that include error phase and defer phase

This diff conveys the scope and goal of the refactoring effort:

diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
index aa16f6a5..51d10f10 100644
--- a/pkg/controller/controller.go
+++ b/pkg/controller/controller.go
@@ -383,7 +383,7 @@ func (c *Controller) handleActionSet(as *crv1alpha1.ActionSet) (err error) {
 
 // nolint:gocognit
 func (c *Controller) runAction(ctx context.Context, as *crv1alpha1.ActionSet, aIDX int) error {
-	action := as.Spec.Actions[aIDX]
+	action := *as.Spec.Actions[aIDX].DeepCopy()
 	c.logAndSuccessEvent(ctx, fmt.Sprintf("Executing action %s", action.Name), "Started Action", as)
 	bpName := as.Spec.Actions[aIDX].Blueprint
 	bp, err := c.crClient.CrV1alpha1().Blueprints(as.GetNamespace()).Get(ctx, bpName, v1.GetOptions{})
@@ -414,65 +414,85 @@ func (c *Controller) runAction(ctx context.Context, as *crv1alpha1.ActionSet, aI
 	t.Go(func() error {
 		var coreErr error
 		defer func() {
+			// always run deferPhase after normal phases exit
 			var deferErr error
 			if deferPhase != nil {
 				deferErr = c.executeDeferPhase(ctx, deferPhase, tp, bp, action.Name, aIDX, as)
 			}
-			c.renderActionsetArtifacts(ctx, as, aIDX, ns, name, action.Name, bp, tp, coreErr, deferErr)
-		}()
 
-		for i, p := range phases {
-			ctx = field.Context(ctx, consts.PhaseNameKey, p.Name())
-			c.logAndSuccessEvent(ctx, fmt.Sprintf("Executing phase %s", p.Name()), "Started Phase", as)
-			err = param.InitPhaseParams(ctx, c.clientset, tp, p.Name(), p.Objects())
-			var output map[string]interface{}
-			var msg string
-			if err == nil {
-				output, err = p.Exec(ctx, *bp, action.Name, *tp)
-			} else {
-				msg = fmt.Sprintf("Failed to init phase params: %#v:", as.Status.Actions[aIDX].Phases[i])
+			if coreErr == nil && deferErr == nil && len(as.Status.Actions[aIDX].Artifacts) > 0 {
+				coreErr = c.renderActionsetArtifacts(ctx, as, aIDX, ns, name, action.Name, bp, tp, coreErr, deferErr)
 			}
-			var rf func(*crv1alpha1.ActionSet) error
-			if err != nil {
-				coreErr = err
-				rf = func(ras *crv1alpha1.ActionSet) error {
+
+			// non-nil coreErr will always be added to the actionset status because
+			// it is likely to be the root cause of the actionset failure.
+			var f func(ras *crv1alpha1.ActionSet) error
+			if coreErr != nil {
+				f = func(ras *crv1alpha1.ActionSet) error {
+					ras.Status.State = crv1alpha1.StateFailed
+					ras.Status.Error = crv1alpha1.Error{
+						Message: coreErr.Error(),
+					}
+					return nil
+				}
+			} else if deferErr != nil {
+				f = func(ras *crv1alpha1.ActionSet) error {
 					ras.Status.State = crv1alpha1.StateFailed
 					ras.Status.Error = crv1alpha1.Error{
-						Message: err.Error(),
+						Message: deferErr.Error(),
 					}
-					ras.Status.Actions[aIDX].Phases[i].State = crv1alpha1.StateFailed
 					return nil
 				}
 			} else {
-				coreErr = nil
-				rf = func(ras *crv1alpha1.ActionSet) error {
-					ras.Status.Actions[aIDX].Phases[i].State = crv1alpha1.StateComplete
-					// this updates the phase output in the actionset status
-					ras.Status.Actions[aIDX].Phases[i].Output = output
+				f = func(ras *crv1alpha1.ActionSet) error {
+					ras.Status.State = crv1alpha1.StateComplete
 					return nil
 				}
 			}
 
-			if rErr := reconcile.ActionSet(ctx, c.crClient.CrV1alpha1(), ns, name, rf); rErr != nil {
+			if err := reconcile.ActionSet(ctx, c.crClient.CrV1alpha1(), ns, name, f); err != nil {
+				reason := fmt.Sprintf("ActionSetFailed Action: %s", action.Name)
+				msg := fmt.Sprintf("Failed to update ActionSet: %s:", name)
+				c.logAndErrorEvent(ctx, msg, reason, err, as, bp)
+				return
+			}
+		}()
+
+		for i, p := range phases {
+			ctx = field.Context(ctx, consts.PhaseNameKey, p.Name())
+			c.logAndSuccessEvent(ctx, fmt.Sprintf("Executing phase %s", p.Name()), "Started Phase", as)
+
+			if err := param.InitPhaseParams(ctx, c.clientset, tp, p.Name(), p.Objects()); err != nil {
+				as.Status.Actions[aIDX].Phases[i].State = crv1alpha1.StateFailed
+				coreErr = err
+
 				reason := fmt.Sprintf("ActionSetFailed Action: %s", as.Spec.Actions[aIDX].Name)
-				msg := fmt.Sprintf("Failed to update phase: %#v:", as.Status.Actions[aIDX].Phases[i])
-				c.logAndErrorEvent(ctx, msg, reason, rErr, as, bp)
-				coreErr = rErr
-				return nil
+				msg := fmt.Sprintf("Failed to init phase params: %#v:", as.Status.Actions[aIDX].Phases[i])
+				c.logAndErrorEvent(ctx, msg, reason, err, as, bp)
+				break
 			}
 
+			var (
+				output map[string]interface{}
+				msg    string
+			)
+			output, err := p.Exec(ctx, *bp, action.Name, *tp)
 			if err != nil {
+				as.Status.Actions[aIDX].Phases[i].State = crv1alpha1.StateFailed
+				coreErr = err
+
 				reason := fmt.Sprintf("ActionSetFailed Action: %s", as.Spec.Actions[aIDX].Name)
-				if msg == "" {
-					msg = fmt.Sprintf("Failed to execute phase: %#v:", as.Status.Actions[aIDX].Phases[i])
-				}
+				msg = fmt.Sprintf("Failed to execute phase: %#v:", as.Status.Actions[aIDX].Phases[i])
 				c.logAndErrorEvent(ctx, msg, reason, err, as, bp)
-				coreErr = err
-				return nil
+				break
 			}
+
+			as.Status.Actions[aIDX].Phases[i].State = crv1alpha1.StateComplete
+			as.Status.Actions[aIDX].Phases[i].Output = output
 			param.UpdatePhaseParams(ctx, tp, p.Name(), output)
 			c.logAndSuccessEvent(ctx, fmt.Sprintf("Completed phase %s", p.Name()), "Ended Phase", as)
 		}
+
 		return nil
 	})
 	return nil
@@ -491,47 +511,24 @@ func (c *Controller) executeDeferPhase(ctx context.Context,
 	aIDX int,
 	as *crv1alpha1.ActionSet,
 ) error {
-	actionsetName, actionsetNS := as.GetName(), as.GetNamespace()
 	ctx = field.Context(ctx, consts.PhaseNameKey, as.Status.Actions[aIDX].DeferPhase.Name)
 	c.logAndSuccessEvent(ctx, fmt.Sprintf("Executing deferPhase %s", as.Status.Actions[aIDX].DeferPhase.Name), "Started deferPhase", as)
 
 	output, err := deferPhase.Exec(context.Background(), *bp, actionName, *tp)
-	var rf func(*crv1alpha1.ActionSet) error
 	if err != nil {
-		rf = func(as *crv1alpha1.ActionSet) error {
-			as.Status.State = crv1alpha1.StateFailed
-			as.Status.Error = crv1alpha1.Error{
-				Message: err.Error(),
-			}
-			as.Status.Actions[aIDX].DeferPhase.State = crv1alpha1.StateFailed
-			return nil
-		}
-	} else {
-		rf = func(as *crv1alpha1.ActionSet) error {
-			as.Status.Actions[aIDX].DeferPhase.State = crv1alpha1.StateComplete
-			as.Status.Actions[aIDX].DeferPhase.Output = output
-			return nil
-		}
-	}
-	var msg string
-	if rErr := reconcile.ActionSet(context.Background(), c.crClient.CrV1alpha1(), actionsetNS, actionsetName, rf); rErr != nil {
-		reason := fmt.Sprintf("ActionSetFailed Action: %s", as.Spec.Actions[aIDX].Name)
-		msg := fmt.Sprintf("Failed to update defer phase: %#v:", as.Status.Actions[aIDX].DeferPhase)
-		c.logAndErrorEvent(ctx, msg, reason, rErr, as, bp)
-		return rErr
-	}
+		as.Status.Actions[aIDX].DeferPhase.State = crv1alpha1.StateFailed
 
-	if err != nil {
 		reason := fmt.Sprintf("ActionSetFailed Action: %s", as.Spec.Actions[aIDX].Name)
-		if msg == "" {
-			msg = fmt.Sprintf("Failed to execute defer phase: %#v:", as.Status.Actions[aIDX].DeferPhase)
-		}
+		msg := fmt.Sprintf("Failed to execute defer phase: %#v:", as.Status.Actions[aIDX].DeferPhase)
 		c.logAndErrorEvent(ctx, msg, reason, err, as, bp)
-		return err
+	} else {
+		as.Status.Actions[aIDX].DeferPhase.State = crv1alpha1.StateComplete
+		as.Status.Actions[aIDX].DeferPhase.Output = output
+
+		c.logAndSuccessEvent(ctx, fmt.Sprintf("Completed deferPhase %s", as.Status.Actions[aIDX].DeferPhase.Name), "Ended deferPhase", as)
+		param.UpdateDeferPhaseParams(context.Background(), tp, output)
 	}
 
-	c.logAndSuccessEvent(ctx, fmt.Sprintf("Completed deferPhase %s", as.Status.Actions[aIDX].DeferPhase.Name), "Ended deferPhase", as)
-	param.UpdateDeferPhaseParams(context.Background(), tp, output)
 	return nil
 }
 
@@ -542,64 +539,16 @@ func (c *Controller) renderActionsetArtifacts(ctx context.Context,
 	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 {
-			if coreErr == nil && deferErr == nil {
-				ras.Status.State = crv1alpha1.StateComplete
-			} else {
-				ras.Status.State = crv1alpha1.StateFailed
-			}
-
-			return nil
-		}); 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)
-		}
-		return
-	}
+) error {
 	// Render the artifacts
+	artTpls := as.Status.Actions[aIDX].Artifacts
 	arts, err := param.RenderArtifacts(artTpls, *tp)
-	var af func(*crv1alpha1.ActionSet) error
 	if err != nil {
-		af = func(ras *crv1alpha1.ActionSet) error {
-			ras.Status.State = crv1alpha1.StateFailed
-			ras.Status.Error = crv1alpha1.Error{
-				Message: err.Error(),
-			}
-			return nil
-		}
-	} else {
-		af = func(ras *crv1alpha1.ActionSet) error {
-			ras.Status.Actions[aIDX].Artifacts = arts
-			// 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
-		}
-	}
-	// Update ActionSet
-	if aErr := reconcile.ActionSet(ctx, c.crClient.CrV1alpha1(), actionsetNS, actionsetName, af); aErr != nil {
-		reason := fmt.Sprintf("ActionSetFailed Action: %s", actionName)
-		msg := fmt.Sprintf("Failed to update Output Artifacts: %#v:", artTpls)
-		c.logAndErrorEvent(ctx, msg, reason, aErr, as, bp)
-		return
+		return err
 	}
 
-	if err != nil {
-		reason := fmt.Sprintf("ActionSetFailed Action: %s", actionName)
-		msg := "Failed to render output artifacts"
-		c.logAndErrorEvent(ctx, msg, reason, err, as, bp)
-		return
-	}
+	as.Status.Actions[aIDX].Artifacts = arts
+	return nil
 }
 
 func (c *Controller) logAndErrorEvent(ctx context.Context, msg, reason string, err error, objects ...runtime.Object) {

[github-actions]

This issue is marked as stale due to inactivity. Add a new comment to reactivate it.

[pavannd1]

🚀

[ihcsim]

I have a patch for this; will see if I can break it up into smaller chunks.

[github-actions]

This issue is marked as stale due to inactivity. Add a new comment to reactivate it.

[github-actions]

This issue is closed due to inactivity. Feel free to reopen it, if it's still relevant.