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.

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

Revise StorageClass docs #42551

Merged
merged 3 commits into from
Dec 21, 2023

Conversation

sftim
Copy link
Contributor

@sftim sftim commented Aug 15, 2023

Update https://kubernetes.io/docs/concepts/storage/storage-classes/ in light of v1.28 changes and for general tidying.

Also, swap an example manifest to use code shortcode.

### Default StorageClass
{{% code file="storage/storageclass-low-latency.yaml" %}}

## Default StorageClass
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The defaulting behavior is now generally available, so I thought it was worth mentioning here.


For instructions on setting the default StorageClass, see
[Change the default StorageClass](/docs/tasks/administer-cluster/change-default-storage-class/).
Note that certain cloud providers may already define a default StorageClass.

### Provisioner
## Provisioner
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The table in this section is now in alphabetical order.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 15, 2023
@k8s-ci-robot k8s-ci-robot requested a review from bradtopol August 15, 2023 20:28
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Aug 15, 2023
@k8s-ci-robot k8s-ci-robot requested a review from thockin August 15, 2023 20:28
@k8s-ci-robot k8s-ci-robot added sig/docs Categorizes an issue or PR as relevant to SIG Docs. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 15, 2023
@sftim sftim force-pushed the 20230815_revise_storageclass_docs branch from 5adcaa0 to 1cd6d80 Compare August 15, 2023 21:16
@netlify
Copy link

netlify bot commented Aug 15, 2023

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit ddb1892
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/6582f0b4e1cbb60008c3a068
😎 Deploy Preview https://deploy-preview-42551--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@sftim
Copy link
Contributor Author

sftim commented Aug 15, 2023

Relevant to #42176

@sftim sftim force-pushed the 20230815_revise_storageclass_docs branch from 1cd6d80 to 1805ae1 Compare August 15, 2023 21:35
@sftim sftim force-pushed the 20230815_revise_storageclass_docs branch from 1805ae1 to bbb5da4 Compare September 7, 2023 13:02
@kbhawkey
Copy link
Contributor

@kbhawkey
Copy link
Contributor

@kubernetes/sig-storage-misc

@k8s-ci-robot k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Sep 13, 2023
@@ -172,23 +193,21 @@ and [taints and tolerations](/docs/concepts/scheduling-eviction/taint-and-tolera

The following plugins support `WaitForFirstConsumer` with dynamic provisioning:

- CSI volumes, provided that the specific CSI driver supports this
Copy link
Member

Choose a reason for hiding this comment

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

This list is woefully out of date. Maybe best to remove GCEPersistentDisk and just mention CSI (and Local below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, we can get a current list of in-tree providers that do provide that support. If it's in-tree, we try to document it somewhere. Even if it's also deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msau42 is there a way we can get SIG Storage to provide a list of storage plugins with that support? If not, I'll drop the mention.

Copy link
Member

Choose a reason for hiding this comment

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

Off the top of my head, we have local, gce-pd, aws-ebs, azure-disk, azure-file. Maybe vsphere and ceph? @jsafrane can you confirm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The awsElasticBlockStore and azureDisk volume types are not included in Kubernetes v1.28 - is that right?

Copy link
Member

Choose a reason for hiding this comment

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

In 1.29, all cloud-based volume plugins are migrated to CSI, so just local remains in-tree and supports WaitForFirstConsumer. So CSI and local is more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bear in mind: these are the docs for v1.28

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO it'd be nice to get this page updated ahead of the v1.29 release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(v1.29 is now released, and I've rebased)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any feature gate that lets you re-enable in tree support for azure file? I think we may have some work to do, as a project, to make sure the docs are up to date around those removals.

@sftim sftim force-pushed the 20230815_revise_storageclass_docs branch from bbb5da4 to df1e395 Compare October 24, 2023 09:16
@sftim sftim force-pushed the 20230815_revise_storageclass_docs branch from df1e395 to 39cc206 Compare November 6, 2023 12:56
{{< /note >}}

You can create a PersistentVolumeClaim without specifying a `storageClassName`
for the new PVC, and you can do so even when no default StorageClass exists
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for the new PVC, and you can do so even when no default StorageClass exists
, and you can do so even when no default StorageClass exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer the original - what's not right with it?

content/en/docs/concepts/storage/storage-classes.md Outdated Show resolved Hide resolved
existing PVCs without `storageClassName`. For the PVCs that either have an empty
value for `storageClassName` or do not have this key, the control plane then
updates those PVCs to set `storageClassName` to match the new default StorageClass.
If you have an existing PVC where the `storageClassName` is `""`, and you configure
Copy link
Contributor

Choose a reason for hiding this comment

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

This statement is inconsistent with the previous one.
If storageClassName: "" is specified, a new default StorageClass is then added, what will happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(this text is copied from https://kubernetes.io/docs/concepts/storage/persistent-volumes/#retroactive-default-storageclass-assignment; if that text is wrong, we should fix both occurrences).

Copy link
Member

Choose a reason for hiding this comment

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

To me, who knows how default storage classes work, the PR text looks correct.

However, it would be great to distinguish between "empty value" (the filed is missing or nil) and empty string "" more explicitly. How it's done in other API fields where nil and "" means something different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the API it's actually null (JSON does not have a nil value, and we usually represent API details as JSON - much more readable than protobuf, IMO). We don't have a strong convention though. "" is something most people would think means empty string and not “field unset”.

@sftim sftim marked this pull request as draft November 13, 2023 16:58
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2023
@sftim sftim force-pushed the 20230815_revise_storageclass_docs branch 2 times, most recently from 0a225c2 to c4e1c77 Compare November 22, 2023 21:40
@sftim sftim marked this pull request as ready for review November 22, 2023 21:42
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 22, 2023
@sftim
Copy link
Contributor Author

sftim commented Dec 7, 2023

/hold

Avoid provoking that merge conflict. I can backport this to release-1.28 if this merges into main after the 1.29 (or 1.30, etc) release.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 7, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2023
@sftim sftim force-pushed the 20230815_revise_storageclass_docs branch from 25c0c2a to ecf28d0 Compare December 16, 2023 13:53
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2023
@sftim
Copy link
Contributor Author

sftim commented Dec 16, 2023

/hold cancel

v1.29 is released

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2023
@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 Dec 19, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 207fb7495527ec693f9f3284baa180c6109b393c

@kbhawkey
Copy link
Contributor

Hello, @sftim
Are any comments or suggestions (including nits and other content changes) that must be addressed before merging this pull request?
Thanks

@sftim
Copy link
Contributor Author

sftim commented Dec 20, 2023

Are any comments or suggestions (including nits and other content changes) that must be addressed before merging this pull request? Thanks

There are no nits that must be addressed; nits are feedback that don't block a merge (at least, that's how the Kubernetes project uses the term). I can see #42551 (comment) as open, but my opinion is that there's nothing to do.

Ultimately, I'm the author here and it's not for me to judge the approval. It's a change to the live site. However, I would recommend approving it.

I can see that a rebase is needed but Prow hasn't yet noticed. One forced push coming right up.

@sftim sftim force-pushed the 20230815_revise_storageclass_docs branch from ecf28d0 to cc148df Compare December 20, 2023 13:43
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 20, 2023
Comment on lines +146 to +151
| :------------------- | :----------------------------------------------- |
| Azure File | 1.11 |
| CSI | 1.24 |
| FlexVolume | 1.13 |
| Portworx | 1.11 |
| rbd | 1.11 |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This table is now in alphabetical order.

sftim and others added 3 commits December 20, 2023 13:48
Co-authored-by: Qiming Teng <tengqm@outlook.com>
Co-authored-by: Shannon Kularathna <ax3shannonkularathna@gmail.com>
Use sentence case for headings.
@sftim sftim force-pushed the 20230815_revise_storageclass_docs branch from cc148df to ddb1892 Compare December 20, 2023 13:48
@kbhawkey
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kbhawkey

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

The pull request process is described here

Needs approval from an approver in each of these files:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 21, 2023
@kbhawkey
Copy link
Contributor

This pull request has a technical review and LGTM.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 21, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: c0b0cccfb22a6371128ecab82fa71b4f50236342

@k8s-ci-robot k8s-ci-robot merged commit 98da395 into kubernetes:main Dec 21, 2023
6 checks passed
@sftim sftim deleted the 20230815_revise_storageclass_docs branch February 16, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. 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
Development

Successfully merging this pull request may close these issues.

7 participants