fix inconsistency in documentation of default storageclass #48200

iheartNathan · 2024-10-04T22:45:31Z

Description

fixes this issue

Issue

Closes: #42288

linux-foundation-easycla · 2024-10-04T22:45:34Z

The committers listed above are authorized under a signed CLA.

✅ login: iheartNathan (55edcdc)

k8s-ci-robot · 2024-10-04T22:45:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign natalisucks for approval. For more information see the Kubernetes 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:

content/en/docs/OWNERS

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

k8s-ci-robot · 2024-10-04T22:45:39Z

Welcome @iheartNathan!

It looks like this is your first PR to kubernetes/website 🎉. 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/website 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. 😃

netlify · 2024-10-04T22:55:03Z

✅ Pull request preview available for checking

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`55edcdc`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/670bd1f7c6132d0008070efd
😎 Deploy Preview	https://deploy-preview-48200--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

tengqm · 2024-10-04T23:22:52Z

content/en/docs/reference/access-authn-authz/admission-controllers.md

-must revisit their `StorageClass` objects and mark only one as default.
+This admission controller does nothing when no default `StorageClass` exists. When more than one storage
+class is marked as default, and you then create a PersistentVolumeClaim with no storageClassName set, 
+Kubernetes uses the most recently created default StorageClass.


I am not sure this revision is correct.

This is the behaviour I experienced when I tested on a cluster.
This is expected behaviour mentioned here - https://kubernetes.io/docs/concepts/storage/persistent-volumes/#class-1

By design, storage classes are referenced by a PVC which is dynamically provisioned. If a PVC is not dynamically provisioned, a user don't need to care about storageClassName, right?

When documenting a feature, please make sure you understand the use case(s) and what
you write here will not cause further confusion.

I don't think a PVC is what's dynamically provisioned, what's dynamically provisioned is a PV from a storageclass by a PVC.
A storageclass may or may not be referenced by a PVC, if it does reference a storageclass a PV is created from the referenced storageclass, if it doesn't it uses a default storageclass to dynamically provision the PV.
If there are more than one default storageclass , and there is no storageclass referenced by the PVC, the PVC uses the more recently created default stroageclass to dynamically provision the PV (this is made possible by the mutating admission controller - DefaultStorageClass)
This is also documented here - https://kubernetes.io/docs/concepts/storage/storage-classes/#default-storageclass

A storageclass may or may not be referenced by a PVC, if it does reference a storageclass a PV is created from the referenced storageclass, if it doesn't it uses a default storageclass to dynamically provision the PV.

The point here that not all PV are dynamically provisioned. A PVC which does not reference any StorageClass still has to be dynamically provisioned? I don't believe so.

I agree with #48200 (comment)

The change looks correct to me @tengqm

This change is partially correct. We have to scope the PVs to those that are dynamically provisioned.

sftim · 2024-10-07T16:40:48Z

content/en/docs/reference/access-authn-authz/admission-controllers.md

-class is marked as default, it rejects any creation of `PersistentVolumeClaim` with an error and an administrator
-must revisit their `StorageClass` objects and mark only one as default.
+This admission controller does nothing when no default `StorageClass` exists. When more than one storage
+class is marked as default, and you then create a PersistentVolumeClaim with no storageClassName set, 


(nit)

Suggested change

class is marked as default, and you then create a PersistentVolumeClaim with no storageClassName set,

class is marked as default, and you then create a PersistentVolumeClaim with no `storageClassName` set,

Does the defaulting at admission time happen even if volumeName is set? If not, maybe we should mention that.

What I experienced - the admission controller assigned the most recent class to the PVC, but the PVC was in a pending status until a volume with a name matching the volumeName on the PVC and the storageClassName on both the PV and PVC were matching was statically provisioned.

Consider this...
I have a PV statically provisioned, and I know its name. Now I'm creating a PVC to bind to that PV. In this PVC, I'm not specifying a storageClassName but I'm setting the volumeName to express my intent. This is a pretty valid (and a common) use case.
Now, re-read your revision. Does it still look correct?

@sftim, @tengqm - #48200 (comment) has been included in the revision.

tengqm · 2024-10-14T00:09:34Z

content/en/docs/reference/access-authn-authz/admission-controllers.md

+class is marked as default, and you then create a `PersistentVolumeClaim` with no `storageClassName` set, 
+Kubernetes uses the most recently created default `StorageClass`.
+When a `PersistentVolumeClaim` is created with a specified `volumeName`, it remains in a pending state 
+if the static volume's `storageClassName` does not match the `StorageClass` on the `PersistentVolumeClaim`.


What if I'm creating a PVC with 'volumeName' specified, but no 'storageClassName'?
The PVC still has to be checked for storage class matching?

Yes, the PVC still checks for matching storage class, the below show the events on a PVC with a specified volumeName that has been mutated by the DefaultStroageClass admission controller.

controlplane $ k describe pvc pvc-nginx Name: pvc-nginx Namespace: default StorageClass: nfs-csi Status: Pending Volume: pv-nginx Labels: <none> Annotations: <none> Finalizers: [kubernetes.io/pvc-protection] Capacity: 0 Access Modes: VolumeMode: Filesystem Used By: nginx-nfs-example Events: Type Reason Age From Message ---- ------ ---- ---- ------- Warning VolumeMismatch 3s (x3 over 23s) persistentvolume-controller Cannot bind to requested volume "pv-nginx": storageClassName does not match

I mean ... remove the 'storageClassName' from the PVC, setting only 'volumeName'.
Will this binding succeed? I guess so.
The admission controller has nothing to check for this PVC, right?

The admission controller may backfill the storage class for the PVC when the binding is a success. But that is a different topic. The PVC will not be pending for storage class matching.

I can't remove the storageClass from the PVC, setting only volumeName, the spec is immutable after creation.
As long as here is at least one storage class setup as default, here is also no way to create a PVC without a storageClass, the admission controller is always on the look out for PVC without one specified to mutate it.

I can't remove the storageClass from the PVC, setting only volumeName, the spec is immutable after creation.

Immutable should be fine.

As long as here is at least one storage class setup as default, here is also no way to create a PVC without a storageClass, the admission controller is always on the look out for PVC without one specified to mutate it.

This is a surprise for me. Maybe we need to consult SIG Storage for this.

We discussed this at sig storage triage today. The intended behavior is that if you set storageClassName to nil and there is a default storageclass set, then storageClassName will be set with the default storageclass.

If you specifically don't want a default storageclass to be set, then you need to set storageClassName to empty string.

There is a bug with the "retroactive default storageclass" feature though where it's not following this intended behavior, but I don't think we're discussing the retroactive scenario here (ie default storageclass gets configured after the PVC is created)

The question is this, @msau42:

If I am creating a PVC in a cluster, and the cluster has a default storage class.
I'm not specifying storageClassName in my PVC spec, and I'm only specifying volumeName, can the PVC be created?

Right. I can understand that the PVC may and may not receive a storageClassName after creation, but that is a different topic.

k8s-ci-robot added the language/en Issues or PRs related to English language label Oct 4, 2024

k8s-ci-robot requested review from derekwaynecarr and lavalamp October 4, 2024 22:45

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 4, 2024

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 4, 2024

tengqm reviewed Oct 4, 2024

View reviewed changes

sftim reviewed Oct 7, 2024

View reviewed changes

iheartNathan force-pushed the 42288-Inconsistent-documentation-of-default-StorageClass branch from 8dbccc7 to ee39a3a Compare October 7, 2024 20:37

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 7, 2024

iheartNathan requested a review from sftim October 7, 2024 23:06

fix inconsistency in documentation of default storageclass

55edcdc

iheartNathan force-pushed the 42288-Inconsistent-documentation-of-default-StorageClass branch from ee39a3a to 55edcdc Compare October 13, 2024 13:58

iheartNathan requested a review from tengqm October 13, 2024 14:34

tengqm reviewed Oct 14, 2024

View reviewed changes

carlory mentioned this pull request Oct 14, 2024

Don't assign a storage class to claim when the user asked for a specific volume. kubernetes/kubernetes#128048

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix inconsistency in documentation of default storageclass #48200

fix inconsistency in documentation of default storageclass #48200

iheartNathan commented Oct 4, 2024

linux-foundation-easycla bot commented Oct 4, 2024 •

edited

Loading

k8s-ci-robot commented Oct 4, 2024

k8s-ci-robot commented Oct 4, 2024

netlify bot commented Oct 4, 2024 •

edited

Loading

tengqm Oct 4, 2024

iheartNathan Oct 4, 2024

tengqm Oct 5, 2024

iheartNathan Oct 5, 2024

tengqm Oct 5, 2024

sftim Oct 7, 2024

tengqm Oct 7, 2024

sftim Oct 7, 2024

sftim Oct 7, 2024

iheartNathan Oct 7, 2024

tengqm Oct 7, 2024

iheartNathan Oct 13, 2024

tengqm Oct 14, 2024

iheartNathan Oct 14, 2024

tengqm Oct 14, 2024

tengqm Oct 14, 2024

iheartNathan Oct 14, 2024 •

edited

Loading

tengqm Oct 14, 2024

msau42 Oct 16, 2024

tengqm Oct 16, 2024 •

edited

Loading

	class is marked as default, and you then create a PersistentVolumeClaim with no storageClassName set,
	class is marked as default, and you then create a PersistentVolumeClaim with no `storageClassName` set,

fix inconsistency in documentation of default storageclass #48200

Are you sure you want to change the base?

fix inconsistency in documentation of default storageclass #48200

Conversation

iheartNathan commented Oct 4, 2024

Description

Issue

linux-foundation-easycla bot commented Oct 4, 2024 • edited Loading

k8s-ci-robot commented Oct 4, 2024

k8s-ci-robot commented Oct 4, 2024

netlify bot commented Oct 4, 2024 • edited Loading

✅ Pull request preview available for checking

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iheartNathan Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tengqm Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Oct 4, 2024 •

edited

Loading

netlify bot commented Oct 4, 2024 •

edited

Loading

iheartNathan Oct 14, 2024 •

edited

Loading

tengqm Oct 16, 2024 •

edited

Loading