updating doc to add note on task results

[pritidesai]

Changes

Task execution order is set such that the task producing a result is executed before the task referencing that result. In case of a failure of a referenced task, pipeline exits without executing the rest of the pipeline tasks. But when a referenced task succeeds but does not instantiate the result, the task with referencing result is not executed and pipeline fails. Pipeline exits with InvalidTaskResultReference since the result did not exist.

An example of such task and pipeline execution here.

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

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

PipelineRuns that fail due to a missing or mistyped result reference in variables will now be marked with a InvalidTaskResultReference reason in their status

[pritidesai]

this is coming from today's API Work Group discussion here

[bobcatfish]

/hold

discussed a bit in the API working group meeting today, but I'm not sure this is the right error for this situation. Also afaik this error only happens when something is trying to use the result that is missing, if a Task provides a result but doesn't and nothing consumes it, I don't think there is an error but there should be (I'm like 80% sure?)

So I think instead we should change this behavior to be consistent and maybe use a different error.

[pritidesai]

thanks @bobcatfish, I just verified that pipeline does not produce any error if a task is set to provide a result but none of the PipelineTask consumes it, for example, task sum-two-inputs produces sum-result but not consumed by any other task:

apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: sum-two-ints
  annotations:
    description: |
      A simple task that sums the two provided integers
spec:
  params:
    - name: a
      type: string
      default: "1"
      description: The first integer
    - name: b
      type: string
      default: "1"
      description: The second integer
  results:
    - name: sum-result
      description: The sum of the two provided integers
  steps:
    - name: sum
      image: bash:latest
      script: |
        #!/usr/bin/env bash
        echo $(params.a)
        echo $(params.b)
        echo -n $(( "$(params.a)" + "$(params.b)" )) | tee $(results.sum-result.path)
---

apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: format-result
spec:
  params:
    - name: result
      type: string
      default: "0"
  steps:
    - name: print-sum
      image: ubuntu
      script: echo -n "*** Result = $(params.result) ***"
---

apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: sum-and-print-pipeline
spec:
  params:
    - name: a
      type: string
      default: "1"
    - name: b
      type: string
      default: "1"
  tasks:
    - name: sum-inputs
      taskRef:
        name: sum-two-ints
      params:
        - name: a
          value: "$(params.a)"
        - name: b
          value: "$(params.b)"
    - name: print-sum
      taskRef:
        name: format-result
      params:
        - name: result
          value: "$(params.a)"
#          value: "$(tasks.sum-inputs.results.sum-result)"
---

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: sum-and-print-pipeline-run
spec:
  pipelineRef:
    name: sum-and-print-pipeline
  params:
    - name: a
      value: "100"
    - name: b
      value: "1000"
---

Pipeline exits with Succeeded and have the result in the status of that TaskRun:

       "sum-and-print-pipeline-run-sum-inputs-mvx9s": {
                "pipelineTaskName": "sum-inputs",
                "status": {
                   "taskResults": [
                        {
                            "name": "sum-result",
                            "value": "1100"
                        }
                    ],

Change this to produce validation failure (MissingTaskResultReference)? This can be captured at the PipelineTask validation before creating any resources or tasks.

Yup I agree the PipelineValidationFailed is very generic but thats what we have today. How about change this to something like InvalidTaskResultReference? 🤔

[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
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	88.6%	90.9%	2.3

[pritidesai]

@bobcatfish I have replaced the failure PipelineValidationFailed with InvalidTaskResultReference

[ghost]

/approve

[ghost]

Re-reading @bobcatfish's comment it sounds like we might want to update our TaskRun reconciler code to reject completed TaskRuns which haven't output all the results listed in the TaskSpec. The code which is pulling the results out of the termination log is here: https://github.com/tektoncd/pipeline/blob/master/pkg/reconciler/taskrun/taskrun.go#L606-L642 so it seems we could compare the given results to those in the spec and update the TaskRun's status to failed if they don't match?

We could do this as part of this PR or in a separate one. @pritidesai @bobcatfish wdyt?

[ghost]

/approve cancel

Removing approve until we get consensus on what we want to do.

[pritidesai]

@sbwsg @bobcatfish I could create a separate PR for TaskRun reconciler changes where taskRun is declared failed if results are not populated.

updateTaskRunResourceResult is only executed when a taskRun is declared successful. I think the taskRun.status should not be updated and make it successful without checking all the results. Otherwise, we declare a taskRun success and updateTaskRunResourceResult would change the status to failure if results are not populated. So before marking taskRun as complete and successful here we add a check to validate results. @sbwsg what are your thoughts?

Also, after we change this behavior, pipeline might not run into InvalidTaskResultReference since the taskRun will be declared as failure instead of successful like we do today 🤔

[pritidesai]

We still need a PR for MissingTaskResultReference (taskResults of a task are neither referenced in any pipeline task nor in pipeline results), I can create one after we get agreement on the proposed behavior, @bobcatfish wdyt?

[pritidesai]

I created an issue to track this discussion and any pending PRs: #2701

[ghost]

So before marking taskRun as complete and successful here we add a check to validate results. @sbwsg what are your thoughts?

Sounds good, this makes total sense to me!

after we change this behavior, pipeline might not run into InvalidTaskResultReference since the taskRun will be declared as failure instead of successful like we do today 🤔

Yeah I think you're right about that! One edge case: what if the Task does write a Result but chooses to make it an empty file? The TaskResult will be an empty string. Personally I think that's acceptable - the Task author has intentionally chosen to output a blank Result, rather than misnamed or forgotten to emit it.

We still need a PR for MissingTaskResultReference (taskResults of a task are neither referenced in any pipeline task nor in pipeline results)

I'm less sure about this one. If a Task produces Results but they go unused by any PipelineTasks or PipelineResults, I think that's fine. Example: a git-clone Task produces a commitSHA result. Should the user be required to make use of that result in their Pipeline? That doesn't feel quite right to me. I might be misunderstanding though?

[pritidesai]

/kind misc

[pritidesai]

@bobcatfish @sbwsg

I have reduced the scope down of this PR to change the error message from PipelineValidationFailed to InvalidTaskResultReference to make it more explicit in case of a failure and added note in pipelines.doc for the same.

[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:

• OWNERS [sbwsg]

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

[vdemeester]

@pritidesai what is the status of this ? it needs a rebase 🙃

[pritidesai]

/remove-kind misc

[pritidesai]

sorry for delay @vdemeester its ready for review again
@bobcatfish I am dropping hold as I have narrowed down scope of the changes here. Its now fixing the error reported and updating doc for the same.
/hold cancel

[ghost]

This looks good to me but I've already approved. @tektoncd/core-maintainers anyone available to review and lgtm?

[ghost]

Given the reduced scope of this change and the age of this PR I'm going to go ahead and assume there's no more feedback and give this an lgtm in addition to my approve earlier.

/lgtm

[bobcatfish]

Thanks for following up @sbwsg - should there be a release note to go along with this? It seems like the implication is the user facing behavior is changing (if only slightly)

[ghost]

Good thinking I've updated the release-note in the PR description.