Validate PVC requested AccessModes against StorageClass's underlying plugin's supported AccessModes #46540
Comments
k8s-ci-robot
added
the
sig/storage
|
@wongma7: Reiterating the mentions to trigger a notification: In response to this:
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. |
|
Also sorry in advance if this discussion already happened somewhere, couldn't find it. It's somewhat similar to the fstype stuff discussed today. (And IMO orthogonal to the discussion about the "actual meaning of AccessModes wrt scheduling, mount vs. attach; I am talking purely about user expectations, i.e. what is already documented per plugin.) |
|
also ping @jsafrane |
|
There is some related discussion about AccessModes here- kubernetes-retired/external-storage#110 (comment) |
|
I think it is doable to perform this validation, if we introduce AccessMode as a field on StorageClass, but general validation of PVCs against what volume plugin actually supports, isn't backward compatible. For example - user may rely on this behaviour to use 2 pods with same PVCs (as long as both pods are scheduled on same node). |
|
Yeah I mean preventing administrators from creating EBS RWM PVs might be overreaching. But in our position acting as a PV-creating administrator, we should provide the best UX possible. Even if that means breaking people who are currently aware they can treat the PVC AccessModes as a "don't care" when using dynamic provisioning. Because to be clear, introducing new validation obviously does break backward compatibility, just not to that extent you're saying The issue of enforcing AccessModes at mount/attach time is I think being worked on elsewhere and IMO while it's difficult not to conflate that issue with this issue, we can safely ignore it while we work on this if we just say the docs are 100% correct. Like this: EBS!=RWM, and so users, whatever their understand of RWO vs RWM, definitely don't want EBS when they ask for RWM. So let's error out early instead of giving them a bad PV. |
|
Generally we do not strengthen validation for any reason other than security, even if we think it's a better experience. The risk of breaking existing clusters is too high |
|
What if we made the validation opt-in by making this hypothetical StorageClass AccessModes field optional? kube won't automatically set it, admins can weigh the risk and set it themselves. If empty, do nothing & maintain current behavior; if filled in, validate PVCs against it. |
|
Generally yes, that works. We wouldn't be able to automatically
populate that field except on new installations, but for new
installations that might be ok
|
|
I think that adding SupportedAcessModes to StorageClass could work. |
|
Implementation question: validation code would need to I don't suppose there's any other example of validation where one object's validity depends on another? |
|
It isn't. The dynamic provisioner should write status to an invalid
pvc. Not at pvc create time.
|
|
Yes that's the only way provisioners can bubble up errors now (leave PVC pending and write an e.g. 'ProvisioningFailed' event on the PVC), but I have to consider the possibility of introducing create time validation Because it would be nice to have + because it was the original RFE behind this issue... and also because we have other features that might benefit from getting this pattern done: e.g. resize: StorageClasses could maybe have a 'Resizable' field just like 'SupportedAccessModes', and then PVCs could be validated against that if they ask for a resizable PV. @gnufied that sound right? This issue has a lot of overlap with the unresolved discussion about combining class + selector. It also has a lot of overlap with discussions over people's (my, at least) UX distaste/confusion over the 'my PVC is stuck in pending and I don't know why' situation. All this to say, I'd personally like create time validation of some kind. Something better than updating the PVC's events. Anyway, we have an entire release cycle to figure this out so I'll try to bring this up in storage sig and solicit more opinions on whether it's worth thinking about. |
|
Anything that can reasonably change over the lifetime must be represented
in status (pod state). Anything that never should change over lifetime can
be done at create (service ip). Dynamic provisioning is in a weird spot in
the middle, in that
A) there can be multiple provisioners competing for a class (although not
always)
B) a storage class can change over the lifetime of a cluster, although at a
rate slower than the lifetime of many PVCs
If we are going to strongly enforce use of storage class, we can more
strongly enforce create time. If we are going to weakly enforce storage
classes, create time should also be weak. Initializers may actually help
sort this problem out for pvc / dynamic provisioning in general, in that we
can let the provisioner init *and* validate pvc
On Jun 2, 2017, at 1:19 PM, Matthew Wong <notifications@github.com> wrote:
Yes that's the only way provisioners can bubble up errors now (leave PVC
pending and write an e.g. 'ProvisioningFailed' event on the PVC), but I
have to consider the possibility of introducing create time validation
Because it would be nice to have + because it was the original RFE behind
this issue... and also because we have other features that might benefit
from getting this pattern done: e.g. resize: StorageClasses could maybe
have a 'Resizable' field just like 'SupportedAccessModes', and then PVCs
could be validated against that if they ask for a resizable PV. @gnufied
<https://github.com/gnufied> that sound right?
This issue has a lot of overlap with the unresolved discussion about
combining class + selector.
It also has a lot of overlap with discussions over people's (my, at least)
UX distaste/confusion over the 'my PVC is stuck in pending and I don't know
why' situation.
All this to say, I'd personally like create time validation of some kind.
Something better than updating the PVC's events. Anyway, we have an entire
release cycle to figure this out so I'll try to bring this up in storage
sig and solicit more opinions on whether it's worth thinking about.
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#46540 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_pz1zy8yvdjk7PxGHbk3NRfRUrPx_ks5sAEQBgaJpZM4NoCwg>
.
|
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. #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 ```
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/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 ```
|
Issues go stale after 90d of inactivity. Prevent issues from auto-closing with an If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or |
k8s-ci-robot
added
the
lifecycle/stale
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/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 ```
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or |
k8s-ci-robot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
Currently, when a user creates a PVC asking for a certain StorageClass, and dynamic provisioning gets invoked, the AccessModes field is effectively ignored: the provisioned PV is created to have AccessModes matching the PVC's regardless of whether the underlying plugin actually supports it.
e.g. in an AWS cluster a user creates a PVC asking for a "standard" class aws-ebs volume & RWM AccessModes. They'll get a RWM PV and, if/when the multiple pods attempting to use it fail (i.e. the vol fails to attach to multiple nodes), they'll wonder what went wrong and may have no way of understanding what went wrong.
There is no way for a StorageClass (admin) to communicate to users that it only supports creating PVs with lesser AccessModes than what the user may need.
Mount options set something of a precedent by validating at PV creation time whether the specified plugin actually supports mount options. IMO we should do something similar here, validate at PVC creation time whether the specified class's plugin actually supports the requested AccessModes according to the documentation here: https://kubernetes.io/docs/concepts/storage/persistent-volumes/#access-modes.
We should also (automatically) put plugin's supported AccessModes in every StorageClass so that users can determine which StorageClasses support what AccessModes.
Thoughts please?
@kubernetes/sig-storage-feature-requests @kubernetes/sig-storage-bugs @gnufied
The text was updated successfully, but these errors were encountered: