TEP-0007: Conditions Beta #159
Conversation
tekton-robot
added
the
do-not-merge/work-in-progress
tekton-robot
added
the
size/XL
jerop
force-pushed
the
conditions
branch
4 times, most recently
from
688d3d3
to
7ca16c5
Compare
There was a problem hiding this comment.
This is looking great to me! I'd like to hear if folks who have strong feelings about this have any issues / suggestions ( @jlpettersson @afrittoli ) and I left some thoughts, but I think we could merge this as "proposed" asap (maybe next day or two unless folks raise large issues with it as is) and then open another PR to get it to "implementable" after that
It is In large - this looks good to me - good job @jerop I have some minor comments/suggestions - but they can all be changed/addressed in a later state. |
tekton-robot
removed
the
do-not-merge/work-in-progress
teps/0007-conditions-beta.md
Outdated
|
|
||
| `Key` is the input for the `Guard` checking - can be from `Parameters` or `Results`. `Values` is an array of string values. `Operator` represents a key's relationship to a set of values. | ||
|
|
||
| Valid [Operators](https://github.com/kubernetes/kubernetes/blob/7f23a743e8c23ac6489340bbb34fa6f1d392db9d/staging/src/k8s.io/apimachinery/pkg/selection/operator.go#L24-L32) are `In`, `NotIn`, `Exists` and `DoesNotExist`. If the `Operator` is `In` or `NotIn`, the `Values` array must be non-empty and if the operator is `Exists` or `DoesNotExist`, the `Values` array must be empty. |
There was a problem hiding this comment.
First of all - I really like the introduction of a lightweight way to write condition-expressions - like with matchExpressions.
matchExpressions is designed for Kubernetes Labels. Labels consist of pairs of a Key and a Value. Therefor the first thing is a key: in the syntax and the operators Exists and DoesNotExists means that the key exists - independent of what Value it has.
Does it make sense for us to adapt the wording and possible the operators? For us, we want to use Param or a TaskResult as input instead of a Label-key. Should we use e.g. param: and taskResult: or something similar that fits us better - instead of key:? e.g. is more intuitive for end users of Tekton?
I think the In and NotIn operators fits us good, and users understand them. But if we want to use Exists and DoesNotExists - we should at least explain what it means in our context - or maybe abandon them - since they are not directly transferrable to our context?
We may also be interested in other operators, e.g. IsTrue and IsNotTrue / IsFalse - and maybe IsEmpty and IsNotEmpty - but that may also be added later.
These would just be shortcuts for common things that always can be used with In in combination of values: [""] or values: ["true"] / values: ["false"]
E.g.
name: worskspace-contains-dockerfile
key: $(tasks.dockerfile-check.results.skip)
operator: In
values: ["true"]
could be optimized to:
name: worskspace-contains-dockerfile
key: $(tasks.dockerfile-check.results.skip)
operator: IsTrue
There was a problem hiding this comment.
TLDR; we might want to start with the operators In and NotIn
and wait or potentially abandon Exists and DoesNotExists - in our context.
key: might not be the field name that fits best in our context.
There was a problem hiding this comment.
I was thinking we can keep the generic name (maybe rename it to input) so that it's flexible to accept different string inputs from different tekton resources using the string reference '$(x.y.z)', so that even as Tekton evolves, we won't have to modify to support them here. For example:
Agreed that we should proceed with In and NotIn, then we can explore other Operators later if needed -- mentioned the examples you provided.
name: check-file-exists
input: '$(tasks.file-exists.results.exists)'
operator: In
values: ['true']
---
name: branch-is-main
input: `$(params.branch)`
operator: In
values: [‘main’]
---
name: always-false
input: ‘false’
operator: In
values: [‘’]There was a problem hiding this comment.
yes, input: sounds better than key: in our context.
tekton-robot
added
size/L
teps/0007-conditions-beta.md
Outdated
|
|
||
| `Key` is the input for the `Guard` checking - can be from `Parameters` or `Results`. `Values` is an array of string values. `Operator` represents a key's relationship to a set of values. | ||
|
|
||
| Valid [Operators](https://github.com/kubernetes/kubernetes/blob/7f23a743e8c23ac6489340bbb34fa6f1d392db9d/staging/src/k8s.io/apimachinery/pkg/selection/operator.go#L24-L32) are `In`, `NotIn`, `Exists` and `DoesNotExist`. If the `Operator` is `In` or `NotIn`, the `Values` array must be non-empty and if the operator is `Exists` or `DoesNotExist`, the `Values` array must be empty. |
There was a problem hiding this comment.
I was thinking we can keep the generic name (maybe rename it to input) so that it's flexible to accept different string inputs from different tekton resources using the string reference '$(x.y.z)', so that even as Tekton evolves, we won't have to modify to support them here. For example:
Agreed that we should proceed with In and NotIn, then we can explore other Operators later if needed -- mentioned the examples you provided.
name: check-file-exists
input: '$(tasks.file-exists.results.exists)'
operator: In
values: ['true']
---
name: branch-is-main
input: `$(params.branch)`
operator: In
values: [‘main’]
---
name: always-false
input: ‘false’
operator: In
values: [‘’]
jerop
force-pushed
the
conditions
branch
3 times, most recently
from
997c5e0
to
056ccf4
Compare
| input: '$(tasks.file-exists.results.exists)' | ||
| operator: In | ||
| values: ['true'] | ||
| taskRef: |
There was a problem hiding this comment.
I find this example easy to read, easy to reason about, easy to understand. As this example is written now, this is also not too hard to implement.
tekton-robot
removed
lgtm
| 1. Users need a way to specify logic - `Guards` - to determine if a `Task` should execute or not. | ||
| 1. Users need a way to specify whether to execute dependent `Tasks` when `Guards` of a parent `Task` evaluate to `False`. | ||
| 1. Users should be able to specify multiple `Guards` for a given `Task`. | ||
| 1. Users should be able to use `Outputs` from parent `Tasks` and static input to specify `Guards`. |
There was a problem hiding this comment.
-
What happens when one of the
Guardsfail? Do we exit the pipeline just like its done today? -
How are we addressing
Guardnot producing any task result? The way its works withDAGis: ADAGtask could exit successfully without instantiating a task result/tekton/results/foo. The pipeline exits with failure if this result is referenced by any other task since the result file does not exist. We could follow the same behavior. 🤔
There was a problem hiding this comment.
What happens when one of the Guards fail? Do we exit the pipeline just like its done today?
That is not how it works today. If a guarded tasks condition evaluates to false that task is omitted, and the tasks that depend on it is omitted - but the Pipeline continue to run to completion.
This was a bit unclear from docs before, but I tried to expand on that in tektoncd/pipeline#2636
There was a problem hiding this comment.
- What happens when one of the
Guardsfail? Do we exit the pipeline just like its done today?
Building on @jlpettersson's response, note that in this proposal, the Guards are the When Expressions (with input, operator, and values) -- not the Tasks that produce the Results that the When Expressions operate on.
As it is today with any other Task, if that parent Task fails, the Pipeline will fail. However, if the Guard (When Expression) evaluates to False, the guarded Task will be skipped and it's dependents too (except when continueAfterSkip is specified)
- How are we addressing
Guardnot producing any task result? The way its works withDAGis: ADAGtask could exit successfully without instantiating a task result/tekton/results/foo. The pipeline exits with failure if this result is referenced by any other task since the result file does not exist. We could follow the same behavior. 🤔
So it's not a Guard that produces a Result, the Guard operates on a Result from a parent Task. I agree that, when the Guard is set to operate on a missing resource (Param, Result, etc), that it should exit the Pipeline since it doesn't exist. Added it to the TEP, thank you 🙏
| 1. Users need a way to specify logic - `Guards` - to determine if a `Task` should execute or not. | ||
| 1. Users need a way to specify whether to execute dependent `Tasks` when `Guards` of a parent `Task` evaluate to `False`. | ||
| 1. Users should be able to specify multiple `Guards` for a given `Task`. | ||
| 1. Users should be able to use `Outputs` from parent `Tasks` and static input to specify `Guards`. |
There was a problem hiding this comment.
- What happens when one of the
Guardsfail? Do we exit the pipeline just like its done today?
Building on @jlpettersson's response, note that in this proposal, the Guards are the When Expressions (with input, operator, and values) -- not the Tasks that produce the Results that the When Expressions operate on.
As it is today with any other Task, if that parent Task fails, the Pipeline will fail. However, if the Guard (When Expression) evaluates to False, the guarded Task will be skipped and it's dependents too (except when continueAfterSkip is specified)
- How are we addressing
Guardnot producing any task result? The way its works withDAGis: ADAGtask could exit successfully without instantiating a task result/tekton/results/foo. The pipeline exits with failure if this result is referenced by any other task since the result file does not exist. We could follow the same behavior. 🤔
So it's not a Guard that produces a Result, the Guard operates on a Result from a parent Task. I agree that, when the Guard is set to operate on a missing resource (Param, Result, etc), that it should exit the Pipeline since it doesn't exist. Added it to the TEP, thank you 🙏
|
|
||
| ```yaml | ||
| name: branch-is-main | ||
| input: $(params.branch) |
teps/0007-conditions-beta.md
Outdated
|
|
||
| ### Skipping | ||
|
|
||
| Provide more flexibility when a `Guard` executes as `False` by adding a field, `continueAfterSkip`, used to specify whether execute the `Tasks` that are ordering-dependent on the skipped guarded `Task`. The `continueAfterSkip` field defaults to `false`/`no` and users can set it to `true`/`yes` (case insensitive) to allow for execution of the rest of the branch. |
There was a problem hiding this comment.
Yes, added the clarification
| A `Task` branch is made up of dependent `Tasks`, where there are two types of dependencies: | ||
| - _Resource dependency_: based on resources needed from parent `Task`, which includes Workspaces, `Results` and Resources. | ||
| - _Ordering dependency_: based on runAfter which provides sequencing of `Tasks` when there may not be resource dependencies. | ||
|
|
There was a problem hiding this comment.
Agreed that it adds an implicit resource dependency, added a note about it to the TEP
| - name: check-name-matches | ||
| input: "$(params.githubCommand)" | ||
| key: In | ||
| values: ["", "/test $(params.checkName)"] |
There was a problem hiding this comment.
thanks for looking into this @jlpettersson!
will test it with the when expression I translated it to when we have a poc and yes, in the worst case it can be in a script
There was a problem hiding this comment.
Looking great! Lemme know when you want to merge as "proposed" @jerop , you've got the 👍 from me at least :)
I'm so glad we were able to find a simple solution that gives us so much with so little!!!!!!! ❤️ thanks @jerop and @jlpettersson 🙇♀️
teps/0007-conditions-beta.md
Outdated
| ``` | ||
|
|
||
| ### Efficiency | ||
| To improve the efficiency of guarded execution of `Tasks`, we need to avoid spinning up new `Pods` to check `Guards`. We can use string expressions for `Guard` checking, but we want to avoid adding an opinionated and complex expression language to the Tekton API to ensure Tekton can be supported by as many systems as possible. We propose using a simple expression syntax `When Expressions` (similar to Kubernetes' `When Expressions`) to evaluate `Guards` efficiently. The components of `When Expressions` are `Input`, `Operator` and `Values`. `Input` is the input for the `Guard` checking which can be static inputs or outputs from parent `Tasks`, such as `Parameters` or `Results`. `Values` is an array of string values. `Operator` represents an `Input`'s relationship to a set of `Values`. `Operators` we will use in `Guards` are `In` and `NotIn`. The `Values` array must be non-empty. When we have more than one `Guard`, the guarded `Task` will be executed when all the `Guards` evaluate to `True`. Note that when a `Guard` uses a resource from another `Task`, such as a `Result`, it introduces an implicit resource dependency that makes the guarded `Task` dependent on the resource-producing `Task`. |
There was a problem hiding this comment.
could you include a link to the kubernetes docs and/or code on their match expressions?
(also nitpick but could you add some newlines in this paragraph when the lines get long - makes it easier to comment on specific parts of the paragraph at least :D). it also might be easier to read as several paragraphs, e.g. current looks like:
might be easier to read as something like:
The components of When Expressions are Input, Operator and Values:
- Input is the input for the Guard checking which can be static inputs or outputs from parent Tasks, such as Parameters or Results.
- ...
| - name: check-file-exists | ||
| input: '$(tasks.file-exists.results.exists)' | ||
| operator: In | ||
| values: ['true'] |
There was a problem hiding this comment.
double checking: you can do variable replacement in the "values" field as well as "input" is that right?
There was a problem hiding this comment.
yes, I think it's more useful when you can do replacement in both of them -- will add a note on that too
teps/0007-conditions-beta.md
Outdated
| values: [‘’] | ||
| ``` | ||
|
|
||
| We can explore adding more `Operators` later if needed, such as `IsTrue`, `IsFalse`, `IsEmpty` and `IsNotEmpty`. |
There was a problem hiding this comment.
links to the k8s equivalents of for all of these will help - so folks know why we are mentioning these specific values and not others
(and maybe explicitly mention the fact that < and > are not included here, tho they exist in k8s match expressions?)
There was a problem hiding this comment.
added the links and clarifications, thanks
| A `Task` branch is made up of dependent `Tasks`, where there are two types of dependencies: | ||
| - _Resource dependency_: based on resources needed from parent `Task`, which includes Workspaces, `Results` and Resources. | ||
| - _Ordering dependency_: based on runAfter which provides sequencing of `Tasks` when there may not be resource dependencies. | ||
|
|
There was a problem hiding this comment.
es explicit task ordering is specified with runAfter like @jerop classified it as ordering dependency. Task results (causes implicit dependency) classified as resource dependency.
@jlpettersson it is somewhat by design - i say "somewhat" because it didnt become clear to us until maybe the last 6 months but basically it seems useful to be able to distinguish between these two types of dependencies, and it's not so much that they are explicit vs implicit but that they are, like @pritidesai said:
- Ordering dependency (runAfter)
- Resource dependency (result, pipeline resource, one day workspaces)
Having the controller be able to be aware of the difference between these 2 cases is useful - e.g. specifically in this case we know that (2) + continueAfterSkip is bad; if we only had (1) we wouldn't know that
Back to implicit vs explicit tho: we had some debate about the from syntax that PipelineResources use vs. the variable replacement which implies ordering that we used for results (e.g. $(tasks.foo.results.bar))
Personally I prefer the explicit from - when you see from, you know that ordering is being required as well as a value being used. this is the doc where we discussed it tho im not sure if this makes it clear why we went with variable interpolation instead of from. i think adding support for "from" could still be on the table but id be surprised if we removed the variable interpolation option
|
|
||
| #### CelRun Custom Task | ||
|
|
||
| If we implement guarded execution of `Tasks` is implemented using [Tasks that produce Skip Result](#tasks-that-produce-skip-result), we can then extend it to use [`CustomTasks`](https://docs.google.com/document/d/10nQSeIse7Ld4fLg4lhfgUmNKtewfaFNET3zlMdRnBuQ/edit#heading=h.nz0qjg4cmzp0) to build and experiment with using string expressions for simple `Guards`. In Triggers, we use [Common Expression Language](https://opensource.google/projects/cel) for filtering using a `CEL` interceptor. We can provide a `CelRun CustomTask`, so that we experiment with `CEL` without putting it into the Tekton API. After experimentation with `CelRun` `CustomTask` for a while, we will revisit whether we want to add it to the Tekton API. |
There was a problem hiding this comment.
nice! if this gets merged as "implementable" maybe as a follow up we can at least create an issue about creating a CELRun controller, tho it will no longer block this :D
| workspace: source-repo | ||
| - name: echo-file-exists | ||
| when: | ||
| - name: check-file-exists |
There was a problem hiding this comment.
It would be nice if name: field is optional, similar to name: for steps. Now this is mostly a description (as the names of steps are). And like in this example the input: is self-explanatory.
There was a problem hiding this comment.
sounds good, will make it optional in the implementation
|
Let's merge this as "proposed" and keep discussing in a PR that updates this to "implementable" :D /lgtm |
|
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Conditions is a CRD used to specify a criteria to determine whether or not a Task executes. When other Tekton resources were migrated to beta, it remained in alpha because it was missing features needed by users. After analyzing the feature requests and discussing with users, we have identified that the most critical gaps in Conditions are simplicity, efficiency and skipping. We want to address these gaps so that it can work well with the other Pipeline resources and users can count on its stability.
|
/lgtm |
|
easycla had me worried for a second there XD |
|
/lgtm |
Added Conditions Beta TEP in tektoncd#159 Let's discuss the TEP further and explore making it implementable as per the TEP workflow.
Added Conditions Beta TEP in tektoncd#159 Let's discuss the TEP further and explore making it implementable as per the TEP workflow.
Added Conditions Beta TEP in tektoncd#159 Let's discuss the TEP further and explore making it implementable as per the TEP workflow.
Added Conditions Beta TEP in #159 Let's discuss the TEP further and explore making it implementable as per the TEP workflow.
Proposing the design of When Expressions Status and discussing the alternatives discussed in tektoncd/pipeline#3139 Related TEP: tektoncd#159
Proposing the design of When Expressions Status and discussing the alternatives discussed in tektoncd/pipeline#3139 Related TEP: tektoncd#159
Proposing the design of When Expressions Status and discussing the alternatives discussed in tektoncd/pipeline#3139 Related TEP: tektoncd#159
Proposing the design of When Expressions Status and discussing the alternatives discussed in tektoncd/pipeline#3139 Related TEP: #159
Signed-off-by: Hector Fernandez <hector@chainguard.dev> Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Conditions is a CRD used to specify a criteria to determine whether or
not a Task executes. When other Tekton resources were migrated to beta,
it remained in alpha because it was missing features needed by users.
After analyzing the feature requests and discussing with users, we have
identified that the most critical gaps in Conditions are simplicity,
efficiency and skipping. We want to address these gaps so that it can
work well with the other Pipeline resources and users can count on its
stability.