WIP: Add "volume" PipelineResource #1087

abayer · 2019-07-16T15:45:45Z

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.

This is just the initial work - the unit tests are not complete, and there need to be e2e tests, obviously, but I just wanted to get this initial work up for evaluation.

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.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task and TaskRun to build and release this image

Release Notes

TODO

Examples of user facing changes:
- API changes
- Bug fixes
- Any changes in behavior

tekton-robot · 2019-07-16T15:45:48Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: abayer
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: imjasonh

If they are not already assigned, you can assign the PR to them by writing /assign @imjasonh in a comment when ready.

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:

OWNERS

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

fixes tektoncd#1062 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. This is just the initial work - the unit tests are not complete, and there need to be e2e tests, obviously, but I just wanted to get this initial work up for evaluation. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

bobcatfish · 2019-07-16T19:05:47Z

Amazing!!!! @dlorenc and I were just talking about needing something like this :D (re. #1076)

Side question: @abayer I think jenkins x uses input output linking - if you have this resource, would this be the main Resource type you'd be linking, or do you use linking for other types as well?

abayer · 2019-07-16T19:15:10Z

@bobcatfish Yes! Right now, we use a git resource for that, but if/when the volume resource is available, we'll use that instead.

bobcatfish · 2019-07-16T19:16:12Z

Yesss amazing, this lines up perfectly! :D 😈

dlorenc · 2019-07-17T17:51:37Z

Yesss amazing, this lines up perfectly! :D 😈

Yay!

The cluster type currently has no defined behavior as output resources, and the git type behaves incorrectly. The new Volume type should be used instead for the use-cases supported by the old Git resource. This is part of the large cleanup in tektoncd#1076, and should not be submitted until after tektoncd#1087.

bobcatfish

I'm very excited to add this! I added some minor feedback, also a request: can this PR include docs at https://github.com/tektoncd/pipeline/blob/master/docs/resources.md explaining how to use the new resource?

side note, in slack i think we (@dlorenc @abayer me) discussed adding a flag that would allow a user to specify their own PVC that they previously created, instead of always making one? could be a feature we add later tho :D

bobcatfish · 2019-08-05T14:29:48Z

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


i'm a bit confused by the "nothing should be mounted here" comment (maybe it means there will be an error if something is already mounted there?)

bobcatfish · 2019-08-05T14:30:35Z

pkg/apis/pipeline/v1alpha1/volume_resource.go

+	}}, nil
+}
+
+// GetDownloadContainerSpec returns an array of container specs to download gcs storage object


i think this comment is out of date

bobcatfish · 2019-08-05T14:37:49Z

pkg/apis/pipeline/v1alpha1/volume_resource_test.go

+		name           string
+		volumeResource *v1alpha1.VolumeResource
+		wantContainers []corev1.Container
+		wantErr        bool


if possible i think tests end up a bit easier to read and update if instead of having wantErr we create separate tests for error cases (and in this case we're not actually using wantErr)

bobcatfish · 2019-08-05T14:38:32Z

pkg/apis/pipeline/v1alpha1/volume_resource_test.go

+		wantContainers []corev1.Container
+		wantErr        bool
+	}{{
+		name: "valid volume resource config",


in cases where we only have one test case, the test might be a bit easier to read without the table driven logic

bobcatfish · 2019-08-05T14:40:58Z

pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go

@@ -146,6 +157,33 @@ func AddInputResource(
 	return taskSpec, nil
 }

+func addVolumeFetchStep(kubeClient kubernetes.Interface, taskRun *v1alpha1.TaskRun, volumeResource *v1alpha1.VolumeResource, sourcePaths []string) ([]corev1.Container, []corev1.Volume, error) {


would be great to have unit tests for addVolumeFetchStep and addVolumeUploadStep (in fact i think it would also probably make a bit more sense if we put functions like this in the same package/file with the associated resources if we can? e.g. functions inside of pkg/apis/pipeline/v1alpha1/volume_resource.go

abayer · 2019-08-05T14:46:04Z

@bobcatfish - oh yeah, so many tests to add. =) I should be able to get back to this after DevOps World next week, if @dlorenc doesn't snap and drive this to the finish line before I get a chance. =)

hferentschik · 2019-08-07T08:47:34Z

Is this still the PR to watch for this feature or is there some other parallel work I should look at. Would love to see this feature getting into the next release of Tekton :-)

bobcatfish · 2019-08-12T18:04:48Z

closing in favor of #1184 - reopen if i am mistaken!

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 16, 2019

tekton-robot requested review from bobcatfish and vdemeester July 16, 2019 15:45

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jul 16, 2019

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 16, 2019

abayer mentioned this pull request Jul 16, 2019

Meta pipeline and execution pipeline need to share the same volume jenkins-x/jx#4660

Closed

abayer force-pushed the volume-resource branch from 2f5c49f to a800e93 Compare July 16, 2019 16:29

abayer mentioned this pull request Jul 17, 2019

Resources, PVCs and Chaining #1076

Closed

vdemeester mentioned this pull request Jul 22, 2019

Configure storage class for PVC of PipelineRuns #1101

Closed

dlorenc mentioned this pull request Jul 22, 2019

Disallow the Cluster and Git resource types as Output resources. #1109

Closed

3 tasks

dlorenc mentioned this pull request Jul 30, 2019

Refactor the Storage resource interface to remove a type switch state… #1135

Merged

3 tasks

bobcatfish reviewed Aug 5, 2019

View reviewed changes

tekton-robot mentioned this pull request Aug 5, 2019

Allow the customization of the storage class for the PVC of pipelines #1148

Merged

3 tasks

dlorenc mentioned this pull request Aug 12, 2019

Add "volume" PipelineResource #1184

Closed

3 tasks

bobcatfish closed this Aug 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add "volume" PipelineResource #1087

WIP: Add "volume" PipelineResource #1087

abayer commented Jul 16, 2019

tekton-robot commented Jul 16, 2019

bobcatfish commented Jul 16, 2019

abayer commented Jul 16, 2019

bobcatfish commented Jul 16, 2019

dlorenc commented Jul 17, 2019

bobcatfish left a comment

bobcatfish Aug 5, 2019

bobcatfish Aug 5, 2019

bobcatfish Aug 5, 2019

bobcatfish Aug 5, 2019

bobcatfish Aug 5, 2019

abayer commented Aug 5, 2019

hferentschik commented Aug 7, 2019

bobcatfish commented Aug 12, 2019

WIP: Add "volume" PipelineResource #1087

WIP: Add "volume" PipelineResource #1087

Conversation

abayer commented Jul 16, 2019

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jul 16, 2019

bobcatfish commented Jul 16, 2019

abayer commented Jul 16, 2019

bobcatfish commented Jul 16, 2019

dlorenc commented Jul 17, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Aug 5, 2019

Choose a reason for hiding this comment

bobcatfish Aug 5, 2019

Choose a reason for hiding this comment

bobcatfish Aug 5, 2019

Choose a reason for hiding this comment

bobcatfish Aug 5, 2019

Choose a reason for hiding this comment

bobcatfish Aug 5, 2019

Choose a reason for hiding this comment

abayer commented Aug 5, 2019

hferentschik commented Aug 7, 2019

bobcatfish commented Aug 12, 2019