Add "volume" PipelineResource #1184

dlorenc · 2019-08-12T16:57:23Z

Changes

This change adds a new resource type called "Volume", that exposes a Kubernetes Volume to the Task. This volume can be a reference to an existing Volume, or one can be created automatically.

This builds on #1087 and should obsolete it.

Fixes #1062

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

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

A new Storage resource type called "Volume" is now available.

googlebot · 2019-08-12T16:57:27Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

tekton-robot · 2019-08-12T17:00:10Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/storage_resource.go	45.0%	42.9%	-2.1
pkg/apis/pipeline/v1alpha1/taskrun_types.go	78.6%	75.9%	-2.7
pkg/apis/pipeline/v1alpha1/volume_resource.go	Do not exist	30.0%

abayer · 2019-08-12T17:16:51Z

@googlebot I consent

googlebot · 2019-08-12T17:16:54Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

abayer · 2019-08-12T17:21:08Z

@dlorenc Looks dang good at first glance. I'll give it a closer review ASAP.

bobcatfish

I am excited about this!

Few requests:

Can has docs on new resource type at https://github.com/tektoncd/pipeline/blob/master/docs/resources.md
Looks like there is a bunch more coverage we could add, at least for storage_resource and volume_resource:
Can has example and/or end to end test that uses the new type

bobcatfish · 2019-08-12T18:06:47Z

pkg/apis/pipeline/v1alpha1/resource_types.go

-	GetUploadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error)
-	GetDownloadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error)
+	GetDownloadVolumeSpec(*TaskSpec, *TaskRun, kubernetes.Interface) ([]corev1.Volume, error)
+	GetUploadVolumeSpec(*TaskSpec, *TaskRun, kubernetes.Interface) ([]corev1.Volume, error)


can we include docstrings for these to explain what the function is expected to do (and what the new params are for?)

bobcatfish · 2019-08-12T18:06:59Z

pkg/apis/pipeline/v1alpha1/volume_resource.go

+	}
+}
+
+func (s *VolumeResource) GetDownloadVolumeSpec(ts *TaskSpec, tr *TaskRun, c kubernetes.Interface) ([]corev1.Volume, error) {


can has docstrings for all new functions 🙏

do you think it could make sense to pass specifically in the function for creating volumes, instead of the whole kubernetes.Interface? (would probably make it easier to write some unit tests for that also)

bobcatfish · 2019-08-12T18:11:50Z

pkg/apis/pipeline/v1alpha1/volume_resource.go

+)
+
+// VolumeResource is a volume from which to get artifacts which is required
+// by a Build/Task for context (e.g. a archive from which to build an image).


probably can use "task" instead of "build/task"

bobcatfish · 2019-08-12T18:13:28Z

pkg/apis/pipeline/v1alpha1/volume_resource.go

+func (s *VolumeResource) GetPvcMount() corev1.VolumeMount {
+	return corev1.VolumeMount{
+		Name:      s.Name,         // resource pvc name
+		MountPath: VolumeMountDir, // nothing should be mounted here


the comments above dont really add a lot , i think we've been copy and pasting them around for a while now

cameronbraid · 2019-08-20T14:22:27Z

It might be a good idea to support specifying a storage class in addition to the volume size

googlebot · 2019-09-17T23:35:17Z

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.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@pritidesai

As part of tektoncd#1184 I need to call `GetSetup` on all PipelineResources early on in PipelineRun execution. Since PipelineRuns declare all their resource up front, I wanted to be able to resolve all of them at once, then call `GetSetup` on all of them. Also, as Pipelines got more complex (we added Conditions) it turned out we were retrieving the resources in a few different places. Also in tektoncd#1324 @pritidesai is making it so that these Resources can be provided by spec. By resolving all of this up front at once, we can simplify the logic later on. And you can see in this commit that we are able to reduce the responsibilities of ResolvePipelineRun a bit too!

@pritidesai

As part of tektoncd#1184 I need to call `GetSetup` on all PipelineResources early on in PipelineRun execution. Since PipelineRuns declare all their resource up front, I wanted to be able to resolve all of them at once, then call `GetSetup` on all of them. Also, as Pipelines got more complex (we added Conditions) it turned out we were retrieving the resources in a few different places. Also in tektoncd#1324 @pritidesai is making it so that these Resources can be provided by spec. By resolving all of this up front at once, we can simplify the logic later on. And you can see in this commit that we are able to reduce the responsibilities of ResolvePipelineRun a bit too!

@pritidesai

As part of #1184 I need to call `GetSetup` on all PipelineResources early on in PipelineRun execution. Since PipelineRuns declare all their resource up front, I wanted to be able to resolve all of them at once, then call `GetSetup` on all of them. Also, as Pipelines got more complex (we added Conditions) it turned out we were retrieving the resources in a few different places. Also in #1324 @pritidesai is making it so that these Resources can be provided by spec. By resolving all of this up front at once, we can simplify the logic later on. And you can see in this commit that we are able to reduce the responsibilities of ResolvePipelineRun a bit too!

tekton-robot · 2019-10-01T19:25:17Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/build_gcs_resource.go	86.1%	83.8%	-2.3
pkg/apis/pipeline/v1alpha1/cloud_event_resource.go	88.2%	83.3%	-4.9
pkg/apis/pipeline/v1alpha1/cluster_resource.go	76.3%	74.4%	-2.0
pkg/apis/pipeline/v1alpha1/gcs_resource.go	87.2%	85.0%	-2.2
pkg/apis/pipeline/v1alpha1/git_resource.go	78.9%	75.0%	-3.9
pkg/apis/pipeline/v1alpha1/image_resource.go	56.2%	52.9%	-3.3
pkg/apis/pipeline/v1alpha1/pipelineresource_validation.go	89.4%	87.5%	-1.9
pkg/apis/pipeline/v1alpha1/pull_request_resource.go	85.0%	81.0%	-4.0
pkg/apis/pipeline/v1alpha1/resource_types.go	11.8%	16.7%	4.9
pkg/apis/pipeline/v1alpha1/storage_resource.go	95.0%	85.7%	-9.3
pkg/apis/pipeline/v1alpha1/taskrun_types.go	72.7%	69.6%	-3.2
pkg/apis/pipeline/v1alpha1/volume_resource.go	Do not exist	63.2%
pkg/reconciler/pipelinerun/pipelinerun.go	82.6%	81.7%	-0.9
pkg/reconciler/taskrun/taskrun.go	71.6%	72.2%	0.6
test/pvc.go	Do not exist	0.0%

googlebot · 2019-10-01T19:50:02Z

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.

bobcatfish · 2019-10-04T14:26:40Z

        message: 'Invalid TaskSpec: Operation cannot be fulfilled on resourcequotas "gke-resource-quotas":
          StorageError: invalid object, Code: 4, Key: /registry/resourcequotas/arendelle-7hqfr/gke-resource-quotas,
          ResourceVersion: 0, AdditionalErrorMsg: Precondition failed: UID in precondition:
          65c88d9d-e482-11e9-8227-42010a8001ce, UID in object meta: '

wut

tekton-robot · 2019-10-09T21:07:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign dlorenc
You can assign the PR to them by writing /assign @dlorenc in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

googlebot · 2019-10-09T21:07:32Z

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.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-10-10T19:02:06Z

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.

tekton-robot · 2019-10-10T19:04:14Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/build_gcs_resource.go	88.2%	85.7%	-2.5
pkg/apis/pipeline/v1alpha1/cloud_event_resource.go	88.2%	83.3%	-4.9
pkg/apis/pipeline/v1alpha1/cluster_resource.go	76.3%	74.4%	-2.0
pkg/apis/pipeline/v1alpha1/gcs_resource.go	91.4%	88.9%	-2.5
pkg/apis/pipeline/v1alpha1/git_resource.go	78.9%	75.0%	-3.9
pkg/apis/pipeline/v1alpha1/image_resource.go	56.2%	52.9%	-3.3
pkg/apis/pipeline/v1alpha1/pipelineresource_validation.go	89.4%	87.5%	-1.9
pkg/apis/pipeline/v1alpha1/pull_request_resource.go	85.0%	81.0%	-4.0
pkg/apis/pipeline/v1alpha1/resource_types.go	11.8%	75.7%	63.9
pkg/apis/pipeline/v1alpha1/storage_resource.go	95.0%	90.5%	-4.5
pkg/apis/pipeline/v1alpha1/taskrun_types.go	76.9%	74.1%	-2.8
pkg/apis/pipeline/v1alpha1/volume_resource.go	Do not exist	64.1%
pkg/reconciler/pipelinerun/pipelinerun.go	82.7%	81.8%	-0.9
pkg/reconciler/taskrun/resources/input_resources.go	95.7%	93.8%	-2.0
pkg/reconciler/taskrun/resources/output_resource.go	92.1%	89.7%	-2.4
pkg/reconciler/taskrun/taskrun.go	71.6%	72.2%	0.6
test/pvc.go	Do not exist	0.0%

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>

googlebot · 2019-10-10T19:56:41Z

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.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

bobcatfish · 2019-10-10T19:57:50Z

I'm gonna close this and start a new pull request.

tekton-robot · 2019-10-10T19:58:41Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/build_gcs_resource.go	88.2%	85.7%	-2.5
pkg/apis/pipeline/v1alpha1/cloud_event_resource.go	88.2%	83.3%	-4.9
pkg/apis/pipeline/v1alpha1/cluster_resource.go	76.3%	74.4%	-2.0
pkg/apis/pipeline/v1alpha1/gcs_resource.go	91.4%	88.9%	-2.5
pkg/apis/pipeline/v1alpha1/git_resource.go	78.9%	75.0%	-3.9
pkg/apis/pipeline/v1alpha1/image_resource.go	56.2%	52.9%	-3.3
pkg/apis/pipeline/v1alpha1/pipelineresource_validation.go	89.4%	91.4%	2.0
pkg/apis/pipeline/v1alpha1/pull_request_resource.go	85.0%	81.0%	-4.0
pkg/apis/pipeline/v1alpha1/resource_types.go	11.8%	75.7%	63.9
pkg/apis/pipeline/v1alpha1/storage_resource.go	95.0%	90.5%	-4.5
pkg/apis/pipeline/v1alpha1/taskrun_types.go	76.9%	74.1%	-2.8
pkg/apis/pipeline/v1alpha1/volume_resource.go	Do not exist	71.8%
pkg/reconciler/pipelinerun/pipelinerun.go	82.7%	81.8%	-0.9
pkg/reconciler/taskrun/resources/input_resources.go	95.7%	93.8%	-2.0
pkg/reconciler/taskrun/resources/output_resource.go	92.1%	89.7%	-2.4
pkg/reconciler/taskrun/taskrun.go	71.6%	72.2%	0.6
test/pvc.go	Do not exist	0.0%

tekton-robot · 2019-10-10T20:14:28Z

@dlorenc: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-tekton-pipeline-build-tests	`18bd7ff`	link	`/test pull-tekton-pipeline-build-tests`
pull-tekton-pipeline-integration-tests	`18bd7ff`	link	`/test pull-tekton-pipeline-integration-tests`

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.

tekton-robot requested review from abayer and bobcatfish August 12, 2019 16:57

googlebot added the cla: no label Aug 12, 2019

tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 12, 2019

googlebot added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Aug 12, 2019

bobcatfish changed the title ~~Abayer volume~~ Add "volume" PipelineResource Aug 12, 2019

bobcatfish mentioned this pull request Aug 12, 2019

WIP: Add "volume" PipelineResource #1087

Closed

3 tasks

bobcatfish reviewed Aug 12, 2019

View reviewed changes

bobcatfish mentioned this pull request Sep 5, 2019

Make PipelineResources (mostly) immutable #1279

Closed

bobcatfish modified the milestone: Pipelines 0.7 🐱 Sep 6, 2019

bobcatfish mentioned this pull request Sep 10, 2019

Allow passing individual files or sets of files as Task inputs/outputs #259

Closed

bobcatfish force-pushed the abayer-volume branch from e72692e to 666fcd7 Compare September 17, 2019 23:35

googlebot added cla: no and removed cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit labels Sep 17, 2019

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 17, 2019

bobcatfish force-pushed the abayer-volume branch 2 times, most recently from 1caba45 to 7811c5f Compare September 17, 2019 23:39

bobcatfish added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Sep 17, 2019

bobcatfish force-pushed the abayer-volume branch from 9ca1640 to f414a5d Compare October 1, 2019 19:22

bobcatfish added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Oct 1, 2019

tekton-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 9, 2019

googlebot added cla: no and removed cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit labels Oct 9, 2019

bobcatfish force-pushed the abayer-volume branch from 277546e to 193ac90 Compare October 10, 2019 19:01

bobcatfish added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Oct 10, 2019

bobcatfish force-pushed the abayer-volume branch from 193ac90 to 18bd7ff Compare October 10, 2019 19:56

googlebot added cla: no and removed cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit labels Oct 10, 2019

bobcatfish closed this Oct 10, 2019

bobcatfish mentioned this pull request Oct 10, 2019

Add "volume" PipelineResource 🔊 #1416

Closed

3 tasks

bobcatfish mentioned this pull request Oct 10, 2019

Add "volume" PipelineResource 🔊 #1417

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "volume" PipelineResource #1184

Add "volume" PipelineResource #1184

dlorenc commented Aug 12, 2019 •

edited by bobcatfish

Loading

googlebot commented Aug 12, 2019

tekton-robot commented Aug 12, 2019

abayer commented Aug 12, 2019

googlebot commented Aug 12, 2019

abayer commented Aug 12, 2019

bobcatfish left a comment

bobcatfish Aug 12, 2019

bobcatfish Aug 12, 2019

bobcatfish Aug 12, 2019

bobcatfish Aug 12, 2019

bobcatfish Aug 12, 2019

cameronbraid commented Aug 20, 2019

googlebot commented Sep 17, 2019

tekton-robot commented Oct 1, 2019

googlebot commented Oct 1, 2019

bobcatfish commented Oct 4, 2019

tekton-robot commented Oct 9, 2019

googlebot commented Oct 9, 2019

googlebot commented Oct 10, 2019

tekton-robot commented Oct 10, 2019

googlebot commented Oct 10, 2019

bobcatfish commented Oct 10, 2019

tekton-robot commented Oct 10, 2019

tekton-robot commented Oct 10, 2019

Add "volume" PipelineResource #1184

Add "volume" PipelineResource #1184

Conversation

dlorenc commented Aug 12, 2019 • edited by bobcatfish Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

googlebot commented Aug 12, 2019

tekton-robot commented Aug 12, 2019

abayer commented Aug 12, 2019

googlebot commented Aug 12, 2019

abayer commented Aug 12, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Aug 12, 2019

Choose a reason for hiding this comment

bobcatfish Aug 12, 2019

Choose a reason for hiding this comment

bobcatfish Aug 12, 2019

Choose a reason for hiding this comment

bobcatfish Aug 12, 2019

Choose a reason for hiding this comment

bobcatfish Aug 12, 2019

Choose a reason for hiding this comment

cameronbraid commented Aug 20, 2019

googlebot commented Sep 17, 2019

tekton-robot commented Oct 1, 2019

googlebot commented Oct 1, 2019

bobcatfish commented Oct 4, 2019

tekton-robot commented Oct 9, 2019

googlebot commented Oct 9, 2019

googlebot commented Oct 10, 2019

tekton-robot commented Oct 10, 2019

googlebot commented Oct 10, 2019

bobcatfish commented Oct 10, 2019

tekton-robot commented Oct 10, 2019

tekton-robot commented Oct 10, 2019

dlorenc commented Aug 12, 2019 •

edited by bobcatfish

Loading