KEP-5489: Enable defining a default StorageClass for each specific volume access mode

[savirg]

• One-line PR description: Enable defining a default StorageClass for each specific volume access mode

• Issue link: Enable defining a default StorageClass for each specific volume access mode #5489

• Other comments:

[k8s-ci-robot]

Welcome @savirg!

It looks like this is your first PR to kubernetes/enhancements 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/enhancements has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @savirg. 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 /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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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-sigs/prow repository.

[msau42]

/ok-to-test

[msau42]

/assign

[msau42]

/retest

[msau42]

The challenge with e2e tests is different environments won't have the supported storage providers to test the full provisioning path. And changing the default storageclasses may not be allowed in some environments. I think for e2e tests, we need to leave it to the cluster provider to install appropriate storageclasses, and in the tests, if the default storageclasses are set, then we trigger the tests.

[msau42]

This approach is actually a bit more complex. In order to choose which storageClassName to set, we typically have to iterate over all StorageClasses to find the ones with the right annotations and also consider all the priority rules that you outlined earlier. CEL cannot do this kind of logic, so we would need to implement a controller that evaluates the PVC and the StorageClasses, and then associate a ConfigMap to the PVC with the final result.

[sunnylovestiramisu]

What happens if:

1. Multiple StorageClasses are set as the default for the same access mode?
2. If no default is set for a given access mode, does it fail back to the existing default storage class?

[savirg]

1. If multiple StorageClass objects are annotated as the default for the same access mode, the system will select the one with the most recent metadata.creationTimestamp as the default for that mode.
2. If no default is set for a given access mode, the system falls back to the existing global default StorageClass (the one annotated with storageclass.kubernetes.io/is-default-class: "true"). If neither a mode-specific nor a global default exists, PVC provisioning will fail.

[sunnylovestiramisu]

Users now need to understand not only which StorageClasses exist, but also which are defaults for which access modes. This adds cognitive load and increases the chance of using the "wrong" storage unintentionally. Users may inadvertently consume more expensive storage if the default changes, impacting cost controls.

Cluster admins must ensure that defaults are kept up to date and that only one default exists per access mode at any time. Changing defaults could cause a race where, briefly, no default exists, or more than one default exists.

Documentation and tooling must be updated to surface this mapping clearly(update k8s public documentation etc.)

[savirg]

/assign @deads2k

[Madhu-1]

Is this only for the volume access mode, any plan to include the same with combination with volume Mode as well? fileSystem+RWX select one storageclass and blockMode+RWX another storageclass?

[sunnylovestiramisu]

Hmm that is a feature request. But

• volumeMode defines how a volume is consumed by a pod, either as a pre-formatted filesystem or as a raw block device.
• accessMode (e.g., ReadWriteOnce, ReadOnlyMany, ReadWriteMany) dictates the access policy, defining how many pods can access a volume and from how many nodes.

Keeping them separate will reduce the complexity in implementation and less confusion for users about which StorageClass is used. Also the logic for how to select a StorageClass based on a complex set of custom rules is a perfect example of a workflow. Different organizations have vastly different needs, how do you suggest us to address the rules or even orders for all the providers?

A better approach to address this use case is probably using a custom mutating controller can modify the object in the request.

[Madhu-1]

Hmm that is a feature request. But

• volumeMode defines how a volume is consumed by a pod, either as a pre-formatted filesystem or as a raw block device.
• accessMode (e.g., ReadWriteOnce, ReadOnlyMany, ReadWriteMany) dictates the access policy, defining how many pods can access a volume and from how many nodes.

Keeping them separate will reduce the complexity in implementation and less confusion for users about which StorageClass is used. Also the logic for how to select a StorageClass based on a complex set of custom rules is a perfect example of a workflow. Different organizations have vastly different needs, how do you suggest us to address the rules or even orders for all the providers?

A better approach to address this use case is probably using a custom mutating controller can modify the object in the request.

@sunnylovestiramisu I understand that this is a new feature request, and I was exploring whether it's possible to target this scenario. However, I also recognize the complexity it could introduce. Currently, the annotation in this PR is kept fairly generic, so extending it further might be challenging unless we consider making the annotation key more specific to the accessMode.

As mentioned in the earlier use case, a similar one could involve using RWX volumes for things like registry/logging (NFS), or for virtual machines (block storage). In these scenarios, both volumeMode and accessMode become important factors to consider together.

[kannon92]

I mentioned this on slack but annotations are usually discouraged as supported apis.

Why not make this an actual API field?

[sunnylovestiramisu]

Can we add an alternatives session of:

1. Standard Field - e.g. adding a field like defaultForAccessMode: ReadWriteOnce to a StorageClass
2. Priority + Field Selector - instead of a simple field, you assign priorities to resources and use field selectors to pick the “highest-priority” matching resource. e.g. StorageClasses could have a priority and the controller can pick the highest priority
3. Priority + Field Selector + Namespace Selector - same as 2 but it also considers namespaces

[savirg]

Done, added these in the alternatives section.

[sunnylovestiramisu]

We discussed it at the sig-storage meeting today, please see this note.

Using a standard field will increase the complexity if we account for backward compatibility(need to support it in the existing default storage class as well, and we need to support annotation approach forever anyway). And at the community meeting, everyone is okay with the annotation approach.

For future plans like if we will do storage API v2, we can revisit the standard field approach.

[thockin]

It seems like the main reason NOT to use an MAP is ordering WRT the built-in admission, is that right? Is there another reason?

Could we add AccessModes []core.PersistentVolumeAccessMode to StorageClass and then choose a default based on the existing annotation? After writing this out, I guess the problem is if you have two block-ish classes which both support RO, RWO, and RWOP,and each wants to be the default for a different set of modes, that's ambiguous.

[thockin]

MAP gives the cluster owner ALL the power to do arbitrary default assignment, as long as the built-in admission control doesn't set a value before MAP gets there (yet another problem with built-in things vs. extensions). It seems like the almost-perfect solution. As long as you don't elect one of the StorageClass objects as the default, the built-in is a no-op, so you can set it based on any criteria you want, right?

What am I missing?

[sunnylovestiramisu]

What is the migration story for most service providers that actually have default storage class set on the cluster? Also how can MAP list all the available storage classes so that we can pick the right one(instead of hard code with a known name)?

[thockin]

What is the migration story for most service providers that actually have default storage class set

Take it off? It's just an annotation.

how can MAP list all the available storage classes so that we can pick the right one(instead of hard code with a known name)?

Maybe I don't follow - how is this different that hard-coding one with a known name and sticking an annotation on it? I thinking of an expression like:

has(spec.accessModes) && 
    spec.accessModes[0] == "ReadWriteOnce" ? "blockish-class" :
    spec.accessModes[0] == "ReadWriteMany" ? "nfsish-class" :
    ""

[sunnylovestiramisu]

What if customer wants to change the default storage class based on access modes? Because if GKE as a service is specifying this in an MAP, the customer will need to delete that default storage class in the GKE cluster and recreate one with the same name?

Currently at least for the annotation, they can change the annotation to a new storage class they prefer?

[thockin]

if GKE as a service is specifying this in an MAP, the customer will need to delete that default storage class in the GKE cluster and recreate one with the same name?

I'm not following what you mean. If the cluster provider (GKE) provides the MAP, either they have to let the user take ownership of it (including deleting it!) or they have to assert that users can't do that (e.g. by recreating or re-setting it). Is it worthwhile to jump on a slack or something?

[kannon92]

PRR shadow:

I didn't see anything outstanding on PRR for alpha.

I still have a question on #5492 (comment) but I don't think that impacts PRR.

/assign @deads2k

[msau42]

Is this only for the volume access mode, any plan to include the same with combination with volume Mode as well? fileSystem+RWX select one storageclass and blockMode+RWX another storageclass?

We discussed this briefly and the current thought is that blockMode is an advanced use case that we wouldn't expect most stateful workloads to use, and the workloads that would use it would have very specific storage requirements and would likely need their own dedicated storageclasses.

[Madhu-1]

would it make sense to call storageclass.kubernetes.io/is-default-class-for-mode as storageclass.kubernetes.io/is-default-class-for-access-mode so that we can extend for volume mode something like storageclass.kubernetes.io/is-default-class-for-volume-mode in future?

[sunnylovestiramisu]

Yeah we should change the naming to is-default-class-for-access-mode to be more specific.

[savirg]

Done, updated the annotation to is-default-class-for-access-mode.

[kannon92]

To be honest, @erictune ran into this with his first gang scheduling proposal.

Annotations are hard to deprecate and we don't provide any protection against mutating annotations.

I'm not as knowledgeable in storage classes but most features that originall propose an annotation or label eventually switch to a proper API as we can better guarantee defaulting and mutating logic around an API field.

cc @thockin

[nixpanic]

Currently the cluster default StorageClass is marked by an annotation already:

apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  annotations:
    storageclass.kubernetes.io/is-default-class: "true"
...

This proposal seems to use the same method to add more options. With the current annotation already well understood by cluster administrators, the new annotation looks simple enough to get adopted.

@savirg, maybe you can mention more clearly that the design is based on the existing storageclass.kubernetes.io/is-default-class annotation?

To me this looks really useful. Our Ceph-CSI users sometimes pick the wrong type of storage because there is little guidance. With this, users need to care less and will likely make fewer mistakes. (Commonly users pick CephFS, an NFS-like type, for workloads that are better suited on RBD, a local-network-disk type. Typically RWX vs RWO.)

[kannon92]

I’ll leave this to api-review and sigs.

Thanks for adding the alternatives section!

[msau42]

We discussed this during the sig-storage meeting on Oct 2, and agreed that the annotation approach is sufficient given that it is an extension of the existing default StorageClass feature which is annotation-based. Changing it to a field would require us to also add support to the existing feature and add additional complexity of backwards compatibility and handling conflicts if both field and annotation are specified, as well as the cognitive overhead for existing users as well.

We agreed that if we do a v2 of the API we can move to a field at that time.

[savirg]

Thanks @nixpanic! I've updated the doc to make it clearer that the design is based on the existing storageclass.kubernetes.io/is-default-class annotation.

[deads2k]

The same care is supposed to happen for annotations.

cc @liggitt

[deads2k]

annotations introduce a unique problem of field-squatting, where the annotations have values, but are not honored and then suddenly on an upgrade become active.

[deads2k]

PRR lgtm, but I'm not really clear on why we'd add more annotation instead of adding fields for what we mean.

[thockin]

WRT adding fields -- the selection of a default isn't really the purvey of the object itself, it's a policy decision imposed upon it. Adding fields would only be reasonable (IMO) if they are really describing the storage and not the policy.

[deads2k]

WRT adding fields -- the selection of a default isn't really the purvey of the object itself, it's a policy decision imposed upon it. Adding fields would only be reasonable (IMO) if they are really describing the storage and not the policy.

The thread above with MAP looks nicer than annotations.

[deads2k]

the PRR is good, so I'll mark that aspect, but MAP sounds like a nicer plan that went beta already.

[deads2k]

PRR approve

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deads2k, savirg
Once this PR has been reviewed and has the lgtm label, please ask for approval from msau42. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• keps/prod-readiness/OWNERS [deads2k]
• keps/sig-storage/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[thockin]

I know there was some work to prove out MAP - can we get an update here?

[savirg]

We are going to explore MAP solution for now. So, won't be merging this enhancement doc.