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

Fabian-K · 2019-08-02T08:59:10Z

Changes

This commit enables to use storage classes other than the cluster-wide default storage class for the PVC of pipelines. The customization can be defined in the config map config-artifact-pvc.yaml - similar to how the size of the PVC can already be defined. By default, it will fallback to the cluster-wide default storage class. fixes #1101

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.

Release Notes

Allow the definition of a storage class for the artifact pvc using the ConfigMap `config-artifact-pvc`

This commit enables to use storage classes other than the cluster-wide default storage class for the PVC of pipelines. The customization can be defined in the config map config-artifact-pvc.yaml - similar to how the size of the PVC can already be defined. By default, it will fallback to the cluster-wide default storage class. fixes tektoncd#1101

googlebot · 2019-08-02T08:59:14Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

Fabian-K · 2019-08-02T09:01:38Z

@googlebot I signed it!

googlebot · 2019-08-02T09:01:41Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

afrittoli

Thank you for this.
This functionality might go away, but unless we manage to remove before next release, I think it's worth getting this in, as it will help folks running Tekton on clouds where a non-default storageClass is important.

afrittoli · 2019-08-02T10:05:56Z

pkg/artifacts/artifacts_storage.go

-			pvcSpec := GetPVCSpec(pr, pvcSize)
+			var pvcStorageClassName *string
+			if pvcStorageClassNameStr == "" {
+				pvcStorageClassName = nil


Since this goes to nil if it's not defined, it will work fine on existing (or new) clusters that do not include the storageClass in the config map.

afrittoli · 2019-08-02T10:13:59Z

/ok-to-test

tekton-robot · 2019-08-02T10:16:27Z

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/artifacts/artifacts_storage.go	79.5%	80.9%	1.4

afrittoli · 2019-08-02T14:55:53Z

/lgtm

bobcatfish

My only feedback is that I'd like the install docs to include a bit more info on how to configure this - I don't know much about storage classes myself so I'd be a bit lost in knowing what kind of values I can use! I don't want to block the PR on that tho so @Fabian-K if you are able to add that to the install docs in a follow up PR that would be awesome 🙏 but no worries if not! Thanks for updating the install docs anyway :D

Like @afrittoli said this functionality will likely be going away - but we also need to make sure that #1087 (which will replace this in the long run!) uses this same configuration!

/approve
/meow space

bobcatfish · 2019-08-05T14:43:50Z

docs/install.md

@@ -109,6 +109,7 @@ The PVC option can be configured using a ConfigMap with the name
 `config-artifact-pvc` and the following attributes:

 - size: the size of the volume (5Gi by default)
+- storageClassName: the storage class of the volume (default storage class by default)


maybe link to some docs here on what kind of values can be used?

tekton-robot · 2019-08-05T14:48:15Z

@bobcatfish:

In response to this:

My only feedback is that I'd like the install docs to include a bit more info on how to configure this - I don't know much about storage classes myself so I'd be a bit lost in knowing what kind of values I can use! I don't want to block the PR on that tho so @Fabian-K if you are able to add that to the install docs in a follow up PR that would be awesome 🙏 but no worries if not! Thanks for updating the install docs anyway :D

Like @afrittoli said this functionality will likely be going away - but we also need to make sure that #1087 (which will replace this in the long run!) uses this same configuration!

/approve
/meow space

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.

tekton-robot · 2019-08-05T14:48:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, Fabian-K

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

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

…of pipelines This commit adds a link to the k8s documentation for storage classes as a follow up for tektoncd#1148.

…of pipelines This commit adds a link to the k8s documentation for storage classes as a follow up for #1148.

tekton-robot requested review from abayer and afrittoli August 2, 2019 08:59

googlebot added the cla: no label Aug 2, 2019

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 2, 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 2, 2019

afrittoli reviewed Aug 2, 2019

View reviewed changes

tekton-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 2, 2019

tekton-robot assigned afrittoli Aug 2, 2019

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

bobcatfish reviewed Aug 5, 2019

View reviewed changes

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

tekton-robot merged commit 33ea1a0 into tektoncd:master Aug 5, 2019

Fabian-K added a commit to Fabian-K/pipeline that referenced this pull request Aug 6, 2019

Documentation adjusted for customizing the storage class for the PVC …

d1054fe

…of pipelines This commit adds a link to the k8s documentation for storage classes as a follow up for tektoncd#1148.

Fabian-K mentioned this pull request Aug 6, 2019

Documentation adjust for customizing the storage class for the PVC of pipelines #1165

Merged

2 tasks

tekton-robot pushed a commit that referenced this pull request Aug 6, 2019

Documentation adjusted for customizing the storage class for the PVC …

e3f4f4a

…of pipelines This commit adds a link to the k8s documentation for storage classes as a follow up for #1148.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Fabian-K commented Aug 2, 2019

googlebot commented Aug 2, 2019

Fabian-K commented Aug 2, 2019

googlebot commented Aug 2, 2019

afrittoli left a comment

afrittoli Aug 2, 2019

afrittoli commented Aug 2, 2019

tekton-robot commented Aug 2, 2019

afrittoli commented Aug 2, 2019

bobcatfish left a comment

bobcatfish Aug 5, 2019

tekton-robot commented Aug 5, 2019

tekton-robot commented Aug 5, 2019

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

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

Conversation

Fabian-K commented Aug 2, 2019

Changes

Submitter Checklist

Release Notes

googlebot commented Aug 2, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Fabian-K commented Aug 2, 2019

googlebot commented Aug 2, 2019

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli Aug 2, 2019

Choose a reason for hiding this comment

afrittoli commented Aug 2, 2019

tekton-robot commented Aug 2, 2019

afrittoli commented Aug 2, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Aug 5, 2019

Choose a reason for hiding this comment

tekton-robot commented Aug 5, 2019

tekton-robot commented Aug 5, 2019