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

Don't provision for PVCs with AccessModes unsupported by plugin #47274

Merged
merged 1 commit into from
Jun 13, 2017
Merged

Don't provision for PVCs with AccessModes unsupported by plugin #47274

merged 1 commit into from
Jun 13, 2017

Conversation

wongma7
Copy link
Contributor

@wongma7 wongma7 commented Jun 9, 2017 •
edited
Loading

Fail early in case the user actually expects e.g. RWM from AWS when in reality that isn't possible.
@eparis @gnufied

edit: this needs release note because it's a breaking bugfix; will write one.

#46540

Fix dynamic provisioning of PVs with inaccurate AccessModes by refusing to provision when PVCs ask for AccessModes that can't be satisfied by the PVs' underlying volume plugin

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 9, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jun 9, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @wongma7. 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.

I understand the commands that are listed here.

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.

@k8s-ci-robot 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 Jun 9, 2017
@wongma7 wongma7 changed the title Don't provision for PVCs with AccessModes unsupported by plugin WIP: Don't provision for PVCs with AccessModes unsupported by plugin Jun 9, 2017
@gnufied
Copy link
Member

gnufied commented Jun 9, 2017

@k8s-bot ok to test

@k8s-ci-robot 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 Jun 9, 2017
@wongma7 wongma7 changed the title WIP: Don't provision for PVCs with AccessModes unsupported by plugin Don't provision for PVCs with AccessModes unsupported by plugin Jun 9, 2017
@wongma7
Copy link
Contributor Author

wongma7 commented Jun 9, 2017

@k8s-bot pull-kubernetes-bazel test this

W0609 22:30:16.201] ERROR: /workspace/k8s.io/kubernetes/pkg/controller/volume/persistentvolume/BUILD:55:1: null failed: filter_exec_bin failed: error executing command

seems relevant but hack/update-bazel.sh did nothing?

@wongma7
Copy link
Contributor Author

wongma7 commented Jun 12, 2017

/release-note

@k8s-ci-robot k8s-ci-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 Jun 12, 2017
@wongma7
Copy link
Contributor Author

wongma7 commented Jun 12, 2017

@kubernetes/sig-storage-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jun 12, 2017
@rootfs
Copy link
Contributor

rootfs commented Jun 12, 2017

also add to storage os https://github.com/kubernetes/kubernetes/tree/master/pkg/volume/storageos
@wongma7

@k8s-github-robot k8s-github-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 Jun 12, 2017
@wongma7 wongma7 mentioned this pull request Jun 12, 2017
Closed
@wongma7
Copy link
Contributor Author

wongma7 commented Jun 12, 2017

/retest

#45706

@wongma7
Copy link
Contributor Author

wongma7 commented Jun 12, 2017

/retest

@wongma7
Copy link
Contributor Author

wongma7 commented Jun 12, 2017

#47362

@mtanino
Copy link

mtanino commented Jun 12, 2017

@wongma7 @rootfs
How about hostpath volume plugin? This also has provision function and handles access mode.

@wongma7
Copy link
Contributor Author

wongma7 commented Jun 12, 2017

I intentionally left that out, it's only for testing purposes and so we should allow for devs to make "don't care" PVCs with dummy AccessModes

@mtanino
Copy link

mtanino commented Jun 12, 2017

@wongma7
Ok. I got your intention.

@mtanino
Copy link

mtanino commented Jun 13, 2017

/retest

@gnufied
Copy link
Member

gnufied commented Jun 13, 2017

@wongma7 didn't we plan to introduce a field called AccessMode in StorageClass too? Why did we drop that idea?

@wongma7
Copy link
Contributor Author

wongma7 commented Jun 13, 2017

@gnufied we (I) haven't yet given up on that; this is the quick & easy bug fix but long-term we imo need to add that and/or create-time validation

@jsafrane
Copy link
Member

/assign

@jsafrane
Copy link
Member

/approve the change in general
Is it something we want in 1.7?

@jsafrane
Copy link
Member

It seems we need 1.7 issue to merge it there.

@eparis
Copy link
Contributor

eparis commented Jun 13, 2017

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@eparis eparis added this to the v1.7 milestone Jun 13, 2017
@eparis
Copy link
Contributor

eparis commented Jun 13, 2017

@thockin I'd vote for 1.7. This could actually break someone. If they have a PVC they copy/paste in or something. I'd rather fix it sooner than leave the broken behaviour for later.

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 13, 2017
@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 Jun 13, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 46540

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

Automatic merge from submit-queue (batch tested with PRs 46929, 47391, 47399, 47428, 47274)

@k8s-github-robot k8s-github-robot merged commit 38837b0 into kubernetes:master Jun 13, 2017
@wongma7 wongma7 mentioned this pull request Jun 16, 2017
Merged
@wongma7 wongma7 mentioned this pull request Jun 28, 2017
Closed
dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018
Merge pull request kubernetes#47274 from wongma7/accessmodes-provision
9fc70f2 
Automatic merge from submit-queue (batch tested with PRs 46929, 47391, 47399, 47428, 47274)

Don't provision for PVCs with AccessModes unsupported by plugin

Fail early in case the user actually expects e.g. RWM from AWS when in reality that isn't possible.
@eparis @gnufied 

edit: this needs release note because it's a breaking bugfix; will write one.

kubernetes#46540
```release-note
Fix dynamic provisioning of PVs with inaccurate AccessModes by refusing to provision when PVCs ask for AccessModes that can't be satisfied by the PVs' underlying volume plugin
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pmorie pmorie

@jsafrane jsafrane

@bprashanth bprashanth

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. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.
10 participants
@wongma7 @k8s-ci-robot @gnufied @rootfs @mtanino @jsafrane @eparis @k8s-github-robot @pmorie @bprashanth