Add workspace types for Task and TaskRun with validation #1639

bobcatfish · 2019-11-27T22:10:14Z

Changes

This allows users to use Volumes with Tasks such that:

The actual volumes to use (or subdirectories on those volumes) are
provided at runtime, not at Task authoring time
At Task authoring time you can declare that you expect a volume to
be provided and control what path that volume should end up at
Validation will be provided that the volumes (workspaces) are actually
provided at runtime

Before this change, there were two ways to use Volumes with Tasks:

VolumeMounts were explicitly declared at the level of a step
Volumes were declared in Tasks, meaning the Task author controlled the
name of the volume being used and it wasn't possible at runtime to use
a subdir of the volume
Or the Volume could be provided via the podTemplate, if the user
realized this was possible

None of this was validated and could cause unexpected and hard to
diagnose errors at runtime.

We have also limited (at least initially) the types of volume source
being supported instead of expanding to all volume sources, tho we can
expand it later if we want to and if users need it. This would reduce
the API surface that a Tekton compliant system would need to conform to
(once we actually define what conformance means!).

Part of #1438

In future commits we will add support for workspaces to Pipelines and
PipelineRuns as well; for now if a user tries to use a Pipeline with a
Task that requires a Workspace, it will fail at runtime because it is
not (yet) possible for the Pipeline and PipelineRun to provide
workspaces.

Future work:

Support for readonly
Support for configmap + secret volume types
Support in Pipeline + PipelineRun

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 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

Adds the concept of "workspaces" to Tasks. This allows Task authors to declare directories they expect to be provided via Volumes at runtime. At runtime, TaskRuns can provide PVCs or EmptyDirs to use for these directories.

skaegi · 2019-12-02T14:28:11Z

docs/taskruns.md

+- name: myworkspace
+  persistentVolumeClaim:
+    claimName: mypvc
+  volumeSubPath: my-subdir


should be just subPath to match api

whoops, thanks for catching this!

vdemeester

Nice 👼

Any reason to use StepTemplates for this ? "Add validation to ensure that the mountPaths don't clash with any explicitly declared volumeMounts" might be a valid reason 🤔

Randomize the volume name + volumeMount names so we don't have to worry about clashing with existing volumeMounts + volume names

This would mean the user has to use the variable substitution to get the name of the mountpoint right ? I think that's fair though 👼

Add support for ConfigMaps (and secrets?)

That would make sense, but we certainly can do that in follow-ups 😉

bobcatfish · 2019-12-03T19:07:25Z

Any reason to use StepTemplates for this ?

Basically just b/c it was convenient! 😅 maybe one way to make this a bit cleaner would be to create a new "step template" just for this, merge it with the user provided step template, and pass the result into the pod creation function - not sure how much different that'd be tho 🤔

bobcatfish · 2019-12-03T21:10:48Z

This would mean the user has to use the variable substitution to get the name of the mountpoint right ? I think that's fair though 👼

sgtm!

tekton-robot · 2019-12-03T21:22:00Z

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/workspace_validation.go	Do not exist	100.0%
pkg/reconciler/taskrun/taskrun.go	74.8%	73.9%	-0.9
pkg/workspace/apply.go	Do not exist	94.4%
pkg/workspace/validate.go	Do not exist	100.0%

skaegi · 2019-12-03T21:47:49Z

One question... is there a particular concern and reason why the WorkspaceBinding embeds a PVCSource / EmptyDir instead of just a volume name? I already have cases where I would really like to embed Secrets and ConfigMaps as volumes and this would be helpful. Is there some concern about supporting some kinds of volume types?

Also... could we consider adding a readOnly boolean on the WorkspaceBinding. I believe the implementation is to emit a VolumeMount which already supports this field.

vdemeester · 2019-12-04T14:13:19Z

One question... is there a particular concern and reason why the WorkspaceBinding embeds a PVCSource / EmptyDir instead of just a volume name? I already have cases where I would really like to embed Secrets and ConfigMaps as volumes and this would be helpful. Is there some concern about supporting some kinds of volume types?

Initially I thought "to be able to get the correct type of volume", but we don't really have to. So I'm now asking myself the same question 😝

Also... could we consider adding a readOnly boolean on the WorkspaceBinding. I believe the implementation is to emit a VolumeMount which already supports this field.

👍

ghost · 2019-12-04T14:27:51Z

Here I've taken the example workspace taskrun yaml from this PR and added a volume to the podTemplate in the taskrun:

apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: my-pvc
spec:
  resources:
    requests:
      storage: 5Gi
  volumeMode: Filesystem
  accessModes:
    - ReadWriteOnce
---
apiVersion: tekton.dev/v1alpha1
kind: TaskRun
metadata:
  generateName: custom-volume-
spec:
  workspaces:
    - name: custom
      volumeName: mypd
      subPath: my-subdir
    - name: custom2
      volumeName: mypd
  podTemplate:
    volumes:
      - name: mypd
        persistentVolumeClaim:
          claimName: my-pvc
  taskSpec:
    steps:
    - name: write
      image: ubuntu
      command: ["/bin/bash"]
      args: ["-c", "echo stuff > /workspace/custom/foo"]
    - name: read
      image: ubuntu
      command: ["/bin/bash"]
      args: ["-c", "cat /workspace/custom/foo | grep stuff"]
    - name: write2
      image: ubuntu
      command: ["/bin/bash"]
      args: ["-c", "echo stuffs > /workspace/custom2/foo"]
    - name: read2
      image: ubuntu
      command: ["/bin/bash"]
      args: ["-c", "cat /workspace/custom2/foo | grep stuffs"]
    workspaces:
    - name: custom
    - name: custom2

Is this about what you'd expect @skaegi , @vdemeester ?

ghost · 2019-12-04T14:32:04Z

In regards support for configmaps and secrets - we have plans to implement those as part of this (it's in the TODOs section at the top).

Is there any other reason to separate them out like this rather than just having the volume type embedded in the workspace binding?

ghost · 2019-12-04T14:33:46Z

Also - readOnly is definitely on the roadmap for workspaces! whether or not it's part of this PR.

ghost · 2019-12-04T14:39:15Z

(for a bit of context - @bobcatfish and I wrote it initially to use the podTemplate volumes but it felt like extra yaml when we could make it part of the binding spec and leave the podTemplate out of things completely)

vdemeester · 2019-12-04T15:22:45Z

Is this about what you'd expect @skaegi , @vdemeester ?

Oh, ok, I would maybe prefer the following (but 😛) to reduce a bit the verbosity of the thing.

# […]
spec:
  workspaces:
    - name: custom
      persistentVolumeClaim:
        claimName: my-pvc
      subPath: my-subdir
    - name: custom2
      persistentVolumeClaim:
        claimName: my-pvc
# […]

skaegi · 2019-12-04T15:37:18Z

Thanks for the example @sbwsg -- that helps. I forgot that taskruns don't directly have volumes and agree the extra podSpec stuff just doesn't look great and does not enhance readability.

skaegi · 2019-12-04T19:31:19Z

I went back through our current usage to verify and can confirm that if we had...

configMap [ConfigMapVolumeSource]
emptyDir [EmptyDirVolumeSource]
persistentVolumeClaim [PersistentVoluemClaimVolumeSource]
secret: [SecretVolumeSource]

all of our cases are covered.

We also on occasion might use downwardAPI volumes however that is always static and not parameterized. Especially in the context of a TaskRun I found downwardAPI a bit strange as it ends up looking up values against the Pod so I'm happy to not have it present in the WorkspaceBinding.

@bobcatfish a while back we had wondered about removing volumes from the Task. The TLDR; is I think we want to retain support. Looking through our usage I see cases where a Task wants to do work in emptyDir memory and also cases where we're mounting the hostPath. We might also directly generate non-parameterizable Tasks that hardcode the pvcs and devices involved so having volumes grants us flexibility there too.

Each test case for parameter substitution application was being given a totally separate test case, with the variables being used being declared in different places across the file. For tektoncd#1639 I came along and wanted to start adding more tests for workspace substitution and found it hard to tell where to start so I: * Combined most of the test cases for param subsitution into one test so you can easily see everything that is being tested (none of the test cases conflicted with each other and can easily be applied together) * I kept the array param test cases separate cuz they seemd to be testing distinct test cases * The Volume test cases were a bit odd b/c they were trying to make sure substitution was _applied_ to volumes, but there is no volume specific function so they were calling an internal function and passing in dummy values that are not representative of the actual values you'd substitute for volumes so instead I folded these test cases into the param application test. Probably the resource application test case should be made quite similar to the param test but it seemed like some of the resource stuff was distinct and had to be tested in isolateion (e.g. just outputs, just inputs, etc.) Also removed some depreated (and duplicated!) volume tests: in tektoncd#1311 I removed support for ${} but instead of removing these tests I just updated them, making them duplicates of the above test cases.

bobcatfish · 2019-12-06T22:03:01Z

This should be ready for a real review @skaegi @vdemeester @sbwsg ! 🎉

tekton-robot · 2019-12-06T22:03: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/task_validation.go	81.2%	83.4%	2.2
pkg/apis/pipeline/v1alpha1/taskrun_validation.go	95.3%	96.2%	0.9
pkg/apis/pipeline/v1alpha1/workspace_types.go	Do not exist	100.0%
pkg/apis/pipeline/v1alpha1/workspace_validation.go	Do not exist	100.0%
pkg/reconciler/taskrun/taskrun.go	74.8%	74.0%	-0.8
pkg/workspace/apply.go	Do not exist	94.1%
pkg/workspace/validate.go	Do not exist	100.0%
test/builder/task.go	81.4%	81.9%	0.5

ghost

/lgtm

vdemeester

On thought: we need to be aware of #1701 👼
Otherwise, looks good to me 👼

/cc @skaegi

vdemeester · 2019-12-12T09:14:54Z

I1210 22:23:15.091] pkg/apis/pipeline/v1alpha1/git_resource.go:125:16: undefined: pipeline.WorkspaceDir
I1210 22:23:15.092] pkg/apis/pipeline/v1alpha1/pull_request_resource.go:142:15: undefined: pipeline.WorkspaceDir
I1210 22:23:15.092] pkg/apis/pipeline/v1alpha1/workspace_types.go:44:23: undefined: pipeline.WorkspaceDir

@bobcatfish Did you forget to commit something ? 🤔

bobcatfish · 2019-12-16T14:14:07Z

Thanks @vdemeester ! @skaegi you said you this approach looks like it'll work for you, @sbwsg mentioned you wanted to try this out a bit and give us some more feedback. Since we still have to do the Pipeline and PipelineRun portion of this, I'm assuming you'd be okay with merging this PR and continuing to iterate on the design as we keep working on it (let me know if not!)

tekton-robot · 2019-12-16T14:14:48Z

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/workspace/apply.go	Do not exist	94.1%
pkg/workspace/validate.go	Do not exist	100.0%
test/builder/task.go	81.8%	82.3%	0.5

bobcatfish · 2019-12-16T14:52:58Z

I1216 14:37:17.611] pkg/workspace/apply_test.go:29: File is not `gofmt`-ed with `-s` (gofmt)
I1216 14:37:17.611] 			"custom": corev1.Volume{
I1216 14:37:17.611] pkg/pod/pod.go:34:2: `homeVolumeName` is unused (varcheck)
I1216 14:37:17.611] 	homeVolumeName      = "tekton-internal-home"
I1216 14:37:17.612] 	^
I1216 14:37:17.612] pkg/pod/pod.go:33:2: `workspaceVolumeName` is unused (varcheck)
I1216 14:37:17.612] 	workspaceVolumeName = "tekton-internal-workspace"
I1216 14:37:17.612] 	^

whoops

tekton-robot · 2019-12-16T15:00:13Z

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/workspace/apply.go	Do not exist	94.1%
pkg/workspace/validate.go	Do not exist	100.0%
test/builder/task.go	81.8%	82.3%	0.5

ghost · 2019-12-16T17:57:25Z

/lgtm

vdemeester

/lgtm

but @bobcatfish it needs a rebase (again 😛 )

tekton-robot · 2019-12-17T10:33:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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~~ [vdemeester]

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

vdemeester · 2019-12-17T15:52:45Z

/hold cancel

This allows users to use Volumes with Tasks such that: - The actual volumes to use (or subdirectories on those volumes) are provided at runtime, not at Task authoring time - At Task authoring time you can declare that you expect a volume to be provided and control what path that volume should end up at - Validation will be provided that the volumes (workspaces) are actually provided at runtime Before this change, there were two ways to use Volumes with Tasks: - VolumeMounts were explicitly declared at the level of a step - Volumes were declared in Tasks, meaning the Task author controlled the name of the volume being used and it wasn't possible at runtime to use a subdir of the volume - Or the Volume could be provided via the podTemplate, if the user realized this was possible None of this was validated and could cause unexpected and hard to diagnose errors at runtime. It's possible folks might be specifying volumes already in the Task or via the stepTemplate that might collide with the names we are using for the workspaces; instead of validating this and making the Task author change these, we can instead randomize them! We have also limited (at least initially) the types of volume source being supported instead of expanding to all volume sources, tho we can expand it later if we want to and if users need it. This would reduce the API surface that a Tekton compliant system would need to conform to (once we actually define what conformance means!). Part of tektoncd#1438 In future commits we will add support for workspaces to Pipelines and PipelineRuns as well; for now if a user tries to use a Pipeline with a Task that requires a Workspace, it will fail at runtime because it is not (yet) possible for the Pipeline and PipelineRun to provide workspaces. Co-authored-by: Scott <[email protected]>

ghost · 2019-12-17T15:56:39Z

/lgtm

tekton-robot · 2019-12-17T15:58:35Z

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/workspace/apply.go	Do not exist	94.1%
pkg/workspace/validate.go	Do not exist	100.0%
test/builder/task.go	81.8%	82.3%	0.5

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 Nov 27, 2019

tekton-robot requested review from dibyom and dlorenc November 27, 2019 22:10

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

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

skaegi reviewed Dec 2, 2019

View reviewed changes

vdemeester reviewed Dec 2, 2019

View reviewed changes

bobcatfish force-pushed the volumes_are_awesome branch from f9b302e to 268a69d Compare December 3, 2019 21:20

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 Dec 3, 2019

ghost mentioned this pull request Dec 5, 2019

Question: A way to pass variables between task steps. #1476

Closed

bobcatfish mentioned this pull request Dec 6, 2019

Simplify variable substitution tests ⚗️ #1702

Merged

1 task

bobcatfish force-pushed the volumes_are_awesome branch from 268a69d to 86f73be Compare December 6, 2019 22:00

bobcatfish marked this pull request as ready for review December 6, 2019 22:02

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2019

ghost reviewed Dec 11, 2019

View reviewed changes

tekton-robot assigned ghost Dec 11, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2019

vdemeester reviewed Dec 12, 2019

View reviewed changes

tekton-robot requested a review from skaegi December 12, 2019 09:14

bobcatfish force-pushed the volumes_are_awesome branch from cee110f to bf2ad87 Compare December 16, 2019 14:12

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2019

bobcatfish force-pushed the volumes_are_awesome branch from bf2ad87 to 60ef850 Compare December 16, 2019 14:57

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2019

vdemeester approved these changes Dec 17, 2019

View reviewed changes

tekton-robot assigned vdemeester Dec 17, 2019

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 17, 2019

vdemeester added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2019

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 17, 2019

bobcatfish force-pushed the volumes_are_awesome branch from 60ef850 to eb23ad7 Compare December 17, 2019 15:56

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2019

vdemeester removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2019

tekton-robot merged commit c55d311 into tektoncd:master Dec 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add workspace types for Task and TaskRun with validation #1639

Add workspace types for Task and TaskRun with validation #1639

bobcatfish commented Nov 27, 2019 •

edited

Loading

skaegi Dec 2, 2019

bobcatfish Dec 3, 2019

vdemeester left a comment

bobcatfish commented Dec 3, 2019

bobcatfish commented Dec 3, 2019

tekton-robot commented Dec 3, 2019

skaegi commented Dec 3, 2019

vdemeester commented Dec 4, 2019

ghost commented Dec 4, 2019 •

edited by ghost

Loading

ghost commented Dec 4, 2019 •

edited by ghost

Loading

ghost commented Dec 4, 2019

ghost commented Dec 4, 2019

vdemeester commented Dec 4, 2019

skaegi commented Dec 4, 2019

skaegi commented Dec 4, 2019

bobcatfish commented Dec 6, 2019

tekton-robot commented Dec 6, 2019

ghost left a comment

vdemeester left a comment

vdemeester commented Dec 12, 2019

bobcatfish commented Dec 16, 2019

tekton-robot commented Dec 16, 2019

bobcatfish commented Dec 16, 2019

tekton-robot commented Dec 16, 2019

ghost commented Dec 16, 2019

vdemeester left a comment

tekton-robot commented Dec 17, 2019

vdemeester commented Dec 17, 2019

ghost commented Dec 17, 2019

tekton-robot commented Dec 17, 2019

Add workspace types for Task and TaskRun with validation #1639

Add workspace types for Task and TaskRun with validation #1639

Conversation

bobcatfish commented Nov 27, 2019 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

skaegi Dec 2, 2019

Choose a reason for hiding this comment

bobcatfish Dec 3, 2019

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

bobcatfish commented Dec 3, 2019

bobcatfish commented Dec 3, 2019

tekton-robot commented Dec 3, 2019

skaegi commented Dec 3, 2019

vdemeester commented Dec 4, 2019

ghost commented Dec 4, 2019 • edited by ghost Loading

ghost commented Dec 4, 2019 • edited by ghost Loading

ghost commented Dec 4, 2019

ghost commented Dec 4, 2019

vdemeester commented Dec 4, 2019

skaegi commented Dec 4, 2019

skaegi commented Dec 4, 2019

bobcatfish commented Dec 6, 2019

tekton-robot commented Dec 6, 2019

ghost left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester commented Dec 12, 2019

bobcatfish commented Dec 16, 2019

tekton-robot commented Dec 16, 2019

bobcatfish commented Dec 16, 2019

tekton-robot commented Dec 16, 2019

ghost commented Dec 16, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Dec 17, 2019

vdemeester commented Dec 17, 2019

ghost commented Dec 17, 2019

tekton-robot commented Dec 17, 2019

bobcatfish commented Nov 27, 2019 •

edited

Loading

ghost commented Dec 4, 2019 •

edited by ghost

Loading

ghost commented Dec 4, 2019 •

edited by ghost

Loading