Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement support for mount options in PVs #41906

Merged
merged 1 commit into from
Mar 1, 2017
Merged

Implement support for mount options in PVs #41906

merged 1 commit into from
Mar 1, 2017

Conversation

gnufied
Copy link
Member

@gnufied gnufied commented Feb 22, 2017 •
edited
Loading

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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 22, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Feb 22, 2017
@rootfs
Copy link
Contributor

rootfs commented Feb 22, 2017

Let's just add options to ReadWriteMany volume types (e.g. nfs and glusterfs) for now, and wait till provisioner is able to provision volumes and format into a desired fs type. Otherwise, PD/EBS/Cinder/etc volumes could be formatted into either xfs or extX, the mount options won't be meaningful if fs type mismatches.

@gnufied
Copy link
Member Author

gnufied commented Feb 22, 2017

@rootfs can you elaborate more? For attachable volume types - if file system on volume source and actual file system on device doesn't match - then mounting itself will fail regardless of supplied mount options.

The formatting is only attempted when device is not formatted to any file system type. And in that case - whatever mounting options specified in PV should work.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2017
@gnufied gnufied changed the title [WIP] Implement support for mount options [WIP] Implement support for mount options in PVs Feb 23, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2017
saad-ali
saad-ali suggested changes Feb 23, 2017
View reviewed changes
Copy link
Member

@saad-ali saad-ali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, let's switch to annotation and target beta for 1.6

pkg/volume/volume.go Outdated
@@ -121,6 +121,9 @@ type Mounter interface {
SetUpAt(dir string, fsGroup *int64) error
// GetAttributes returns the attributes of the mounter.
GetAttributes() Attributes

// SupportsMountOption returns if volume plugins support Mount options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Comment could be more descriptive =) Interface method comments should be descriptive enough that it is very clear to someone new coming in trying to implement it what they need to do (without having to look at other implementations).

pkg/volume/util/operationexecutor/operation_generator.go Outdated
@@ -383,6 +383,8 @@ func (og *operationGenerator) GenerateMountVolumeFunc(
newMounterErr)
}

checkMountOptionSupport(og, volumeToMount, volumeMounter)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think error handling should be explicit: where possible, we should avoid silently swallowing a user's intention and, instead, explicitly fail their request to indicate that we are not doing what was asked and tell them what they can do to rectify it. So I would prefer to fail the mount call here if these options are passed in and the volume doesn't support it.

Additionally, I would add validation at API object creation time, if that is not too painful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do.

pkg/volume/util.go
@@ -354,3 +354,29 @@ func UnmountViaEmptyDir(dir string, host VolumeHost, volName string, volSpec Spe
}
return wrapped.TearDownAt(dir)
}

// MountOptionFromSpec extracts and joins mount options from volume spec with supplied options
func MountOptionFromSpec(spec *Spec, options ...string) []string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add unit tests for these methods.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

pkg/volume/util.go Outdated

// JoinMountOptions joins mount options eliminating duplicates
func JoinMountOptions(userOptions []string, systemOptions []string) []string {
allMountOptions := map[string]string{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really what you need here is a set not a map. Maybe use https://github.com/kubernetes/kubernetes/tree/master/pkg/util/sets for clarity.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@saad-ali saad-ali assigned saad-ali and unassigned bprashanth Feb 23, 2017
@saad-ali saad-ali mentioned this pull request Feb 23, 2017
Merged
@saad-ali saad-ali added this to the v1.6 milestone Feb 23, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 23, 2017
@redbaron
Copy link
Contributor

what happens with this change if it is not ready before 1.6 freeze? will it be pushed to 1.7?

@humblec
Copy link
Contributor

humblec commented Feb 24, 2017

@rootfs @gnufied I can define the whiltelist/desired options for glusterfs.

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 24, 2017
@gnufied
Copy link
Member Author

gnufied commented Feb 24, 2017

@humblec there is no need of that now. We got rid of whitelisting requirements. Admins can specify whatever they want in mountOptions for supported plugin types.

@smarterclayton smarterclayton removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 27, 2017
@smarterclayton
Copy link
Contributor

Thanks, lgtm from API side

@gnufied
Copy link
Member Author

gnufied commented Feb 27, 2017

Also updated release notes etc to reflect new mount-options string. I will open another PR to change the string in design proposal, but I don't think that should block this PR.

@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2017
This was referenced Feb 27, 2017
Merged
Merged
@saad-ali
Copy link
Member

This PR and the ScaleIO PR (#38924) and the Portworx PR (#39535) will race: which ever ones get in first will force the others to to rebase. Just FYI

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2017
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 28, 2017
@gnufied
Copy link
Member Author

gnufied commented Feb 28, 2017

I had to rebase this PR to accommodate master changes. cc @saad-ali @smarterclayton

@eparis eparis added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2017
@gnufied
Copy link
Member Author

gnufied commented Mar 1, 2017

@k8s-bot test this

@gnufied
Implement support for mount options in PVs
2d3008f 
Add support for mount options via annotations on PVs
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: gnufied, k8s-merge-robot, saad-ali, smarterclayton

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

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

@gnufied
Copy link
Member Author

gnufied commented Mar 1, 2017

@eparis @saad-ali had to rebase this PR because portworx got merged first. Needs another /lgtm

@eparis eparis added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 3bc342c into kubernetes:master Mar 1, 2017
@ixdy ixdy mentioned this pull request Mar 8, 2017
Closed
@soltysh soltysh mentioned this pull request Mar 8, 2017
Closed
This was referenced Mar 20, 2017
Closed
Closed
Closed
dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018
Merge pull request kubernetes#41906 from gnufied/implement-mount-options
dd2306a 
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.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saad-ali saad-ali saad-ali approved these changes

@eparis eparis eparis left review comments

@rootfs rootfs rootfs left review comments

@smarterclayton smarterclayton smarterclayton left review comments

@childsb childsb childsb left review comments

Assignees

@saad-ali saad-ali

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.

Support Volume Mount Options