From b3019e062656a8a4ee237c5e9b50e0bb671152c3 Mon Sep 17 00:00:00 2001 From: Daniil Fedotov Date: Wed, 22 May 2024 14:42:55 -0400 Subject: [PATCH] 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 https://github.com/kanisterio/kanister/issues/2882 Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- pkg/controller/controller.go | 96 ++++++++++++++++++++++++++---------- 1 file changed, 69 insertions(+), 27 deletions(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index b280a871f9..e2566c5e3f 100644 --- a/pkg/controller/controller.go +++ b/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 +}