TEP-0126: Allow Task sidecars to be specified in PipelineRun #877
Conversation
|
|
tekton-robot
added
the
size/L
|
/kind tep |
tekton-robot
added
the
kind/tep
|
Thank you @michaelsauter for this interesting proposal! About the EasyCLA check, Tekton contributions require the CLA to be signed individually or by your employer. |
|
/assign @afrittoli |
tekton-robot
added
the
needs-rebase
|
@afrittoli Thanks, I will clarify this but may take a few days. |
|
@michaelsauter are you still interested in pursuing this TEP? |
|
@afrittoli Yes, definitely. Sorry the CLA thing got stuck somewhere and I need to chase it up. But the feature we definitely want and I believe is generally really helpful to make Tekton tasks reusable. |
|
@pritidesai: GitHub didn't allow me to assign the following users: pxp928. Note that only tektoncd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
I'm actually still chasing this up every now and then and there has been some movement in the new year, maybe I get it signed "soon" 😄 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
removed
the
needs-rebase
|
@afrittoli Finally, got the EasyCLA done ... what are the next steps? |
teps/0126-allow-task-sidecars-to-be-specified-in-pipelinerun.md
Outdated
Show resolved
Hide resolved
teps/0126-allow-task-sidecars-to-be-specified-in-pipelinerun.md
Outdated
Show resolved
Hide resolved
| claimName: my-source | ||
| ``` | ||
|
|
||
| This would behave exactly the same as if the `golang-test` task had specified the sidecar itself. |
There was a problem hiding this comment.
this is interesting - does it open a door to specify other task-level fields in pipelineruns? for example, specifying pre steps or post steps? 🤔
There was a problem hiding this comment.
I'm not sure it does directly, but I think I might see your point. This change introduces the ability to inject arbitrary code through the PipelineRun into the Pod, with the security concerns associated with it.
This is not something totally new, as we allow the task image to be parametrised, however, it allows the execution of extra code regardless of what the task author planned for.
If we add something like this, perhaps we could let authors disable it on a per-task basis, i.e. it's ok to inject a sidecar when running tests but not when building an artifact, so the author of the build task could disable injection.
This bring me back to the Pipeline vs PipelineRun approach - I think the change would be easier to accept if it was introduced at Pipeline level since that is still an authoring time artifact, which can be vetted and signed.
There was a problem hiding this comment.
If we add something like this, perhaps we could let authors disable it on a per-task basis, i.e. it's ok to inject a sidecar when running tests but not when building an artifact, so the author of the build task could disable injection.
Should this be the "initial" author's concern. We don't really control how will our Task and Pipeline be used. For example, a resolver can today dynamically inject code / mutate a Task prior to be passed to the controller, and there is no way around it 🙃.
There was a problem hiding this comment.
@michaelsauter - thank you taking this on!
Are we opening a door to override other aspects of the task definition, such as specifying steps at runtime? there have been requests to prepend and postpend steps in task specs - would supporting appending sidecars set a precedent for appending steps?
@michaelsauter maintainers need to review the proposal - for further process details, see https://github.com/tektoncd/community/tree/main/teps#reviewing-and-merging-teps cc @tektoncd/core-maintainers |
Co-authored-by: Jerop Kipruto <[email protected]>
Co-authored-by: Jerop Kipruto <[email protected]>
I didn't come across the need for that so far, so this TEP focuses only on sidecars. However it sounds good to me to consider similar use cases in this context and think about if they should be allowed or not. |
|
|
||
| ### Non-Goals | ||
|
|
||
| * Pipeline level sidecars, that live for the duration of the whole pipeline run, see https://github.com/tektoncd/pipeline/issues/2973. |
There was a problem hiding this comment.
I know Pipeline-level sidecars are out of scope here, but I'm curious why they wouldn't work for the use case you've identified? The main drawback seems to be that the sidecar lasts for the duration of the PipelineRun, while you only need them for one task. However, this could be combined with pipelines in pipelines to create a testing sub-pipeline with a database sidecar.
There was a problem hiding this comment.
There are two reasons I listed it here:
- The referenced issue was closed due to lack of interest.
- I understand pipeline-level sidecars as a completely new feature, while my proposal is only about exposing existing functionality (task-level sidecars) during pipeline authoring time. My hope (without having read the code) is that it would be much easier to add.
There was a problem hiding this comment.
Thanks! My hope is that we'll prioritize pipeline-level sidecars in the near term, as they're useful for caching docker builds in addition to the integration test use case you've mentioned here. You're right that the feature you're proposing is easier to implement, but we aim to prioritize features that best meet user needs, even if they're a bit harder to create :) I'm just trying to understand here if pipeline-level sidecars are a viable alternative proposal, and it sounds like they are?
There was a problem hiding this comment.
My hope is that we'll prioritize pipeline-level sidecars in the near term, as they're useful for caching docker builds in addition to the integration test use case you've mentioned here.
Ah! Did not think of the use case of caching docker builds. That sounds interesting as well.
You're right that the feature you're proposing is easier to implement, but we aim to prioritize features that best meet user needs, even if they're a bit harder to create :) I'm just trying to understand here if pipeline-level sidecars are a viable alternative proposal, and it sounds like they are?
Makes sense to me. I could live with both pipeline-level or task-level sidecars. Of course, pipeline-level would be a waste of resources for my use case, but then again would be the better option for cases where the sidecar is needed in multiple tasks.
There was a problem hiding this comment.
FYI, I've opened #943 proposing pipeline-level sidecars, primarily in the context of docker builds because I think the use case is more clear than for integration tests. LMK if you'd like to collaborate on this!
There was a problem hiding this comment.
Cool! Looking forward to this. Unfortunately I won't be able to contribute at this point as we had a bit of a shift of priorities in the last month.
The need for presteps/poststeps was to fetch/produce data e.g. git-clone as a prestep in a golang-test task - see #502 for further details. Related motivation and use cases for Task Composition and Scheduling are discussed in #316 - see TEP-0044 for the current state of the proposal. I'm wondering, if you could define a new Task with the Sidecars you need and we supported executing it alongside the main Task in one Pod, would that work for you? I ask this because it seems to me that the bigger problem is how we compose and execute Tasks. If we allow specifying Sidecars in PipelineRuns which are then added to Task specifications, we will set a precedent for specifying Steps in PipelineRuns which are then added to Task specifications. I propose that we explore the motivation for this TEP in the context of the broader problem space of composing and executing Tasks - what do you think? |
Hmm this sounds a bit hack-ish to me. Why would I need another task, to define a sidecar? And how would I ensure that the sidecar in task B is running for as long as task A needs it? While I think TEP-0044 is much needed, I don't think it should be used to solve this feature request.
I disagree. Exchanging sidecars is basically the same as configuring compute resources. It does not alter the task steps. I think this distinction is possible to maintain going forward. |
|
/assign @jerop |
| - name: version | ||
| value: "0.6" | ||
| sidecars: | ||
| - image: postgres:14 |
There was a problem hiding this comment.
@michaelsauter In this example, I'm curious to know how the go tests would get the address of the sidecar service? Normally, I would expect the database address to be passed in through an environment variable or similar, but I'm not sure there's a way to configure this for the golang-test catalog task. Would you have to bake a localhost address into your tests?
There was a problem hiding this comment.
Right, my proposal does not address this question. As far as I know there is no way to pass in env variables from a pipeline run, so the address of the Postgres instance would need to be hardcoded in some way. E.g. through an "env file" which tells the app where to find the database locally, on CI, in QA, in prod, etc. In our case we typically set CI=true in the build tasks so that the app can use that variable to choose the right config.
There was a problem hiding this comment.
It might be possible to use the flags parameter or some other generic parameter.
BTW, TEP-0101 has been implemented so you can specify an env in a pod template as part of a pipeline run: https://github.com/tektoncd/community/blob/main/teps/0101-env-in-pod-template.md
|
|
||
| ## Motivation | ||
|
|
||
| Make tasks more reusable. |
| - name: version | ||
| value: "0.6" | ||
| sidecars: | ||
| - image: postgres:14 |
There was a problem hiding this comment.
It might be possible to use the flags parameter or some other generic parameter.
BTW, TEP-0101 has been implemented so you can specify an env in a pod template as part of a pipeline run: https://github.com/tektoncd/community/blob/main/teps/0101-env-in-pod-template.md
| claimName: my-source | ||
| ``` | ||
|
|
||
| This would behave exactly the same as if the `golang-test` task had specified the sidecar itself. |
There was a problem hiding this comment.
I'm not sure it does directly, but I think I might see your point. This change introduces the ability to inject arbitrary code through the PipelineRun into the Pod, with the security concerns associated with it.
This is not something totally new, as we allow the task image to be parametrised, however, it allows the execution of extra code regardless of what the task author planned for.
If we add something like this, perhaps we could let authors disable it on a per-task basis, i.e. it's ok to inject a sidecar when running tests but not when building an artifact, so the author of the build task could disable injection.
This bring me back to the Pipeline vs PipelineRun approach - I think the change would be easier to accept if it was introduced at Pipeline level since that is still an authoring time artifact, which can be vetted and signed.
|
|
||
| ### Goals | ||
|
|
||
| * Extend `PipelineRun` to allow to specify all aspects of sidecars, matching how [sidecars are specified in tasks](https://tekton.dev/docs/pipelines/tasks/#specifying-sidecars). |
There was a problem hiding this comment.
Why PipelineRun directly and not start with a Pipeline?
A Pipeline can be signed and verified, and it provides the same benefits of Task reusability.
If someone wants to reproduce a test run in their local cluster, having the correct setup in the Pipeline would provide a nicer user experience.
There was a problem hiding this comment.
The problem with Pipeline is that it doesn't allow to e.g. specify compute resource (at least not to my understanding).
Some context: We have a YAML file in each repo which specifies a list of Tekton tasks. That file is read by a "pipeline manager" which is triggered when a push happens in the repo, and the "pipeline manager" creates a PipelineRun reflecting the tasks in the YAML file in the repo. There never is a Pipeline resource in our cluster - the pipeline is defined in code.
This is also the approach that OpenShift pipelines-as-code has taken (to my understanding). In my experience, the distinction between pipeline and pipeline run is a bit odd and even confusing, especially when using Git as the source of truth for the "pipeline definition". And as I wrote at the beginning, a lot of the features we want to expose to users are only available in pipeline runs, hence we need to expose them to the users.
|
|
||
| I see a bit of conflict between this proposal and the design of [Overriding Task Steps and Sidecars](https://tekton.dev/docs/pipelines/taskruns/#overriding-task-steps-and-sidecars). One might think that `sidecarOverrides` would allow to override which sidecars are used and what their image is etc., however the feature is restricted to `resources` only, at least for now. | ||
|
|
||
| If `sidecarOverrides` would allow to override more fields (in particular, `image` and `env` seem crucial) this may solve part of the use case explained above. However, I do not see how the design of `sidecarOverrides` could be used to e.g. specify sidecars when the task itself does not define any, or add a second sidecar if the task defines one sidecar. Therefore I would see the proposal here separate from `sidecarOverrides` even though some of the motivation is shared. |
There was a problem hiding this comment.
Indeed, sidecarOverrides is not designed to introduce new sidecars
|
|
||
| Workaround #2 is not seen as adequate because the parallel task has a lifetime longer than the task run, a finally task is needed to terminate it, and it uses the feature "parallel tasks" to mimick another feature ("sidecars") instead of giving access to the sidecar feature where the user needs it. | ||
|
|
||
| Creating a task with a sidecar baked in (workaround #3) certainly works, but it prevents reuse: I can't use the `golang-test` task anymore just because there is no sidecar defined. The task I would need to create is exactly the same as `golang-test`, except for the sidecar definition. If there was a way to override sidecars, I could use the task from the ArtifactHub as is and supply the sidecar configuration fitting for the pipeline run. |
There was a problem hiding this comment.
I think this is a bit what @jerop was commenting, but if we had a way to "enhance" a Task with steps (pre/post the task) at runtime or authoring time, this one could be achieve while keeping re-usability of Tasks.
At authoring time, as a user, you would "wrap" the golang-test with your own set of requirements (expressed as steps before and after the "golang-test steps".
| claimName: my-source | ||
| ``` | ||
|
|
||
| This would behave exactly the same as if the `golang-test` task had specified the sidecar itself. |
There was a problem hiding this comment.
If we add something like this, perhaps we could let authors disable it on a per-task basis, i.e. it's ok to inject a sidecar when running tests but not when building an artifact, so the author of the build task could disable injection.
Should this be the "initial" author's concern. We don't really control how will our Task and Pipeline be used. For example, a resolver can today dynamically inject code / mutate a Task prior to be passed to the controller, and there is no way around it 🙃.
|
|
||
| If `sidecarOverrides` would allow to override more fields (in particular, `image` and `env` seem crucial) this may solve part of the use case explained above. However, I do not see how the design of `sidecarOverrides` could be used to e.g. specify sidecars when the task itself does not define any, or add a second sidecar if the task defines one sidecar. Therefore I would see the proposal here separate from `sidecarOverrides` even though some of the motivation is shared. | ||
|
|
||
| In any case, the proposed solution must accomodate the situation where the task already defines one or more sidecars. Then, it must be clear to pipeline run authors how the feature of "specifying / overriding sidecars in pipeline runs" interacts with task-defined sidecars. I see the option to: (a) override the task definition, (b) extend the task defition or (c) allow both, e.g. via different fields. It is not clear for me (yet) what the best option would be. |
There was a problem hiding this comment.
Also, what would happens in sidecars from a PipelineRun comes with the exact same name as an already defined sidecar in the Task definition.
There was a problem hiding this comment.
I guess in such case we should probably fail?
|
Issues go stale after 90d of inactivity. /lifecycle stale Send feedback to tektoncd/plumbing. |
tekton-robot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. /lifecycle rotten Send feedback to tektoncd/plumbing. |
tekton-robot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. /close Send feedback to tektoncd/plumbing. |
|
@tekton-robot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This TEP originates from tektoncd/pipeline#4235 (comment).
Happy to explain this further or discuss in a working group or so.