TEP-0059: Skipping Strategies

[jerop]

In #388, we added the problem statement for TEP-0059: Skipping Strategies that addresses skipping strategies to give users the flexibility to skip a single guarded Task only and unblock execution of its dependent Tasks.

In this change, we add the proposal and discuss alternatives for solving that problem.

Today, WhenExpressions are specified within Tasks but they guard the Task and its dependent Tasks.

To provide flexible skipping strategies, we propose changing the scope of WhenExpressions from guarding a Task
and its dependent Tasks to guarding the Task only.

If a user wants to guard a Task and its dependent Tasks, they can:

1. cascade the WhenExpressions to the dependent Tasks
2. compose the Task and its dependent Tasks as a sub-Pipeline that's guarded and executed together using
   Pipelines in Pipelines

/kind tep

[pritidesai]

This alternative can be further refined to align well with TEP-0050.

But, I dont see this as a con, in the entirety, continue on error be it a process failure or skipped due to when expressions.

[jerop]

This alternative can be further refined to align well with TEP-0050.

please share any ideas you have to refine this alternative 😀

But, I dont see this as a con, in the entirety, continue on error be it a process failure or skipped due to when expressions.

I don't think we'd want to refer to skipping of a task as an error (skipping doesn't cause to pipeline failure)

It'll be more extensible and scalable to pass in the individual execution policies as a list instead of providing execution policies that are combinations of other execution policies, what do you think?

[jerop]

@vdemeester, @sbwsg, @bobcatfish, @afrittoli I'd appreciate your feedback on this alternative as well

[ghost]

I do like the locality of scope to when. Is there a risk here that we have collisions in the meaning given by scope and any meaning given by changes from TEP-0050? For example, are we going to end up in a situation where when.scope could be branch but TEP-0050 has introduced a field that circumvents this scope? And then I guess a follow-up question would be: does this potential conflict of meaning matter?

If we introduce multiple fields that can interact in complex ways like this then we should probably try and get a handle on what those interactions will be and decide if there are design ramifications we can deal with now, or if we'll disallow them entirely through validation, or simply document them, or ... something else.

I went over and re-read TEP-0050 again but because it's at proposal stage atm I'm not totally clear how the concrete implementation there will shake out wrt this feature.

Apologies if I've misunderstood the overlap here!

[jerop]

I do like the locality of scope to when. Is there a risk here that we have collisions in the meaning given by scope and any meaning given by changes from TEP-0050? For example, are we going to end up in a situation where when.scope could be branch but TEP-0050 has introduced a field that circumvents this scope? And then I guess a follow-up question would be: does this potential conflict of meaning matter?

TEP-0050 is about ignoring Task failures only (does not include Task skipping) so I believe that it won't introduce a field that does the same thing as scope or circumvents scope

If we introduce multiple fields that can interact in complex ways like this then we should probably try and get a handle on what those interactions will be and decide if there are design ramifications we can deal with now, or if we'll disallow them entirely through validation, or simply document them, or ... something else.

If we go forward with scope under when, we won't need to add a continueOnSkip execution policy

Added this alternative because it provides an easily extensible way for users to specify execution policies to define workflow behavior for each PipelineTask in one place -- and it'll be well aligned with TEP-0050 which would only add ignoreFailure execution policy

[pritidesai]

I went over and re-read TEP-0050 again but because it's at proposal stage atm I'm not totally clear how the concrete implementation there will shake out wrt this feature.

Yup, its a long overdue, hoping to have the proposal in by end of this week 🤞

[ghost]

Ahh OK I think I understand better now - it's not so much whether there's going to be a conflict in functionality but whether consolidating these two concepts under a single section (executionPolicies) would make sense.

Edit: I made a long rambly post here but in summary I personally prefer when.scope because of how close it is to the expressions.

executionPolicies seems to me like a more general statement on the intended behaviour of the PipelineTask. And here we're dealing very directly with the outcome of a conditional. Putting this under executionPolicies would be kind of like having an ifElsePolicies in a programming language which changes the behaviour of an if statement depending on those policies. From my pov conditionals should be a relatively known quantity - it would be much more difficult to reason about complex bits of code if that were configurable elsewhere. We're not writing a programming language but I do wonder if a similar effect would happen here?

If we go forward with scope under when, we won't need to add a continueOnSkip execution policy

The lingering question on my mind is whether the skips we're discussing here will always be tied to when. Will there ever come a time when continueOnSkip makes sense for other scenarios? This is probably not something we can answer right now but I feel like if it does happen we can always introduce continueOnSkip with semantics that make sense at that time. In other words, maybe one day we consolidate when.scope with a continueOnSkip policy, if it makes sense to do so.

All of this to say: I like how closely tied the scope is to the expressions and I'm uncertain whether a more general skip policy should apply to the outcome of when.

[jerop]

@sbwsg you make strong points about the options for an if statement should be relatively known, so should not be defined elsewhere, will include that in the tep as well! (your post wasn't rambly, reading your thought process was helpful! 😁)

[pritidesai]

/assign

[pritidesai]

/assign @sbwsg

[vdemeester]

This means we will provide a default for scope — which will be Branch in order to not break backward compatibility, right ?
If so, any reason for, at some point (in v1) removing the default value ?

[jerop]

@vdemeester no no, that means that we'll initially support both the current syntax and the proposed syntax

current syntax:

when:
  - input: 'foo'
    operator: in
    values: [ 'bar' ]

proposed syntax:

when:
  scope: Task / Branch
  expressions:
    - input: 'foo'
      operator: in
      values: [ 'bar' ]

then only take the proposed syntax into v1

the proposed syntax would not provide a default for scope, we'll require users to specify it because they have different expectation/intuition for the default behavior (#388) so being explicit about it ensures no one is surprised

I'd argue that this does not break backwards compatibility because the current syntax would continue working as is for a while until we stop supporting it later, what do you think?

/cc @bobcatfish

[vdemeester]

oh I can't read properly it seems 😛
Yes it makes sense to me now 😉

[ghost]

Would it be possible to include a snippet of how the PipelineTask or WhenExpressions struct will look in order to support these two shapes? I'm trying to wrap my head around it but don't fully grasp how the dual format can be supported in code.

[jerop]

To support both shapes under when, we'd detect whether it's a list or dictionary in UnmarshalJSON that implements the json.Unmarshaller interface, using the first character (it's ugly but didn't find another way)

We do the same thing in Parameters to detect whether the type of the value is a String or Array:
https://github.com/tektoncd/pipeline/blob/879ba4f65732808a0614d76d5993af0bac736009/pkg/apis/pipeline/v1beta1/param_types.go#L98-L118

Kubernetes does the same thing in IntOrString to detect whether the type is Int or String:
https://github.com/kubernetes/apimachinery/blob/8daf28983e6ecf28bd8271925ee135c1179ad13a/pkg/util/intstr/intstr.go#L80-L99

[ghost]

Oh, awesome, thanks @jerop! I remember us talking about this before as well; sorry I'd completely forgotten about this approach :D

[jerop]

no worries, I can add this detail in the proposal so other reviewers consider it too

[ghost]

I do like the locality of scope to when. Is there a risk here that we have collisions in the meaning given by scope and any meaning given by changes from TEP-0050? For example, are we going to end up in a situation where when.scope could be branch but TEP-0050 has introduced a field that circumvents this scope? And then I guess a follow-up question would be: does this potential conflict of meaning matter?

If we introduce multiple fields that can interact in complex ways like this then we should probably try and get a handle on what those interactions will be and decide if there are design ramifications we can deal with now, or if we'll disallow them entirely through validation, or simply document them, or ... something else.

I went over and re-read TEP-0050 again but because it's at proposal stage atm I'm not totally clear how the concrete implementation there will shake out wrt this feature.

Apologies if I've misunderstood the overlap here!

[ghost]

/approve

[afrittoli]

This looks like a solid proposal to me, thank you!
Just a note about the deprecation plan / update path.

[afrittoli]

Any reason for having the scope at root level and not associated to individual expressions?
This looks fine to me, it keeps things simpler - but I was wondering if there had been any special reasoning about that already.

I think one of the reasons that is not straight forward to express the behaviour of conditions, is because we don't explicitly express / name branches in the pipeline, they're derived from dependencies. Having the scope attached to a pipeline task seems like a good compromise.

[jerop]

Any reason for having the scope at root level and not associated to individual expressions?
This looks fine to me, it keeps things simpler - but I was wondering if there had been any special reasoning about that already.

interesting 🤔

we haven't considered this idea yet, will think more about it

[afrittoli]

After watching the API WG recording, I wanted to clarify I didn't mean to block the TEP on this question :)
It was just not clear to me why the scope was always attached to the list of conditions.

I've been thinking about a few use cases and as long as there is only one branch under the guarded task, it's possible to do translate this:

A -> B (Cond1@task, Cond2@branch) -> C -> D

into:

A -> B (Cond1@task) -> C (Cond2@branch) -> D

If there is more than one branch after B, it is still possible to do the translation, but Cond2 needs to be repeated on the top of each branch. This feels like a reasonable compromise in favour of having a more simple syntax and implementation. Also the proposed syntax still allows for future developments and adding more granularity in future if we decide to.

[jerop]

wasn't blocked on the question, it was more that i wanted us to consider how the proposed solution would extend to support this scenario in the future so was asking if folks in the API working group had use cases to help in this design

thanks for the follow up @afrittoli!

[afrittoli]

I think it would be good to make a more detailed plan about when we need to deprecate the old syntax.
Would the new syntax enter the API as alpha? If so what would be the plan to move to beta and deprecate the old syntax?

[jerop]

will review the feature gates and add more details here

[pritidesai]

Is there any default value to scope here? Or the users are forced to specify either Task or Branch?

[jerop]

that's correct - we won't provide a default for scope, we'll require users to specify it because they have different expectation/intuition for the default behavior (#388) so being explicit about it ensures no one is surprised

[pritidesai]

I am still trying to understand the proposal here and going back to the same example we had discussed in the continueAfterSkip implementation PR.

How does this proposal address when expressions defined at the parent and some of the dependent tasks. The Task G and its dependents will not be executed at all here if when expressions on task C returns false. There is no problem here but worth documenting the impact of choosing scope:

                 A (when scope: Task)
              /    \
           B      C (when scope: Branch)
         /  \       |
       D   E    G (when scope: Task)
                   / \
                H    I

My overall concern is similar to the comment on previous PR. This proposal is great but has two downsides:

1. As a pipeline author, I have to migrate my pipelines to use this new syntax if I want to take an advantage of this feature (or eventually). We are still in beta so in general should be fine but something to think about.
2. Verbosity of the when expressions is growing even more. Now, in addition to specifying input, operator, and values for each when expression, we are adding scope for a pipelineTask

Piggybacking on @skaegi's example: 😜

tasks:
  - name: skippable-task
    if: $.params.branch == 'dev'
    taskRef:
      name: special-dev-only-task

With scoped when expressions:

tasks:
  - name: skippable-task
    when:
      scope: task
      expressions:
        input: $params.branch
        operator: in
        values: ['dev']
    taskRef:
      name: special-dev-only-task

[jerop]

thanks for the review @pritidesai!

How does this proposal address when expressions defined at the parent and some of the dependent tasks. The Task G and its dependents will not be executed at all here if when expressions on task C returns false. There is no problem here but worth documenting the impact of choosing scope:

                 A (when scope: Task)
              /    \
           B      C (when scope: Branch)
         /  \       |
       D   E    G (when scope: Task)
                   / \
                H    I

yes yes, if the WhenExpressions in task C evaluate to False and scope is Branch then the branch (tasks C, G, H and I) is skipped regardless of the dependent's guards -- will make sure to clarify this with an example in the documentation

My overall concern is similar to the comment on previous PR. This proposal is great but has two downsides:

1. As a pipeline author, I have to migrate my pipelines to use this new syntax if I want to take an advantage of this feature (or eventually). We are still in beta so in general should be fine but something to think about.

it seems to me that pipeline authors have to migrate their pipelines either way to get this feature, at least add one field (two fields in this proposal) -- is there any way we can avoid this?

2. Verbosity of the when expressions is growing even more. Now, in addition to specifying input, operator, and values for each when expression, we are adding scope for a pipelineTask

Piggybacking on @skaegi's example: 😜

tasks:
  - name: skippable-task
    if: $.params.branch == 'dev'
    taskRef:
      name: special-dev-only-task

With scoped when expressions:

tasks:
  - name: skippable-task
    when:
      scope: task
      expressions:
        input: $params.branch
        operator: in
        values: ['dev']
    taskRef:
      name: special-dev-only-task

the first syntax doesn't provide the flexibility to choose whether to skip the task only or the whole branch (it assumes that it guards the task only) while the second provides the flexibility, but agreed that verbosity is growing and happy to discuss any ideas to make this less verbose

another difference is the first one will require us to add an expression language to the API -- we're experimenting with supporting CEL through Custom Tasks without adding it directly to the API

this is one reason not specify scope in individual WhenExpressions as discussed in #407 (comment) (cc @afrittoli)

[pritidesai]

it seems to me that pipeline authors have to migrate their pipelines either way to get this feature, at least add one field (two fields in this proposal) -- is there any way we can avoid this?

yup definitely. We are trying to expand the scope of when expressions from its original design which was based on conditions. I am trying to step back here and question, do we really need when expressions to take that burden and support all the use cases? The when expressions are very helpful and works great for the use cases it was designed for. Now to implement the goal in this TEP skip guard task only, why not introduce one more conditional field, if. If can take a similar pattern (if: <input> <operator> <value>) as when expressions (and can be further discussed) and limit its scope to just the task. Also, if and when both are not allowed on a one single pipelineTask. This makes it simple, flexible, and no migration needed.

[ghost]

How can we figure out what the right balance of verbosity, clarity and features are on this one? It seems like we'll make trade-offs along one of those axes no matter which design we move ahead with.

If an area of the product is complex (and I think all the nuances around conditional execution show that it is) then making it a bit verbose seems like an acceptable trade-off (to me) so long as the purpose is clear and users have the options they need.

[pritidesai]

A (when scope: Task)
|
B (runAfter: A)

This is indirectly modifying the meaning of runAfter, instead of having it run B if A is successful it's now interpolating it as run B if A is successful or skipped. Why not letting the dependencies drive that decision by having them explicitly specify it.

A (when)
|
B (dependent on A)

Now, in the following example, when A is skipped, both B and C will be executed. What if we have a use case to run B if A is skipped/successful but run C only if A is successful. (I am struggling to find a use case but I am sure it’s a very common scenario).

                   A (when scope: Task)
             /              \
   B (runAfter: A)    C (runAfter: A)

[ghost]

Bringing my comment over from today's API WG for any follow-up discussion:

As much as possible I think we should keep the focus on use-cases. If we can't articulate a use-case for a particular shape of graph then it seems much more difficult to make a "value judgement" on whether we should adapt the design or not. Of course if we can capture a use-case then it becomes a lot easier to decide if we want to support it and how the design could adapt to do so.

Maybe a way to resolve this would be to figure out if the tools that we do provide (when, finally, pipelines-in-pipelines) can support the graphs that don't have use-cases? This way we understand where the "escape hatches" are if we ever do a reach a point that someone needs finer-grained control.

[pritidesai]

Thank you all for the great discussion in the API WG this morning. I will try to voice my concerns and suggestions one more time 😉 Please feel free to ignore them 🤣

runAfter was introduced to specify the dependencies (B depends on A) when we had only two states Success or Failure. Pipeline controller/scheduler would block the execution of the tasks if A hit the failure.

Down the line, we introduced conditions which introduced this new internal state Skipped i.e. overall, we had two states Success and Failure and one internal Skipped. The conditions were introduced in such a way that pipeline scheduler would not execute any of the dependent tasks if any of the parent had a condition failure.

We discovered conditions being very expensive, deprecated them with when expressions. When expressions are very efficient and less costlier compared to conditions. But to maintain the backward compatibility, the scheduling logic was kept as is.

Now, we are looking for a way to skip only the task which has when expressions specified and provide a way to continue running dependencies even if one or more parent is Skipped.

My biggest concern is asking the users to update their pipelines to the new syntax even if there is no scheduling changes needed.

Now:

when:
  - input: 'foo'
    operator: in
    values: [ 'bar' ]

After 9 releases:

when:
  scope: Branch
  expressions:
    - input: 'foo'
      operator: in
      values: [ 'bar' ]

Now, since the users are required to change their pipelines, we have an opportunity to design it such that it's easier to adapt and fits more use cases.

Instead of paying the conditions scheduling debt, limit the scope of when expressions to the task by default and have the users explicitly specify when expressions in each dependency. The syntax would not change from what it is today, no need to introduce a new field scope. This new scheduling can be driven by the feature flag. The users relying on the branch scoping of when expressions will add when expression in all the dependencies.

[afrittoli]

A (when scope: Task)
|
B (runAfter: A)

This is indirectly modifying the meaning of runAfter, instead of having it run B if A is successful it's now interpolating it as run B if A is successful or skipped. Why not letting the dependencies drive that decision by having them explicitly specify it.

A (when)
|
B (dependent on A)

Now, in the following example, when A is skipped, both B and C will be executed. What if we have a use case to run B if A is skipped/successful but run C only if A is successful. (I am struggling to find a use case but I am sure it’s a very common scenario).

                   A (when scope: Task)
             /              \
   B (runAfter: A)    C (runAfter: A)

Thanks @pritidesai.
This could be an example use case:

                  (A) Clone repo
                  |
                  (B) Check PR content (runAfter: A)
                   |
                  (C) Build Container Image (when: container image changed - uses result from B)
              /                                        \
  (D) Deploy (runAfter: C)    (E) Update list of builds (runAfter: C)

I want to deploy my application when there is a PR. If there is a change to the docker file I will build it, if not I want to skip the build and deploy anyways. I also have a github page were I list all built images, and I don't want to update that if there is no new image.

Some thoughts from this example.

This is a perfectly valid use case, and I think we can handle it very nicely if we fully embrace Tekton's declarative model with inputs (params) and outputs (results). (C) will publish an image sha as a result. If (C) is skipped, (D) will not run because it has a dependency of the result from (C). (E) also takes the result from (C) as input, but it takes a param input as default value for that result (this feature is not available yet). This means that if (C) does not run, (E) and still run.

If we take this a step further, one could argue that task scope is all we need really. Branch scope can be achieved by creating dependencies between tasks, that are not satisfied if a task is skipped. This approach would force task author to make their tasks more explicit about their inputs and outputs, but it might hinder adoption and re-usability of tasks. Being able to through a branch scope where needed might be helpful.

So perhaps having Task as default would be a good choice? If so we could use the defaulting webhook to set it, so that it would still be explicitly set in the pipeline once it's stored in etcd.

[jerop]

thank you @pritidesai, @sbwsg, @afrittoli, @bobcatfish, @vdemeester for the discussions here and in the api wg group! 🙇🏾

changing default scope of WhenExpressions to Task, cascading WhenExpressions and using Pipelines in Pipelines would provide the flexibility to solve the use cases and scenarios we've looked at

i'm still updating the proposed solution in the tep but thought to share for your early feedback as I fill in the other parts :)

[jerop]

the tep is ready for reviews, please take a look when you can 😀

[ghost]

I think we had a much better path for user migration with the Scoped WhenExpressions Alternative. With the proposal now there may be users who are completely surprised in 9 months when we switch the scope of when and their Pipelines start behaving differently. However I do agree that changing from branch- to task-scoped composes much more nicely with other features.

/approve

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• teps/OWNERS [sbwsg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jerop]

thanks for the review @sbwsg!

With the proposal now there may be users who are completely surprised in 9 months when we switch the scope of when and their Pipelines start behaving differently. However I do agree that changing from branch- to task-scoped composes much more nicely with other features.

will over-communicate during the migration in slack, email and working group meetings (just added this to the migration strategy in the TEP) - the way you did it for HOME & workingDir will be a great example 😁

[pritidesai]

thanks a bunch @jerop for all the hard work, will review the changes next week 🙏

[pritidesai]

🚀 yup it will be essential to over communicate here 👎

[pritidesai]

thanks a bunch @jerop for the patience, appreciate all your efforts fleshing out the design ❤️

/lgtm

[bobcatfish]

hey @jerop i have a late request - im wondering how we want to handle this when we do the v1 release? the 9 months requirement really only applies to the beta types themselves, so with v1 we have a chance to flip the behavior:

• but it would be weird to have beta + v1 behave differently by default
• BUT it would be even weirder to do the v1 release with default behavior we already know we want to change

basically im hoping we can plan to release v1 with the preferred default 🙏 and wondering what your thoughts are (might make sense to add to the TEP also)

[jerop]

hey @bobcatfish 👋🏾

wrote that assuming that in 9 months when we switch to guarding task by default, we'd have not yet released v1 -- but happy to adjust the timeline if we think we'll release v1 sooner

looks that v1 is still a ways off - tektoncd/pipeline#3548 - what do you think @vdemeester?

[jerop]

updates made in #481, thanks @bobcatfish 😁