Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TEP-0048: task results without results - problem statement #240

Merged
merged 1 commit into from
Aug 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
293 changes: 293 additions & 0 deletions teps/0048-task-results-without-results.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,293 @@
---
title: Task Results without Results
authors:
- "@pritidesai"
- "@jerop"
creation-date: 2020-10-20
last-updated: 2021-06-11
status: proposed
---

# TEP-0048: Task Results without Results

<!-- toc -->
- [Summary](#summary)
- [Motivation](#motivation)
- [Goals](#goals)
- [Non-Goals](#non-goals)
- [Requirements](#requirements)
- [Use Cases](#use-cases)
- [References](#references)
<!-- /toc -->

## Summary

A `task` in a `pipeline` can produce a result and that result can be consumed in many ways within that `pipeline`:

* `params` mapping in a consumer `pipelineTask`

```yaml
kind: Pipeline
spec:
tasks:
- name: format-result
taskRef:
name: format-result
params:
- name: result
value: "$(tasks.sum-inputs.results.result)"
```
* `WhenExpressions`

```yaml
kind: Pipeline
spec:
tasks:
- name: echo-file-exists
when:
- input: "$(tasks.check-file.results.exists)"
operator: in
values: ["yes"]
```

* Pipeline Results

```yaml
kind: Pipeline
spec:
tasks:
...
results:
- name: sum
value: $(tasks.second-add.results.sum)
```

Today, `pipeline` is declared `failure` and stops executing further if the result resolution fails because of
a missing task result. There are many reasons for a missing task result:

* a `task` producing task result failed, no result available
* a `task` producing result was skipped/disabled and no result generated
* a `task` producing result did not generate that result even without any failure. We have a
[bug report](https://github.com/tektoncd/pipeline/issues/3497) open to declare
such a task as failure. This reason might not hold true after issue
[#3497]((https://github.com/tektoncd/pipeline/issues/3497)) is fixed.

Here are the major motivations for `pipeline` authors to design their pipelines with the missing task results:

* Implementing the [TEP-0059: Skipping Strategies](0059-skipping-strategies.md)
proposal to limit the scope of `WhenExpressions` to only that task and continue executing the dependencies.

Let's revisit an [example](0059-skipping-strategies.md#use-cases) of sending a Slack notification when someone
manually approves the PR. This is done by sending the `approver`'s name to the `slack-msg` task as the result of
`manual-approval` task.

Further, extending the same use case, when someone approves the PR, the `approver` would be set to an appropriate
name. At the same time, set the task result `approver` to **None** in case the `manual-approval` task is skipped and
the `approver` is not initialized. It is still possible to send a notification that no one approved the PR.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm interesting - in TEP-0059 we were assuming that the user would WANT to skip the manual approval entirely - including the notification. for example, if this was running against a pull request, would you want to send a slack notification every time the PR is updated? maybe but probably not (which is what i understand to be the premise of the use case in TEP-59)

maybe we can tweak the example a bit to make it work here - in this case, where we're running the pipeline on a PR, maybe we need to start a temporary cluster to deploy to? (since we don't want to deploy to prod)

        lint                     unit-tests
         |                           |
         v                           v
 report-linter-output        integration-tests
                                     |
                                     v
                               manual-approval
                               |            |
                               v        (approver)
(if pull-request)          build-image       |
 set-up-cluster                |             v
        |                      |        slack-msg
        v                      |
 (cluster ip, etc.).           |
        |                      v
        -------------->   deploy-image

In this example, when we run the pipeline for a pull request, we want to setup a cluster and pass the info about that cluster to deploy-image, but when we want to deploy to prod, we want to use params that are passed in with the prod cluster

(this is a bit flawed b/c you probably need more than just the ip and a string result might not be the best way to pass that - but maybe we could imagine in a world with dict params + results this makes a bit more sense?)

so we'd need to be able to express that when set-up-cluster is skipped, instead of trying to use it's results and skipping deploy-image (which depends on set-up-cluster), we use the params

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(or we could remove it, the next 2 use cases have it covered!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a useful use case, where the default results are variables that users would need to pass in as parameters

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, you have precisely captured a tweak here with the use case from TEP-0059.


```
lint unit-tests
| |
v v
report-linter-output integration-tests
|
v
manual-approval
| |
v (approver)
build-image |
| v
v slack-msg
deploy-image
```

Let's look at one more simple use case of conditional task.

```
clone-repo
|
v
check-PR-content
|
(image changed)
|
v
build-image
|
(image)
|
______________
| |
v v
deploy-image update-list-of-builds
```

Here, the `pipeline` checks the changes being proposed in a PR. If the changes include updating an image,
`build-image` is executed to build a new image and publish it to a container registry. `deploy-image` deploys
this newly built image after resolving the result from `build-image`. If `build-image` was skipped and did not
create any new image, `deploy-image` need to deploy an already existing latest image which could be set as the
default by the pipeline.

This is not possible today without setting any default for the results. `deploy-image` will fail as the result
resolution fails when `build-image` is not executed.

* Initialize `pipeline` results using the results of one of the two conditional tasks. The `pipeline` has two
conditional tasks, `build-trusted` and `build-untrusted`. The `pipeline` executes one of the tasks based on the type of
the builder. Now, irrespective of how the image was built, propagate the name of the image which was built to the
pipeline results. This is not possible today. The task result resolution fails to resolve the missing result and
declares the consolidating task as a failure along with the `pipeline`.

```
git-clone
trusted | | untrusted
v v
build-trusted build-untrusted
| |
(image) (image)
| |
______________
|
v
propogate APP_IMAGE to pipeline results
```

## Motivation

Missing the task results do not have to be fatal. Provide an option to the `pipeline` author to build `pipeline`
that can continue executing even when a task result is missing.

### Goals

* Enable a `pipeline` to execute the `pipelineTask` when that task is consuming the results of conditional tasks.

* Enable a `pipeline` to produce `pipeline results` produced by the conditional tasks.

### Non-Goals

Producing the task result in case of a failed task is out of the scope of this TEP.

## Requirements

### Use Cases

### Consuming task results from the conditional tasks

`deploy-image` requires a default image name to deploy on a cluster when `build-image` is skipped because the
PR had no changes to a docker file.

```yaml
spec:
tasks:
# Clone runtime repo
- name: git-clone
taskRef:
name: git-clone
# check the content of the PR i.e. the changes proposed
# does any of those changes contain changing a dockerfile
# if so, build a new image, otherwise, skip building an image
- name: check-pr-content
runAfter: [ "git-clone" ]
taskRef:
name: check-pr-content
results:
- name: image-change
# build an image if the platform developer is committing changes to a dockerfile or any other file which is part of
# the image
- name: build-image
runAfter: [ "check-pr-content" ]
when:
- input: "$(tasks.check-pr-content.results.image-change)"
operator: in
values: ["yes"]
taskRef:
name: build-image
results:
- name: image-name
# deploy a newly built image if build-image was successful and produced an image name
# deploy a latest platform by default if there are no changes in this PR
- name: deploy-image
runAfter: [ "build-image" ]
params:
- name: image-name
value: "$(tasks.build-image.results.image-name.path)"
taskRef:
name: deploy-image
# update the page where a list of builds is maintained with this new image
- name: update-list-of-builds
runAfter: [ "build-image" ]
params:
- name: image-name
value: "$(tasks.build-image.results.image-name.path)"
when:
- input: "$(tasks.build-image.status)"
operator: in
values: ["succeeded"]
taskRef:
name: update-list-of-builds
```

### `Pipeline Results` from the conditional tasks

Produce the name of the image as the pipeline result depending on how the image was built.

```yaml
spec:
tasks:
# Clone application repo
- name: git-clone
taskRef:
name: git-clone
# TRUST_BUILDER is set to true at the pipelineRun level if the builder image is trusted
# if the builder image is trusted, executed build-trusted and produce an image name as a result
- name: build-trusted
runAfter: [ "git-clone" ]
when:
- input: "$(params.TRUST_BUILDER)"
operator: in
values: ["true"]
taskRef:
name: build-trusted
results:
- name: image
# TRUST_BUILDER is set to false at the pipelineRun level if the builder image is not trusted
# and needs to run in isolation
# if the builder image is not trusted, executed build-un trusted and produce an image name as a result
- name: build-untrusted
runAfter: [ "git-clone" ]
when:
- input: "$(params.trusted)"
operator: in
values: ["false"]
taskRef:
name: build-untrusted
results:
- name: image
# read result of both build-trusted and build-untrusted and propagate the one which is initialized as a pipeline result
- name: propagate-image-name
runAfter: [ "build-image" ]
params:
- name: trusted-image-name
value: "$(tasks.build-trusted.results.image)"
- name: untrusted-image-name
value: "$(tasks.build-untrusted.results.image)"
taskRef:
name: propagate-image-name
results:
- name: image
# pipeline result
results:
- name: APP_IMAGE
value: $(tasks.propagate-image-name.results.image)
```

Copy link
Contributor

@ScrapCodes ScrapCodes Jul 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it correct to think, another possible use case can be, for resuming failed pipelines.

The tasks that ran fine can be somehow disabled (either with some future TEP or when expression), and their results pre-populated, while copying and creating a new Pipeline/PipelineRun in order to retry/resume.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's an interesting use case for this @ScrapCodes 🤔

my feeling is that in the resuming a pipeline case, usually youd want to use the result value produced by the previous run, and not the default value - tho maybe sometimes you would?

for example, say you were trying to do a build + deploy:

spec:
  tasks:
    - name: build-image
      taskRef:
        name: build-image
      results:
        - name: image-name
    - name: deploy-image
      params:
        - name: image-name
          value: "$(tasks.build-image.results.image-name)" # using the image that was built by build-image
      taskRef:
        name: deploy-image

Maybe deploy-image failed, and you want to run the pipeline again, but you don't want to have to build again, you probably want to use the value of tasks.build-image.results.image-name from the previous run.

If you used the functionality proposed here, you'd be using some default value for the image-name instead of the value that was just built. For example, imagining a syntax for specifying a default to use:

spec:
  tasks:
    - name: build-image
      taskRef:
        name: build-image
      results:
        - name: image-name
    - name: deploy-image
      params:
        - name: image-name
          value: "$(tasks.build-image.results.image-name?my-website:v0.4.0)"
      taskRef:
        name: deploy-image

Let's say this built an image my-website:v0.7.0, but deploy-image failed. If you wanted to re-run, i.e. partially run the pipeline with build-image disabled, I'm guessing you'd want to pass my-website:v0.7.0 (the one you just built) and not use the default my-website:v0.4.0

Probably there are cases for both tho!


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another proposal that came up (I'm not sure where?? i feel like @chhsia0 was involved but not sure) is to introduce a defaulting syntax when using the variable interpolation for results, e.g. something like:

$(tasks.foo.results.bar?somedefault) (or maybe instead of somedefault we require reference to another var?)

I might be kinda crazy but i actually like this option b/c it puts the control completely on the consuming PipelineTask (like option b) but also works for when expressions

if we go this route hopefully we could borrow the syntax from somewhere else

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is interesting -- just wondering how it'd look like if the default was a variable

would it be $(tasks.foo.results.bar?$(params.someDefault)) 🙃 -- maybe $(tasks.foo.results.bar?params.someDefault) is better?

but then how would this look like if we add support for array results proposed in #477?

(back to focusing on the problem statement only lol)

## References

* [Brainstorming on Finally, Task Results, and Default](https://docs.google.com/document/d/1tV1LgPOINnmlDV-oSNdLB39IlLcQRGaYAxYZjVwVWcs/edit?ts=5f905378#)

* [Design Doc - Task Results in Finally](https://docs.google.com/document/d/10iEJqVstY6k3KNvAXgffIJLcHRbPQ-GIAfQk5Dlrf3c/edit#)

* [Issue reported - "when" expressions do not match user expectations](https://github.com/tektoncd/pipeline/issues/3345)

* [Accessing Execution status of any DAG task from finally](https://github.com/tektoncd/community/blob/master/teps/0028-task-execution-status-at-runtime.md)
1 change: 1 addition & 0 deletions teps/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ This is the complete list of Tekton teps:
|[TEP-0045](0045-whenexpressions-in-finally-tasks.md) | WhenExpressions in Finally Tasks | implemented | 2021-06-03 |
|[TEP-0046](0046-finallytask-execution-post-timeout.md) | Finally tasks execution post pipelinerun timeout | implementable | 2021-04-14 |
|[TEP-0047](0047-pipeline-task-display-name.md) | Pipeline Task Display Name | proposed | 2021-02-10 |
|[TEP-0048](0048-task-results-without-results.md) | Task Results without Results | proposed | 2021-06-11 |
|[TEP-0049](0049-aggregate-status-of-dag-tasks.md) | Aggregate Status of DAG Tasks | implemented | 2021-06-03 |
|[TEP-0050](0050-ignore-task-failures.md) | Ignore Task Failures | proposed | 2021-02-19 |
|[TEP-0051](0051-ppc64le-architecture-support.md) | ppc64le Support | proposed | 2021-01-28 |
Expand Down