TEP-0040 implementation - specifying `onError` in a step #4106

pritidesai · 2021-07-20T06:37:36Z

Changes

This PR implements TEP-0040 - ignore a step error 🛎.

When a step in a task results in a failure, the rest of the steps in the task are skipped and the taskRun is declared a failure. If you would like to ignore such step errors and continue executing the rest of the steps in the task, you can specify onError for such a step.

onError can be set to either continue or fail as part of the step definition. If onError is set to continue, the entrypoint sets the original failed exit code of the script in the container terminated state. A step with onError set to continue does not fail the taskRun and continues executing the rest of the steps in a task.

For example,

steps:
  - image: docker.io/library/golang:latest
    name: ignore-unit-test-failure
    onError: continue
    script: |
      go test .

The original failed exit code will be part of the run status:

 "steps": [
    {
      "container": "step-ignore-unit-test-failure",
      "imageID": "...",
      "name": "ignore-unit-test-failure",
      "terminated": {
        "containerID": "...",
        "exitCode": 1,
        "finishedAt": "2021-06-21T18:22:05Z",
        "reason": "Completed",
        "startedAt": "2021-06-21T18:22:05Z"
      }
    },
  ],

And will be available in a file /tekton/steps/0/exitCode.

This is an alpha feature. The enable-api-fields feature flag must be set to "alpha" to specify onError for a step.

This commit includes following changes:

Changing entrypoint to include three new flags on_error, step_metadata_dir, and step_metadata_dir_link.
Adding a new function as part of the runner CreateDirWithSymlink.
Creating a volume /tekton/steps/.
Supporting a path variable $(steps.step-.exitCode.path) and $(steps.step-unnamed-.exitCode.path).
API additions - onError - while defining a step.
Writing exitCode at /tekton/steps/step-/exitCode or /tekton/steps/step-unnamed-/exitCode.
Set the exitCode of a terminated state to a non-zero exit code.
Doc, unit test, and examples for this feature.

/kind feature

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

Use `onError` for a step if you would like to ignore a step error and continue executing the rest of the steps in the task. Set `onError` to `continue` to ignore a step error. When `onError` is set to `continue`,  the entrypoint sets the original failed exit code of the script in the container terminated state. A `step` with `onError` set to `continue` does not fail the `taskRun` and continues executing the rest of the steps in a task.

tekton-robot · 2021-07-20T06:39:42Z

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
cmd/entrypoint/post_writer.go	0.0%	63.6%	63.6
internal/builder/v1beta1/step.go	42.1%	38.1%	-4.0
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.0%	0.1
pkg/entrypoint/entrypointer.go	66.1%	71.0%	4.9
pkg/pod/entrypoint.go	85.9%	87.2%	1.3
pkg/pod/pod.go	87.6%	87.4%	-0.2
pkg/pod/status.go	91.8%	90.8%	-1.0
pkg/reconciler/taskrun/taskrun.go	76.9%	77.0%	0.1

tekton-robot · 2021-07-20T07:00:37Z

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
cmd/entrypoint/post_writer.go	0.0%	63.6%	63.6
internal/builder/v1beta1/step.go	42.1%	38.1%	-4.0
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.0%	0.1
pkg/entrypoint/entrypointer.go	66.1%	71.0%	4.9
pkg/pod/entrypoint.go	85.9%	87.2%	1.3
pkg/pod/pod.go	87.6%	87.4%	-0.2
pkg/pod/status.go	91.8%	90.8%	-1.0
pkg/reconciler/taskrun/taskrun.go	76.9%	77.0%	0.1

tekton-robot · 2021-07-20T16:37:00Z

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
cmd/entrypoint/post_writer.go	0.0%	63.6%	63.6
internal/builder/v1beta1/step.go	42.1%	38.1%	-4.0
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.0%	0.1
pkg/entrypoint/entrypointer.go	66.1%	71.0%	4.9
pkg/pod/entrypoint.go	85.9%	87.2%	1.3
pkg/pod/pod.go	87.6%	87.4%	-0.2
pkg/pod/status.go	91.8%	90.8%	-1.0
pkg/reconciler/taskrun/taskrun.go	76.9%	77.0%	0.1

tekton-robot · 2021-07-20T20:57:17Z

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
cmd/entrypoint/post_writer.go	0.0%	63.6%	63.6
internal/builder/v1beta1/step.go	42.1%	38.1%	-4.0
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.0%	0.1
pkg/entrypoint/entrypointer.go	66.1%	71.0%	4.9
pkg/pod/entrypoint.go	85.9%	87.2%	1.3
pkg/pod/pod.go	87.6%	87.4%	-0.2
pkg/pod/status.go	91.8%	90.8%	-1.0
pkg/reconciler/taskrun/taskrun.go	76.9%	77.0%	0.1

tekton-robot · 2021-07-20T21:51:15Z

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
cmd/entrypoint/post_writer.go	0.0%	63.6%	63.6
internal/builder/v1beta1/step.go	42.1%	38.1%	-4.0
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.0%	0.1
pkg/entrypoint/entrypointer.go	66.1%	71.0%	4.9
pkg/pod/entrypoint.go	85.9%	87.2%	1.3
pkg/pod/pod.go	87.6%	87.4%	-0.2
pkg/pod/status.go	91.8%	90.8%	-1.0
pkg/reconciler/taskrun/taskrun.go	76.9%	77.0%	0.1

tekton-robot · 2021-07-20T23:40:45Z

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
cmd/entrypoint/post_writer.go	0.0%	61.9%	61.9
internal/builder/v1beta1/step.go	42.1%	38.1%	-4.0
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.0%	0.1
pkg/entrypoint/entrypointer.go	66.1%	71.0%	4.9
pkg/pod/entrypoint.go	85.9%	87.2%	1.3
pkg/pod/pod.go	87.6%	87.4%	-0.2
pkg/pod/status.go	91.8%	90.8%	-1.0
pkg/reconciler/taskrun/taskrun.go	76.9%	77.0%	0.1

tekton-robot · 2021-07-20T23:58:52Z

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
cmd/entrypoint/post_writer.go	0.0%	61.9%	61.9
internal/builder/v1beta1/step.go	42.1%	38.1%	-4.0
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.0%	0.1
pkg/entrypoint/entrypointer.go	66.1%	71.0%	4.9
pkg/pod/entrypoint.go	85.9%	87.2%	1.3
pkg/pod/pod.go	87.6%	87.4%	-0.2
pkg/pod/status.go	91.8%	90.8%	-1.0
pkg/reconciler/taskrun/taskrun.go	76.9%	77.0%	0.1

pritidesai · 2021-07-22T16:36:54Z

cmd/entrypoint/main.go

+	stepPath = flag.String("step_path", "", "Relative step path, creates the specified path under /tekton/steps/ which "+
+		"can be used to store the step metadata e.g. <step-name> in /tekton/steps/<step-name>/")
+	stepPathLink = flag.String("step_path_link", "", "Relative step path, creates a symbolic link to the "+
+		"specified step path e.g. <step-index> in /tekton/steps/<step-index>")


Three optional flags here introduced here:

onError - this flag is initialized to either continue or fail based on the step definition

stepPath - the relative path specified in this flag is created under /tekton/steps/

stepPathLink - the relative path specified in this flag is created as a symbolic link to stepPath

What's the reasoning about stepPath and stepPathLink ? (aka are they required for this feature, if yes, how/why?)

pritidesai · 2021-07-22T16:40:34Z

cmd/entrypoint/main.go

+			}
+			// log and exit only if a step error must cause run failure
+			if e.OnError != entrypoint.ContinueOnError {
+				log.Fatalf("Error executing command (ExitError): %v", err)


Checking if onError is set to continue to allow a step error. By default, the entrypoint considers a container exiting with non-zero as error and hence not included any special logic for onError set to fail. We can change this logic if needed.

pritidesai · 2021-07-22T16:41:59Z

cmd/entrypoint/post_writer.go

@@ -20,3 +20,41 @@ func (*realPostWriter) Write(file string) {
 		log.Fatalf("Creating %q: %v", file, err)
 	}
 }
+


Two more functions are introduced (1) to create a file and write the specified content in the file (2) to create a directory and a symbolic link.

pritidesai · 2021-07-22T16:47:23Z

pkg/entrypoint/entrypointer.go

+		e.WritePostFile(e.PostFile, nil)
+		e.WriteExitCodeFile(e.StepPathLink, "0")
+	default:
+		// for a step without continue on error and any error, write a post file with .err


creating an InternalTektonResultType with a failed exit code similar to StartedAt time which is then read while updating the pod status.

pritidesai · 2021-07-22T17:22:00Z

The changes are ready for review, I will add more unit tests if we agree with the overall implementation 🙏 Appreciate your comments 🥺

vdemeester

Few question, more on design than on code (things I kinda missed during the TEP, that's fine though 😛)

vdemeester · 2021-08-05T15:07:46Z

cmd/entrypoint/main.go

+	stepPath = flag.String("step_path", "", "Relative step path, creates the specified path under /tekton/steps/ which "+
+		"can be used to store the step metadata e.g. <step-name> in /tekton/steps/<step-name>/")
+	stepPathLink = flag.String("step_path_link", "", "Relative step path, creates a symbolic link to the "+
+		"specified step path e.g. <step-index> in /tekton/steps/<step-index>")


What's the reasoning about stepPath and stepPathLink ? (aka are they required for this feature, if yes, how/why?)

vdemeester · 2021-08-05T15:10:55Z

docs/tasks.md

+steps:
+  - image: docker.io/library/golang:latest
+    name: ignore-unit-test-failure
+    onError: continue


Just realizing now that… we could use onError with the debug feature too, like onError: debug 😅

vdemeester · 2021-08-05T15:13:27Z

docs/tasks.md

+#### Accessing Step's `exitCode` in subsequent `Steps`
+
+A step can access the exit code of any previous step using the `path` similar to a task result, for example:
+
+```shell
+$(steps.step-<step-name>.exitCode.path)
+```
+
+The `exitCode` of a step without any name can be referenced using:
+
+```shell
+$(steps.step-unnamed-<step-index>.exitCode.path)
+```
+
+If you would like to use the tekton internal path, you can access the exit code by reading the file
+(which is not recommended though since the path might change in the future):
+
+```shell
+cat /tekton/steps/step-<step-name>/exitCode
+```
+
+And, access the step exit code without a step name:
+
+```shell
+cat /tekton/steps/step-unnamed-<step-index>/exitCode
+```


This feels a little bit weird to me, as for example, the index for unnamed might appear confusing : step-unnamed-2 is the 2nd unnamed step or the 2nd step (and thus the first unnamed) ?

Maybe we could have /tekton/steps/by-index/<index>/… and /tekton/steps/by-name/<name>/… (and use link or copy there) ?

This feels a little bit weird to me, as for example, the index for unnamed might appear confusing : step-unnamed-2 is the 2nd unnamed step or the 2nd step (and thus the first unnamed) ?

This is designed to match how the containers are named. step-unnamed-2 is the third step (and the first unnamed).

The containers are named step-<name of the step> if the step has a name else they are named step-unnamed-<step-index> where <step-index> is the index of the step in the entire list of the steps.

kubectl get tr test-taskrun-4m59w -o json | jq .status.steps | jq '.[] | .container' "step-step0" "step-unnamed-1" "step-step2"

Maybe we could have /tekton/steps/by-index/<index>/… and /tekton/steps/by-name/<name>/… (and use link or copy there)?

Let me know if it still sounds confusing, we can make it better.

Ok this is less confusing I think (especially if it follows the same).
I still think having that /tekton/steps/by-index/… and …/by-name/… would make sense 😛 (I may be biased by linux, but I am a fan of this type of construct in the /dev virtual fs)

vdemeester · 2021-08-05T15:15:18Z

pkg/entrypoint/entrypointer.go

+	// create a step path pipeline.StepPath/step-<step-name>/ if a step name is specified
+	// or pipeline.StepPath/step-unnamed-<step-index>/ if a step name is missing
+	// Create a symlink:
+	// ln -s pipeline.StepPath/<step-index> pipeline.StepPath/<step-name>
+	e.PostWriter.CreatePath(e.StepPath, e.StepPathLink)
+


For the future, I think we should write in /tekton/steps/by-index/<index>/… and then link to elsewhere (either by name, or to keep backward compat, …)

vdemeester · 2021-08-05T15:16:13Z

pkg/pod/entrypoint.go

@@ -25,6 +25,8 @@ import (
 	"path/filepath"
 	"strings"

+	"github.com/tektoncd/pipeline/pkg/apis/pipeline"
+


nit: extra empty line 😝

ghost

Looks great, I left a bunch of nitty code / style comments but it looks pretty close to me!

ghost · 2021-08-05T15:20:26Z

cmd/entrypoint/post_writer.go

+	}
+}
+
+// CreatePath creates the specified path and a symbolic link to the path


Suggest CreateDirWithSymlink or similar as the func name. CreatePath is nice and concise but the expected behaviour is much more specific than the name implies I think?

thanks @sbwsg for the suggestion, have renamed it to CreateDirWithSymlink 👍

ghost · 2021-08-05T15:20:45Z

cmd/entrypoint/post_writer.go

+		return
+	}
+	if err := os.MkdirAll(source, 0770); err != nil {
+		log.Fatalf("Creating file path %q: %v", source, err)


tiny nit: suggest Creating directory %q: %v since it's MkdirAll

cmd/entrypoint/post_writer.go

ghost · 2021-08-05T15:22:39Z

docs/developers/README.md

@@ -142,6 +142,8 @@ of how this directory is used:
  * These folders are [part of the Tekton API](../api_compatibility_policy.md):
    * `/tekton/results` is where [results](#results) are written to
      (path available to `Task` authors via [`$(results.name.path)`](../variables.md))
+    * `/tekton/steps` is where the `step` exitCodes are written to


This is great. Would also love to see a section in this doc talking a bit more in-depth about the implementation details of step exit codes / onerror if possible? e.g. purpose of both the directory & the symlink, how the exitcode is shared between steps, etc...

I have added two sections in the end of the README.

What and Why of /tekton/steps - https://github.com/tektoncd/pipeline/pull/4106/files#diff-fb2c200965217efdd737c2d18433086a2a6cd879a5b0213fe8253cb14546be6fR472

How to access the exit code of a step from any subsequent step in a task - https://github.com/tektoncd/pipeline/pull/4106/files#diff-fb2c200965217efdd737c2d18433086a2a6cd879a5b0213fe8253cb14546be6fR601

ghost · 2021-08-05T15:25:03Z

docs/tasks.md

+A step can access the exit code of any previous step using the `path` similar to a task result, for example:
+
+```shell
+$(steps.step-<step-name>.exitCode.path)


Here the exit code is inside the file pointed to by path, is that right? So to read the exit code you need to cat $(steps.step-X.exitCode.path)? Could the line before this read something more like, A step can access the exit code of any previous step by reading the file pointed to by the exitCode path variable:

thanks, yup, reworded and makes more sense now 🤣

ghost · 2021-08-05T15:32:50Z

pkg/entrypoint/entrypointer.go

+	OnError string
+	// StepPath is the path to that step directory where any step related metadata can be stored
+	StepPath string
+	// StepPathLink is the path which needs to be linked to the StepPath


Would be useful to explain the intended purpose of the symlink too I think. Without the context it's a bit tricky to understand why it would be useful.

added a bit of doc here and also explanation in the developers doc - https://github.com/tektoncd/pipeline/pull/4106/files#diff-fb2c200965217efdd737c2d18433086a2a6cd879a5b0213fe8253cb14546be6fR593

ghost · 2021-08-05T15:33:38Z

pkg/entrypoint/entrypointer.go

+	// create a step path pipeline.StepPath/step-<step-name>/ if a step name is specified
+	// or pipeline.StepPath/step-unnamed-<step-index>/ if a step name is missing
+	// Create a symlink:
+	// ln -s pipeline.StepPath/<step-index> pipeline.StepPath/<step-name>


If we rename the method from CreatePath to CreateDirAndSymlink (or similar) I expect this comment could explain the purpose of creation rather than the mechanics. E.g. could read something like, // Create the directory where we will store the exit codes (and eventually other metadata) of Steps.

yup make sense, done ✅

ghost · 2021-08-05T15:45:45Z

pkg/entrypoint/entrypointer.go

+	// set it to "fail" to indicate the entrypoint to exit the taskRun if the container exits with non zero exit code
+	// set it to "continue" to indicate the entrypoint to continue executing the rest of the steps irrespective of the container exit code
+	OnError string
+	// StepPath is the path to that step directory where any step related metadata can be stored


Nit: suggest StepDirectory or StepDir just to be super explicit. Also suggest for StepDirLink or similar.

renamed it to StepMetadataDir to match the following suggestion 👍

ghost · 2021-08-05T15:48:52Z

cmd/entrypoint/main.go

+		"Set to \"fail\" to declare a failure with a step error and stop executing the rest of the steps.")
+	stepPath = flag.String("step_path", "", "Relative step path, creates the specified path under /tekton/steps/ which "+
+		"can be used to store the step metadata e.g. <step-name> in /tekton/steps/<step-name>/")
+	stepPathLink = flag.String("step_path_link", "", "Relative step path, creates a symbolic link to the "+


nit: suggest step_dir and step_dir_link (or maybe even step_metadata_dir? too much?) just to be super explicit about usage. Mirrors the explicitness of wait_file / post_file (though not termination_path 😭 )

yup, no harm in being a little more verbose, renamed to step_metadata_dir and step_metadata_dir_link

ghost · 2021-08-05T15:50:08Z

cmd/entrypoint/main.go

+	stepPath = flag.String("step_path", "", "Relative step path, creates the specified path under /tekton/steps/ which "+
+		"can be used to store the step metadata e.g. <step-name> in /tekton/steps/<step-name>/")
+	stepPathLink = flag.String("step_path_link", "", "Relative step path, creates a symbolic link to the "+
+		"specified step path e.g. <step-index> in /tekton/steps/<step-index>")


I'm a tad confused about <step-index> in /tekton/steps/<step-index>. Does the user pass in just <step-index> or the full /tekton/steps/0?

yup the user pass in just the <step-index>, added an explanation in the developers doc - https://github.com/tektoncd/pipeline/pull/4106/files#diff-fb2c200965217efdd737c2d18433086a2a6cd879a5b0213fe8253cb14546be6fR590

Ah, we might have a slight mismatch to tackle here. In the new unit tests we're passing the absolute paths over to the entrypoint from the pod package.

I think passing the absolute paths is a fine way to go, I just wanna make sure the arg help text here lines up with that.

Hey @sbwsg I am so sorry to confuse you 😭 it is taking the absolute path similar to the others i.e.

"-wait_file", filepath.Join(mountPoint, fmt.Sprintf("%d", i-1)), "-post_file", filepath.Join(mountPoint, fmt.Sprintf("%d", i)), "-termination_path", terminationPath, "-step_metadata_dir", filepath.Join(pipeline.StepsDir, name), "-step_metadata_dir_link", filepath.Join(pipeline.StepsDir, fmt.Sprintf("%d", i)), }

I have updated the doc and this file as well. While implementing I had a relative path in mind but going with how the other flags are implemented, chose to go with absolute path and forgot all about it 😢

Awesome, makes total sense, cheers @pritidesai !

pritidesai · 2021-08-06T03:47:40Z

What's the reasoning about stepPath and stepPathLink ? (aka are they required for this feature, if yes, how/why?)

They are required for this feature since this feature implements creating a file exitCode under the stepPath. stepPath is the flag which notifies the entrypoint what directory to create under /tekton/steps/ where the step related metadata can be stored, starting with exitCode in this PR and anything more in the future.

ghost

It would be great to introduce a test of some kind that checks how results are treated in the event of failure. I see that the pipelinerun example includes a result from a step but it would be super useful to have some test code ensuring it's returned as expected. This might be best suited as an alpha integration test?

There are also 2 things which I think might be worth documenting as part of this PR:

Behavior of Task Results written by Steps that continue on error. (probably as part of tasks.md?)
Interaction of this feature with the debug/breakpoint feature. It looks like specifying breakpoint behaviour overrides the onError behaviour? (again probably worth capturing in tasks.md?)

Otherwise this lgtm!

ghost · 2021-08-10T14:56:27Z

pkg/entrypoint/entrypointer.go

@@ -104,6 +120,10 @@ func (e Entrypointer) Go() error {
 		_ = logger.Sync()
 	}()

+	// Create the directory where we will store the exit codes (and eventually other metadata) of Steps.
+	// Create a symlink to the directory for easier access by the index instead of a step name.


👍 nice, thanks a lot for tackling that.

bobcatfish · 2021-08-10T16:08:34Z

Hey @pritidesai ! Just a drive by comment, I added a comment to the TEP PR for this (tektoncd/community#463 (comment)), I think there's some behavior worth fleshing out in the TEP - maybe you already dealt with this in the PR and it's just a matter of updating the TEP, or maybe we need to discuss further.

tekton-robot · 2021-08-10T17:22:06Z

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
cmd/entrypoint/post_writer.go	0.0%	77.8%	77.8
internal/builder/v1beta1/step.go	42.1%	38.1%	-4.0
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.0%	0.1
pkg/entrypoint/entrypointer.go	66.1%	71.0%	4.9
pkg/pod/entrypoint.go	85.9%	87.2%	1.3
pkg/pod/pod.go	87.1%	86.9%	-0.2
pkg/pod/status.go	91.8%	90.8%	-1.0
pkg/reconciler/taskrun/resources/apply.go	99.1%	99.2%	0.0
pkg/reconciler/taskrun/taskrun.go	77.5%	77.6%	0.1

tekton-robot · 2021-08-10T18:13:26Z

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
cmd/entrypoint/post_writer.go	0.0%	77.8%	77.8
internal/builder/v1beta1/step.go	42.1%	38.1%	-4.0
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.0%	0.1
pkg/entrypoint/entrypointer.go	66.1%	71.0%	4.9
pkg/pod/entrypoint.go	85.9%	87.2%	1.3
pkg/pod/pod.go	87.1%	86.9%	-0.2
pkg/pod/status.go	91.8%	90.8%	-1.0
pkg/reconciler/taskrun/resources/apply.go	99.1%	99.2%	0.0
pkg/reconciler/taskrun/taskrun.go	77.5%	77.6%	0.1

tekton-robot · 2021-08-10T23:49:55Z

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
cmd/entrypoint/post_writer.go	0.0%	77.8%	77.8
internal/builder/v1beta1/step.go	42.1%	38.1%	-4.0
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.0%	0.1
pkg/entrypoint/entrypointer.go	66.1%	71.0%	4.9
pkg/pod/entrypoint.go	85.9%	87.2%	1.3
pkg/pod/pod.go	87.1%	86.9%	-0.2
pkg/pod/status.go	91.8%	90.8%	-1.0
pkg/reconciler/taskrun/resources/apply.go	99.1%	99.2%	0.0
pkg/reconciler/taskrun/taskrun.go	77.5%	77.6%	0.1

pritidesai · 2021-08-10T23:52:56Z

It would be great to introduce a test of some kind that checks how results are treated in the event of failure. I see that the pipelinerun example includes a result from a step but it would be super useful to have some test code ensuring it's returned as expected. This might be best suited as an alpha integration test?

I have added an additonal e2e test here, please let me know if you find any additional tests which we can include 🙏

There are also 2 things which I think might be worth documenting as part of this PR:

Behavior of Task Results written by Steps that continue on error. (probably as part of tasks.md?)

Added a section in tasks.md

Interaction of this feature with the debug/breakpoint feature. It looks like specifying breakpoint behaviour overrides the onError behaviour? (again probably worth capturing in tasks.md?)

Yes that is correct, breakpoint overrides the step error functionality and breakpoint feature provides different tools to mark a step as failure/success. Have captured it briefly here

Otherwise this lgtm!

thanks @sbwsg for reviewing the PR

this commit implements tep-0049 - ignore a step error When a `step` in a `task` results in a failure, the rest of the steps in the `task` are skipped and the `taskRun` is declared a failure. If you would like to ignore such step errors and continue executing the rest of the steps in the task, you can specify `onError` for such a `step`. `onError` can be set to either `continue` or `fail` as part of the step definition. If `onError` is set to `continue`, the entrypoint sets the original failed exit code of the script in the container terminated state. A `step` with `onError` set to `continue` does not fail the `taskRun` and continues executing the rest of the steps in a task. This is an alpha feature. The `enable-api-fields` feature flag must be set to `"alpha"` to specify `onError` for a `step`. This commit includes following changes: * Changing entrypoint to include three new flags `onError`, `stepMetadataDir`, and `stepMetadataDirLink`. * Adding one new function as part of the runner CreateDirWithSymlink * Creating a volume `/tekton/steps/` * Supporting a path variable $(steps.step-<stepName>.exitCode.path) and $(steps.step-unnamed-<stepIndex>.exitCode.path) * API spec `onError` while defining a step * Writing exitCode at /tekton/steps/step-<step-name>/exitCode or /tekton/steps/step-unnamed-<step-index>/exitCode * Set the exitCode of a terminated state to a non-zero exit code * Doc, unit test, and examples for this feature

tekton-robot · 2021-08-10T23:58:55Z

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
cmd/entrypoint/post_writer.go	0.0%	77.8%	77.8
internal/builder/v1beta1/step.go	42.1%	38.1%	-4.0
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.0%	0.1
pkg/entrypoint/entrypointer.go	66.1%	71.0%	4.9
pkg/pod/entrypoint.go	85.9%	87.2%	1.3
pkg/pod/pod.go	87.1%	86.9%	-0.2
pkg/pod/status.go	91.8%	90.8%	-1.0
pkg/reconciler/taskrun/resources/apply.go	99.1%	99.2%	0.0
pkg/reconciler/taskrun/taskrun.go	77.5%	77.6%	0.1

ghost · 2021-08-11T00:00:19Z

docs/tasks.md

+to access the result and run with the resolved value.
+
+Now, a step can fail before initializing a result and the `pipeline` can ignore such step failure. But, the  `pipeline`
+will fail with `InvalidTaskResultReference` if it has a task consuming that task result. For example, any task


Oh wow, I hadn't even considered this case. Good point!

ghost · 2021-08-11T00:05:39Z

/lgtm

@skaegi

Pipeline 0.27 was released in August 2021 which introduced an alpha feature to specify onError in a step. This feature was implemented in the PR tektoncd#4106: tektoncd#4106 To enable this feature, the Tekton Pipeline deployment had to set enable-api-fields to alpha. After discussing this in the API WG: https://docs.google.com/document/d/17PodAxG8hV351fBhSu7Y_OIPhGTVgj6OJ2lPphYYRpU/edit#heading=h.ig94pf1w10xc We have sent an email out to collect the feedback from the users. @skaegi is looking forward to this promotion. This commit is updating task validation to no longer require enable-api-fields is set to alpha. The documentation is updated to moved it out of the alpha section. The example yamls moved out of alpha.

@skaegi

Pipeline 0.27 was released in August 2021 which introduced an alpha feature to specify onError in a step. This feature was implemented in the PR tektoncd#4106: tektoncd#4106 To enable this feature, the Tekton Pipeline deployment had to set enable-api-fields to alpha. After discussing this in the API WG: https://docs.google.com/document/d/17PodAxG8hV351fBhSu7Y_OIPhGTVgj6OJ2lPphYYRpU/edit#heading=h.ig94pf1w10xc We have sent an email out to collect the feedback from the users. @skaegi is looking forward to this promotion. This commit is updating task validation to no longer require enable-api-fields is set to alpha. The documentation is updated to moved it out of the alpha section. The example yamls moved out of alpha. Updated e2e to not include alpha flag.

@skaegi

Pipeline 0.27 was released in August 2021 which introduced an alpha feature to specify onError in a step. This feature was implemented in the PR tektoncd#4106: tektoncd#4106 To enable this feature, the Tekton Pipeline deployment had to set enable-api-fields to alpha. After discussing this in the API WG: https://docs.google.com/document/d/17PodAxG8hV351fBhSu7Y_OIPhGTVgj6OJ2lPphYYRpU/edit#heading=h.ig94pf1w10xc We have sent an email out to collect the feedback from the users. @skaegi is looking forward to this promotion. This commit is updating task validation to no longer require enable-api-fields is set to alpha. The documentation is updated to moved it out of the alpha section. The example yamls moved out of alpha. Updated e2e to not include alpha flag.

@skaegi

Pipeline 0.27 was released in August 2021 which introduced an alpha feature to specify onError in a step. This feature was implemented in the PR tektoncd#4106: tektoncd#4106 To enable this feature, the Tekton Pipeline deployment had to set enable-api-fields to alpha. After discussing this in the API WG: https://docs.google.com/document/d/17PodAxG8hV351fBhSu7Y_OIPhGTVgj6OJ2lPphYYRpU/edit#heading=h.ig94pf1w10xc We have sent an email out to collect the feedback from the users. @skaegi is looking forward to this promotion. This commit is updating task validation to no longer require enable-api-fields is set to alpha. The documentation is updated to moved it out of the alpha section. The example yamls moved out of alpha. Updated e2e to not include alpha flag.

@skaegi

Pipeline 0.27 was released in August 2021 which introduced an alpha feature to specify onError in a step. This feature was implemented in the PR tektoncd#4106: tektoncd#4106 To enable this feature, the Tekton Pipeline deployment had to set enable-api-fields to alpha. After discussing this in the API WG: https://docs.google.com/document/d/17PodAxG8hV351fBhSu7Y_OIPhGTVgj6OJ2lPphYYRpU/edit#heading=h.ig94pf1w10xc We have sent an email out to collect the feedback from the users. @skaegi is looking forward to this promotion. This commit is updating task validation to no longer require enable-api-fields is set to alpha. The documentation is updated to moved it out of the alpha section. The example yamls moved out of alpha. Updated e2e to not include alpha flag.

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Jul 20, 2021

tekton-robot requested review from bobcatfish and imjasonh July 20, 2021 06:37

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 20, 2021

pritidesai force-pushed the tep-0040-implementable branch from 0f3c355 to ad79fa7 Compare July 20, 2021 06:56

pritidesai force-pushed the tep-0040-implementable branch from ad79fa7 to 073b516 Compare July 20, 2021 16:33

pritidesai added this to the Pipelines v0.27 milestone Jul 20, 2021

pritidesai force-pushed the tep-0040-implementable branch from 073b516 to c56c41e Compare July 20, 2021 20:54

pritidesai force-pushed the tep-0040-implementable branch from c56c41e to eeafbd8 Compare July 20, 2021 21:49

pritidesai force-pushed the tep-0040-implementable branch from eeafbd8 to cecf3d4 Compare July 20, 2021 23:38

pritidesai force-pushed the tep-0040-implementable branch from cecf3d4 to 8f8b798 Compare July 20, 2021 23:56

vdemeester self-assigned this Jul 22, 2021

pritidesai commented Jul 22, 2021

View reviewed changes

pritidesai mentioned this pull request Jul 27, 2021

Finally task is skipped when dependent on results from a failed task #4118

Closed

vdemeester reviewed Aug 5, 2021

View reviewed changes

ghost reviewed Aug 5, 2021

View reviewed changes

pritidesai force-pushed the tep-0040-implementable branch from 8f8b798 to 6807c96 Compare August 6, 2021 06:54

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2021

ghost reviewed Aug 10, 2021

View reviewed changes

pritidesai force-pushed the tep-0040-implementable branch from cdd3389 to dbed261 Compare August 10, 2021 17:19

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2021

pritidesai force-pushed the tep-0040-implementable branch from dbed261 to b312e44 Compare August 10, 2021 18:11

pritidesai force-pushed the tep-0040-implementable branch from b312e44 to d8d2171 Compare August 10, 2021 23:47

pritidesai force-pushed the tep-0040-implementable branch from d8d2171 to fdeb0b1 Compare August 10, 2021 23:57

pritidesai changed the title ~~TEP-0040 implementation - specifying onError for a step~~ TEP-0040 implementation - specifying onError in a step Aug 10, 2021

ghost reviewed Aug 11, 2021

View reviewed changes

tekton-robot assigned ghost Aug 11, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 11, 2021

tekton-robot merged commit 92746a2 into tektoncd:main Aug 11, 2021

This was referenced Aug 12, 2021

Add debug with breakpoint configuration to PipelineRun #4145

Closed

Use stopAndFail instead of fail with onError 🧐 tektoncd/community#497

Merged

pritidesai mentioned this pull request Sep 10, 2021

[proposal] Capturing step exit codes in Tasks #2800

Closed

pritidesai mentioned this pull request Sep 21, 2021

Promoting onError to beta 🎓 #4251

Merged

5 tasks

tlawrie mentioned this pull request May 9, 2022

Support onError in tasks boomerang-io/community#346

Open

TEP-0040 implementation - specifying onError in a step #4106

TEP-0040 implementation - specifying onError in a step #4106

Conversation

pritidesai commented Jul 20, 2021 • edited

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jul 20, 2021

tekton-robot commented Jul 20, 2021

tekton-robot commented Jul 20, 2021

tekton-robot commented Jul 20, 2021

tekton-robot commented Jul 20, 2021

tekton-robot commented Jul 20, 2021

tekton-robot commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai commented Jul 22, 2021

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai Aug 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai Aug 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai commented Aug 6, 2021

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish commented Aug 10, 2021

tekton-robot commented Aug 10, 2021

tekton-robot commented Aug 10, 2021

tekton-robot commented Aug 10, 2021

pritidesai commented Aug 10, 2021

tekton-robot commented Aug 10, 2021

Choose a reason for hiding this comment

ghost commented Aug 11, 2021

TEP-0040 implementation - specifying `onError` in a step #4106

TEP-0040 implementation - specifying `onError` in a step #4106

pritidesai commented Jul 20, 2021 •

edited

pritidesai Aug 6, 2021 •

edited

pritidesai Aug 6, 2021 •

edited