TEP-0011: Redirecting Step Output Streams

[chhsia0]

TEP for pipeline#2925 and pipeline#3103.

Consuming outputs of a step in another step is a common pattern in writing Tasks. However, this is currently tedious to do. Task authors have to overwrite the image entrypoint with either sh -c or the script field to wrap up the command to run with an explicit output stream redirection. This is not even possible if the image does not come with a shell, e.g., images built by ko.

To achieve the functionality of output stream redirection between steps and even Tasks, this TEP proposes to add new fields to steps for Task authors to specify paths to store stdout/stderr to.

[vdemeester]

Indeed, that might be a problem. We may want to restrict it to workspaces but it's gonna be tricky to validate

[imjasonh]

I think this is an argument for just always writing stdout/stderr to some Tekton-owned path and documenting where that is. If a later step wants to read it, they can.

[chhsia0]

Yeah this is why I explicitly call this out in this section so we can discuss if "canonical paths" as you suggest is a better way.

[chhsia0]

On the other hand, if we make it explicit that this is a path, most users would naturally be aware that if they want to use the same path in different containers, they have to make a shared mount point.

[imjasonh]

FWIW, this should be doable with a script, instead of requiring a whole new image entrypoint.

script: git describe --tags > /tekton/results/tags

[chhsia0]

script is essentially overwriting the entrypoint ;)

But yeah this is generally not a big problem, just a bit tedious. The only problem, again, is when the image doesn't have a shell and the command to run doesn't have a flag to write output to a file.

[imjasonh]

That is true. Maybe I'm not understanding what the downside is in that case. Using script mode in this way is pretty straightforward and doesn't require maintaining another image.

[ghost]

My main concern would be to document and push users towards using workspaces for the stdout / stderr files so that they can be shared around without accidentally exploding the size of results and the termination message.

I was wondering if you considered any of the alternatives described in the comments on tektoncd/pipeline#2925 - in particular inverting the control a little bit and allowing a subsequent step to provide a filter to run over a previous steps stdout and limit the amount of data shuttled from step to step. Even if those alternatives are rejected it's probably still worth adding them to this doc to capture them I think?

[chhsia0]

@sbwsg Sure I'll go through pipeline#2925 and add the discussions here. For promoting users to use workspaces, I feel that it's better giving users options but document the pros and cons and the suggested ways, as well as a validation and/or runtime check (which we already have, as you pointed out). I can image the most common use cases for sharing data through results are sending just a couple bytes, and using workspaces (backed by PVC I believe?) for such purpose is too heavy-weight.

[ghost]

I can image the most common use cases for sharing data through results are sending just a couple bytes, and using workspaces (backed by PVC I believe?) for such purpose is too heavy-weight.

Having read your example yaml in the TEP I think I understand a bit better where you're coming from. It looks like you're talking about the sort of approach where one step writes its stdout to a shared location on disk, and another step then filters through the data with jq and writes only a tiny portion of it to a result. That's great and I totally understand that use case and I think this is the perfect place for results. Similarly to @imjasonh I'm not actually sure this needs to be explicitly called out as a goal - Tekton will already work like this today so long as the user writes the stdout to a location shared by multiple steps.

However, I do want to be careful that when we come to document / promote the feature we have to be super clear to users that they cannot slam all of stdout/stderr from every step into a task result and expect it to "just work". I think that's where I'm getting stuck a bit - if a user sees this feature and adds stdout: $(results.stepXstdout.path) to every single Step they are going to have a very bad time. I think we could safely expect a number of very similar github bug reports to be generated around it very quickly and ideally I'd like to avoid that.

[ghost]

Also, just to add - PVCs are only required if the data needs to be shared between Tasks of a Pipeline. An emptyDir workspace is shared by all steps in a task and is as close to zero-configuration as a volume definition can get.

[chhsia0]

For sharing thing between steps within a task then workspaces isn't even needed. An emptyDir volume in the task is sufficient. See my example between the 1st and 2nd steps.

[chhsia0]

I call it out as a goal in the sense that I'd like to enable user to configure the paths, instead of using some built-in paths, where you won't be able to make it work with Task results without an additional step. If this is not needed in the goal I could remove it.

[ghost]

I call it out as a goal in the sense that I'd like to enable user to configure the paths

Gotcha! Makes sense. I don't think you have to remove it in that case, but my suggestion would be to rephrase the goal to be "Enable a user to configure the path where the stream is written"? I got caught up on the result part and I missed the broader idea.

[ghost]

Do you think this might become configurable one day? i.e. a user may decide they don't want the stdout logged to terminal, only the file?

I am wondering if that would change the desirable shape of the struct if so :-

type Step struct {
  // ...
  StdoutConfig struct {
    ToPath string
    ToTerminal bool
  }
}

[chhsia0]

It probably makes sense to have an additional layer of structure. Do you have anything else in mind that stdout could be redirected to? About the ToTerminal option, at least it should be turned on by default, mainly from a debuggability point of view: it would be impossible to retrieve the stdout from e.g., emptyDir if the task fails.

This extra layer, on the other hand, would make the config structure more verbose.

[ghost]

Having thought about this a bit more - I wonder if "hiding" or limiting the stdout log would be more a runtime concern. Something to configure at the taskrun level instead of task. I'm o.k. to move ahead with the existing proposal of just the path string!

[ghost]

i'm curious about the interleaving - would it be line-buffered or would the user end up with halves of lines from each stream spliced together?

[ghost]

I also wonder if we'd want a "prefix: " in the interleaved scenario like:

stdout: tests 3 / 6 completed
stdout: tests 4 / 6 completed
stderr: warning: foo bar
stdout: tests 5 / 6 completed

and, if so, whether that'd also be something the user wants to configure?

[chhsia0]

The "interleaving" behaves similar to redirecting both stdout and stderr to the same stream in shell, e.g,

./command > log 2>&1

IIUC go runtime doesn't implement a buffer and it directly goes to the write syscall, and thus line-buffering only happens if the *os.File is associated with a TTY. You can find some implementation details about how we "redirect" stdout in our entrypoint customization here:
https://github.com/tektoncd/pipeline/pull/3103/files#diff-18256282f9059ed3c2b641b9c2d8ac6fR29
https://github.com/tektoncd/pipeline/pull/3103/files#diff-8c0170fa2830f4fdfc8067ba745cf734R49

We could use a bufio to implement a line buffer before writing to the file, but I'd imagine there would still be corner cases (e.g., super long lines) where the interleaving happens within a line. And most of the time the interleaving is not a big deal so I'm not sure if it is worth complicating the code to have line buffering.

An alternative is to forbid user from specifying the same path for stdout and stderr. If you the user really wants that, they can use the Pod logs API.

Prefixing is a fancy idea, but IMO it's incremental and can be added when there's an actual need. If users really want separation, they should put them to different files. I'm also afraid that we're opening an can of worms, e.g., one may want to add timestamps or other information. Especially timestamping is something we should not touch and leave it to the Pod logs API :)

[ghost]

OK, understood!

[imjasonh]

Is the issue here that boskosctl doesn't include a shell, and so script is not useable, and therefore you'd have to jump through hoops to get its output? Would it be possible to get a shell added to the boskosctl image, perhaps by simply basing it on alpine?

[chhsia0]

I'm not the one using boskosctl. @bobcatfish is ;) And it seems to have a shell.

IMO the problem here is that Tekton shouldn't limit how users build and use images. Especially some images are build to act like command line tools (i.e., comes with decent default entrypoints so running such images has the same UX as running utility programs), and right now if you want to composite multiple utility images to achieve a complex task, you have to ensure that there must be a shell for all images, or the image entrypoints must have an option to put output to a file.

[chhsia0]

The actual use case I have is, I'm working on a DSL and a tool for user to run any image, and I can redirect the stdout to either task results or somewhere else, without the need to know the details of an image. How to run the image is provided by the user and completely transparent to my tool. In other words, I don't have control to the image, and thus impossible to ensure that there is always a shell and synthesize a script for that shell.

[chhsia0]

It seems common for users to prefer some third-party “official” images instead of maintaining their own fork to include a shell.

[bobcatfish]

boskosctl is actually a bad example b/c it DOES have a shell, but otherwise: what @chhsia0 said :D

[imjasonh]

I'm not sure I understand the first point: the entrypoint doesn't leak into the Task definition if it uses script -- if anything, the presence of a shell is assumed using script, but the original entrypoint value is ignored completely.

For the second point, the Task author could tee to stdout and wherever else they want that data to end up.

[chhsia0]

By "leaking" I mean that users cannot treat any third-party utility image as a black box and just pass in appropriate arguments. For example, if you use ko to build an image with shell, you have to know that the binary sits in /ko-app/... and bake this information into your shell.

If I later update my image to move the entrypoint binary around, steps passing args can still run, but steps with scripts will fail.

[pritidesai]

/kind tep

[ghost]

minor nit but otherwise LGTM

[chhsia0]

Attaching some great discussions related to this change:

https://tektoncd.slack.com/archives/CLCCEBUMU/p1597969084015700

Pierre Tasci:appleincwhite: 5:18 PM
FWIW it feels a bit odd to make this change at the Tekton level to alleviate something related to the internals of the image. I totally see how users might not want to author their own images but to me at least, it seems like the gap there is in tooling around the image rather than Tektons tooling in how it is executed if that make sense.

chhsiao 5:24 PM
IMO Tekton should be a platform to run any image without assumptions, i.e., treat them as black boxes, like k8s Pod API. (It seems we both agreen on this 🙂 ) But now there are Tekton features (say, task results) that some images cannot use, so it seems an incompleteness/limitation in the API.

Jason Hall 5:37 AM
That's a reasonable argument. I do think that I've tended to think of Tasks as the unit of pluggability/reusability, and the specifics of the images inside those Tasks is up to the Task author, and should generally be a black box to end users and even operators. I'd love to hear from other folks who are generating TaskRuns/PipelineRuns from some other configuration though, since that's not a use case I have as much first-hand experience with.

https://tektoncd.slack.com/archives/CLCCEBUMU/p1598024780034800?thread_ts=1598023428.029200&cid=CLCCEBUMU

Pierre Tasci:appleincwhite: 4 days ago
I think you make some excellent points @Christie Wilson. If I am hearing it correctly (let me know if I am not), the core of the assumption here is that small, focused images are better in some way to larger images.
I only have my own experience to go on here so pardon the naïveté but I haven’t found that to be particularly true. It often hurts debuggability and testability to have bare-bones images. I think of Kaniko as a prime example. Great tool but because it is built off scratch, everyone I know who uses it, actually uses the debug version because they attach a busybox shell into it. I will take ease of development and familiarity over additional complexity most times.
I do think there is a point here. The way steps are hooked up together right now is minimal at best. Even passing a file between steps requires hardcoded, explicit handoff. For that reason, I often end up writing larger scripts than I would otherwise desire. Rather than unix pipe redirection, perhaps there is a larger story about inter-step interaction here?

https://tektoncd.slack.com/archives/CLCCEBUMU/p1598040443039700?thread_ts=1598023428.029200&cid=CLCCEBUMU

Cory Cross 4 days ago
pipes are untyped and hard to test and full of edge cases. I think powershell should be the better inspiration if something's needed intra-Task

Jason Hall 4 days ago
Good point! Now you're making a even stronger argument that there's lots of complexity down this path, and lots of prior art we can learn from before we jump in and implement the next easy enhancement. 🙃

[vdemeester]

Overall looks good to me. I am definitely voting for Option 1 as it is way more flexible.
For Option 2, what happens if the we refer a step stdoutPath that doesn't have stdout: true ? Should it fail, should we default to empty ? Having $(step.<name>.stdoutPath) gives the impression to the user (at least to me) that it's gonna be available for all the steps (hence we do capture the stdout on all of them).

[chhsia0]

Good point. I can think of two options:
Option 2.1: Validate the task definition and reject the task if users refer to a non-existent step stdout path.
Option 2.2: No explicit Stdout/Stderr flags at all. If $(steps.<name>.stdoutPath) exists in the task definition, capture the stdout stream of the corresponding step.

In general I also like Option 1 (allowing users to specify paths) better. Anyway, we should settle with one option then I'll move the other to "alternatives."

[bobcatfish]

Thanks @chhsia0 !

I have 3 main pieces of feedback:

1. Is it possible and reasonable to do stdout only, at least at first? I'm deeply suspicious that stderr is mostly only relevant at runtime
2. I'd like to revisit the variable interpolation and not provide a way to automatically expose stdout/stderr as results
3. Can we support both an "auto" mode AND allow the user to specify an explicit path?

[bobcatfish]

another alternative would be to do nothing in particular in tekton and leave this up to the Task and image authors, which effectively means if you want to communicate data between steps, or you want to capture data for results, you need a shell or you need your binaries to support outputting to a file

[bobcatfish]

$(results.<name>.path) to the StdoutPath field to extract the stdout of a step into a Task result

I'm a bit confused by this, you've got an example below: stdout: $(results.project-id.path)

in this case, what path does stdout actually get written to? is this magically writing stdout to a location on disk and then also exposing it as a result?

i dont think we should automatically wire stdout/stderr to any results. if we do, i think we could do it in a subsequent proposal after this

my alternative suggestion for the example below is that if you want to expose stdout as a result, you'd need to add a step after parse-project-id that writes stdout as a result

No new variable substitution for accessing the values of StdoutPath and StderrPath fields will be provided so variable substitution can remain single-pass.

can you explain a bit more? i think it would be very convenient to use $(steps.foo.stdout.path) or similar, and that's exactly how id want to use this feature, i wouldnt want to have to hardcode the path in multiple places

[vdemeester]

in this case, what path does stdout actually get written to? is this magically writing stdout to a location on disk and then also exposing it as a result?

$(results.project-io.path) would be interpolated and be the path of the results (so /tekton/result/project-io or something), and thus, stdout would be written in. Just like if you use $(workspaces.some.path)/foo.

can you explain a bit more? i think it would be very convenient to use $(steps.foo.stdout.path) or similar, and that's exactly how id want to use this feature, i wouldnt want to have to hardcode the path in multiple places

So one of the problem with $(steps.foo.stdout.path) is that, it may be used without the previous task having stdout captured. You don't necessarly hardcode the path, or at least the full path ; most likely you would use $(workspace.foo.path)/somewhere/afile as a path, and re-use the same later on.

[bobcatfish]

$(results.project-io.path) would be interpolated and be the path of the results (so /tekton/result/project-io or something), and thus, stdout would be written in. Just like if you use $(workspaces.some.path)/foo.

oh that makes sense! i didnt realize you could do that and i've been hardcoding the path everywhere. just ignore that comment from me then @chhsia0!

So one of the problem with $(steps.foo.stdout.path) is that, it may be used without the previous task having stdout captured. You don't necessarly hardcode the path, or at least the full path ; most likely you would use $(workspace.foo.path)/somewhere/afile as a path, and re-use the same later on.

coud you explain a bit more about this? we could check if folks are using $(steps.foo.stdout.path) for a step that doesn't have stdout capturing configured. it still seems really useful to me

[bobcatfish]

we could check if folks are using $(steps.foo.stdout.path) for a step that doesn't have stdout capturing configured

actually without this feature, we wouldn't be able to help out anyone making this mistake

[chhsia0]

This is one of the main differences between Option 1 and Option 2 if we also want the variable substitution mechanism to remain single-pass. With the single-pass assumption, Option 1 gives us a way to capture stdout into results, workspaces, volumes, ... but it's harder to validate if the specified path exists (and IMO it's Task authors' responsibility: they're responsible to ensure any path used in their Tasks is valid); Option 2 provides more guarantees and validations, but is more limited.

[bobcatfish]

curious why you decided to use volume mounts here - workspaces are intended to replace volumes and volumemounts as much as possible (i would have liked to have removed them entiredly tektoncd/pipeline#2058) b/c they allow volumes to be supplied at runtime vs task authoring time

[chhsia0]

Using volumes in Tasks instead of workspaces provides us a way to encapsulate storage information that's internal to Tasks. IIUC if you use a workspace in a Task, you have to surface that to your Pipeline definition, even if you just want to provide a temporary storage for steps within a Task.

So if the storage not used to pass data between Tasks in a pipeline (e.g., emptyDir, configMap and secret), I prefer using volumes and don't expose that to the Pipeline definition.

Also IMO using emptyDir for a workspace might give people an elusion that the emptyDir is shared.

[bobcatfish]

is there a reasonable option that provides both, e.g. the ability to write to a specific location AND the ability to specify an explicit path? e.g.

type Step struct {
    Stdout PathConfig `json:"stdout,omitempty"`
}

type PathConfig struct{
   Auto bool
   Path string
}

So if you dont care where stdout ends up and you just want to pass it between steps you can use:

  - name: boskosctl
    image: gcr.io/k8s-staging-boskos/boskosctl
    ...
    stdout:
      auto: true
  - name: parse-project-id
    image: imega/jq
    args:
    - -r
    - .name
    - $(steps.boskosctl.stdout.path)

But if you do care you can use:

  - name: boskosctl
    image: gcr.io/k8s-staging-boskos/boskosctl
    ...
    stdout:
      path: /data/boskosctl-stdout
  - name: parse-project-id
    image: imega/jq
    args:
    - -r
    - .name
    - $(steps.boskosctl.stdout.path)

[chhsia0]

This boils down to whether we allow people to use $(workspaces.<name>.path) or $(results.<name>.path) or $(resources.outputs.<name>.path) in the specified path. If yes and if we want to keep a single-pass variable substitution, then we cannot provide $(steps.<name>.stdout.path).

[vdemeester]

See #182 (comment), I don't see why we would only add stdout and not stderr. From my point of view, it's not up to tekton to know which one is valuable/used by the user.

[bobcatfish]

From my point of view, it's not up to tekton to know which one is valuable/used by the user.

I don't think we've had any users say they actually need stderr so it feels like we don't need it, and if we don't need it, why not wait to add it until we do.

Sounds like you feel strongly @vdemeester tho

[chmouel]

I believe there is a use case for having stderr passed over,

For example, as a user I would like to write a tekton pipeline for python based software to do some linting of my python code.

I look around and find this container provided by the pylint authors https://hub.docker.com/r/eeacms/pylint/ which I can reuse directly in my tekton task.

I would like in a following task being able to capture the output (stdout) or the errors (stderr) of the task so I can for example post it back to GitHub or somewhere else...

Being able to do this directly without having to write a custom script based on eeacms/pylint/ to redirect stderr to stdout or store the stderr to my workspace make it more useful and easy for me.

[vdemeester]

/cc @tektoncd/core-maintainers

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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:

• teps/OWNERS [vdemeester]

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

[ghost]

All my comments are addressed and I'd like to see this move ahead. Adding hold to give @bobcatfish time to respond to the remaining open threads of conversation.

/lgtm

/hold

[bobcatfish]

I've added a few comments but I think they're all nitpicks :D Generally looks great! Thanks for your patience with the back and forth @chhsia0 !

I'm leaving the hold just because I think one of those nitpicks is in an example that isn't quite right, feel free to remove the hold!

[bobcatfish]

yet! :D

[bobcatfish]

boskosctl is actually a bad example b/c it DOES have a shell, but otherwise: what @chhsia0 said :D

[bobcatfish]

@chhsia0 one last nitpick, it looks like this PR has 14 commits right now, can those be squashed into 1?

Feel free to remove:
/hold

[chhsia0]

/hold cancel

[chhsia0]

I usually don't squash the commits in a PR because it allows reviewers to see diff since last review and the squash could be done through "Squash and merge." But sure I'm happy to do that now 👍

[vdemeester]

@chhsia0 😝 but it's a bot that merges, and only through rebase, so it would be nice, now that it's validated, that we would squash the commits before merge 😛

[vdemeester]

… and as I commented, you did it @chhsia0 🤗 😻
/lgtm