Proposal for taking mount option to GA #771

gnufied · 2017-06-29T12:29:46Z

Also add support for specifying mount options
in storage classes.

cc @kubernetes/sig-storage-api-reviews @liggitt

Also add support for specifying mount options in storage classes.

ianchakeres · 2017-06-30T04:43:56Z

/sig storage

wongma7 · 2017-06-30T22:44:29Z

contributors/design-proposals/mount-options.md

+and added as a field to PVs.
+
+If admin has configured mount option for a storage type that does not support mount options,
+then `mountOptions` parameter will be simply ignored for such volume types.  Also, if configured


we have always returned a "provisioning failed" error when the storageclass has an invalid/unrecognized param

That seems correct, updating the proposal to reflect this.

liggitt · 2017-07-10T03:03:57Z

contributors/design-proposals/mount-options.md


 ## Preventing users from specifying mount options in inline volume specs of Pod

 While mount options enable more flexibility in how volumes are mounted, it can result
 in user specifying options that are not supported or are known to be problematic when
 using inline volume specs.

-After much deliberation it was decided that - `mount-options` as an API parameter will not be supported
+After much deliberation it was decided that - `mountOptions` as an API parameter will not be supported


will the volume source API structs used within a PV be cloned to add this new field?

No, the mountOptions is not a field of volume source APIs. It is a field of PV itself as proposed above

gnufied · 2017-07-12T18:24:47Z

@liggitt @saad-ali ping on this one again.

nrvnrvn · 2017-07-13T16:39:32Z

Imagine I want to mount /tmp as an EmptyDir of medium: Memory.
The recommendation is to mount /tmp with nosuid,nodev,noexec options. And this is what I see when running docker run --tmpfs /tmp:

tmpfs on /tmp type tmpfs (rw,nosuid,nodev,noexec,relatime)

But in kubernetes I observe:

tmpfs on /tmp type tmpfs (rw,relatime)

The proposal mentions PersistentVolumes and StorageClasses but how could one pass proper options to the EmptyDir?

nrvnrvn · 2017-07-13T16:47:51Z

err. it looks impossible atm https://github.com/kubernetes/kubernetes/blob/74f19437742abcff81510dc347f2cc70ec190e99/pkg/volume/empty_dir/empty_dir.go#L258
And it turns out that the emptydir case is beyond the proposal...

gnufied · 2017-07-20T19:28:53Z

contributors/design-proposals/mount-options.md

+provisioner: kubernetes.io/glusterfs
+parameters:
+  type: gp2
+  mountOptions: "auto_mount"


To continue on discussion from sig meeting today. @saad-ali I think having this in parameters frees us from having to perform validation of storageclass itself and such decision can be deferred to individual provisioner.

I think mountOptions should be a first class field in the StorageClass because it is controlling a first class field in the PV object (not specific to a single volume plugin). As far as validation is concerned, for in-tree plugins you can call SupportsMountOption(...) on the provisioner. The tricky bit will be how to pass this through for out-of-tree volume provisioners, but that problem needs to be solved for reclaimPolicy (kubernetes/kubernetes#47987) as well.

agree it should be 1st class for that reason.

I think the reason gnufied is hesitant to do create-time validation is we actually can't call SupportsMountOption due to, iirc, the cost of importing all the volume plugin code into the validation code. But that was resolved, even if it was a bit ugly..

It's true if we do create-time validation of storageClass.mountOptions for in-tree plugins there will be some asymmetry between in- & out-. But I don't think solving that is worth it. While admins deploying out-of-tree provisioners may appreciate the upfront warning that they've configured their storageClass with an unsupported mountOptions field, realistically they care more about mistakes in the parameters field which won't show up until run-time anyway... So I think they'll settle for run-time validation of storageClass.mountOptions.

Plus, this problem shouldn't exist for reclaimPolicy because every plugin already supports Retain, they can't opt-out of that like they can mount options.

I am fine making it first class field on StorageClass. The minor catch is - we can't really do creation time validation of storageClass for verifying if it supports mount options without hardcoding name of volume plugins that will support it (like literally hardcoding plugin name strings).

But I think that is relatively minor issue, because in my quick survey all volume types that support dynamic provisioning also support mount options (i.e at plugin level in current master) and hence there is no practical benefit of validation the SC for mount options during creation. So, what we can do is:

Make mountOption first class field for StorageClass.

Don't do any validation of mount option when storageClass is created.

Perform provisioning time SupportsMountOption validation and check if mount option is supported. This is really just a safety net. As I said above, all volume types that support dynamic provisioning also support mount options - so for most part this check will always be successful. Also, even if we didn't check during provisioning the PV creation will be anyways rejected if volume type doesn't support mount options.

Make mountOption first class field for StorageClass.

Don't do any validation of mount option when storageClass is created.

Perform provisioning time SupportsMountOption validation and check if mount option is supported. This is really just a safety net. As I said above, all volume types that support dynamic provisioning also support mount options - so for most part this check will always be successful. Also, even if we didn't check during provisioning the PV creation will be anyways rejected if volume type doesn't support mount options.

SGTM!

saad-ali · 2017-07-28T03:09:13Z

contributors/design-proposals/mount-options.md

+```
+
+
+Beta support for mount options introduced via `mount-options` annotation will be supported for near future


Beta -> Alpha?

No it was beta - https://kubernetes.io/docs/concepts/storage/persistent-volumes/#mount-options

rata · 2017-07-30T05:12:35Z

contributors/design-proposals/mount-options.md


 ## Preventing users from specifying mount options in inline volume specs of Pod

 While mount options enable more flexibility in how volumes are mounted, it can result
 in user specifying options that are not supported or are known to be problematic when
 using inline volume specs.

-After much deliberation it was decided that - `mount-options` as an API parameter will not be supported
+After much deliberation it was decided that - `mountOptions` as an API parameter will not be supported
 for inline volume specs.


So, if I understand correctly, this won't be possible to specify on pod.spec.volumes, right? I mean, as in a pod, that you can specify a volume and fsType, like in the example here: https://kubernetes.io/docs/concepts/storage/volumes/#awselasticblockstore

Can you please elaborate on the much deliberation?

Don't get me wrong, I'm not saying I'm against it, I just want to understand a little more. It seems weird that you can have the fsType there but not mountOptions. But I understand that pvs and pvc are preferred now, also.

So, if you have some time, can I ask what was taken into account on this deliberation, the trade offs of different approaches considered, etc.? Something high level is fine. I understand it's a busy time on the release, so no worries if you can't :)

I think bulk of discussion is here - #321 (review)

The tldr; of the discussion is - allowing normal users to specify mountOptions can enable users to compromise the host operating system itself. Users can disable selinux or can cause kernel panic which would crash the host operating system. That is why mount option should be carefully controlled.

@thanks a lot for the link! I guess I'm with @saad-ali, but all makes sense and it's nice to see the background. Thanks :)

rata · 2017-07-30T05:20:41Z

contributors/design-proposals/mount-options.md

+then a "provisioning failed" event will be added to PVC and PVC will stay in pending state.
+
+Also, if configured mount option is invalid then corresponding mount time failure error will be added to pod object.
+

 ## Preventing users from specifying mount options in inline volume specs of Pod

 While mount options enable more flexibility in how volumes are mounted, it can result
 in user specifying options that are not supported or are known to be problematic when
 using inline volume specs.


When using inline volumes? What is more problematic about an in-line volume? Coordinate the fsType and the mount options? If that guess is on track, then it's more problematic for file-system dependent mount options (there are file-system independent options too) and when not specifyng in-line the fsType?

responded above.

Thanks again! :)

childsb · 2017-08-17T16:49:18Z

/lgtm

k8s-github-robot · 2017-08-17T16:50:57Z

Automatic merge from submit-queue

Automatic merge from submit-queue (batch tested with PRs 50919, 51410, 50099, 51300, 50296) Take mount options to GA by adding PV.spec.mountOptions **What this PR does / why we need it**: Implements kubernetes/community#771 issue: kubernetes/enhancements#168 **Special notes for your reviewer**: TODO: - ~StorageClass mountOptions~ As described in proposal, this adds PV.spec.mountOptions + mountOptions parameter to every plugin that is both provisionable & supports mount options. (personally, even having done all the work already, i don't agree w/ the proposal that mountOptions should be SC parameter but... :)) **Release note**: ```release-note Add mount options field to PersistentVolume spec ```

@liggitt

Automatic merge from submit-queue Proposal for taking mount option to GA Also add support for specifying mount options in storage classes. cc @kubernetes/sig-storage-api-reviews @liggitt

Proposal for taking mount option to GA

4ecc91b

Also add support for specifying mount options in storage classes.

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

gnufied mentioned this pull request Jun 29, 2017

Support Volume Mount Options kubernetes/enhancements#168

Closed

wongma7 reviewed Jun 30, 2017

View reviewed changes

liggitt reviewed Jul 10, 2017

View reviewed changes

Update error handling description

58f9efd

gnufied commented Jul 20, 2017

View reviewed changes

saad-ali added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jul 28, 2017

saad-ali reviewed Jul 28, 2017

View reviewed changes

rata reviewed Jul 30, 2017

View reviewed changes

k8s-github-robot assigned sarahnovotny and grodrigues3 Aug 15, 2017

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 15, 2017

grodrigues3 unassigned sarahnovotny and grodrigues3 Aug 16, 2017

k8s-github-robot assigned idvoretskyi and sarahnovotny Aug 16, 2017

k8s-ci-robot assigned childsb Aug 17, 2017

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

k8s-github-robot merged commit c21e730 into kubernetes:master Aug 17, 2017

wongma7 mentioned this pull request Aug 18, 2017

Take mount options to GA by adding PV.spec.mountOptions kubernetes/kubernetes#50919

Merged

M0nsieurChat mentioned this pull request Apr 29, 2019

[snapshot-provisioner] support the storageClass.mountOptions parameter kubernetes-retired/external-storage#1165

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal for taking mount option to GA #771

Proposal for taking mount option to GA #771

gnufied commented Jun 29, 2017

ianchakeres commented Jun 30, 2017

wongma7 Jun 30, 2017

gnufied Jul 12, 2017

liggitt Jul 10, 2017

gnufied Jul 10, 2017

gnufied commented Jul 12, 2017

nrvnrvn commented Jul 13, 2017

nrvnrvn commented Jul 13, 2017

gnufied Jul 20, 2017

saad-ali Jul 28, 2017

wongma7 Aug 14, 2017

gnufied Aug 23, 2017 •

edited

Loading

saad-ali Aug 23, 2017

saad-ali Jul 28, 2017

gnufied Jul 31, 2017

rata Jul 30, 2017 •

edited

Loading

gnufied Jul 31, 2017 •

edited

Loading

rata Jul 31, 2017 •

edited

Loading

rata Jul 30, 2017

gnufied Jul 31, 2017

rata Jul 31, 2017

childsb commented Aug 17, 2017

k8s-github-robot commented Aug 17, 2017

		```


		Beta support for mount options introduced via `mount-options` annotation will be supported for near future

Proposal for taking mount option to GA #771

Proposal for taking mount option to GA #771

Conversation

gnufied commented Jun 29, 2017

ianchakeres commented Jun 30, 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

gnufied commented Jul 12, 2017

nrvnrvn commented Jul 13, 2017

nrvnrvn commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied Aug 23, 2017 • edited Loading

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

rata Jul 30, 2017 • edited Loading

Choose a reason for hiding this comment

gnufied Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

rata Jul 31, 2017 • edited Loading

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

childsb commented Aug 17, 2017

k8s-github-robot commented Aug 17, 2017

gnufied Aug 23, 2017 •

edited

Loading

rata Jul 30, 2017 •

edited

Loading

gnufied Jul 31, 2017 •

edited

Loading

rata Jul 31, 2017 •

edited

Loading