Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[TEP-0112] Replace volumes with Workspaces [Proposal] #720

Merged
merged 1 commit into from
Jun 28, 2022
Merged

[TEP-0112] Replace volumes with Workspaces [Proposal] #720

merged 1 commit into from
Jun 28, 2022

Conversation

lbernick
Copy link
Member

@lbernick lbernick commented Jun 7, 2022 •
edited
Loading

This TEP proposes removing volumes from the Task spec in the v1 API, as well as removing
volumeMounts and volumeDevices for the container-like fields (Step, StepTemplate, and Sidecar).

/kind tep

@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Jun 7, 2022
@tekton-robot tekton-robot requested review from dibyom and wlynch June 7, 2022 01:13
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 7, 2022
@lbernick
Copy link
Member Author

lbernick commented Jun 7, 2022

Addresses tektoncd/pipeline#2058

@vdemeester @skaegi PTAL

@lbernick
Copy link
Member Author

lbernick commented Jun 7, 2022

/hold

need to do a bit more digging--will revisit the proposal to support hostPath
@wlynch relevant for you as well

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 7, 2022
@lbernick
Copy link
Member Author

lbernick commented Jun 8, 2022

/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2022
@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2022
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2022
@lbernick
Copy link
Member Author

/assign @dibyom @pierretasci

@lbernick lbernick force-pushed the tep-111 branch 2 times, most recently from 8fc72a1 to 40b3093 Compare June 14, 2022 14:22
@pritidesai pritidesai mentioned this pull request Jun 14, 2022
Merged
4 tasks
@lbernick lbernick force-pushed the tep-111 branch 2 times, most recently from 54e660c to 007cabb Compare June 16, 2022 17:50
@lbernick
Copy link
Member Author

I chatted with @pierretasci yesterday about the "dynamic volumes" use case, and added it here as an existing use case that workspaces don't support. I need to think more about what we should do about this, but for now I added "dynamic workspaces" as an optional workspace enhancement. We are both in agreement that this is the right long-term direction for volumes/workspaces, but want to avoid removing support for an existing use case even if it isn't an "intended" use of the API.

I've also added more detail on the options for different volume types we might want to support in workspaces, including embedding the k8s volumesource in workspacebinding. For now I think the existing workspace binding options are sufficient for the use cases we're targeting.

afrittoli
afrittoli reviewed Jun 17, 2022
View reviewed changes
teps/0111-replace-volumes-with-workspaces.md Outdated
Comment on lines 115 to 118
The only Catalog Task using hostPath volumes is [`kind`](https://hub.tekton.dev/tekton/task/kind).
For more information on why `kind` on Kubernetes requires hostPath volumes, see
https://github.com/kubernetes-sigs/kind/issues/303#issuecomment-521384993 and
[Running KIND Inside A Kubernetes Cluster For Continuous Integration](https://d2iq.com/blog/running-kind-inside-a-kubernetes-cluster-for-continuous-integration)
(TL;DR: because `kind` needs to create nested Docker containers).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the alternative here if we removed workspaces?
I agree that using volumes directly instead of workspaces is not a best practice, but unless we have an alternative here I wonder if we could address this by documentation, i.e. make sure that volumes are not used in any example / catalog task unless were strictly required. We could add a special page / section to document this feature as something we do not recommend to use unless in specific documented cases and specify security risks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this as an option to "alternatives". Right now, I don't think we would have an alternative for this using workspaces.

@lbernick lbernick changed the title [TEP-0111] Remove volumes from Task spec [TEP-0111] Replace volumes with Workspaces Jun 17, 2022
@lbernick
Copy link
Member Author

@pritidesai @afrittoli @pierretasci thanks for the feedback so far!
I think there are a few too many open questions so I've marked this as "proposed", removed the "proposal" section, and put the ideas being floated under "alternatives" (can always add more). Let's align on the use cases we're addressing and then we can figure out what to do about it.

@lbernick lbernick changed the title [TEP-0111] Replace volumes with Workspaces [TEP-0111] Replace volumes with Workspaces [Proposal] Jun 17, 2022
@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2022
@lbernick
[TEP-0112] Proposal: Replace volumes with workspaces
0f4f48f 
This TEP proposes removing volumes from the Task spec in the v1 API, as well as removing
volumeMounts and volumeDevices for the container-like fields (Step, StepTemplate, and Sidecar).
It does not suggest a preferred solution yet, only alternatives.
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2022
@lbernick lbernick changed the title [TEP-0111] Replace volumes with Workspaces [Proposal] [TEP-0112] Replace volumes with Workspaces [Proposal] Jun 28, 2022
dibyom
dibyom reviewed Jun 28, 2022
View reviewed changes
teps/0112-replace-volumes-with-workspaces.md
- `task.spec.volumes`
- `task.spec.steps.volumeMounts`
- `task.spec.steps.volumeDevices`
- `task.spec.sidecars.volumeMounts`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm less sure we want to remove volumes for sidecars - sidecars could be regular containers that users need to run e.g. a db for an integration test task

dibyom
dibyom reviewed Jun 28, 2022
View reviewed changes
teps/0112-replace-volumes-with-workspaces.md
[security risks](https://blog.quarkslab.com/kubernetes-and-hostpath-a-love-hate-relationship.html)
they pose.

#### Support CSI volumes in Workspace Bindings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSI volume support is already underway tektoncd/pipeline#4446

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2022
@pierretasci
Copy link
Contributor

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2022
@tekton-robot tekton-robot merged commit 1b3d0b7 into tektoncd:main Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierretasci pierretasci pierretasci left review comments

@Tomcli Tomcli Tomcli left review comments

@afrittoli afrittoli afrittoli left review comments

@pritidesai pritidesai pritidesai left review comments

@dibyom dibyom dibyom approved these changes

@wlynch wlynch Awaiting requested review from wlynch

Assignees

@dibyom dibyom

@pierretasci pierretasci

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
TEP Steward Assignments
Status: UnAssigned
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@lbernick @tekton-robot @pierretasci @pritidesai @afrittoli @dibyom @Tomcli