Remove automatic PVC creation #1284
Labels
lifecycle/rotten
Denotes an issue or PR that has aged beyond stale and will be auto-closed.
priority/important-longterm
Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Comments
|
(note the volume resource MUST be added before we can remove this) |
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this issue
Sep 27, 2019
In tektoncd#1324 we updated PipelineRuns to allow for embedding ResourceSpecs in PipelineRuns. This commit makes it so that the logic for resolving (i.e. deciding if PipelineResources are specified by Spec or Ref) is shared by PipelineRuns + TaskRuns. This is done by making it so that the "binding" uses the same type underneath. The only reason they can't be the exact same type is that TaskRuns additionally need the "path" attribute, which is actually only used for PVC copying, which will be removed in tektoncd#1284, and then we should be able to remove paths entirely and the type can be the same. Also added some additional comments around the use of `SelfLink`, and made sure it was well covered in the reconciler test.
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this issue
Sep 27, 2019
In tektoncd#1324 we updated PipelineRuns to allow for embedding ResourceSpecs in PipelineRuns. This commit makes it so that the logic for resolving (i.e. deciding if PipelineResources are specified by Spec or Ref) is shared by PipelineRuns + TaskRuns. This is done by making it so that the "binding" uses the same type underneath. The only reason they can't be the exact same type is that TaskRuns additionally need the "path" attribute, which is actually only used for PVC copying, which will be removed in tektoncd#1284, and then we should be able to remove paths entirely and the type can be the same. Also added some additional comments around the use of `SelfLink`, and made sure it was well covered in the reconciler test.
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this issue
Sep 27, 2019
In tektoncd#1324 we updated PipelineRuns to allow for embedding ResourceSpecs in PipelineRuns. This commit makes it so that the logic for resolving (i.e. deciding if PipelineResources are specified by Spec or Ref) is shared by PipelineRuns + TaskRuns. This is done by making it so that the "binding" uses the same type underneath. The only reason they can't be the exact same type is that TaskRuns additionally need the "path" attribute, which is actually only used for PVC copying, which will be removed in tektoncd#1284, and then we should be able to remove paths entirely and the type can be the same. Also added some additional comments around the use of `SelfLink`, and made sure it was well covered in the reconciler test.
2 tasks
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this issue
Oct 1, 2019
In tektoncd#1324 we updated PipelineRuns to allow for embedding ResourceSpecs in PipelineRuns. This commit makes it so that the logic for resolving (i.e. deciding if PipelineResources are specified by Spec or Ref) is shared by PipelineRuns + TaskRuns. This is done by making it so that the "binding" uses the same type underneath. The only reason they can't be the exact same type is that TaskRuns additionally need the "path" attribute, which is actually only used for PVC copying, which will be removed in tektoncd#1284, and then we should be able to remove paths entirely and the type can be the same. Also added some additional comments around the use of `SelfLink`, and made sure it was well covered in the reconciler test.
tekton-robot
pushed a commit
that referenced
this issue
Oct 1, 2019
In #1324 we updated PipelineRuns to allow for embedding ResourceSpecs in PipelineRuns. This commit makes it so that the logic for resolving (i.e. deciding if PipelineResources are specified by Spec or Ref) is shared by PipelineRuns + TaskRuns. This is done by making it so that the "binding" uses the same type underneath. The only reason they can't be the exact same type is that TaskRuns additionally need the "path" attribute, which is actually only used for PVC copying, which will be removed in #1284, and then we should be able to remove paths entirely and the type can be the same. Also added some additional comments around the use of `SelfLink`, and made sure it was well covered in the reconciler test.
bobcatfish
added a commit
to dlorenc/build-pipeline
that referenced
this issue
Oct 9, 2019
In order to actually schedule a pod using two volume resources, we had to make a couple changes: - 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.
bobcatfish
added a commit
to dlorenc/build-pipeline
that referenced
this issue
Oct 10, 2019
In order to actually schedule a pod using two volume resources, we had to make a couple changes: - 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.
bobcatfish
added a commit
to dlorenc/build-pipeline
that referenced
this issue
Oct 10, 2019
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>
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this issue
Oct 10, 2019
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>
dibyom
added
the
priority/important-longterm
|
Depends-on on what we decide to do with pipeline resources. Removing the priority label for now |
|
Rotten issues close after 30d of inactivity. /close Send feedback to tektoncd/plumbing. |
|
Stale issues rot after 30d of inactivity. /lifecycle rotten Send feedback to tektoncd/plumbing. |
tekton-robot
added
the
lifecycle/rotten
|
@tekton-robot: Closing this issue. 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. |
|
Closing this seems fair - #1673 should result in us deciding if this is the way to go or not (e.g. do we completely remove this functionality) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Expected Behavior
Once we have #1062, users can use the Volume resource whenever they want to use a PVC. We should no longer create and link PVCs for them.
Actual Behavior
Today whenever an output of one task is linked to an input, we use a volume to share the data between them. Also we create PVCs even when they are not needed, and TaskRuns that are created by PipelineRuns will expect their existence (See #937)
Additional Info
This will be a backwards incompatible change.
The text was updated successfully, but these errors were encountered: