Add "volume" PipelineResource 🔊 #1417
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will allow copying content either into or out of a `TaskRun`, either to an existing volume or a newly created volume. The immediate use case is for copying a pipeline's workspace to be made available as the input for another pipeline's workspace without needing to deal with uploading everything to a bucket. The volume, whether already existing or created, will not be deleted at the end of the `PipelineRun`, unlike the artifact storage PVC. The Volume resource is a sub-type of the general Storage resource. Since this type will require the creation of a PVC to function (to be configurable later), this commit adds a Setup interface that PipelineResources can implement if they need to do setup that involves instantiating objects in Kube. This could be a place to later add features like caching, and also is the sort of design we'd expect once PipelineResources are extensible (PipelineResources will be free to do whatever setup they need). The behavior of this volume resource is: 1. For inputs, copy data _from_ the PVC to the workspace path 2. For outputs, copy data _to_ the PVC from the workspace path If a user does want to control where the data is copied from, they can: 1. Add a step that copies from the location they want to copy from on disk to /workspace/whatever 2. Use the "targetPath" argument in the PipelineResource to control the location the data is copied to (still relative to targetPath https://github.com/tektoncd/pipeline/blob/master/docs/resources.md#controlling-where-resources-are-mounted) 3. Use `path` https://github.com/tektoncd/pipeline/blob/master/docs/resources.md#overriding-where-resources-are-copied-from (tbd if we want to keep this feature post PVC) The underlying PVC will need to be created by the Task reonciler, if only a TaskRun is being used, or by the PipelineRun reconciler if a Pipeline is being used. The PipelineRun reconciler cannot delegate this to the TaskRun reconciler b/c when two different reconcilers create PVCs and Tekton is running on a regional GKE cluster, they can get created in different zones, resulting in a pod that tries to use both being unschedulable. In order to actually schedule a pod using two volume resources, we had to: - Use a storage class that can be scheduled in a GKE regional cluster https://cloud.google.com/kubernetes-engine/docs/how-to/persistent-volumes/regional-pd - Either use the same storage class for the PVC attached automatically for input/output linking or don't use the PVC (chose the latter!) This commit removes automatic PVC copying for input output linking of the VolumeResource b/c since it itself is a PVC, there is no need to copy between an intermediate PVCs. This makes it simpler to make a Task using the VolumeResource schedulable, removes redundant copying, and removes a side effect where if a VolumeResources output was linked to an input, the Task with the input would see _only_ the changes made by the output and none of the other contents of the PVC. Also removing the docs on the `paths` param (i.e. "overriding where resources are copied from") because it was implemented such that it would only work in the output -> input linking PVC case and can't actually be used by users and it will be removed in tektoncd#1284. fixes tektoncd#1062 Co-authored-by: Dan Lorenc <lorenc.d@gmail.com> Co-authored-by: Christie Wilson <bobcatfish@gmail.com>
|
[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 |
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
tekton-robot
added
the
size/XXL
|
Carrying over from #1416 from @abayer :
Currently not cleaned up at all!
If you created a PVC with the same name as the Volume Resource would create, it would get reused. TODO, either in this same PR or in follow up PRs:
|
bobcatfish
added
cla: yes
|
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) ℹ️ Googlers: Go here for more info. |
|
/test pull-tekton-pipeline-integration-tests |
|
@bobcatfish: The following tests failed, say
Full PR test history. Your PR dashboard. 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 understand the commands that are listed here. |
|
/hold |
tekton-robot
added
the
do-not-merge/hold
|
Hi @bobcatfish , am a new enthusiast for Tekton. I wanted to know the status on this PR , willing to contribute if there is still lot of work before this one goes in and need a helping hand. |
|
We've started to discuss the possibility of giving volumes their own dedicated field in the TaskRun spec. See #1438 for a bit more detail on the thinking around this. |
ghost
reviewed
Oct 22, 2019
| name: volume-resource-2 | ||
| spec: | ||
| type: storage | ||
| params: |
There was a problem hiding this comment.
I'm getting an error running this from the webhook:
Error from server (InternalError): error when creating "./examples/pipelineruns/volume-output-pipelinerun.yaml": Internal error occurred: admission webhook "webhook.tekton.dev" denied the request: mutation failed: missing field(s): spec.params.size
Error from server (InternalError): error when creating "./examples/pipelineruns/volume-output-pipelinerun.yaml": Internal error occurred: admission webhook "webhook.tekton.dev" denied the request: mutation failed: missing field(s): spec.params.size
There was a problem hiding this comment.
shoot, makes one wonder how they ever got this to run XD XD XD
im guessing i added that validation after running it 🤦♀ oh well XD
|
After lots of discussions and the pipelineresource working group meetings it looks like we might not need this after all so I'm gonna close this PR. There are a few minor changes I made in this PR that I think are still useful so I'll open separate PRs for them (like I probably should have done in the first place! XD). I'll update #1062 with why this VolumeResource is probably not needed and what folks should expect instead. (@ravikiranbukka thanks for wanting to contribute to this! You might be interested in the update I'll put in #1062 - and if it turns out the solutions we're aiming at won't meet your use case we may need to open this back up again :)) |
This was referenced Oct 25, 2019
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this pull request
Oct 25, 2019
While working on tektoncd#1417, which we decided in the end not to merge, I made a bunch of updates to docstrings and comments which are now in this commit: - Adding more detail to some docstrings - Adding missing docstrings - Updating docstrings that were sometimes wrong (e.g. a function name had changed) - Removing some comments in the Conditions logic that were no longer accurate - Added comments in some sections of code that took me a while to understand to hopefully make it faster next time :)
2 tasks
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this pull request
Oct 25, 2019
Task modifiers for PipelineResources add pre and post steps and sometimes Volumes to a pod spec. Adding a step or a volume that already exists will create a pod that cannot run so this commit adds validation: - If a modifier tries to add a step that already exists (i.e. the name is the same) that's an error - If a modifier tries to add a volume that already exists but the volume it references is the same, we assume that two resources need the same volume, so it's not an error, but we only add it wonce - If a modifier tries to add a volume that already exists but the volume it references is different, that means two resources need different volumes but we cant add both since the names are the same, so that's an error I encountered a problem with this while working on tektoncd#1417 (which we decided not to merge) where I tried to use two VolumeResources and there was a bug where they both had the same name, so the pod was not schedulable. I had to work backwards from the error that the pod was invalid and thought it would be much more handy to get the error earlier, so I added this.
2 tasks
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this pull request
Oct 25, 2019
Task modifiers for PipelineResources add pre and post steps and sometimes Volumes to a pod spec. Adding a step or a volume that already exists will create a pod that cannot run so this commit adds validation: - If a modifier tries to add a step that already exists (i.e. the name is the same) that's an error - If a modifier tries to add a volume that already exists but the volume it references is the same, we assume that two resources need the same volume, so it's not an error, but we only add it wonce - If a modifier tries to add a volume that already exists but the volume it references is different, that means two resources need different volumes but we cant add both since the names are the same, so that's an error I encountered a problem with this while working on tektoncd#1417 (which we decided not to merge) where I tried to use two VolumeResources and there was a bug where they both had the same name, so the pod was not schedulable. I had to work backwards from the error that the pod was invalid and thought it would be much more handy to get the error earlier, so I added this.
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this pull request
Oct 25, 2019
While working on tektoncd#1417 (which we decided not to go ahead with) I made a _lot_ of unit tests fail. B/c I was changing how steps were added (for the volume resource) these failures were often in the middle of big diffs. It turns out we were inconsistent about how we used cmp.Diff: anything that is in the first argument but not in the second gets a `-` before it, anything that is in the second argument but not in the first gets a `+`, which is why the example at https://godoc.org/github.com/google/go-cmp/cmp#Diff passes these arguments as `cmp.Diff(want, got)`. If we reverse them, which many of our tests did, the results are counter intuitive: the "extra" stuff gets a `-` and the missing stuff gets a `+`. It seems like a small thing but it's really hard to wrap your head around! So this commit takes a bunch of these tests (not all of them, just the ones I was touchin in tektoncd#1417) and: - Makes them consistently pass `(want, got)` - Annotates the failure message with a legend for `-` and `+` - Adds more details to the messages when applicable so you know what the diff is that you're looking at
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this pull request
Oct 25, 2019
While working on tektoncd#1417 (which we decided not to go ahead with) I made a _lot_ of unit tests fail. B/c I was changing how steps were added (for the volume resource) these failures were often in the middle of big diffs. It turns out we were inconsistent about how we used cmp.Diff: anything that is in the first argument but not in the second gets a `-` before it, anything that is in the second argument but not in the first gets a `+`, which is why the example at https://godoc.org/github.com/google/go-cmp/cmp#Diff passes these arguments as `cmp.Diff(want, got)`. If we reverse them, which many of our tests did, the results are counter intuitive: the "extra" stuff gets a `-` and the missing stuff gets a `+`. It seems like a small thing but it's really hard to wrap your head around! So this commit takes a bunch of these tests (not all of them, just the ones I was touchin in tektoncd#1417) and: - Makes them consistently pass `(want, got)` - Annotates the failure message with a legend for `-` and `+` - Adds more details to the messages when applicable so you know what the diff is that you're looking at
2 tasks
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this pull request
Oct 25, 2019
Task modifiers for PipelineResources add pre and post steps and sometimes Volumes to a pod spec. Adding a step or a volume that already exists will create a pod that cannot run so this commit adds validation: - If a modifier tries to add a step that already exists (i.e. the name is the same) that's an error - If a modifier tries to add a volume that already exists but the volume it references is the same, we assume that two resources need the same volume, so it's not an error, but we only add it wonce - If a modifier tries to add a volume that already exists but the volume it references is different, that means two resources need different volumes but we cant add both since the names are the same, so that's an error I encountered a problem with this while working on tektoncd#1417 (which we decided not to merge) where I tried to use two VolumeResources and there was a bug where they both had the same name, so the pod was not schedulable. I had to work backwards from the error that the pod was invalid and thought it would be much more handy to get the error earlier, so I added this.
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this pull request
Oct 25, 2019
Task modifiers for PipelineResources add pre and post steps and sometimes Volumes to a pod spec. Adding a step or a volume that already exists will create a pod that cannot run so this commit adds validation: - If a modifier tries to add a step that already exists (i.e. the name is the same) that's an error - If a modifier tries to add a volume that already exists but the volume it references is the same, we assume that two resources need the same volume, so it's not an error, but we only add it wonce - If a modifier tries to add a volume that already exists but the volume it references is different, that means two resources need different volumes but we cant add both since the names are the same, so that's an error I encountered a problem with this while working on tektoncd#1417 (which we decided not to merge) where I tried to use two VolumeResources and there was a bug where they both had the same name, so the pod was not schedulable. I had to work backwards from the error that the pod was invalid and thought it would be much more handy to get the error earlier, so I added this.
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this pull request
Oct 25, 2019
While working on tektoncd#1417, which we decided in the end not to merge, I made a bunch of updates to docstrings and comments which are now in this commit: - Adding more detail to some docstrings - Adding missing docstrings - Updating docstrings that were sometimes wrong (e.g. a function name had changed) - Removing some comments in the Conditions logic that were no longer accurate - Added comments in some sections of code that took me a while to understand to hopefully make it faster next time :)
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this pull request
Oct 25, 2019
While working on tektoncd#1417 (which we decided not to go ahead with) I made a _lot_ of unit tests fail. B/c I was changing how steps were added (for the volume resource) these failures were often in the middle of big diffs. It turns out we were inconsistent about how we used cmp.Diff: anything that is in the first argument but not in the second gets a `-` before it, anything that is in the second argument but not in the first gets a `+`, which is why the example at https://godoc.org/github.com/google/go-cmp/cmp#Diff passes these arguments as `cmp.Diff(want, got)`. If we reverse them, which many of our tests did, the results are counter intuitive: the "extra" stuff gets a `-` and the missing stuff gets a `+`. It seems like a small thing but it's really hard to wrap your head around! So this commit takes a bunch of these tests (not all of them, just the ones I was touchin in tektoncd#1417) and: - Makes them consistently pass `(want, got)` - Annotates the failure message with a legend for `-` and `+` - Adds more details to the messages when applicable so you know what the diff is that you're looking at
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this pull request
Oct 25, 2019
The test was dynamically adding the expected PVC volumes. This created two problems: 1. If there was a bug in the logic that was adding them dynamically, we we could end up covering a bug 2. This made it confusing to try to reason about what the actual expected pod structure was. In tektoncd#1417 I had to make a lot of changes to these tests and it was super confusing to look at the expected structure and sometimes see no volumes (or even volumes: nil) when you knew volumes were supposed to be there (I was scared there was a serious bug for a while) So now the expected state explicitly lists all the volumes it expects. This also revealed that one test case was using `pipelinerun-parent` as the name of the parent pipelinerun even though all the rest used just `pipelinerun` which was also confusing when I was making changes b/c I initially thought there was some special case where tekton itself would add "parent" but no.
2 tasks
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this pull request
Oct 25, 2019
When I was working on tektoncd#1417 (which added the ability to create PVCs for the Volume Resource and has since been abandoned) I was surprised to realize that the reconciler test for the PipelineRun controller doesn't verify that the PVC has been created. It does make sure its attached and this is covered by the end to end tests that wouldn't work without it, but I thought it made sense to test here too since we can.
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this pull request
Oct 25, 2019
The test was dynamically adding the expected PVC volumes. This created two problems: 1. If there was a bug in the logic that was adding them dynamically, we we could end up covering a bug 2. This made it confusing to try to reason about what the actual expected pod structure was. In tektoncd#1417 I had to make a lot of changes to these tests and it was super confusing to look at the expected structure and sometimes see no volumes (or even volumes: nil) when you knew volumes were supposed to be there (I was scared there was a serious bug for a while) So now the expected state explicitly lists all the volumes it expects. This also revealed that one test case was using `pipelinerun-parent` as the name of the parent pipelinerun even though all the rest used just `pipelinerun` which was also confusing when I was making changes b/c I initially thought there was some special case where tekton itself would add "parent" but no.
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this pull request
Oct 25, 2019
When I was working on tektoncd#1417 (which added the ability to create PVCs for the Volume Resource and has since been abandoned) I was surprised to realize that the reconciler test for the PipelineRun controller doesn't verify that the PVC has been created. It does make sure its attached and this is covered by the end to end tests that wouldn't work without it, but I thought it made sense to test here too since we can.
2 tasks
tekton-robot
pushed a commit
that referenced
this pull request
Oct 26, 2019
While working on #1417 (which we decided not to go ahead with) I made a _lot_ of unit tests fail. B/c I was changing how steps were added (for the volume resource) these failures were often in the middle of big diffs. It turns out we were inconsistent about how we used cmp.Diff: anything that is in the first argument but not in the second gets a `-` before it, anything that is in the second argument but not in the first gets a `+`, which is why the example at https://godoc.org/github.com/google/go-cmp/cmp#Diff passes these arguments as `cmp.Diff(want, got)`. If we reverse them, which many of our tests did, the results are counter intuitive: the "extra" stuff gets a `-` and the missing stuff gets a `+`. It seems like a small thing but it's really hard to wrap your head around! So this commit takes a bunch of these tests (not all of them, just the ones I was touchin in #1417) and: - Makes them consistently pass `(want, got)` - Annotates the failure message with a legend for `-` and `+` - Adds more details to the messages when applicable so you know what the diff is that you're looking at
tekton-robot
pushed a commit
that referenced
this pull request
Oct 26, 2019
While working on #1417, which we decided in the end not to merge, I made a bunch of updates to docstrings and comments which are now in this commit: - Adding more detail to some docstrings - Adding missing docstrings - Updating docstrings that were sometimes wrong (e.g. a function name had changed) - Removing some comments in the Conditions logic that were no longer accurate - Added comments in some sections of code that took me a while to understand to hopefully make it faster next time :)
tekton-robot
pushed a commit
that referenced
this pull request
Oct 31, 2019
Task modifiers for PipelineResources add pre and post steps and sometimes Volumes to a pod spec. Adding a step or a volume that already exists will create a pod that cannot run so this commit adds validation: - If a modifier tries to add a step that already exists (i.e. the name is the same) that's an error - If a modifier tries to add a volume that already exists but the volume it references is the same, we assume that two resources need the same volume, so it's not an error, but we only add it wonce - If a modifier tries to add a volume that already exists but the volume it references is different, that means two resources need different volumes but we cant add both since the names are the same, so that's an error I encountered a problem with this while working on #1417 (which we decided not to merge) where I tried to use two VolumeResources and there was a bug where they both had the same name, so the pod was not schedulable. I had to work backwards from the error that the pod was invalid and thought it would be much more handy to get the error earlier, so I added this.
tekton-robot
pushed a commit
that referenced
this pull request
Oct 31, 2019
When I was working on #1417 (which added the ability to create PVCs for the Volume Resource and has since been abandoned) I was surprised to realize that the reconciler test for the PipelineRun controller doesn't verify that the PVC has been created. It does make sure its attached and this is covered by the end to end tests that wouldn't work without it, but I thought it made sense to test here too since we can.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
This will allow copying content either into or out of a
TaskRun,either to an existing volume or a newly created volume. The immediate
use case is for copying a pipeline's workspace to be made available as
the input for another pipeline's workspace without needing to deal
with uploading everything to a bucket. The volume, whether already
existing or created, will not be deleted at the end of the
PipelineRun, unlike the artifact storage PVC.The Volume resource is a sub-type of the general Storage resource.
Since this type will require the creation of a PVC to function (to be
configurable later), this commit adds a Setup interface that
PipelineResources can implement if they need to do setup that involves
instantiating objects in Kube. This could be a place to later add
features like caching, and also is the sort of design we'd expect once
PipelineResources are extensible (PipelineResources will be free to do
whatever setup they need).
The behavior of this volume resource is:
If a user does want to control where the data is copied from, they can:
disk to /workspace/whatever
location the data is copied to (still relative to targetPath
https://github.com/tektoncd/pipeline/blob/master/docs/resources.md#controlling-where-resources-are-mounted)
pathhttps://github.com/tektoncd/pipeline/blob/master/docs/resources.md#overriding-where-resources-are-copied-from(tbd if we want to keep this feature post PVC)
The underlying PVC will need to be created by the Task reonciler, if
only a TaskRun is being used, or by the PipelineRun reconciler if a
Pipeline is being used. The PipelineRun reconciler cannot delegate this
to the TaskRun reconciler b/c when two different reconcilers create PVCs
and Tekton is running on a regional GKE cluster, they can get created in
different zones, resulting in a pod that tries to use both being
unschedulable.
In order to actually schedule a pod using two volume resources, we had
to:
https://cloud.google.com/kubernetes-engine/docs/how-to/persistent-volumes/regional-pd
for input/output linking or don't use the PVC (chose the latter!)
This commit removes automatic PVC copying for input output linking of
the VolumeResource b/c since it itself is a PVC, there is no need to
copy between an intermediate PVCs. This makes it simpler to make a Task
using the VolumeResource schedulable, removes redundant copying, and
removes a side effect where if a VolumeResources output was linked to an
input, the Task with the input would see only the changes made by the
output and none of the other contents of the PVC.
Also removing the docs on the
pathsparam (i.e. "overriding whereresources are copied from") because it was implemented such that it
would only work in the output -> input linking PVC case and can't
actually be used by users and it will be removed in #1284.
fixes #1062
(Continuing #1184)
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes