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

Add storageClass.mountOptions and use it in all applicable plugins #51228

Merged
merged 2 commits into from
Aug 30, 2017
Merged

Add storageClass.mountOptions and use it in all applicable plugins #51228

merged 2 commits into from
Aug 30, 2017

Conversation

wongma7
Copy link
Contributor

@wongma7 wongma7 commented Aug 23, 2017 •
edited
Loading

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

issue: kubernetes/enhancements#168

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

@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/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 23, 2017
gnufied
gnufied reviewed Aug 24, 2017
View reviewed changes
Copy link
Member

@gnufied gnufied left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me. left some minor comments.

pkg/controller/volume/persistentvolume/pv_controller.go Outdated
if !plugin.SupportsMountOption() && len(options.MountOptions) > 0 {
strerr := fmt.Sprintf("Cannot provision volume because the provisioner doesn't support mount options but the StorageClass has mount options %v", options.MountOptions)
glog.V(2).Infof("cannot provision volume for claim %q with StorageClass %q because the provisioner doesn't support mount options but the StorageClass has mount options %v", claimToClaimKey(claim), storageClass.Name, options.MountOptions)
ctrl.eventRecorder.Event(claim, v1.EventTypeWarning, events.ProvisioningFailed, strerr)
Copy link
Member

Choose a reason for hiding this comment

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

should we not just return from here? Because if PV has mount options and volume plugin doesn't support it, it will be rejected by API server anyways.

Copy link
Member

Choose a reason for hiding this comment

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

Lets add a test for this perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, return statement is missing.

will add a test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 28, 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 Aug 28, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 28, 2017
@wongma7 wongma7 force-pushed the mount-options-sc branch 2 times, most recently from ac945b7 to abc7160 Compare August 28, 2017 23:03
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 29, 2017 •
edited
Loading

@wongma7: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-bazel abc716014e2887564f5c6dbc7402d6b877003000 link /test pull-kubernetes-bazel

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.

@wongma7
Copy link
Contributor Author

wongma7 commented Aug 29, 2017 •
edited
Loading

/test pull-kubernetes-bazel
/assign @jsafrane
please help review the plugin changes, thanks!! (last two commits only)

@wongma7 wongma7 changed the title WIP: Add storageClass.mountOptions and use it in all applicable plugins Add storageClass.mountOptions and use it in all applicable plugins Aug 29, 2017
@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Aug 29, 2017
@thockin
Copy link
Member

thockin commented Aug 29, 2017

/approve

This is not a code review

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2017
@jsafrane
Copy link
Member

lgtm in general, will wait for final PR

@wongma7
Copy link
Contributor Author

wongma7 commented Aug 29, 2017

@jsafrane final pr is ready, (rebased and the first 2 commits disappeared : )) )

@jsafrane
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 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 Aug 29, 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 Aug 29, 2017
@wongma7
Copy link
Contributor Author

wongma7 commented Aug 29, 2017

rebased, please re-add lgtm @jsafrane tytyty

@jsafrane
Copy link
Member

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, thockin, wongma7

Associated issue: 168

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

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

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 583c4a4 into kubernetes:master Aug 30, 2017
@wongma7 wongma7 mentioned this pull request Aug 30, 2017
Merged
dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018
Merge pull request kubernetes#51228 from wongma7/mount-options-sc
fef0e7e 
Automatic merge from submit-queue

Add storageClass.mountOptions and use it in all applicable plugins

split off from 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.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnufied gnufied gnufied left review comments

Assignees

@jsafrane jsafrane

@caesarxuchao caesarxuchao

@thockin thockin

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. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@wongma7 @k8s-ci-robot @thockin @jsafrane @k8s-github-robot @gnufied @caesarxuchao