search

tektoncd / pipeline

A cloud-native Pipeline resource.
https://tekton.dev
Apache License 2.0
8.52k stars 1.78k forks source link

Remove "volumes" from Task #2058

Closed bobcatfish closed 1 year ago

bobcatfish commented 4 years ago

Expected Behavior

Task authors should declare when they need files (workspaces) + block devices (#2057 ) but they should leave it to the TaskRun to provide these.

Actual Behavior

Having volumes as a field in Tasks means that Task authors are dictating at Task authoring time what volumes will be provided to a Task.

Additional Info

We should remove Volumes from the TaskSpec before beta (if we can) to indicate it is behavior we don't want to support going forward.

We could also prevent people from using the volumeMounts and volumeDevices fields in a step as well but that feels like it's not the worst thing to leave in since it's not yet runtime information? Open to other thoughts tho.

vdemeester commented 4 years ago

/area api /kind cleanup

vdemeester commented 4 years ago

I like that idea, I wonder if we should do this for v1alpha2 directly (hence having this in 0.11) :wink:

vdemeester commented 4 years ago

Putting it in the milestone to make sure we discuss it quickly :angel:

skaegi commented 4 years ago

Apart from the block device case the only other case I can think of is volume as a task implementation detail that we don't allow customization of... but that's sort of weak.

kav commented 4 years ago

Is there a reason not to use volumes and volumeMounts to inject various relatively static configuration (.netrc, repositories.yaml) from secrets into specific directories in specific task steps so tools find them seamlessly? I've got a fair bit of that and it works great. Not sure how workspaces would be used there.

bobcatfish commented 4 years ago

That's a good point @kav ! Do you have a sense for whether you'd rather declare that kind of information at Task authoring time, or at runtime?

ghost commented 4 years ago

various relatively static configuration (.netrc, repositories.yaml) from secrets into specific directories in specific task steps so tools find them seamlessly?

This is a usecase for workspaces that I've been thinking about a lot recently! It would be cool if you could define workspaces in a TaskRun that are not mentioned at all in a Task and then Tekton would include those workspaces in the step containers as well. Static configs from secrets are a great use case of where this could be useful. I was thinking about it in terms of the .ssh directory but it's great to see some other examples where this might also be useful.

bobcatfish commented 4 years ago

Another use case maybe is sharing data between sidecars and steps, ala the docker in docker example https://github.com/tektoncd/pipeline/blob/master/examples/taskruns/dind-sidecar.yaml

BUT maybe being able to inject the workspace would be better after all b/c then at runtime you have more control.... maybe workspace with a default of emptyDir? (@sbwsg)

kav commented 4 years ago

That's a good point @kav ! Do you have a sense for whether you'd rather declare that kind of information at Task authoring time, or at runtime?

Some of it is authoring related information around configuring this task for this environment and some falls under runtime. My taskruns are generated via triggers so I could put "configuration" info there?

Not sure that addresses step volume mount location being task side info. In essence the Task, by virtue of owning the container, is really the only one that can decide the "right" place for an ssh key or a netrc file.

Might be worth defining some things, and apologies that I'm bringing Helm to the party so might muddle things. You all might have different terms. Taking a stab, feel free to stab back ;):

There is static context which is just the fixed content authored into the task; i.e. build a container, run a jenkinsfile, run a gitlab build file.

There is environment context which is all the stuff the task needs to play nice with the servers it touches, this is fixed across all runs but multiple installs of the same task could vary here. It tends to be very specific to the static context. For example repositories.yaml needs to go to in one directory if the step uses helm2 and another for helm3. Each of these environment objects is needed only by some steps based on what those steps do. I don't really mind them hanging about but I could see not wanting all steps to have helm push permissions for example. I drive all this via Secrets linked to authored volumes since it's almost all credentials. So it's a bit "half at authoring, half at install/run time". Most end up in very specifically placed file or another on a per step basis so steps can seamlessly rely on them.

There is build context which would include source and run name, pr url , author, and all the stuff that varies by taskrun. Today for me this all comes through the TriggerTemplates.

I could use TaskRun config to push environment context I suppose, it feels like mount points would still need to be in the task. I like it in the Task since it corresponds to step specific info. Locality of change is the same. To take the helm case; imagine I have a Task that was using Helm 2 and I'm switching to Helm 3. Today, with secrets and volumes, I open the Task object and change the mount for the volume as well as the container image for that step and boom I'm done.

I also don't have any Pipelines or PipelineRuns; haven't needed them as yet but I suspect the same applies.

ghost commented 4 years ago

From WG it sounds like we should mark this as deprecated before we wholesale remove. If we are going to do this then I will add a notice to the 0.11 release.

bobcatfish commented 4 years ago

Decided not to remove it, closing this for now, can open more issues to continue to look into volumes in future.

lbernick commented 2 years ago

/reopen /assign

We want to revisit the decision not to remove volumes from Task spec before releasing our v1 api and committing to supporting it for at least a year (following our compatibility policy), especially since there's not much detail on why the decision was made not to do this for beta.

I'll look into this and either open a TEP proposing this change, or update this issue with a summary of why we don't want to do this.

tekton-robot commented 2 years ago

@lbernick: Reopened this issue.

In response to [this](https://github.com/tektoncd/pipeline/issues/2058#issuecomment-1142644196): >/reopen >/assign > >We want to revisit the decision not to remove volumes from Task spec before releasing our v1 api and committing to supporting it for at least a year (following our compatibility policy), especially since there's not much detail on why the decision was made not to do this for beta. > >I'll look into this and either open a TEP proposing this change, or update this issue with a summary of why we don't want to do this. Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
kav commented 2 years ago

Let me know if you'd like a deeper dive on the example above. It's been a few years and I'm not working on that at the moment but happy to discuss the conceptual framework at a deeper level at the least.

lbernick commented 2 years ago

Thanks so much @kav! I think some features have been added which may address the use cases you've described above. To summarize what I see as the current status of this issue:

Why remove volumes from Task? A long-term goal for Tekton is to make our API more platform-agnostic, so that it does not need to be implemented on Kubernetes. Removing volumes would be a step in this direction.

Why not remove volumes from Task? Workspaces support bindings from PVCs, secrets, configmaps, and emptyDir, but not all types of volume sources (and we don't plan to support all types of volume sources). Users may have use cases for these volume types that we aren't aware of.

A better abstraction here might be something that allows people to implement different backing sources, so that Tekton isn't responsible for supporting all these volume types as workspace backings-- FYI @jerop this is relevant to the "data interfaces" problem space (and @skaegi you may also have opinions here?)

What about Step.VolumeMounts and Sidecar.VolumeMounts? We support making workspaces available to individual Steps and Sidecars, so I believe these fields can be replaced with workspaces. For Sidecars especially, some users may still be using VolumeMounts since we added support for workspaces in Sidecars a bit later (example). We should promote workspaces in sidecars to beta unless there's a good reason not to.

What about Step.VolumeDevices and Sidecar.VolumeDevices? Volume devices aren't supported by workspaces, but I don't know of any use cases for this field. I'm guessing we have it mainly because we used to embed the Kubernetes container definition in Step and Sidecar. FYI @imjasonh @vdemeester

What are the use cases for Task.Volumes, and can they be replaced with workspaces? The use cases we list for Volumes are:

@kav do you think the use cases you've described would be addressed by these features?

What should we do going forward? (IMO) I think we should go ahead with this deprecation to make our API more platform-agnostic. I don't know what use cases exist for types of volumes in Tasks other than the workspace bindings we support, but a better long-term solution IMO would be some way to allow more types of backings for workspaces without requiring support to be built into Pipelines.

We should promote workspaces in sidecars to beta and update our code examples to use workspaces instead of volumes/volumeMounts before deprecating volumes in Tasks.

vdemeester commented 2 years ago

Why not remove volumes from Task? Workspaces support bindings from PVCs, secrets, configmaps, and emptyDir, but not all types of volume sources (and we don't plan to support all types of volume sources). Users may have use cases for these volume types that we aren't aware of.

A better abstraction here might be something that allows people to implement different backing sources, so that Tekton isn't responsible for supporting all these volume types as workspace backings-- FYI @jerop this is relevant to the "data interfaces" problem space (and @skaegi you may also have opinions here?)

I think, as long as we support a "extensible" mechanisms to be bound in workspace (such as CSI volumes), we could remove volumes and thus "limit" the scope of things we actually supported to be mounted in a Task. If there is something missing that is needed, we will get request for it and we can implement it.

kav commented 2 years ago

Looking at the details and the docs I think so. I might be repeating myself a bit to warm my thinking up so apologies if this is a repeat of the above. My biggest worry was maintaining a boundary between

  1. The config that needs to be provided to build an app of X type
  2. The config that needs to be provided to build this specific app Y that is of type X
  3. The config that relates to this specific build run Z of app Y that is of type X

To dig in a bit on why the distinction above:

  1. As an expert in building (go, node, helm, etc) apps I can create Tekton resources that describe how to optimally build test and publish those apps. Everyone can contribute and share best practices.
  2. As someone building a Tekton pipeline I can leverage 1 and provide my organizations specific details (e.g. registry push credentials) without deeply understanding the ins and outs of 1. Workspaces
  3. As someone building a Tekton pipeline I want to encode run details into my build process, track my runs in kubernetes, report on run details etc.

Workspace is potentially the interface boundary between 1 and 2 which means as a author of 1 I'd need to provide a template Workspace to a consumer for them to fill out with sensible defaults they could match for their pvcs and secrets and whatnot. Or they could override as needed if they used different names, that's probably reasonable and not an uncommon pattern in the space.

Note: It might be interesting to add a slice of "on a platform" to the above taxonomy as it sounds like that's what the factorization is about.

lbernick commented 2 years ago

I still think this is the right long term direction for volumes and workspaces, but there are a few use cases for volumes not currently being met by workspaces. We should try to address these gaps (or think more about what we want our api for volume/volumemount-like things to look like as a whole), but I think this requires a bit more thought and I don't want to rush through it just so we can remove volumes before v1. So I think volumes can stay in our v1 api, although of course I'm open to discussion here.

tekton-robot commented 2 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale with a justification. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close with a justification. If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

tekton-robot commented 1 year ago

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten with a justification. Rotten issues close after an additional 30d of inactivity. If this issue is safe to close now please do so with /close with a justification. If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

tekton-robot commented 1 year ago

Rotten issues close after 30d of inactivity. Reopen the issue with /reopen with a justification. Mark the issue as fresh with /remove-lifecycle rotten with a justification. If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

tekton-robot commented 1 year ago

@tekton-robot: Closing this issue.

In response to [this](https://github.com/tektoncd/pipeline/issues/2058#issuecomment-1382826654): >Rotten issues close after 30d of inactivity. >Reopen the issue with `/reopen` with a justification. >Mark the issue as fresh with `/remove-lifecycle rotten` with a justification. >If this issue should be exempted, mark the issue as frozen with `/lifecycle frozen` with a justification. > >/close > >Send feedback to [tektoncd/plumbing](https://github.com/tektoncd/plumbing). Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.