Support "fstype" parameter in dynamically provisioned PVs #45345

codablock · 2017-05-04T10:39:36Z

This PR is a replacement for #40805. I was not able to push fixes and rebases to the original branch as I don't have access to the Github organization anymore.

I assume the PR will need a new "ok to test"

ORIGINAL PR DESCRIPTION

What this PR does / why we need it: This PR allows specifying the desired FSType when dynamically provisioning volumes with storage classes. The FSType can now be set as a parameter:

kind: StorageClass
apiVersion: storage.k8s.io/v1beta1
metadata:
  name: test
provisioner: kubernetes.io/azure-disk
parameters:
  fstype: xfs

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #37801

Special notes for your reviewer:
The PR also implicitly adds checks for unsupported parameters.

Release note:

Support specifying of FSType in StorageClass

k8s-reviewable · 2017-05-04T10:39:41Z

This change is

k8s-ci-robot · 2017-05-04T10:39:43Z

Hi @codablock. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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.

spiffxp · 2017-05-08T15:08:47Z

@k8s-bot ok to test

jsafrane

Please update also https://github.com/kubernetes/kubernetes/tree/master/examples/persistent-volume-provisioning

jsafrane · 2017-05-09T11:17:36Z

pkg/volume/azure_dd/azure_provision.go

@@ -107,6 +107,7 @@ var _ volume.Provisioner = &azureDiskProvisioner{}

 func (a *azureDiskProvisioner) Provision() (*v1.PersistentVolume, error) {
 	var sku, location, account string
+	var FSType *string


Shouldn't it be "ext4" by default so it's consistent among all volume plugins?

If I remember correct, Azure already does the default fstype handling later and I did not want to touch that part. Azure was well tested by me at that time and it worked well.

Just pushed an update to the examples.

jsafrane · 2017-05-09T11:24:05Z

@k8s-bot gce etcd3 e2e test this
@k8s-bot kops aws e2e test this

jsafrane · 2017-05-09T12:19:50Z

examples/persistent-volume-provisioning/README.md

 ```

 * `type`: `io1`, `gp2`, `sc1`, `st1`. See AWS docs for details. Default: `gp2`.
 * `zone`: AWS zone. If not specified, a random zone from those where Kubernetes cluster has a node is chosen.
 * `iopsPerGB`: only for `io1` volumes. I/O operations per second per GiB. AWS volume plugin multiplies this with size of requested volume to compute IOPS of the volume and caps it at 20 000 IOPS (maximum supported by AWS, see AWS docs).
 * `encrypted`: denotes whether the EBS volume should be encrypted or not. Valid values are `true` or `false`.
 * `kmsKeyId`: optional. The full Amazon Resource Name of the key to use when encrypting the volume. If none is supplied but `encrypted` is true, a key is generated by AWS. See AWS docs for valid ARN value.
+* `fstype`: fstype that are supported by kubernetes. Default: `"ext4"`.


fstype -> fsType?

I copied this line 1:1 from vSphere where fstype was already supported. If I change it to fsType, I'll change it there as well. What do you think?

It's a typo, please change it to fsType. Thanks!

humblec · 2017-05-11T07:21:32Z

@codeblock LGTM. Please rebase.

tpetr · 2017-05-17T15:47:23Z

Anything I can do to help move this along?

codablock · 2017-05-18T12:15:15Z

@k8s-bot gce etcd3 e2e test this

bbeaudreault · 2017-05-23T20:54:12Z

I'm blocked by this as well

codablock · 2017-05-24T10:13:49Z

Can anyone help with the test failures please? They look completely unrelated but keep failing all the time and I really don't know what to look for.

wongma7 · 2017-05-26T17:03:37Z

Some statefulset tests rely on dynamic provisioning, they are definitely related.

Dug through controller manager logs for you:

I0524 08:25:06.367294 5 pv_controller.go:1336] failed to provision volume for claim "e2e-tests-statefulset-71bln/datadir-ss-0" with StorageClass "standard": invalid option "type" for volume plugin kubernetes.io/gce-pd

wongma7 · 2017-05-26T17:09:20Z

pkg/volume/aws_ebs/aws_ebs.go

+		case volume.VolumeParameterFSType:
+			FSType = v
+		default:
+			return nil, fmt.Errorf("invalid option %q for volume plugin %s", k, c.plugin.GetPluginName())


There is existing parameters (type, zone) parsing in gce_util.go; this is why the tests are failing

same for aws, cinder, photon, and vsphere, they all have _util.go files where the new parameter should be added

saad-ali · 2017-05-26T18:46:10Z

CC @kubernetes/sig-storage-pr-reviews @gnufied @thockin @wattsteve @childsb

Erin, brought this PR up as related to our discussion about FSs this morning. FWIW, this aligns with the direction I was thinking. FSType is codified as a parameter in the provisioner. Once @jsafrane's "user selectable storage class options" is complete, cluster admins would also be able to filter FS types, it if they want, along with other StorageClass parameters. As implemented it sets the FSType in on the PV object which is then used during mounting--the nice thing about that is it works seamlessly with pre-provisioned volumes.

kangarlou · 2017-07-05T15:24:12Z

I agree with the sentiment that parameters in the storage class should always be treated as opaque. As a result, we shouldn't promote any keywords in the parameters section of the storage class. I also think fsType shouldn't be a keyword outside of the parameters section as for the most part it's limited to iSCSI/SAN volumes and that there are countless other knobs that other plugins require. Where do we draw the line? Should mount options be added as new keywords in the storage class?

I'm of the opinion that it's perfectly ok for a volume plugin to make use of fsType or other knobs as opaque parameters in the storage class. Based on the type of the parameter and security/operational concerns, volume plugins may or may not allow overwriting the admin-defined value in the storage class by having users make use of PVC annotations. Here is an example of how to use PVC annotations to specify file system type: https://github.com/NetApp/trident#kubernetes-volumes. As you see, it's not even necessary to have a field in the storage class for fsType when it's of no concern to the admin. Obviously, in the future we should use labels to specify volume creation parameters, but till then, PVC annotations are a much better interim solution than redefining existing API objects.

codablock · 2017-07-12T07:30:00Z

Any chance to get a final approval here? The merge robot suggests @smarterclayton for approval.

As more time passes, as harder it gets for me to participate in discussions as I'm not actively working on k8s anymore. I'll do a final rebase when final approval is done if that's ok with everyone, as this PR already required quite a lot of rebases in the past as code seems to move fast in the touched places.

jsafrane · 2017-07-12T07:34:30Z

Unfortunately, examples/OWNERS has very little approvers. I scheduled an item on the next sig-storage meeting to formally approve these API changes next Thursday, after that it could be merged, the code is solid.

codablock · 2017-07-19T11:34:34Z

Did another rebase to prepare for the formal approval. Also, I had to create another PR in kubernetes/examples: kubernetes/examples#28 as the README.md for the examples has moved to this repo.

codablock · 2017-07-19T15:18:37Z

/retest

codablock · 2017-07-19T20:40:33Z

@k8s-bot kops aws e2e test this
@k8s-bot gce etcd3 e2e test this

jsafrane · 2017-07-21T10:32:25Z

/lgtm

k8s-github-robot · 2017-07-21T10:32:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: codablock, jsafrane

Associated issue: 37801

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/volume/OWNERS~~ [jsafrane]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

jsafrane · 2017-07-21T10:33:17Z

Thanks for the PR and your patience!

jsafrane · 2017-07-21T14:28:18Z

/retest

jsafrane · 2017-07-21T15:33:01Z

/retest

jsafrane · 2017-07-21T16:09:29Z

come on...
/retest

codablock · 2017-07-21T17:17:08Z

Are these failures unrelated flakes? Or should I take a deeper look?

jsafrane · 2017-07-21T17:20:51Z

they're flakes

jsafrane · 2017-07-24T10:25:02Z

/retest

k8s-ci-robot · 2017-07-24T10:43:08Z

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

Test name	Commit	Details	Rerun command
Jenkins kops AWS e2e	f8886ced4a93509a92cbb182283eb48eccb513fe	link	`@k8s-bot kops aws e2e test this`
Jenkins GCE etcd3 e2e	f8886ced4a93509a92cbb182283eb48eccb513fe	link	`@k8s-bot gce etcd3 e2e test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

jsafrane · 2017-07-24T11:01:59Z

/retest

k8s-github-robot · 2017-07-24T14:40:46Z

Automatic merge from submit-queue (batch tested with PRs 45345, 49470, 49407, 49448, 49486)

tpetr · 2017-07-24T14:44:18Z

@codablock @jsafrane Woohoo! Thank you for your persistence here.

Automatic merge from submit-queue (batch tested with PRs 45345, 49470, 49407, 49448, 49486) Support "fstype" parameter in dynamically provisioned PVs This PR is a replacement for kubernetes#40805. I was not able to push fixes and rebases to the original branch as I don't have access to the Github organization anymore. I assume the PR will need a new "ok to test" **ORIGINAL PR DESCRIPTION** **What this PR does / why we need it**: This PR allows specifying the desired FSType when dynamically provisioning volumes with storage classes. The FSType can now be set as a parameter: ```yaml kind: StorageClass apiVersion: storage.k8s.io/v1beta1 metadata: name: test provisioner: kubernetes.io/azure-disk parameters: fstype: xfs ``` **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes kubernetes#37801 **Special notes for your reviewer**: The PR also implicitly adds checks for unsupported parameters. **Release note**: ```release-note Support specifying of FSType in StorageClass ```

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 4, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 4, 2017

k8s-github-robot assigned smarterclayton and jsafrane May 4, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 4, 2017

codablock mentioned this pull request May 4, 2017

Support "fstype" parameter in dynamically provisioned PVs #40805

Closed

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 8, 2017

jsafrane reviewed May 9, 2017

View reviewed changes

codablock force-pushed the storageclass_fstype branch from 5faf0d5 to 2b1a373 Compare May 9, 2017 12:08

jsafrane reviewed May 9, 2017

View reviewed changes

codablock force-pushed the storageclass_fstype branch from 2b1a373 to fa98965 Compare May 11, 2017 07:51

codablock force-pushed the storageclass_fstype branch from fa98965 to f8886ce Compare May 18, 2017 09:07

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2017

codablock force-pushed the storageclass_fstype branch from f8886ce to 7a23bcc Compare May 24, 2017 07:51

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 24, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2017

wongma7 reviewed May 26, 2017

View reviewed changes

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label May 26, 2017

dutchiechris mentioned this pull request Jul 4, 2017

RFE: Allow setting of fsType, format, and mount options NetApp/trident#37

Closed

Support "fstype" parameter in dynamically provisioned PVs

8057056

codablock force-pushed the storageclass_fstype branch from f94c9b8 to 8057056 Compare July 19, 2017 11:33

codablock mentioned this pull request Jul 19, 2017

Add fsType parameters to persistent volume provisioning examples kubernetes/examples#28

Merged

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 19, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2017

k8s-github-robot merged commit d286f56 into kubernetes:master Jul 24, 2017

kokhang mentioned this pull request Jul 27, 2017

Implement the fsType parameter for rook.io/block volume provisioner rook/rook#742

Closed

woodlee mentioned this pull request Aug 16, 2017

Make fsType specifyable in volumeClaimTemplates #44478

Open

Support "fstype" parameter in dynamically provisioned PVs #45345

Support "fstype" parameter in dynamically provisioned PVs #45345

Conversation

codablock commented May 4, 2017

k8s-reviewable commented May 4, 2017

k8s-ci-robot commented May 4, 2017

spiffxp commented May 8, 2017

jsafrane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codablock May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humblec commented May 11, 2017

tpetr commented May 17, 2017

codablock commented May 18, 2017

bbeaudreault commented May 23, 2017

codablock commented May 24, 2017

wongma7 commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented May 26, 2017

kangarlou commented Jul 5, 2017 • edited Loading

codablock commented Jul 12, 2017

jsafrane commented Jul 12, 2017

codablock commented Jul 19, 2017

codablock commented Jul 19, 2017

codablock commented Jul 19, 2017

jsafrane commented Jul 21, 2017

k8s-github-robot commented Jul 21, 2017

jsafrane commented Jul 21, 2017

jsafrane commented Jul 21, 2017

jsafrane commented Jul 21, 2017

jsafrane commented Jul 21, 2017

codablock commented Jul 21, 2017

jsafrane commented Jul 21, 2017

jsafrane commented Jul 24, 2017

k8s-ci-robot commented Jul 24, 2017 • edited Loading

jsafrane commented Jul 24, 2017

k8s-github-robot commented Jul 24, 2017

tpetr commented Jul 24, 2017

codablock May 9, 2017 •

edited

Loading

kangarlou commented Jul 5, 2017 •

edited

Loading

k8s-ci-robot commented Jul 24, 2017 •

edited

Loading