When Expressions Status

[jerop]

Tracking discussions about When Expressions Status, particularly:

• how to use the current knative's ConditionSucceeded in When Expressions
• introducing a ConditionsSkipped type to use in place of ConditionSucceeded

@pritidesai's comment:

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's comment:

@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's comment:

@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

/assign

[bobcatfish]

A few options:

1. Do what the existing Conditions feature does and set Successful = False with a descriptive Reason
2. Use Successful = True instead (as @imjasonh said: it has successfully done nothing) with a descriptive Reason
3. Don't populate the Successful condition at all when skipped
4. Same as (3) but also add a new Condition such as "Skipped" which would be true
5. (4) could also be combined with 1 or 2
6. Don't create a "taskrun" object at all, do something else

My preference is (4), when skipped, don't set Condition Successful at all, b/c the TaskRun didn't start running at all

[pritidesai]

I think 4 is the ideal choice here with slight modification, introduce a new condition "Skipped" and set it to true for the task with failing when expressions and all its dependencies.

Pros:

• All the skipped tasks (parent and dependencies) will have a consistent status and part of the pipelinerun status.

Cons:

• Pipelinerun status growing with these additional entries compared what we have today with conditionals.

[GregDritschler]

I think it's desirable to get away from shoehorning the condition status under a non-existent TaskRun name (which is how it worked with Condition). What about introducing a separate status key that lists the skipped pipeline task names? That section could also list why each one was skipped (when check failed or branch skipped). Since when is a new feature this doesn't feel like a backward incompatibility to me.

status:
  skipped:
     - pipelineTaskName:  rune2etest
       reason:  "When test failed"
  taskRuns:
     ireallyexist-run-haha0:
       pipelineTaskName:  runut

[bobcatfish]

I think it's worth exploring @GregDritschler 's suggestion before we decide - it is strange that we pretend a TaskRun has been created when one has not

Quick comment about the other options: downside of using Successful:True in this case is that this might imply things we don't want to imply, e.g. right now Successful:True would imply that a Task's results are available

[jerop]

Thank you for the idea @GregDritschler!

Experimented with it and I think that telling users that WhenExpressions evaluated to false without providing the details may not be very helpful. I think we should add Skipped as a map with PipelineTaskName as key and a WhenExpressionsResults (a list of WhenExpressions and their respective true/false results) as value. I prefer this status update because users can know why a Task was skipped because they can see each WhenExpression and the results from evaluating each of them.

Taking this example:

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

We would get:

$ kubectl describe pipelineruns.tekton.dev pipelinerun-to-skip-task 
Name:         pipelinerun-to-skip-task
Namespace:    default
Labels:       tekton.dev/pipeline=pipelinerun-to-skip-task
Annotations:  <none>
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:  in
        Values:
          bar
        Input:     foo
        Operator:  notin
        Values:
          foo
      Name:  run-this-task
      Task Spec:
        Metadata:
        Steps:
          Image:  ubuntu
          Name:   echo
          Resources:
          Script:  echo "Good Morning!"
      When:
        Input:     foo
        Operator:  in
        Values:
          foo
  Timeout:  1h0m0s
Status:
  Completion Time:  2020-08-27T15:07:34Z
  Conditions:
    Last Transition Time:  2020-08-27T15:07:34Z
    Message:               Tasks Completed: 1 (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:  in
        Values:
          bar
        Input:     foo
        Operator:  notin
        Values:
          foo
      Name:  run-this-task
      Task Spec:
        Metadata:
        Steps:
          Image:  ubuntu
          Name:   echo
          Resources:
          Script:  echo "Good Morning!"
      When:
        Input:     foo
        Operator:  in
        Values:
          foo
  Skipped:
    Skip - This - Task:
      Expression:
        Input:     foo
        Operator:  in
        Values:
          bar
      Is True:  false
      Expression:
        Input:     foo
        Operator:  notin
        Values:
          foo
      Is True:  false
  Start Time:   2020-08-27T15:07:30Z
  Task Runs:
    pipelinerun-to-skip-task-run-this-task-r2djj:
      Pipeline Task Name:  run-this-task
      Status:
        Completion Time:  2020-08-27T15:07:34Z
        Conditions:
          Last Transition Time:  2020-08-27T15:07:34Z
          Message:               All Steps have completed executing
          Reason:                Succeeded
          Status:                True
          Type:                  Succeeded
        Pod Name:                pipelinerun-to-skip-task-run-this-task-r2djj-pod-gm6ll
        Start Time:              2020-08-27T15:07:30Z
        Steps:
          Container:  step-echo
          Image ID:   docker-pullable://ubuntu@sha256:6f2fb2f9fb5582f8b587837afd6ea8f37d8d1d9e41168c90f410a6ef15fa8ce5
          Name:       echo
          Terminated:
            Container ID:  docker://df348b8f64165fd15e3301095510136867756b26cc56802b2f933e5aab60f91a
            Exit Code:     0
            Finished At:   2020-08-27T15:07:33Z
            Reason:        Completed
            Started At:    2020-08-27T15:07:33Z
        Task Spec:
          Steps:
            Image:  ubuntu
            Name:   echo
            Resources:
            Script:  echo "Good Morning!"
      When Expressions Status:
        Expression:
          Input:     foo
          Operator:  in
          Values:
            foo
        Is True:  true
Events:
  Type    Reason     Age    From         Message
  ----    ------     ----   ----         -------
  Normal  Started    2m29s  PipelineRun  
  Normal  Running    2m29s  PipelineRun  Tasks Completed: 0 (Failed: 0, Cancelled 0), Incomplete: 1, Skipped: 1
  Normal  Succeeded  2m25s  PipelineRun  Tasks Completed: 1 (Failed: 0, Cancelled 0), Skipped: 1

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

🌡️  Status

STARTED         DURATION    STATUS
6 minutes ago   4 seconds   Succeeded(Completed)

📦 Resources

 No resources

⚓ Params

 No params

🗂  Taskruns

 NAME                                             TASK NAME       STARTED         DURATION    STATUS
 ∙ pipelinerun-to-skip-task-run-this-task-r2djj   run-this-task   6 minutes ago   4 seconds   Succeeded

cc @pritidesai @bobcatfish

[GregDritschler]

@jerop That looks great to me.

[bobcatfish]

I think I like this direction as well - @jerop I hate to ask you to do more TEP work but I could see this making sense as a short TEP? i.e. capturing the different options and some examples, esp. b/c this has ripple effects on projects like CLI and Dashboard that want to be able to tell when something has been skipped (and iirc struggled a bit with the current Success: false approach)

If possible I'd like to avoid blocking your current work: Of all the options, i wonder which would be the easiest to change? I'm thinking that having a separate Skipped section like you do in your example would be easy to change later if we decided to choose a Status Condition approach, and we could even maintain the Skipped section as well for backward compatibility 🤔

Curious what you think also @pritidesai !

[pritidesai]

@bobcatfish its exciting and indicating might be the solution that we are circling back to maintaining a list of skipped pipeline tasks 😹

I like going with @GregDritschler's suggestion as well.

@jerop any impact on the existing PR #3135 if we go this route?

[jerop]

@jerop any impact on the existing PR #3135 if we go this route?

already added Skipped to the PR

I think I like this direction as well - @jerop I hate to ask you to do more TEP work but I could see this making sense as a short TEP? i.e. capturing the different options and some examples, esp. b/c this has ripple effects on projects like CLI and Dashboard that want to be able to tell when something has been skipped (and iirc struggled a bit with the current Success: false approach)

yes, will open a TEP proposing this solution and discussing the alternatives we considered -- in a flow with implementing when expressions but will write the tep shortly

If possible I'd like to avoid blocking your current work: Of all the options, i wonder which would be the easiest to change? I'm thinking that having a separate Skipped section like you do in your example would be easy to change later if we decided to choose a Status Condition approach, and we could even maintain the Skipped section as well for backward compatibility 🤔

the map of skipped tasks and their when expressions + results is easiest to change in my opinion, changing the ConditionSucceeded/ConditionSkipped later could get confusing

[jerop]

@bobcatfish its exciting and indicating might be the solution that we are circling back to maintaining a list of skipped pipeline tasks 😹

why didn't you do it at that time? what was the concern with doing that when you considered it?

[bobcatfish]

(There's some discussion about this too in the conditions proposal - in general id say we haven't given status design the attention it deserves and our discussions always focus more on spec)

[pritidesai]

Here is my take:

List of skipped tasks

Pipelinerun status maintains a list of skipped tasks which could be empty if none are skipped. After evaluating when expressions, a task is pushed onto this list (with when expressions?) if expressions result in failure. Pipelinerun controller does not create taskrun object for that task. The scheduling algorithm checks if any of the parent tasks exist in the list and adds that task to the list as well but this time no when expressions. By the end of the pipeline, the list would have all the tasks which have been skipped and pipelinerun status must have taskruns for the rest of the tasks in case of success.

ConditionSkipped/ConditionTrue along with list of skipped tasks

If we later decide to create taskrun object with ConditionSkipped or ConditionTrue in addition to maintaining a list of skipped tasks, does not impact scheduling algorithm. The only change it brings is the controller being responsible for creating such taskrun object (along with all its dependencies) which would also show up in the pipelinerun status.

If we decide to drop the list of skipped tasks, no changes to scheduling algorithm since its aware of condition status along with the reason even today.

ContinueAfterSkip flag

Later when we implement continueAfterSkip would impact scheduling with an additional check before adding dependent task to the list of skipped tasks. If continutAfterSkip is set to true, add it to the execution queue instead of skipped tasks list.

@jerop I will be happy to help writing TEP if you would like to.

[jerop]

@pritidesai sure! will send you a draft :)