New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Volume Mount Options #168

Closed
rootfs opened this Issue Jan 20, 2017 · 29 comments

Comments

Projects
@rootfs
Copy link
Member

rootfs commented Jan 20, 2017

Description

We currently support network filesystems: NFS, Glusterfs, Ceph FS, SMB (Azure file), Quobytes, and local filesystems such as ext[3|4] and XFS.

Mount time options that are operationally important and have no security implications should be suppported. Examples are NFS's TCP mode, versions, lock mode, caching mode; Glusterfs's caching mode; SMB's version, locking, id mapping; and more.

One solution is to walk through all the mount options and create API objects for each one of them in their volume source. This solution clearly states what are the supported options. But supporting all valid mount options makes us liable to compatibility issues when mount options changes between OS distributions/releases, or between OSes (Linux, BSD/OSX if they are to be supported in the future)

Second solution also adds an API objects in volume source. Instead of 1-1 mapping, the single API object is a string comma separated blob like version=3,protoc=tcp,nolock. It is the convention used by mount command. Since this solution only supported a subset of filesystem options, it is not subject to mount option changes. Arguably, the mount options are only for expert user who understand what they do. The mount options API object defaults to none.

  • Primary contact (assignee): @gnufied
  • Responsible SIGs: Storage
  • Design proposal link (community repo):
  • Reviewer(s) - (for LGTM) recommend having 2+ reviewers (at least one from code-area OWNERS file) agreed to review. Reviewers from multiple companies preferred:
    Google: @saad-ali @jingxu97
    RedHat: @childsb @jsafrane
  • Approver (likely from SIG/area to which feature belongs): @saad-ali @childsb
  • Feature target (which target equals to which milestone):
    • Alpha release target: 1.6
    • Beta release target: -
    • Stable release target (x.y): 1.8
@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Jan 20, 2017

@mdelio mdelio added the sig/storage label Jan 23, 2017

@mdelio mdelio added this to the v1.6 milestone Jan 23, 2017

@idvoretskyi

This comment has been minimized.

Copy link
Member

idvoretskyi commented Jan 26, 2017

@rootfs are you a person, responsible for the feature?

@gnufied

This comment has been minimized.

Copy link
Member

gnufied commented Jan 26, 2017

This should be assigned to me. I unfortunately don't have enough privilege to do that myself - https://docs.google.com/spreadsheets/d/1t4z5DYKjX2ZDlkTpCnp18icRAQqOE85C1T1r2gqJVck/edit#gid=0

@idvoretskyi

This comment has been minimized.

Copy link
Member

idvoretskyi commented Jan 26, 2017

@gnufied done.

@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Jan 31, 2017

We have a case to pass selinux context as -o context to cifs mount in azure_file volume to resolve permission denied issue.

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Feb 1, 2017

@jingxu97 @saad-ali would be nice to get the design tradeoffs done and get this behind us. It's been lingering for a long time.

@matchstick

This comment has been minimized.

Copy link

matchstick commented Feb 1, 2017

@saad-ali and I talked this over today and we both agree that making sure that working with rootfs on this is one of his now highest priorities, now that he is no longer under the release czar role.

@gnufied

This comment has been minimized.

Copy link
Member

gnufied commented Feb 1, 2017

Yeah it is a top priority for us too. I am working on a design document with @rootfs and will publish it today. I apologize for not sending the design proposal sooner - got caught up in other feature requests.

@gnufied

This comment has been minimized.

Copy link
Member

gnufied commented Feb 1, 2017

@philips

This comment has been minimized.

Copy link
Contributor

philips commented Feb 2, 2017

cc @lpabon

@gnufied

This comment has been minimized.

Copy link
Member

gnufied commented Mar 1, 2017

@spxtr can we keep this open? we need to walk this feature through - beta, GA, docs and whole thing.

@childsb childsb reopened this Mar 2, 2017

@childsb

This comment has been minimized.

Copy link
Member

childsb commented Mar 2, 2017

@spxtr was this closed on purpose?

@idvoretskyi

This comment has been minimized.

Copy link
Member

idvoretskyi commented Mar 8, 2017

@gnufied any update on this feature? Docs and release notes are required (please, provide them to the features spreadsheet.

@spxtr

This comment has been minimized.

Copy link
Member

spxtr commented Mar 8, 2017

Sorry, I simply synced my fork, and thanks to this bug feature, GitHub closed the issue.

@gnufied

This comment has been minimized.

Copy link
Member

gnufied commented Mar 9, 2017

Added release notes to spreadsheet. The documentation PR is - kubernetes/website#2743

@saad-ali saad-ali added stage/beta and removed stage/stable labels Mar 13, 2017

@saad-ali

This comment has been minimized.

Copy link
Member

saad-ali commented Mar 13, 2017

This is beta for 1.6 not stable updated labels and spreadsheet.

@mdelio

This comment has been minimized.

Copy link

mdelio commented May 4, 2017

@gnufied are we planning to bring this to stable in 1.7

@calebamiles calebamiles added this to the v1.7 milestone May 4, 2017

@saad-ali

This comment has been minimized.

Copy link
Member

saad-ali commented May 25, 2017

This is not going to go to GA in 1.7 (it went beta in 1.6). We will punt GA to 1.8.

@saad-ali saad-ali modified the milestones: next-milestone, v1.7 May 25, 2017

@gnufied

This comment has been minimized.

Copy link
Member

gnufied commented Jun 29, 2017

We are going to try and get this GA in 1.8. The corresponding feature request change for taking this feature to GA - kubernetes/community#771

@saad-ali saad-ali modified the milestones: 1.8, next-milestone Jul 12, 2017

@idvoretskyi

This comment has been minimized.

Copy link
Member

idvoretskyi commented Jul 25, 2017

@saad-ali @gnufied please, update the feature description with the new timeline.

@gnufied

This comment has been minimized.

Copy link
Member

gnufied commented Jul 25, 2017

@idvoretskyi I didn't create this issue, @rootfs did - but I do own it. I can't edit the original description unfortunately. The feature went beta in 1.6 and is going GA in 1.8.

@saad-ali saad-ali added stage/stable and removed stage/beta labels Aug 1, 2017

@saad-ali

This comment has been minimized.

Copy link
Member

saad-ali commented Aug 1, 2017

@saad-ali @gnufied please, update the feature description with the new timeline.

Done. This feature is being promoted to stable in 1.8.

@matthewceroni

This comment has been minimized.

Copy link

matthewceroni commented Aug 22, 2017

I see this is support on the PV via an annotation, but what about when using dynamic provisioning?

I attempted to add the annotation to the volumeClaimTemplate but that did not see to work.

{{- if .Values.Persistence.Enabled }}
  volumeClaimTemplates:
  - metadata:
      name: pgdata
      annotations:
        volume.beta.kubernetes.io/storage-class: {{ .Values.Persistence.StorageClass }}
        volume.beta.kubernetes.io/mount-options: "noatime"
    spec:
      accessModes:
      - {{ .Values.Persistence.AccessMode | quote }}
      resources:
        requests:
          storage: {{ .Values.Persistence.Size | quote }}
{{- else }}
      - name: pgdata
        emptyDir: {}
{{- end }}

FYI: This results in an EBS volume (when rendered via helm)

@jsafrane

This comment has been minimized.

Copy link
Member

jsafrane commented Aug 23, 2017

@matthewceroni, in 1.8 we plan to add mountOptions to storageClass.parameters, see kubernetes/community#771

k8s-merge-robot added a commit to kubernetes/kubernetes that referenced this issue Aug 29, 2017

Merge pull request #50919 from wongma7/mount-options
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
```

k8s-merge-robot added a commit to kubernetes/kubernetes that referenced this issue Aug 30, 2017

Merge pull request #51228 from wongma7/mount-options-sc
Automatic merge from submit-queue

Add storageClass.mountOptions and use it in all applicable plugins

split off from #50919 and still dependent on it. cc @gnufied


issue: kubernetes/enhancements#168

```release-note
Add mount options field to StorageClass. The options listed there are automatically added to PVs provisioned using the class.
```

dims pushed a commit to dims/openstack-cloud-controller-manager that referenced this issue Sep 6, 2017

Merge pull request #41906 from gnufied/implement-mount-options
Automatic merge from submit-queue

Implement support for mount options in PVs

**What this PR does / why we need it**:

This PR implements support for mount options in PersistentVolume via `volume.beta.kubernetes.io/mount-options` annotation.

**Which issue this PR fixes** 

Fixes kubernetes/enhancements#168

**Release note**:
```
Enable additional, custom mount options to be passed to PersistentVolume objects via volume.beta.kubernetes.io/mount-options annotation.
```

dims pushed a commit to dims/openstack-cloud-controller-manager that referenced this issue Sep 6, 2017

Merge pull request #51228 from wongma7/mount-options-sc
Automatic merge from submit-queue

Add storageClass.mountOptions and use it in all applicable plugins

split off from kubernetes/kubernetes#50919 and still dependent on it. cc @gnufied


issue: kubernetes/enhancements#168

```release-note
Add mount options field to StorageClass. The options listed there are automatically added to PVs provisioned using the class.
```
@gnufied

This comment has been minimized.

Copy link
Member

gnufied commented Jan 2, 2018

This feature has been implemented as speced. Closing.

/close

dims pushed a commit to dims/openstack-cloud-controller-manager that referenced this issue Jan 13, 2018

Merge pull request #41906 from gnufied/implement-mount-options
Automatic merge from submit-queue

Implement support for mount options in PVs

**What this PR does / why we need it**:

This PR implements support for mount options in PersistentVolume via `volume.beta.kubernetes.io/mount-options` annotation.

**Which issue this PR fixes** 

Fixes kubernetes/enhancements#168

**Release note**:
```
Enable additional, custom mount options to be passed to PersistentVolume objects via volume.beta.kubernetes.io/mount-options annotation.
```

dims pushed a commit to dims/openstack-cloud-controller-manager that referenced this issue Jan 13, 2018

Merge pull request #51228 from wongma7/mount-options-sc
Automatic merge from submit-queue

Add storageClass.mountOptions and use it in all applicable plugins

split off from kubernetes/kubernetes#50919 and still dependent on it. cc @gnufied


issue: kubernetes/enhancements#168

```release-note
Add mount options field to StorageClass. The options listed there are automatically added to PVs provisioned using the class.
```

justaugustus pushed a commit to justaugustus/enhancements that referenced this issue Sep 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment