Conditions Beta: When Expressions

[jerop]

Adding WhenExpressions used to efficiently specify guarded execution
of Tasks, without spinning up new pods. We use WhenExpressions 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.
Further details about the design are in Conditions Beta TEP.

The components of WhenExpressions are Input, Operator
and Values:

• Input is the input for the Guard checking which can be static
  inputs or variables, such as Parameters or Results.
• Operator represents an Input's relationship to a set of Values.
  Operators we will use in WhenExpressions are In and NotIn.
• Values is an array of string values. The Values array must be
  non-empty. It can contain static values or variables (Parameters or
  Results).

The declared WhenExpressions are evaluated before the Task is run.
If all the WhenExpressions evaluate to True, the Task is run.
If any of the WhenExpressions evaluate to False, the Task is
skipped.

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes tests (if functionality changed/added)
• Includes docs (if user facing)
• Commit messages follow commit message best practices
• Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

• If you are adding a new binary/image to the cmd dir, please update
  the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

- To run a Task only when certain criteria are met, it is now possible to guard task execution using
the when field, which allows you to list a series of references to WhenExpressions that contain
an Input, an Operator and Values. The valid Operators are in and notin. The WhenExpressions are ANDed.
- Conditions are deprecated, use WhenExpressions instead.

[jerop]

/kind feature

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
internal/builder/v1beta1/pipeline.go	81.8%	82.0%	0.2
pkg/apis/pipeline/v1beta1/pipeline_types.go	60.0%	63.9%	3.9
pkg/apis/pipeline/v1beta1/pipeline_validation.go	96.1%	96.1%	0.0
pkg/apis/pipeline/v1beta1/when_types.go	Do not exist	69.2%
pkg/apis/pipeline/v1beta1/when_validation.go	Do not exist	100.0%
pkg/reconciler/pipelinerun/pipelinerun.go	86.3%	85.6%	-0.7
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	90.2%	90.3%	0.1
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	87.8%	91.6%	3.8

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
internal/builder/v1beta1/pipeline.go	81.8%	82.0%	0.2
pkg/apis/pipeline/v1beta1/pipeline_types.go	60.0%	63.9%	3.9
pkg/apis/pipeline/v1beta1/pipeline_validation.go	96.1%	96.1%	0.0
pkg/apis/pipeline/v1beta1/when_types.go	Do not exist	69.2%
pkg/apis/pipeline/v1beta1/when_validation.go	Do not exist	100.0%
pkg/reconciler/pipelinerun/pipelinerun.go	86.3%	85.6%	-0.7
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	90.2%	90.3%	0.1
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	87.8%	91.6%	3.8

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
internal/builder/v1beta1/pipeline.go	81.8%	82.0%	0.2
pkg/apis/pipeline/v1beta1/pipeline_types.go	60.0%	63.9%	3.9
pkg/apis/pipeline/v1beta1/pipeline_validation.go	96.1%	96.1%	0.0
pkg/apis/pipeline/v1beta1/when_types.go	Do not exist	69.2%
pkg/apis/pipeline/v1beta1/when_validation.go	Do not exist	100.0%
pkg/reconciler/pipelinerun/pipelinerun.go	86.3%	86.6%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	90.2%	90.3%	0.1
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	87.8%	93.1%	5.3

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
internal/builder/v1beta1/pipeline.go	81.8%	82.0%	0.2
pkg/apis/pipeline/v1beta1/pipeline_types.go	60.0%	63.9%	3.9
pkg/apis/pipeline/v1beta1/pipeline_validation.go	96.1%	94.9%	-1.2
pkg/apis/pipeline/v1beta1/when_types.go	Do not exist	69.2%
pkg/apis/pipeline/v1beta1/when_validation.go	Do not exist	97.5%
pkg/reconciler/pipelinerun/pipelinerun.go	86.3%	86.6%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	90.2%	90.3%	0.1
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	87.8%	91.6%	3.8

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
internal/builder/v1beta1/pipeline.go	81.8%	82.0%	0.2
pkg/apis/pipeline/v1beta1/pipeline_types.go	60.0%	63.9%	3.9
pkg/apis/pipeline/v1beta1/pipeline_validation.go	96.1%	96.4%	0.3
pkg/apis/pipeline/v1beta1/when_types.go	Do not exist	100.0%
pkg/apis/pipeline/v1beta1/when_validation.go	Do not exist	100.0%
pkg/reconciler/pipelinerun/pipelinerun.go	86.3%	86.6%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	90.2%	90.3%	0.1
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	87.8%	91.6%	3.8

[pritidesai]

Sorry @jerop have been slow on reviewing your changes. Just started playing with them.

Ran a simple PipelineRun with one task with when expression evaluating to false which resulted in a TaskRun with status failed similar to what happens when the condition fails for a task (status failed with ConditionCheckFailed).

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: pipelinerun-to-skip-task
spec:
  pipelineSpec:
    tasks:
      - name: skip-this-task
        when:
          - input: "foo"
            operator: notin
            values: ["foo"]
        taskSpec:
          steps:
            - name: echo
              image: ubuntu
              script: |
                echo "Good Night!"

$ tkn pr describe pipelinerun-to-skip-task
Name:        pipelinerun-to-skip-task
Labels:
 tekton.dev/pipeline=pipelinerun-to-skip-task

🌡️  Status

STARTED        DURATION    STATUS
1 minute ago   0 seconds   Succeeded(Completed)

🗂  Taskruns

 NAME                                              TASK NAME        STARTED   DURATION   STATUS
 ∙ pipelinerun-to-skip-task-skip-this-task-25tgt   skip-this-task   ---       ---        Failed(WhenExpressionsEvaluatedToFalse)

But the way users might start using when expressions would be to skip or ignore a task based on the param which was not possible before without writing conditions. For an ignore use case, the status failed might not make sense. Also, the taskrun which is listed here is just the taskrun object created as part of the pipelinerun status not actual taskrun which can not be described anyways:

tkn tr describe pipelinerun-to-skip-task-skip-this-task-25tgt
Error: failed to get TaskRun pipelinerun-to-skip-task-skip-this-task-25tgt: taskruns.tekton.dev "pipelinerun-to-skip-task-skip-this-task-25tgt" not found

@bobcatfish how about changing this status failed to something different to signify that the task was skipped? But then in that case, we have to also think about the tasks which are dependent on this skipped/ignored task. Those tasks are also skipped but are not part of the overall pipelinerun status and does not show up in the list of Taskruns.

[jerop]

@pritidesai thanks for looking into this! currently, knative only provides ConditionTrue, ConditionFalse, and ConditionUnknown for ConditionSucceeded. Had gone with ConditionFalse but open to hearing arguments for using the other options.

I've now updated it to our own status ConditionSkipped:

const (
	ConditionSkipped corev1.ConditionStatus = "Skipped"
)

prtrs.Status.SetCondition(&apis.Condition{
        Type:    apis.ConditionSucceeded,
        Status:  corev1.ConditionStatus(ConditionSkipped),
        Reason:  resources.ReasonWhenExpressionsEvaluatedToFalse,
        Message: fmt.Sprintf("When Expressions for Task %s in PipelineRun %s evaluated to False", rprt.TaskRunName, pr.Name),
}) 

That gives this result in your example:

$ kubectl describe pipelineruns.tekton.dev pipelinerun-to-skip-task
Name:         pipelinerun-to-skip-task
Namespace:    default
Labels:       tekton.dev/pipeline=pipelinerun-to-skip-task
API Version:  tekton.dev/v1beta1
Kind:         PipelineRun
Spec:
  Pipeline Spec:
    Tasks:
      Name:  skip-this-task
      Task Spec:
        Metadata:
        Steps:
          Image:  ubuntu
          Name:   echo
          Resources:
          Script:  echo "Good Night!"

      When:
        Input:     foo
        Operator:  notin
        Values:
          foo
  Timeout:  1h0m0s
Status:
  Completion Time:  2020-08-26T12:02:08Z
  Conditions:
    Last Transition Time:  2020-08-26T12:02:08Z
    Message:               Tasks Completed: 0 (Failed: 0, Cancelled 0), Skipped: 1
    Reason:                Completed
    Status:                True
    Type:                  Succeeded
  Pipeline Spec:
    Tasks:
      Name:  skip-this-task
      Task Spec:
        Metadata:
        Steps:
          Image:  ubuntu
          Name:   echo
          Resources:
          Script:  echo "Good Night!"

      When:
        Input:     foo
        Operator:  notin
        Values:
          foo
  Start Time:  2020-08-26T12:02:08Z
  Task Runs:
    Pipelinerun - To - Skip - Task - Skip - This - Task - Phpfz:
      Pipeline Task Name:  skip-this-task
      Status:
        Conditions:
          Last Transition Time:  2020-08-26T12:02:08Z
          Message:               When Expressions for Task pipelinerun-to-skip-task-skip-this-task-phpfz in PipelineRun pipelinerun-to-skip-task evaluated to False
          Reason:                WhenExpressionsEvaluatedToFalse
          Status:                Skipped
          Type:                  Succeeded
        Pod Name:                
      When Expressions Status:
        Evaluation Results:
          Expression:
            Input:     foo
            Operator:  notin
            Values:
              foo
          Result:  false
        Execute:   false
Events:
  Type    Reason     Age    From         Message
  ----    ------     ----   ----         -------
  Normal  Started    3m32s  PipelineRun  
  Normal  Succeeded  3m32s  PipelineRun  Tasks Completed: 0 (Failed: 0, Cancelled 0), Skipped: 1

$ tkn pr describe pipelinerun-to-skip-task
Name:        pipelinerun-to-skip-task
Namespace:   default

🌡️  Status

STARTED          DURATION    STATUS
18 seconds ago   0 seconds   Succeeded(Completed)

📦 Resources

 No resources

⚓ Params

 No params

🗂  Taskruns

 NAME                                              TASK NAME        STARTED   DURATION   STATUS
 ∙ pipelinerun-to-skip-task-skip-this-task-phpfz   skip-this-task   ---       ---        (WhenExpressionsEvaluatedToFalse)

@pritidesai @bobcatfish what do you think? will add what we decide on about the status to the TEP

[bobcatfish]

Looking great! The main thing I'm wondering about right now is working out what exactly we need to include in the Status

[bobcatfish]

@pritidesai @jerop that's a good point about "failed".

one option would be implementing this PR with the same behavior as we use for existing Conditions, and then separately deciding how we want to handle this in the future? that would mean the potential for a backwards incompatible change though.

i dont want to change the behavior of our existing condition - one reason we implemented it the way we did was to satisfy knative duck typing

if anything, id like to see the addition of a new condition specifically for skipped. and perhaps in that case we do not include ConditionSucceeded at all? the fact that we have a few options here makes me want to suggest that we separate that discussion from this PR if we can - i could also see making a separate TEP about it to make sure we explore the options well

[pritidesai]

one option would be implementing this PR with the same behavior as we use for existing Conditions, and then separately deciding how we want to handle this in the future? that would mean the potential for a backwards incompatible change though.

I am afraid of the backwards incompatibility playing role here specially if the current design lands in a release as is (without any updates to the status).

i dont want to change the behavior of our existing condition - one reason we implemented it the way we did was to satisfy knative duck typing

Yup, I am on same page, keep the condition behavior as is.

if anything, id like to see the addition of a new condition specifically for skipped. and perhaps in that case we do not include ConditionSucceeded at all? the fact that we have a few options here makes me want to suggest that we separate that discussion from this PR if we can - i could also see making a separate TEP about it to make sure we explore the options well

Making separate TEP make sense but again concerned about backward compatibility. One quicker way to address this would be to not include the taskrun at all in the pipelinerun status (drop this if condition). This would exclude the task for which when expression evaluated to false along with all its dependencies. And results in a consistent behavior across the whole branch which was skipped (no taskrun object created at all).

Pipelinerun status has a count of that skipped task:

"status": {
                "completionTime": "2020-08-26T16:38:11Z",
                "conditions": [
                    {
                        "lastTransitionTime": "2020-08-26T16:38:11Z",
                        "message": "Tasks Completed: 0 (Failed: 0, Cancelled 0), Skipped: 1",
                        "reason": "Completed",
                        "status": "True",
                        "type": "Succeeded"
                    }
                ],

And describing PR does not list any taskruns:

 tkn pr describe pipelinerun-to-skip-task
Name:        pipelinerun-to-skip-task
Labels:
 tekton.dev/pipeline=pipelinerun-to-skip-task

🌡️  Status

STARTED          DURATION    STATUS
28 seconds ago   0 seconds   Succeeded(Completed)

🗂  Taskruns

 NAME   TASK NAME   STARTED   DURATION   STATUS

But one drawback to this approach is nothing included in the pipelinerun status for that task which will leave users with uncertainty of what happened here 😱 , was the task/branch skipped v/s not attempted at all:

"taskRuns": {
                    "pipelinerun-to-skip-task-skip-this-task-2jdzf": {
                        "pipelineTaskName": "skip-this-task"
                    }
                }

[bobcatfish]

Maybe we can discuss in the API WG on Monday and find a way to unblock this PR at least - @imjasonh had suggested that having Succeeded:True in this case, regardless of other info we might provide (e.g. a Reason and/or an additional Condition) might be a good path forward, since Succeeded:False feels like it's at the root of the current problems with Conditions

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
internal/builder/v1beta1/pipeline.go	81.8%	82.0%	0.2
pkg/apis/pipeline/v1beta1/pipeline_types.go	60.0%	63.9%	3.9
pkg/apis/pipeline/v1beta1/pipeline_validation.go	97.2%	97.4%	0.2
pkg/apis/pipeline/v1beta1/when_types.go	Do not exist	100.0%
pkg/apis/pipeline/v1beta1/when_validation.go	Do not exist	100.0%
pkg/reconciler/pipelinerun/pipelinerun.go	86.3%	86.5%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	90.2%	90.3%	0.1
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	88.4%	91.9%	3.4

[vdemeester]

/lgtm

[vdemeester]

/hold cancel

[jerop]

Thank you for the reviews @vdemeester, @afrittoli, @pritidesai, @bobcatfish, @dibyom and @sbwsg 🙏