From 2d79b1afb3792ef4701354c9177ef636967d1f04 Mon Sep 17 00:00:00 2001 From: Vivek Singh Date: Mon, 21 Mar 2022 21:54:39 +0100 Subject: [PATCH 01/10] Support `deferPhase` in blueprint action Blueprints can now define `deferPhase` for every action that would be run once all (some in case of failure) the phases of a blueprint action are run. --- pkg/apis/cr/v1alpha1/types.go | 4 + pkg/apis/cr/v1alpha1/zz_generated.deepcopy.go | 5 + pkg/controller/controller.go | 157 +++++++++++++----- pkg/customresource/actionset.yaml | 10 ++ pkg/customresource/blueprint.yaml | 33 ++++ pkg/param/param.go | 12 ++ pkg/phase.go | 49 +++++- 7 files changed, 231 insertions(+), 39 deletions(-) diff --git a/pkg/apis/cr/v1alpha1/types.go b/pkg/apis/cr/v1alpha1/types.go index 6d77fce5fc..ed59102d00 100644 --- a/pkg/apis/cr/v1alpha1/types.go +++ b/pkg/apis/cr/v1alpha1/types.go @@ -126,6 +126,9 @@ type ActionStatus struct { Phases []Phase `json:"phases,omitempty"` // Artifacts created by this phase. Artifacts map[string]Artifact `json:"artifacts,omitempty"` + // DeferPhase is the phase that is executed at the end of an action + // irrespective of the status of other phases in the action + DeferPhase Phase `json:"deferPhase,omitempty"` } // State is the current state of a phase of execution. @@ -194,6 +197,7 @@ type BlueprintAction struct { InputArtifactNames []string `json:"inputArtifactNames,omitempty"` OutputArtifacts map[string]Artifact `json:"outputArtifacts,omitempty"` Phases []BlueprintPhase `json:"phases,omitempty"` + DeferPhase *BlueprintPhase `json:"deferPhase,omitempty"` } // BlueprintPhase is a an individual unit of execution. diff --git a/pkg/apis/cr/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/cr/v1alpha1/zz_generated.deepcopy.go index 8061fef28c..213f6fd00b 100644 --- a/pkg/apis/cr/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/cr/v1alpha1/zz_generated.deepcopy.go @@ -214,6 +214,7 @@ func (in *ActionStatus) DeepCopyInto(out *ActionStatus) { (*out)[key] = *val.DeepCopy() } } + in.DeferPhase.DeepCopyInto(&out.DeferPhase) return } @@ -323,6 +324,10 @@ func (in *BlueprintAction) DeepCopyInto(out *BlueprintAction) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.DeferPhase != nil { + in, out := &in.DeferPhase, &out.DeferPhase + *out = (*in).DeepCopy() + } return } diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 60eae6f03d..07054ab116 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -337,13 +337,23 @@ func (c *Controller) initialActionStatus(namespace string, a crv1alpha1.ActionSp State: crv1alpha1.StatePending, }) } - return &crv1alpha1.ActionStatus{ + + actionStatus := &crv1alpha1.ActionStatus{ Name: a.Name, Object: a.Object, Blueprint: a.Blueprint, Phases: phases, Artifacts: bpa.OutputArtifacts, - }, nil + } + + if bpa.DeferPhase != nil { + actionStatus.DeferPhase = crv1alpha1.Phase{ + Name: bpa.DeferPhase.Name, + State: crv1alpha1.StatePending, + } + } + + return actionStatus, nil } func (c *Controller) handleActionSet(as *crv1alpha1.ActionSet) (err error) { @@ -404,6 +414,11 @@ func (c *Controller) runAction(ctx context.Context, as *crv1alpha1.ActionSet, aI if err != nil { return err } + + // deferPhase is the phase that should be run after every successful or failed action run + // can be specified in blueprint using actions[name].deferPhase + deferPhase, err := kanister.GetDeferPhase(*bp, action.Name, action.PreferredVersion, *tp) + ns, name := as.GetNamespace(), as.GetName() var t *tomb.Tomb t, ctx = tomb.WithContext(ctx) @@ -434,75 +449,141 @@ func (c *Controller) runAction(ctx context.Context, as *crv1alpha1.ActionSet, aI } else { 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 return nil } } + if rErr := reconcile.ActionSet(ctx, c.crClient.CrV1alpha1(), ns, name, rf); rErr != nil { 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) + c.executeDeferPhase(ctx, deferPhase, tp, bp, action.Name, aIDX, as, rErr) return nil } + if err != nil { 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]) } c.logAndErrorEvent(ctx, msg, reason, err, as, bp) + c.executeDeferPhase(ctx, deferPhase, tp, bp, action.Name, aIDX, as, err) return nil } param.UpdatePhaseParams(ctx, tp, p.Name(), output) c.logAndSuccessEvent(ctx, fmt.Sprintf("Completed phase %s", p.Name()), "Ended Phase", as) } - // 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(), ns, name, func(ras *crv1alpha1.ActionSet) error { - ras.Status.State = crv1alpha1.StateComplete - return nil - }); rErr != nil { - reason := fmt.Sprintf("ActionSetFailed Action: %s", action.Name) - msg := fmt.Sprintf("Failed to update ActionSet: %s", name) - c.logAndErrorEvent(ctx, msg, reason, rErr, as, bp) - } - return nil - } - // Render the artifacts - arts, err := param.RenderArtifacts(artTpls, *tp) - var af func(*crv1alpha1.ActionSet) error + c.executeDeferPhase(ctx, deferPhase, tp, bp, action.Name, aIDX, as, nil) + return nil + }) + return nil +} + +// executeDeferPhase executes the phase that is provided as deferPhase in the blueprint actions. deferPhase, +// if provided, must be run eventually for the blueprint action, irrespective of the other phases output. +// Actionset status.state is going to be `complete` iff all of the phases and deferPhase is run successfully +// otherwise respective error message would be logged and recorded as event and actionset's status.state +// would be failed +func (c *Controller) executeDeferPhase(ctx context.Context, + deferPhase *kanister.Phase, + tp *param.TemplateParams, + bp *crv1alpha1.Blueprint, + actionName string, + aIDX int, + as *crv1alpha1.ActionSet, + coreErr error) { + actionsetName, actionsetNS := as.GetName(), as.GetNamespace() + if deferPhase != nil { + 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 { - af = func(ras *crv1alpha1.ActionSet) error { - ras.Status.State = crv1alpha1.StateFailed - ras.Status.Error = crv1alpha1.Error{ - Message: err.Error(), - } + rf = func(as *crv1alpha1.ActionSet) error { + as.Status.Actions[aIDX].DeferPhase.State = crv1alpha1.StateFailed return nil } } else { - af = func(ras *crv1alpha1.ActionSet) error { - ras.Status.Actions[aIDX].Artifacts = arts - ras.Status.State = crv1alpha1.StateComplete + rf = func(as *crv1alpha1.ActionSet) error { + as.Status.Actions[aIDX].DeferPhase.State = crv1alpha1.StateComplete + as.Status.Actions[aIDX].DeferPhase.Output = output return nil } } - // Update ActionSet - if aErr := reconcile.ActionSet(ctx, c.crClient.CrV1alpha1(), ns, name, af); aErr != nil { - reason := fmt.Sprintf("ActionSetFailed Action: %s", action.Name) - msg := fmt.Sprintf("Failed to update Output Artifacts: %#v:", artTpls) - c.logAndErrorEvent(ctx, msg, reason, aErr, as, bp) - 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 } + if err != nil { - reason := fmt.Sprintf("ActionSetFailed Action: %s", action.Name) - msg := "Failed to render output artifacts" + 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) + } c.logAndErrorEvent(ctx, msg, reason, err, as, bp) + return + } + + c.logAndSuccessEvent(ctx, fmt.Sprintf("Completed deferPhase %s", as.Status.Actions[aIDX].DeferPhase.Name), "Ended deferPhase", as) + param.UpdateDeferPhaseParams(context.Background(), tp, output) + } + + // 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 { + ras.Status.State = crv1alpha1.StateComplete 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 nil - }) - return nil + return + } + // Render the 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 { + 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 + } + + 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 + } } func (c *Controller) logAndErrorEvent(ctx context.Context, msg, reason string, err error, objects ...runtime.Object) { diff --git a/pkg/customresource/actionset.yaml b/pkg/customresource/actionset.yaml index d27d2772cf..a524f306aa 100644 --- a/pkg/customresource/actionset.yaml +++ b/pkg/customresource/actionset.yaml @@ -219,6 +219,16 @@ spec: description: Resource name of the referent. type: string type: object + deferPhase: + properties: + name: + type: string + output: + x-kubernetes-preserve-unknown-fields: true + type: object + state: + type: string + type: object phases: description: Phases are sub-actions an are executed sequentially. items: diff --git a/pkg/customresource/blueprint.yaml b/pkg/customresource/blueprint.yaml index 9d2076b50f..42fced12e2 100644 --- a/pkg/customresource/blueprint.yaml +++ b/pkg/customresource/blueprint.yaml @@ -44,6 +44,39 @@ spec: x-kubernetes-preserve-unknown-fields: true type: object type: object + deferPhase: + properties: + args: + x-kubernetes-preserve-unknown-fields: true + type: object + func: + type: string + name: + type: string + objects: + additionalProperties: + properties: + apiVersion: + description: API version of the referent. + type: string + group: + description: API Group of the referent. + type: string + kind: + description: 'Kind of the referent. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#types-kinds' + type: string + name: + description: 'Name of the referent. More info: http://kubernetes.io/docs/user-guide/identifiers#names' + type: string + namespace: + description: 'Namespace of the referent. More info: http://kubernetes.io/docs/user-guide/namespaces' + type: string + resource: + description: Resource name of the referent. + type: string + type: object + type: object + type: object phases: items: properties: diff --git a/pkg/param/param.go b/pkg/param/param.go index d4823d7fee..65026e1477 100644 --- a/pkg/param/param.go +++ b/pkg/param/param.go @@ -52,6 +52,7 @@ type TemplateParams struct { Options map[string]string Object map[string]interface{} Phases map[string]*Phase + DeferPhase *Phase PodOverride crv1alpha1.JSONMap } @@ -510,6 +511,12 @@ func UpdatePhaseParams(ctx context.Context, tp *TemplateParams, phaseName string tp.Phases[phaseName].Output = output } +// UpdateDeferPhaseParams updates the TemplateParams deferPhase output with passed output +// this output would be generated/passed by execution of the phase +func UpdateDeferPhaseParams(ctx context.Context, tp *TemplateParams, output map[string]interface{}) { + tp.DeferPhase.Output = output +} + // InitPhaseParams initializes the TemplateParams with Phase information func InitPhaseParams(ctx context.Context, cli kubernetes.Interface, tp *TemplateParams, phaseName string, objects map[string]crv1alpha1.ObjectReference) error { if tp.Phases == nil { @@ -522,5 +529,10 @@ func InitPhaseParams(ctx context.Context, cli kubernetes.Interface, tp *Template tp.Phases[phaseName] = &Phase{ Secrets: secrets, } + + tp.DeferPhase = &Phase{ + Secrets: secrets, + } + return nil } diff --git a/pkg/phase.go b/pkg/phase.go index 22f12240b8..4ab486c9ed 100644 --- a/pkg/phase.go +++ b/pkg/phase.go @@ -55,7 +55,13 @@ func (p *Phase) Exec(ctx context.Context, bp crv1alpha1.Blueprint, action string return nil, errors.Errorf("Action {%s} not found in action map", action) } // Render the argument templates for the Phase's function - for _, ap := range a.Phases { + phases := []crv1alpha1.BlueprintPhase{} + phases = append(phases, a.Phases...) + if a.DeferPhase != nil { + phases = append(phases, *a.DeferPhase) + } + + for _, ap := range phases { if ap.Name != p.name { continue } @@ -87,6 +93,47 @@ func checkSupportedArgs(supportedArgs []string, args map[string]interface{}) err return nil } +func GetDeferPhase(bp crv1alpha1.Blueprint, action, version string, tp param.TemplateParams) (*Phase, error) { + a, ok := bp.Actions[action] + if !ok { + return nil, errors.Errorf("Action {%s} not found in blueprint actions", action) + } + + if a.DeferPhase == nil { + return nil, nil + } + + defaultVersion, funcVersion, err := getFunctionVersion(version) + if err != nil { + return nil, errors.Wrapf(err, "Failed to get function version") + } + + regVersion := *funcVersion + if _, ok := funcs[a.DeferPhase.Func]; !ok { + return nil, errors.Errorf("Requested function {%s} has not been registered", a.DeferPhase.Func) + } + if _, ok := funcs[a.DeferPhase.Func][regVersion]; !ok { + if funcVersion.Equal(defaultVersion) { + return nil, errors.Errorf("Requested function {%s} has not been registered with version {%s}", a.DeferPhase.Func, version) + } + if _, ok := funcs[a.DeferPhase.Func][*defaultVersion]; !ok { + return nil, errors.Errorf("Requested function {%s} has not been registered with versions {%s} or {%s}", a.DeferPhase.Func, version, DefaultVersion) + } + log.Info().Print("Falling back to default version of the function", field.M{"Function": a.DeferPhase.Func, "PreferredVersion": version, "FallbackVersion": DefaultVersion}) + regVersion = *defaultVersion + } + objs, err := param.RenderObjectRefs(a.DeferPhase.ObjectRefs, tp) + if err != nil { + return nil, err + } + + return &Phase{ + name: a.DeferPhase.Name, + objects: objs, + f: funcs[a.DeferPhase.Func][regVersion], + }, nil +} + // GetPhases renders the returns a list of Phases with pre-rendered arguments. func GetPhases(bp crv1alpha1.Blueprint, action, version string, tp param.TemplateParams) ([]*Phase, error) { a, ok := bp.Actions[action] From 03b9c39d474c2bf60a5a2ed62f17f7a48a97946c Mon Sep 17 00:00:00 2001 From: Vivek Singh Date: Thu, 24 Mar 2022 00:09:04 +0100 Subject: [PATCH 02/10] Update docs for deferPhase changes --- docs/architecture.rst | 6 +++++ docs/templates.rst | 31 +++++++++++++++++++++++ docs/tutorial.rst | 58 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+) diff --git a/docs/architecture.rst b/docs/architecture.rst index ae772c9fec..759e15d5a6 100644 --- a/docs/architecture.rst +++ b/docs/architecture.rst @@ -71,8 +71,14 @@ The definition of a ``BlueprintAction`` is: InputArtifactNames []string `json:"inputArtifactNames"` OutputArtifacts map[string]Artifact `json:"outputArtifacts"` Phases []BlueprintPhase `json:"phases"` + DeferPhase *BlueprintPhase `json:"deferPhase,omitempty"` } +- ``DeferPhase`` is an optional ``BlueprintPhase`` that you want to invoke at + the end of the actions. ``DeferPhase`` can easily be used to do some + cleanup operations after the actual operation's phases/steps. Since + ``DeferPhase`` is used to cleanup the resources it will run even if the + actions' main phases are failed. - ``Kind`` represents the type of Kubernetes object this BlueprintAction is written for. Specifying this is optional and going forward, if this is specified, Kanister will enforce that it matches the ``Object`` kind specified in an ActionSet referencing this diff --git a/docs/templates.rst b/docs/templates.rst index 84227b0523..dab92600ee 100644 --- a/docs/templates.rst +++ b/docs/templates.rst @@ -25,6 +25,7 @@ The TemplateParam struct is defined as: Options map[string]string Object map[string]interface{} Phases map[string]*Phase + DeferPhase *Phase PodOverride crv1alpha1.JSONMap } @@ -609,3 +610,33 @@ Similarly, a phase can use Secrets as arguments: .. code-block:: go "{{ .Phases.phase-name.Secrets.secret-name.Namespace }}" + +DeferPhase +---------- + +``DeferPhase`` is used to capture information returned from Blueprint's ``DeferPhase`` +execution. The information is stored in the ``Phase`` struct that has the below +definition: + +.. code-block:: go + :linenos: + + type Phase struct { + Secrets map[string]v1.Secret + Output map[string]interface{} + } + +The output artifacts generated by ``DeferPhase`` can be referenced by other actions' +phases in the same way, the output artifact of blueprint's normal phases are +referenced. + +For example, an output artifact can be set as follows, considering the ``DeferPhase`` +outputs an artifact with key ``key-name``. + +.. code-block:: go + + "{{ .DeferPhase.Output.key-name }}" + + +Output artifacts that are set using ``DeferPhase`` can be consumed by other actions' +phases using the same way other output artifacts are consumed. diff --git a/docs/tutorial.rst b/docs/tutorial.rst index c3b459dae8..8fad3b0a39 100644 --- a/docs/tutorial.rst +++ b/docs/tutorial.rst @@ -443,6 +443,64 @@ ConfigMap. If you re-execute this Kanister Action, you'll be able to see the Artifact in the ActionSet status. +If you have the use case of ``DeferPhase``, below is how you can set the output artifact +from the output that is being generated from ``DeferPhase`` + +.. code-block:: yaml + + cat < Date: Thu, 24 Mar 2022 12:27:27 +0100 Subject: [PATCH 03/10] Handle the scenario where only deferPhase is failed --- pkg/controller/controller.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 07054ab116..35d8788281 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -501,6 +501,10 @@ func (c *Controller) executeDeferPhase(ctx context.Context, 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 } From f7f81cf79702ec6cb39579ed361a9c7fd4f121f0 Mon Sep 17 00:00:00 2001 From: Vivek Singh Date: Mon, 28 Mar 2022 12:19:22 +0200 Subject: [PATCH 04/10] Try to fix CI by increasing actionset wait timeout --- pkg/controller/controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 47179d1b3a..422732f2c2 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -138,7 +138,7 @@ func (s *ControllerSuite) TestWatch(c *C) { // nolint:unparam func (s *ControllerSuite) waitOnActionSetState(c *C, as *crv1alpha1.ActionSet, state crv1alpha1.State) error { - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() err := poll.Wait(ctx, func(context.Context) (bool, error) { as, err := s.crCli.ActionSets(as.GetNamespace()).Get(ctx, as.GetName(), metav1.GetOptions{}) From 668bac4ceb395a742bbdb439499deca17b22f5dc Mon Sep 17 00:00:00 2001 From: Vivek Singh Date: Wed, 30 Mar 2022 17:06:06 +0200 Subject: [PATCH 05/10] Refactor code and add test for utility functions --- docs/architecture.rst | 10 ++--- docs/templates.rst | 10 ++--- docs/tutorial.rst | 6 +-- pkg/apis/cr/v1alpha1/types.go | 2 +- pkg/controller/controller.go | 13 ++++-- pkg/param/param_test.go | 5 ++- pkg/phase.go | 69 ++++++++++++++++---------------- pkg/phase_test.go | 74 +++++++++++++++++++++++++++++++++++ 8 files changed, 132 insertions(+), 57 deletions(-) diff --git a/docs/architecture.rst b/docs/architecture.rst index 759e15d5a6..9694fccdba 100644 --- a/docs/architecture.rst +++ b/docs/architecture.rst @@ -74,11 +74,6 @@ The definition of a ``BlueprintAction`` is: DeferPhase *BlueprintPhase `json:"deferPhase,omitempty"` } -- ``DeferPhase`` is an optional ``BlueprintPhase`` that you want to invoke at - the end of the actions. ``DeferPhase`` can easily be used to do some - cleanup operations after the actual operation's phases/steps. Since - ``DeferPhase`` is used to cleanup the resources it will run even if the - actions' main phases are failed. - ``Kind`` represents the type of Kubernetes object this BlueprintAction is written for. Specifying this is optional and going forward, if this is specified, Kanister will enforce that it matches the ``Object`` kind specified in an ActionSet referencing this @@ -90,6 +85,11 @@ The definition of a ``BlueprintAction`` is: to the ``BlueprintAction``. - ``Phases`` is a required list of ``BlueprintPhases``. These phases are invoked in order when executing this Action. +- ``DeferPhase`` is an optional ``BlueprintPhase`` that you want to invoke at + the end of the actions. ``DeferPhase`` can easily be used to do some + cleanup operations after the actual operation's phases/steps. Since + ``DeferPhase`` is used to cleanup the resources it will run even if the + actions' main phases are failed. .. code-block:: go :linenos: diff --git a/docs/templates.rst b/docs/templates.rst index dab92600ee..8ac786eff4 100644 --- a/docs/templates.rst +++ b/docs/templates.rst @@ -614,7 +614,7 @@ Similarly, a phase can use Secrets as arguments: DeferPhase ---------- -``DeferPhase`` is used to capture information returned from Blueprint's ``DeferPhase`` +``DeferPhase`` is used to capture information returned from the Blueprint's ``DeferPhase`` execution. The information is stored in the ``Phase`` struct that has the below definition: @@ -626,12 +626,8 @@ definition: Output map[string]interface{} } -The output artifacts generated by ``DeferPhase`` can be referenced by other actions' -phases in the same way, the output artifact of blueprint's normal phases are -referenced. - -For example, an output artifact can be set as follows, considering the ``DeferPhase`` -outputs an artifact with key ``key-name``. +Output artifact can be set as follows, considering the ``DeferPhase`` outputs an artifact +with key ``key-name``. .. code-block:: go diff --git a/docs/tutorial.rst b/docs/tutorial.rst index 8fad3b0a39..2188ccc564 100644 --- a/docs/tutorial.rst +++ b/docs/tutorial.rst @@ -486,11 +486,7 @@ from the output that is being generated from ``DeferPhase`` image: ghcr.io/kanisterio/mysql-sidecar:0.74.0 namespace: "{{ .Deployment.Namespace }}" command: - - bash - - -o - - errexit - - -o - - pipefail + - sh - -c - | echo "DeferPhase" diff --git a/pkg/apis/cr/v1alpha1/types.go b/pkg/apis/cr/v1alpha1/types.go index ed59102d00..7e34e45798 100644 --- a/pkg/apis/cr/v1alpha1/types.go +++ b/pkg/apis/cr/v1alpha1/types.go @@ -126,7 +126,7 @@ type ActionStatus struct { Phases []Phase `json:"phases,omitempty"` // Artifacts created by this phase. Artifacts map[string]Artifact `json:"artifacts,omitempty"` - // DeferPhase is the phase that is executed at the end of an action + // DeferPhase is the phase that is executed at the end of an action // irrespective of the status of other phases in the action DeferPhase Phase `json:"deferPhase,omitempty"` } diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 35d8788281..613030766b 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -418,6 +418,9 @@ func (c *Controller) runAction(ctx context.Context, as *crv1alpha1.ActionSet, aI // deferPhase is the phase that should be run after every successful or failed action run // can be specified in blueprint using actions[name].deferPhase deferPhase, err := kanister.GetDeferPhase(*bp, action.Name, action.PreferredVersion, *tp) + if err != nil { + return err + } ns, name := as.GetNamespace(), as.GetName() var t *tomb.Tomb @@ -425,6 +428,11 @@ func (c *Controller) runAction(ctx context.Context, as *crv1alpha1.ActionSet, aI c.actionSetTombMap.Store(as.Name, t) ctx = field.Context(ctx, consts.ActionsetNameKey, as.GetName()) t.Go(func() error { + var coreErr error + defer func(error) { + c.executeDeferPhase(ctx, deferPhase, tp, bp, action.Name, aIDX, as, err) + }(coreErr) + 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) @@ -459,7 +467,7 @@ func (c *Controller) runAction(ctx context.Context, as *crv1alpha1.ActionSet, aI 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) - c.executeDeferPhase(ctx, deferPhase, tp, bp, action.Name, aIDX, as, rErr) + coreErr = rErr return nil } @@ -469,13 +477,12 @@ func (c *Controller) runAction(ctx context.Context, as *crv1alpha1.ActionSet, aI msg = fmt.Sprintf("Failed to execute phase: %#v:", as.Status.Actions[aIDX].Phases[i]) } c.logAndErrorEvent(ctx, msg, reason, err, as, bp) - c.executeDeferPhase(ctx, deferPhase, tp, bp, action.Name, aIDX, as, err) + coreErr = err return nil } param.UpdatePhaseParams(ctx, tp, p.Name(), output) c.logAndSuccessEvent(ctx, fmt.Sprintf("Completed phase %s", p.Name()), "Ended Phase", as) } - c.executeDeferPhase(ctx, deferPhase, tp, bp, action.Name, aIDX, as, nil) return nil }) return nil diff --git a/pkg/param/param_test.go b/pkg/param/param_test.go index 5f6a2225e6..f31a4bd225 100644 --- a/pkg/param/param_test.go +++ b/pkg/param/param_test.go @@ -721,7 +721,10 @@ func (s *ParamsSuite) TestPhaseParams(c *C) { c.Assert(tp.Phases, IsNil) err = InitPhaseParams(ctx, s.cli, tp, "backup", nil) c.Assert(err, IsNil) - UpdatePhaseParams(ctx, tp, "backup", map[string]interface{}{"version": "0.76.0"}) + UpdatePhaseParams(ctx, tp, "backup", map[string]interface{}{"version": "0.75.0"}) + UpdateDeferPhaseParams(ctx, tp, map[string]interface{}{"version": "0.75.0"}) + // make sure output artifact is set in DeferPhase + c.Assert(tp.DeferPhase.Output, DeepEquals, map[string]interface{}{"version": "0.75.0"}) c.Assert(tp.Phases, HasLen, 1) c.Assert(tp.Phases["backup"], NotNil) c.Assert(tp.Secrets, HasLen, 1) diff --git a/pkg/phase.go b/pkg/phase.go index 4ab486c9ed..d6519fde41 100644 --- a/pkg/phase.go +++ b/pkg/phase.go @@ -103,25 +103,11 @@ func GetDeferPhase(bp crv1alpha1.Blueprint, action, version string, tp param.Tem return nil, nil } - defaultVersion, funcVersion, err := getFunctionVersion(version) + regVersion, err := regFuncVersion(a.DeferPhase.Func, version) if err != nil { - return nil, errors.Wrapf(err, "Failed to get function version") + return nil, err } - regVersion := *funcVersion - if _, ok := funcs[a.DeferPhase.Func]; !ok { - return nil, errors.Errorf("Requested function {%s} has not been registered", a.DeferPhase.Func) - } - if _, ok := funcs[a.DeferPhase.Func][regVersion]; !ok { - if funcVersion.Equal(defaultVersion) { - return nil, errors.Errorf("Requested function {%s} has not been registered with version {%s}", a.DeferPhase.Func, version) - } - if _, ok := funcs[a.DeferPhase.Func][*defaultVersion]; !ok { - return nil, errors.Errorf("Requested function {%s} has not been registered with versions {%s} or {%s}", a.DeferPhase.Func, version, DefaultVersion) - } - log.Info().Print("Falling back to default version of the function", field.M{"Function": a.DeferPhase.Func, "PreferredVersion": version, "FallbackVersion": DefaultVersion}) - regVersion = *defaultVersion - } objs, err := param.RenderObjectRefs(a.DeferPhase.ObjectRefs, tp) if err != nil { return nil, err @@ -134,35 +120,48 @@ func GetDeferPhase(bp crv1alpha1.Blueprint, action, version string, tp param.Tem }, nil } +func regFuncVersion(f, version string) (semver.Version, error) { + funcMu.RLock() + defer funcMu.RUnlock() + + defaultVersion, funcVersion, err := getFunctionVersion(version) + if err != nil { + return semver.Version{}, errors.Wrapf(err, "Failed to get function version") + } + + regVersion := *funcVersion + if _, ok := funcs[f]; !ok { + return semver.Version{}, errors.Errorf("Requested function {%s} has not been registered", f) + } + if _, ok := funcs[f][regVersion]; !ok { + if funcVersion.Equal(defaultVersion) { + return semver.Version{}, errors.Errorf("Requested function {%s} has not been registered with version {%s}", f, version) + } + if _, ok := funcs[f][*defaultVersion]; !ok { + return semver.Version{}, errors.Errorf("Requested function {%s} has not been registered with versions {%s} or {%s}", f, version, DefaultVersion) + } + log.Info().Print("Falling back to default version of the function", field.M{"Function": f, "PreferredVersion": version, "FallbackVersion": DefaultVersion}) + return *defaultVersion, nil + } + + return *funcVersion, nil +} + // GetPhases renders the returns a list of Phases with pre-rendered arguments. func GetPhases(bp crv1alpha1.Blueprint, action, version string, tp param.TemplateParams) ([]*Phase, error) { a, ok := bp.Actions[action] if !ok { return nil, errors.Errorf("Action {%s} not found in action map", action) } - defaultVersion, funcVersion, err := getFunctionVersion(version) - if err != nil { - return nil, errors.Wrapf(err, "Failed to get function version") - } - funcMu.RLock() - defer funcMu.RUnlock() + phases := make([]*Phase, 0, len(a.Phases)) // Check that all requested phases are registered and render object refs for _, p := range a.Phases { - regVersion := *funcVersion - if _, ok := funcs[p.Func]; !ok { - return nil, errors.Errorf("Requested function {%s} has not been registered", p.Func) - } - if _, ok := funcs[p.Func][regVersion]; !ok { - if funcVersion.Equal(defaultVersion) { - return nil, errors.Errorf("Requested function {%s} has not been registered with version {%s}", p.Func, version) - } - if _, ok := funcs[p.Func][*defaultVersion]; !ok { - return nil, errors.Errorf("Requested function {%s} has not been registered with versions {%s} or {%s}", p.Func, version, DefaultVersion) - } - log.Info().Print("Falling back to default version of the function", field.M{"Function": p.Func, "PreferredVersion": version, "FallbackVersion": DefaultVersion}) - regVersion = *defaultVersion + regVersion, err := regFuncVersion(p.Func, version) + if err != nil { + return nil, err } + objs, err := param.RenderObjectRefs(p.ObjectRefs, tp) if err != nil { return nil, err diff --git a/pkg/phase_test.go b/pkg/phase_test.go index 7d4b93ab55..aaef64c02c 100644 --- a/pkg/phase_test.go +++ b/pkg/phase_test.go @@ -35,6 +35,14 @@ type testFunc struct { err error } +type anotherFunc struct { + testFunc +} + +func (a *anotherFunc) Name() string { + return "anotherTestFunc" +} + func (*testFunc) Name() string { return "mock" } @@ -128,3 +136,69 @@ func (s *PhaseSuite) TestCheckSupportedArgs(c *C) { c.Assert(err, tc.err) } } + +func (s *PhaseSuite) TestRegFuncVersion(c *C) { + for _, tc := range []struct { + regWithVersion string + expectedVersion string + queryVersion string + f Func + }{ + { + f: &testFunc{}, + expectedVersion: "v0.0.0", + queryVersion: "v0.0.0", + }, + { + f: &testFunc{}, + regWithVersion: "v0.0.1", + expectedVersion: "v0.0.1", + queryVersion: "v0.0.1", + }, + { + f: &anotherFunc{}, + expectedVersion: "v0.0.0", + queryVersion: "v0.0.0", + }, + { + f: &anotherFunc{}, + regWithVersion: "v1.2.3", + expectedVersion: "v1.2.3", + queryVersion: "v1.2.3", + }, + { + f: &anotherFunc{}, + regWithVersion: "v4.5.6", + expectedVersion: "v4.5.6", + queryVersion: "v4.5.6", + }, + { + f: &anotherFunc{}, + regWithVersion: "v0.9.9", + expectedVersion: "v0.0.0", + // even though we are registering function version v0.9.9 we are querying the same function that is registered with version + // v0.0.0 that is the reason we have v0.0.0 as expectedVersion + queryVersion: "v0.0.0", + }, + { + f: &anotherFunc{}, + regWithVersion: "v0.1.1", + expectedVersion: "v0.0.0", + // since function anotherFunc is not registered with version v11.11.11, we will default to defaultFuncVersion (i.e., v0.0.0) + // that is the reason the expected version hereis v0.0.0 + queryVersion: "v11.11.11", + }, + } { + if tc.regWithVersion == "" { + err := Register(tc.f) + c.Assert(err, IsNil) + } else { + err := RegisterVersion(tc.f, tc.regWithVersion) + c.Assert(err, IsNil) + } + + semVer, err := regFuncVersion(tc.f.Name(), tc.queryVersion) + c.Assert(err, IsNil) + c.Assert(semVer.Original(), Equals, tc.expectedVersion) + } +} From f7a484276ebd85825a93e33119d5e1704280d504 Mon Sep 17 00:00:00 2001 From: Vivek Singh Date: Fri, 1 Apr 2022 17:40:29 +0200 Subject: [PATCH 06/10] Address review comment, modularise functions --- pkg/controller/controller.go | 90 +++++++++++++++++++++--------------- 1 file changed, 52 insertions(+), 38 deletions(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 613030766b..cd5f9e91e3 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -429,9 +429,12 @@ func (c *Controller) runAction(ctx context.Context, as *crv1alpha1.ActionSet, aI ctx = field.Context(ctx, consts.ActionsetNameKey, as.GetName()) t.Go(func() error { var coreErr error - defer func(error) { - c.executeDeferPhase(ctx, deferPhase, tp, bp, action.Name, aIDX, as, err) - }(coreErr) + defer func() { + if deferPhase != nil { + c.executeDeferPhase(ctx, deferPhase, tp, bp, action.Name, aIDX, as) + } + c.renderActionsetArtifacts(ctx, as, aIDX, ns, name, action.Name, bp, tp, coreErr) + }() for i, p := range phases { ctx = field.Context(ctx, consts.PhaseNameKey, p.Name()) @@ -446,6 +449,7 @@ func (c *Controller) runAction(ctx context.Context, as *crv1alpha1.ActionSet, aI } var rf func(*crv1alpha1.ActionSet) error if err != nil { + coreErr = err rf = func(ras *crv1alpha1.ActionSet) error { ras.Status.State = crv1alpha1.StateFailed ras.Status.Error = crv1alpha1.Error{ @@ -455,6 +459,7 @@ func (c *Controller) runAction(ctx context.Context, as *crv1alpha1.ActionSet, aI 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 @@ -500,49 +505,58 @@ func (c *Controller) executeDeferPhase(ctx context.Context, actionName string, aIDX int, as *crv1alpha1.ActionSet, - coreErr error) { +) { actionsetName, actionsetNS := as.GetName(), as.GetNamespace() - if deferPhase != nil { - 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 + 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 } - 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 + } 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 + } - 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) - } - c.logAndErrorEvent(ctx, msg, reason, err, as, bp) - return + 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) } - - c.logAndSuccessEvent(ctx, fmt.Sprintf("Completed deferPhase %s", as.Status.Actions[aIDX].DeferPhase.Name), "Ended deferPhase", as) - param.UpdateDeferPhaseParams(context.Background(), tp, output) + c.logAndErrorEvent(ctx, msg, reason, err, as, bp) + return } + c.logAndSuccessEvent(ctx, fmt.Sprintf("Completed deferPhase %s", as.Status.Actions[aIDX].DeferPhase.Name), "Ended deferPhase", as) + param.UpdateDeferPhaseParams(context.Background(), tp, output) +} + +func (c *Controller) renderActionsetArtifacts(ctx context.Context, + as *crv1alpha1.ActionSet, + aIDX int, + actionsetNS, actionsetName, actionName string, + bp *crv1alpha1.Blueprint, + tp *param.TemplateParams, + coreErr error, +) { // Check if output artifacts are present artTpls := as.Status.Actions[aIDX].Artifacts if len(artTpls) == 0 { From 304c651c3360983fbd245bb259978b454b653de4 Mon Sep 17 00:00:00 2001 From: Vivek Singh Date: Mon, 4 Apr 2022 19:42:32 +0200 Subject: [PATCH 07/10] Fix a bug while only deferPhase was failing --- pkg/controller/controller.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index cd5f9e91e3..b678ca9545 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -431,7 +431,7 @@ func (c *Controller) runAction(ctx context.Context, as *crv1alpha1.ActionSet, aI var coreErr error defer func() { if deferPhase != nil { - c.executeDeferPhase(ctx, deferPhase, tp, bp, action.Name, aIDX, as) + coreErr = c.executeDeferPhase(ctx, deferPhase, tp, bp, action.Name, aIDX, as) } c.renderActionsetArtifacts(ctx, as, aIDX, ns, name, action.Name, bp, tp, coreErr) }() @@ -505,7 +505,7 @@ func (c *Controller) executeDeferPhase(ctx context.Context, actionName string, 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) @@ -533,7 +533,7 @@ func (c *Controller) executeDeferPhase(ctx context.Context, 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 + return rErr } if err != nil { @@ -542,11 +542,12 @@ func (c *Controller) executeDeferPhase(ctx context.Context, msg = fmt.Sprintf("Failed to execute defer phase: %#v:", as.Status.Actions[aIDX].DeferPhase) } c.logAndErrorEvent(ctx, msg, reason, err, as, bp) - return + return err } 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 } func (c *Controller) renderActionsetArtifacts(ctx context.Context, @@ -562,7 +563,12 @@ func (c *Controller) renderActionsetArtifacts(ctx context.Context, 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 { - ras.Status.State = crv1alpha1.StateComplete + if coreErr == nil { + ras.Status.State = crv1alpha1.StateComplete + } else { + ras.Status.State = crv1alpha1.StateFailed + } + return nil }); rErr != nil { reason := fmt.Sprintf("ActionSetFailed Action: %s", actionName) From 4654c016422d37cbdfd018a3fbf6ee4d8ef923cf Mon Sep 17 00:00:00 2001 From: Vivek Singh Date: Tue, 5 Apr 2022 12:27:21 +0200 Subject: [PATCH 08/10] Fix bug where actionset status was being set to complete even after one of the core phases failed The actionset status eventually must be failed if either one of core phases or deferphases failed but we had a problem because of which the actionset status was being set to complete even if the one of the core phases failed. This commit fixes that. It was happening because we were unknowingly setting error to nil in deferPhase because deferPhase ran successfully. --- pkg/controller/controller.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index b678ca9545..858592ab7f 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -430,10 +430,11 @@ func (c *Controller) runAction(ctx context.Context, as *crv1alpha1.ActionSet, aI t.Go(func() error { var coreErr error defer func() { + var deferErr error if deferPhase != nil { - coreErr = c.executeDeferPhase(ctx, deferPhase, tp, bp, action.Name, aIDX, as) + deferErr = c.executeDeferPhase(ctx, deferPhase, tp, bp, action.Name, aIDX, as) } - c.renderActionsetArtifacts(ctx, as, aIDX, ns, name, action.Name, bp, tp, coreErr) + c.renderActionsetArtifacts(ctx, as, aIDX, ns, name, action.Name, bp, tp, coreErr, deferErr) }() for i, p := range phases { @@ -556,14 +557,14 @@ func (c *Controller) renderActionsetArtifacts(ctx context.Context, actionsetNS, actionsetName, actionName string, bp *crv1alpha1.Blueprint, tp *param.TemplateParams, - coreErr error, + 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 { + if coreErr == nil && deferErr == nil { ras.Status.State = crv1alpha1.StateComplete } else { ras.Status.State = crv1alpha1.StateFailed @@ -593,7 +594,7 @@ func (c *Controller) renderActionsetArtifacts(ctx context.Context, 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 { + if coreErr == nil && deferErr == nil { ras.Status.State = crv1alpha1.StateComplete } else { ras.Status.State = crv1alpha1.StateFailed From e8c4853ca0891d228acede87b40cc12324c5cc65 Mon Sep 17 00:00:00 2001 From: Vivek Singh Date: Wed, 6 Apr 2022 11:11:29 +0200 Subject: [PATCH 09/10] Address review comment, reword comments and docs --- docs/architecture.rst | 9 ++++----- docs/templates.rst | 3 +-- docs/tutorial.rst | 4 ++-- pkg/controller/controller.go | 11 ++++++----- pkg/param/param.go | 2 +- 5 files changed, 14 insertions(+), 15 deletions(-) diff --git a/docs/architecture.rst b/docs/architecture.rst index 9694fccdba..5a785a6521 100644 --- a/docs/architecture.rst +++ b/docs/architecture.rst @@ -85,11 +85,10 @@ The definition of a ``BlueprintAction`` is: to the ``BlueprintAction``. - ``Phases`` is a required list of ``BlueprintPhases``. These phases are invoked in order when executing this Action. -- ``DeferPhase`` is an optional ``BlueprintPhase`` that you want to invoke at - the end of the actions. ``DeferPhase`` can easily be used to do some - cleanup operations after the actual operation's phases/steps. Since - ``DeferPhase`` is used to cleanup the resources it will run even if the - actions' main phases are failed. +- ``DeferPhase`` is an optional ``BlueprintPhase`` invoked after the + execution of ``Phases`` defined above. A ``DeferPhase``, when specified, + is executed regardless of the statuses of the ``Phases``. + A ``DeferPhase`` can be used for cleanup operations at the end of an ``Action``. .. code-block:: go :linenos: diff --git a/docs/templates.rst b/docs/templates.rst index 8ac786eff4..68352868ca 100644 --- a/docs/templates.rst +++ b/docs/templates.rst @@ -626,8 +626,7 @@ definition: Output map[string]interface{} } -Output artifact can be set as follows, considering the ``DeferPhase`` outputs an artifact -with key ``key-name``. +Output artifact can be set as follows: .. code-block:: go diff --git a/docs/tutorial.rst b/docs/tutorial.rst index 2188ccc564..2aebf31561 100644 --- a/docs/tutorial.rst +++ b/docs/tutorial.rst @@ -443,8 +443,8 @@ ConfigMap. If you re-execute this Kanister Action, you'll be able to see the Artifact in the ActionSet status. -If you have the use case of ``DeferPhase``, below is how you can set the output artifact -from the output that is being generated from ``DeferPhase`` +If you use a ``DeferPhase``, below is how you can set the output artifact +from the output that is being generated from ``DeferPhase`` as shown below. .. code-block:: yaml diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 858592ab7f..a9080c986e 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -494,11 +494,12 @@ func (c *Controller) runAction(ctx context.Context, as *crv1alpha1.ActionSet, aI return nil } -// executeDeferPhase executes the phase that is provided as deferPhase in the blueprint actions. deferPhase, -// if provided, must be run eventually for the blueprint action, irrespective of the other phases output. -// Actionset status.state is going to be `complete` iff all of the phases and deferPhase is run successfully -// otherwise respective error message would be logged and recorded as event and actionset's status.state -// would be failed +// executeDeferPhase executes the phase provided as a deferPhase in the blueprint action. +// deferPhase, if provided, must be run at the end of the blueprint action, irrespective of the +// statuses of the other phases. ActionSet `status.state` is going to be `complete` IFF all the +// phases and deferPhase are run successfully +// On failure, corresponding error messages are logged and recorded as events and the +// ActionSet's `status.state` is set to `failed`. func (c *Controller) executeDeferPhase(ctx context.Context, deferPhase *kanister.Phase, tp *param.TemplateParams, diff --git a/pkg/param/param.go b/pkg/param/param.go index 65026e1477..9e05c139ca 100644 --- a/pkg/param/param.go +++ b/pkg/param/param.go @@ -512,7 +512,7 @@ func UpdatePhaseParams(ctx context.Context, tp *TemplateParams, phaseName string } // UpdateDeferPhaseParams updates the TemplateParams deferPhase output with passed output -// this output would be generated/passed by execution of the phase +// This output would be generated/passed by execution of the phase func UpdateDeferPhaseParams(ctx context.Context, tp *TemplateParams, output map[string]interface{}) { tp.DeferPhase.Output = output } From faa967dd18eb97f5c6913d8bcb1589c206191e69 Mon Sep 17 00:00:00 2001 From: Vivek Singh Date: Wed, 6 Apr 2022 11:34:06 +0200 Subject: [PATCH 10/10] Update the wait actionset timeout to 20 seconds --- pkg/controller/controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 422732f2c2..13172ed868 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -138,7 +138,7 @@ func (s *ControllerSuite) TestWatch(c *C) { // nolint:unparam func (s *ControllerSuite) waitOnActionSetState(c *C, as *crv1alpha1.ActionSet, state crv1alpha1.State) error { - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) defer cancel() err := poll.Wait(ctx, func(context.Context) (bool, error) { as, err := s.crCli.ActionSets(as.GetNamespace()).Get(ctx, as.GetName(), metav1.GetOptions{})