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

Fixed few issues with Storage concepts section. #16377

Closed
wants to merge 1 commit into from

Conversation

@draghuram
Copy link
Contributor

draghuram commented Sep 16, 2019

  • Removed plural api object names such as PersistentVolumes.

  • Explicitly mentioned alpha status of snapshot APIs.

  • Fixed #16321.

  • Other minor changes.

This PR is created based on feedback in this thread:
https://groups.google.com/forum/#!msg/kubernetes-sig-storage/EBGjZJ-0gGs/0wWhZMgXAgAJ

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Sep 16, 2019

Welcome @draghuram!

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. 😃

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Sep 16, 2019

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Sep 16, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign rajakavitha1
You can assign the PR to them by writing /assign @rajakavitha1 in a comment when ready.

The full list of commands accepted by this bot can be found 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

@netlify

This comment has been minimized.

Copy link

netlify bot commented Sep 16, 2019

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 1bc263d

https://deploy-preview-16377--kubernetes-io-master-staging.netlify.com

@draghuram draghuram force-pushed the draghuram:storagedoc branch from fcb11b6 to 78fcdd7 Sep 16, 2019
@netlify

This comment has been minimized.

Copy link

netlify bot commented Sep 16, 2019

Deploy preview for kubernetes-io-master-staging ready!

Built with commit fcb11b6

https://deploy-preview-16377--kubernetes-io-master-staging.netlify.com

@netlify

This comment has been minimized.

Copy link

netlify bot commented Sep 16, 2019

Deploy preview for kubernetes-io-master-staging ready!

Built with commit fcc10a7

https://deploy-preview-16377--kubernetes-io-master-staging.netlify.com

@draghuram

This comment has been minimized.

Copy link
Contributor Author

draghuram commented Sep 16, 2019

I signed CNCF's CLA so that check should pass now.

@@ -32,7 +32,7 @@ The {{< glossary_tooltip text="CSI" term_id="csi" >}} Volume Cloning feature add

A Clone is defined as a duplicate of an existing Kubernetes Volume that can be consumed as any standard Volume would be. The only difference is that upon provisioning, rather than creating a "new" empty Volume, the back end device creates an exact duplicate of the specified Volume.

The implementation of cloning, from the perspective of the Kubernetes API simply adds the ability to specify an existing unbound PVC as a dataSource during new pvc creation.
The implementation of cloning, from the perspective of the Kubernetes API simply adds the ability to specify an existing bound PVC as a dataSource during new pvc creation.

This comment has been minimized.

Copy link
@jeffvance

jeffvance Sep 16, 2019

should add comma after "API" and before "simply".

This comment has been minimized.

Copy link
@draghuram

draghuram Sep 17, 2019

Author Contributor

Done.

While `VolumeSnapshots` allow a user to consume abstract storage resources, cluster administrators
need to be able to offer a variety of `VolumeSnapshotContents` without exposing
users to the details of how those volume snapshots should be provisioned. For these needs
Cluster administrators need to be able to offer different ways in

This comment has been minimized.

Copy link
@jeffvance

jeffvance Sep 16, 2019

what about : "In order to support storage vendor's diverse needs while hiding the underlying provisioning details, multiple ways of generating snapshots are possible via VolumeSnaphotClasses."

This comment has been minimized.

Copy link
@draghuram

draghuram Sep 17, 2019

Author Contributor

Both versions look ok to me so let us see if there are comments from any others.

This comment has been minimized.

Copy link
@msau42

msau42 Oct 3, 2019

Member

How about "Provider-specific configuration parameters for dynamically taking snapshots are specified in a VolumeSnapshotClass"?


#### Dynamic
When none of the static `VolumeSnapshotContents` the administrator created matches a user's `VolumeSnapshot`,
When none of the static `VolumeSnapshotContent` resources the administrator created matches a user's `VolumeSnapshot`,

This comment has been minimized.

Copy link
@jeffvance

jeffvance Sep 16, 2019

match -- singular

This comment has been minimized.

Copy link
@draghuram

draghuram Sep 17, 2019

Author Contributor

Done.

@jeffvance

This comment has been minimized.

Copy link

jeffvance commented Sep 16, 2019

/lgtm

@xing-yang

This comment has been minimized.

Copy link
Contributor

xing-yang commented Sep 16, 2019

/assign @xing-yang

there is the `VolumeSnapshotClass` resource.

Users need to be aware of the following when using this feature:

* API Objects `VolumeSnapshot`, `VolumeSnapshotContent`, and `VolumeSnapshotClass` are CRDs, not part of the core API.
* API Objects `VolumeSnapshot`, `VolumeSnapshotContent`, and `VolumeSnapshotClass` are CRDs, not part of the core API. The CSI drivers that support the volume snapshot functionality will automatically install these required CRDs.

This comment has been minimized.

Copy link
@xing-yang

xing-yang Sep 17, 2019

Contributor

FYI, CSI drivers won't automatically install the snapshot CRDs after snapshot APIs become Beta. So this sentence will be removed after the snapshot API going to Beta work is complete (planned for 1.17).

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Member

Add a note that the CRD install will be decoupled in the future

This comment has been minimized.

Copy link
@draghuram

draghuram Sep 17, 2019

Author Contributor

Done.


## Provisioning Volumes from Snapshots

A new volume, pre-populated with data from a snapshot, can be provisioned using *dataSource*

This comment has been minimized.

Copy link
@xing-yang

xing-yang Sep 17, 2019

Contributor

"pre-populated" sounds like static binding which is not necessarily true here. How about this?

A new volume can be provisioned using dataSource field in the PersistentVolumeClaim object to indicate the volume will be populated with data from a snapshot.

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Member

I don't think it sounds like static binding (which we should stop calling static binding, and use the term pre-provisioned snapshots). Especially in the future when we introduce the notion of data populators.

This comment has been minimized.

Copy link
@xing-yang

xing-yang Sep 17, 2019

Contributor

Ok, I see "pre-populated" is also used in our snapshot blog. I'm okay with that.

Why should we stop calling static binding? It is used for PV/PVC as well.

This comment has been minimized.

Copy link
@msau42

msau42 Sep 20, 2019

Member

We should update PVC docs too, but that's a separate issue. I think "pre-existing" is a more descriptive word than "static". If you say "bind to a pre-existing snapshot", I think that is more self-explanatory than "statically binding a snapshot".

@@ -16,7 +16,7 @@ weight: 20

{{% capture overview %}}

This document describes the current state of `PersistentVolumes` in Kubernetes. Familiarity with [volumes](/docs/concepts/storage/volumes/) is suggested.
This document describes the current state of *persistent volumes* in Kubernetes. Familiarity with [volumes](/docs/concepts/storage/volumes/) is suggested.

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Member

why is persistent volumes bolded?

This comment has been minimized.

Copy link
@draghuram

draghuram Sep 17, 2019

Author Contributor

To highlight the concept of persistent volumes. The previous version used backquotes resulting in rendering with monospace font that gave the impression that there is an API object called "PersistentVolumes". Since that is not the case, I replaced it with highlighted words "persistent volumes". The same reason applies to changes at many other places in this PR so if there is consensus about the right format, I can apply the same format at other places as well. One option is to simply call them "PVs" as we do in some places.

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks Sep 19, 2019

Member

It's not a special word. Please remove the formatting. Thank you. :)

This comment has been minimized.

Copy link
@msau42

msau42 Sep 20, 2019

Member

@mistyhacks what's the guideline for referring to K8s object names in the plural?

This comment has been minimized.

Copy link
@msau42

msau42 Sep 20, 2019

Member

When should we use the full object name, when can we abbreviate to PVC, etc?

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks Sep 20, 2019

Member

Marking up objects is covered here: https://kubernetes.io/docs/contribute/style/style-guide/#use-camel-case-for-api-objects (use camel-case, a bit further down it says not to split it into separate words like "Persistent Volume Claim")

I struggled to find where we explicitly say that we don't use mono text for object kinds, but I couldn't. I did find instances of it in the style guide itself, and I consider that somewhat definitive. For example: https://kubernetes.io/docs/contribute/style/style-guide/#use-simple-and-direct-language

We don't seem to have any stance about pluralizing, but as far as I know it's normal in Kubernetes to say, for instance, Ingresses. @steveperry-53 @Bradamant3 Do you have thoughts or more background?

I did notice that the PVC page does use mono text for the object kinds, so I guess you can leave yours in because the rest of the page could be cleaned up as part of a broader automated sweep. I was being more picky on this CL because it's good to ensure style adherence when a big new block of text is introduced.

resources, it is common that users need `PersistentVolumes` with varying
properties, such as performance, for different problems. Cluster administrators
need to be able to offer a variety of `PersistentVolumes` that differ in more
While *persistent volume claims* allow a user to consume abstract storage

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Member

why are we not using the K8s object type names here?

This comment has been minimized.

Copy link
@draghuram

draghuram Sep 17, 2019

Author Contributor

Since there is no object type "PersistentVolumeClaims". One alternative is to say "PersistentVolumeClaim objects".

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks Sep 19, 2019

Member

Again, please remove this extra formatting, throughout.

[storage class](/docs/concepts/storage/storage-classes/) and
the administrator must have created and configured that class in order for dynamic
provisioning to occur. Claims that request the class `""` effectively disable
dynamic provisioning for themselves.

To enable dynamic storage provisioning based on storage class, the cluster administrator
To enable dynamic storage provisioning based on *storage class*, the cluster administrator

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Member

StorageClass?

This comment has been minimized.

Copy link
@draghuram

draghuram Sep 17, 2019

Author Contributor

Done.

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks Sep 20, 2019

Member

Please use the Kubernetes object names. I added relevant links to the style guide in a previous comment.

@@ -181,7 +181,7 @@ However, the particular path specified in the custom recycler pod template in th

{{< feature-state for_k8s_version="v1.11" state="beta" >}}

Support for expanding PersistentVolumeClaims (PVCs) is now enabled by default. You can expand
Support for expanding *persistent volume claims* (PVCs) is now enabled by default. You can expand

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Member

PersistentVolumeClaims?

This comment has been minimized.

Copy link
@draghuram

draghuram Sep 17, 2019

Author Contributor

There is no object called "PersistentVolumeClaims".

@@ -318,7 +318,7 @@ Currently, storage size is the only resource that can be set or requested. Futu

{{< feature-state for_k8s_version="v1.13" state="beta" >}}

Prior to Kubernetes 1.9, all volume plugins created a filesystem on the persistent volume.
Prior to Kubernetes 1.9, all volume plugins created a filesystem on the *persistent volume*.

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Member

PersistentVolume?

This comment has been minimized.

Copy link
@draghuram

draghuram Sep 17, 2019

Author Contributor

Done.

A VolumeSnapshotContent of a particular class can only be bound to VolumeSnapshots requesting
that class. A VolumeSnapshotContent with no `snapshotClassName` has no class and can only be bound
to VolumeSnapshots that request no particular class.
A `VolumeSnapshotContent` of a particular class can only be bound to *volume snapshots* requesting

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Member

VolumeSnapshots?

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Member

@xing-yang is this accurate? Doesn't VolumeSnapshot also need to explicitly specify the VolumeSnapshotContentName?

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Member

Does the class only matter for dynamic provisioning?

This comment has been minimized.

Copy link
@xing-yang

xing-yang Sep 17, 2019

Contributor

Content has the class name in the current API, but will be removed once the bringing snapshot to Beta API PR is merged. The intent of this PR is not to address changes in the new Beta APIs.

This comment has been minimized.

Copy link
@msau42

msau42 Sep 20, 2019

Member

@xing-yang can you clarify my first question? Doesn't VolumeSnapshot need to request the exact VolumeSnapshotContent object? We only support explicit binding for snapshots, we don't pick from a pre-existing pool.


## Provisioning Volumes from Snapshots

A new volume, pre-populated with data from a snapshot, can be provisioned using *dataSource*

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Member

I don't think it sounds like static binding (which we should stop calling static binding, and use the term pre-provisioned snapshots). Especially in the future when we introduce the notion of data populators.

@@ -123,10 +125,18 @@ spec:

### Class

A volume snapshot can request a particular class by specifying the name of a
A *volume snapshot* can request a particular class by specifying the name of a

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Member

VolumeSnapshot?

This comment has been minimized.

Copy link
@draghuram

draghuram Sep 17, 2019

Author Contributor

Done.

[VolumeSnapshotClass](/docs/concepts/storage/volume-snapshot-classes/)
using the attribute `snapshotClassName`.
Only VolumeSnapshotContents of the requested class, ones with the same `snapshotClassName`
as the VolumeSnapshot, can be bound to the VolumeSnapshot.
Only *volume snapshot contents* of the requested class, ones with the same `snapshotClassName`

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Member

VolumeSnapshotContents?

This comment has been minimized.

Copy link
@draghuram

draghuram Sep 17, 2019

Author Contributor

There is no object called "VolumeSnapshotContents".

Only VolumeSnapshotContents of the requested class, ones with the same `snapshotClassName`
as the VolumeSnapshot, can be bound to the VolumeSnapshot.
Only *volume snapshot contents* of the requested class, ones with the same `snapshotClassName`
as the `VolumeSnapshot`, can be bound to that `VolumeSnapshot`.

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Member

Also, must it be dynamically provisioned? @xing-yang

This comment has been minimized.

Copy link
@xing-yang

xing-yang Sep 17, 2019

Contributor

Right

@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 17, 2019
@draghuram

This comment has been minimized.

Copy link
Contributor Author

draghuram commented Sep 17, 2019

I have pushed a commit to handle few of the review comments. For now, this is a separate commit. Please let me know if I need to squash it with the previous one (I don't know the recommended procedure as this is my first PR here). Thanks.

@draghuram draghuram force-pushed the draghuram:storagedoc branch from b3bd77d to 4f20721 Sep 19, 2019
@draghuram

This comment has been minimized.

Copy link
Contributor Author

draghuram commented Sep 19, 2019

I fixed one merge conflict and pushed a single commit containing all the changes.

@xing-yang

This comment has been minimized.

Copy link
Contributor

xing-yang commented Sep 19, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 19, 2019
@@ -25,16 +25,16 @@ This document describes the current state of `PersistentVolumes` in Kubernetes.

## Introduction

Managing storage is a distinct problem from managing compute. The `PersistentVolume` subsystem provides an API for users and administrators that abstracts details of how storage is provided from how it is consumed. To do this we introduce two new API resources: `PersistentVolume` and `PersistentVolumeClaim`.
Managing storage is a distinct problem from managing compute. The `PersistentVolume` subsystem provides an API for users and administrators that abstracts details of how storage is provided and how it is consumed. To do this we introduce two new API resources: `PersistentVolume` and `PersistentVolumeClaim`.

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks Sep 19, 2019

Member

Please remove "for users and administrators"

"is provided and consumed", please remove the extra words.

suggest "To configure storage, you create a PersistentVolume and PersistentVolumeClaim."

This comment has been minimized.

Copy link
@draghuram

draghuram Sep 25, 2019

Author Contributor

I experimented with various versions without any success. So I would like leave this refactoring to more experienced people.

@@ -56,13 +56,13 @@ A cluster administrator creates a number of PVs. They carry the details of the r
#### Dynamic
When none of the static PVs the administrator created matches a user's `PersistentVolumeClaim`,
the cluster may try to dynamically provision a volume specially for the PVC.
This provisioning is based on `StorageClasses`: the PVC must request a
This provisioning is based on `StorageClass` objects: the PVC must request a

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks Sep 19, 2019

Member

How about removing "This...objects:" and rephraing the next part:

The storage class must exist before you try to dynamically provision storage."

[storage class](/docs/concepts/storage/storage-classes/) and
the administrator must have created and configured that class in order for dynamic
provisioning to occur. Claims that request the class `""` effectively disable
dynamic provisioning for themselves.

To enable dynamic storage provisioning based on storage class, the cluster administrator
To enable dynamic storage provisioning based on `StorageClass`, the cluster administrator

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks Sep 19, 2019

Member

This reads like a prerequisite and should be introduced earlier. "You must enable.... before enabling...."

@@ -82,7 +82,7 @@ Pods use claims as volumes. The cluster inspects the claim to find the bound vol
Once a user has a claim and that claim is bound, the bound PV belongs to the user for as long as they need it. Users schedule Pods and access their claimed PVs by including a `persistentVolumeClaim` in their Pod's volumes block. [See below for syntax details](#claims-as-volumes).

### Storage Object in Use Protection
The purpose of the Storage Object in Use Protection feature is to ensure that Persistent Volume Claims (PVCs) in active use by a pod and Persistent Volume (PVs) that are bound to PVCs are not removed from the system as this may result in data loss.
The purpose of the *Storage Object in Use Protection* feature is to ensure that Persistent Volume Claims (PVCs) in active use by a pod and Persistent Volumes (PVs) that are bound to PVCs are not removed from the system as this may result in data loss.

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks Sep 19, 2019

Member

Why not simplify: "Storage Object in Use Protection prevents PVCs and PVs from being removed while they are being used by Pods, to protect against data loss."

@@ -318,7 +318,7 @@ Currently, storage size is the only resource that can be set or requested. Futu

{{< feature-state for_k8s_version="v1.13" state="beta" >}}

Prior to Kubernetes 1.9, all volume plugins created a filesystem on the persistent volume.
Prior to Kubernetes 1.9, all volume plugins created a filesystem on the `PersistentVolume`.

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks Sep 19, 2019

Member

Remove -- this version has been out of the support window for some time.

@@ -318,7 +318,7 @@ Currently, storage size is the only resource that can be set or requested. Futu

{{< feature-state for_k8s_version="v1.13" state="beta" >}}

Prior to Kubernetes 1.9, all volume plugins created a filesystem on the persistent volume.
Prior to Kubernetes 1.9, all volume plugins created a filesystem on the `PersistentVolume`.
Now, you can set the value of `volumeMode` to `block` to use a raw block device, or `filesystem`

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks Sep 19, 2019

Member

I don't think you need to talk about how things were before in an unsupported version. Please rephrase.

@@ -390,10 +390,10 @@ Currently, only NFS and HostPath support recycling. AWS EBS, GCE PD, Azure Disk,

### Mount Options

A Kubernetes administrator can specify additional mount options for when a Persistent Volume is mounted on a node.
A Kubernetes administrator can specify additional mount options for when a `PersistentVolume` is mounted on a node.

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks Sep 19, 2019

Member

Replace with just "You can". We use second person singular, to speak to the user directly, as much as possible. Do a sweep throughout the topic.


{{< note >}}
Not all Persistent volume types support mount options.
Not all persistent volume types support mount options.

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks Sep 19, 2019

Member

Which do? Maybe "Only support mount options: "

Copy link
Member

mistyhacks left a comment

I did a partial review. Please address the comments and let me know when I should take another look. Thanks for your patch!

@draghuram

This comment has been minimized.

Copy link
Contributor Author

draghuram commented Sep 20, 2019

@mistyhacks Thanks for extensive review. I just wanted to point out that many comments are regarding text that I haven't changed in this PR. I am not opposed to making the suggested changes but just wanted to mention it. BTW, I am still not clear about the best practice regarding plural object names. Should we use object name in plural but without backquotes? For example, "VolumeSnapshots" as opposed to "VolumeSnapshots".

In case of persistent volumes, their abbreviation seems to be prevalent so one alternative there would be to use "PVs" and "PVCs".

Lastly, there is one part of this PR which fixes a bug in the document (#16321). I am thinking of moving it into its own PR so that the error can be fixed while we continue to discuss other changes.

@sftim

This comment has been minimized.

Copy link
Contributor

sftim commented Sep 24, 2019

/sig storage

@sftim

This comment has been minimized.

Copy link
Contributor

sftim commented Sep 24, 2019

@draghuram if you're talking about a Kubernetes object like Namespace, Pod, DaemonSet, etc, then the style guide pretty much never puts those names in quotes or backticks.

Sometimes in English localization it's appropriate to write “namespace”, “namespaces”, “services” etc because those are also the English words for the relevant concept. That's fine so long as it's very clear to readers which meaning you intend.

This is to prevent them showing up in monospace format and giving the
impression that they refer to API objects.

One such example in the current doc: `PersistentVolumes`.

I also refactored content at few places based on review comments.
@draghuram draghuram force-pushed the draghuram:storagedoc branch from 4f20721 to fcc10a7 Sep 25, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Sep 25, 2019

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 25, 2019
@draghuram

This comment has been minimized.

Copy link
Contributor Author

draghuram commented Sep 25, 2019

I made some changes based on review comments. More over, couple of changes originally present in this PR have been merged with master using separate PRs. So I removed those changes from here.

This provisioning uses the concept of [storage classes](/docs/concepts/storage/storage-classes/) which requires the
following:

- You need to enable the `DefaultStorageClass` [admission controller](/docs/reference/access-authn-authz/admission-controllers/#defaultstorageclass)

This comment has been minimized.

Copy link
@msau42

msau42 Oct 3, 2019

Member

default storageclass is not strictly required, generally recommended

This comment has been minimized.

Copy link
@msau42

msau42 Oct 3, 2019

Member

Also these steps are probably better in an admin-facing page, rather than a user-facing page. Maybe https://kubernetes.io/docs/tasks/administer-cluster/change-default-storage-class/ could be a good place to move this to? We can reference it from here

@@ -56,18 +56,21 @@ A cluster administrator creates a number of PVs. They carry the details of the r
#### Dynamic
When none of the static PVs the administrator created matches a user's `PersistentVolumeClaim`,

This comment has been minimized.

Copy link
@msau42

msau42 Oct 3, 2019

Member

I would like to move away from using the term "static" to describe manually created volumes. Can we change all references to "static" to something like "manually pre-created" or "pre-existing"?

among the comma-delimited, ordered list of values for the `--enable-admission-plugins` flag of
the API server component. For more information on API server command line flags,
please check [kube-apiserver](/docs/admin/kube-apiserver/) documentation.
This provisioning uses the concept of [storage classes](/docs/concepts/storage/storage-classes/) which requires the

This comment has been minimized.

Copy link
@msau42

msau42 Oct 3, 2019

Member

How about something like this?

"Dynamic provisioning requires a StorageClass to be created and configured by an administrator. PVCs can request a StorageClass by specifying its name in the StorageClassName field. A nil StorageClassName will use the default StorageClass configured in the cluster. An empty string StorageClassName disables dynamic provisioning"

created and configured that class.

Claims that request the class `""` effectively disable dynamic
provisioning for themselves.

### Binding

This comment has been minimized.

Copy link
@msau42

msau42 Oct 3, 2019

Member

I think we should remove the details of control loops in this section. That's more of an implementation detail. The important points for users are:

  • PVC to PV binding is 1-1
  • If a pre-existing PV is available that can satisfy the PVC, then it will bind to the pre-existing PV first. Otherwise, dynamic provisioning will be triggered.
@@ -23,61 +23,60 @@ This document describes the current state of `VolumeSnapshots` in Kubernetes. Fa

Similar to how API resources `PersistentVolume` and `PersistentVolumeClaim` are used to provision volumes for users and administrators, `VolumeSnapshotContent` and `VolumeSnapshot` API resources are provided to create volume snapshots for users and administrators.

A `VolumeSnapshotContent` is a snapshot taken from a volume in the cluster that has been provisioned by an administrator. It is a resource in the cluster just like a PersistentVolume is a cluster resource.
A `VolumeSnapshotContent` is a snapshot of a volume in the cluster that has been provisioned by an administrator. It is a resource in the cluster just like a `PersistentVolume` is a cluster resource.

This comment has been minimized.

Copy link
@msau42

msau42 Oct 3, 2019

Member

remove "in the cluster"? I'm not sure it really adds value


## Lifecycle of a volume snapshot and volume snapshot content

`VolumeSnapshotContents` are resources in the cluster. `VolumeSnapshots` are requests for those resources. The interaction between `VolumeSnapshotContents` and `VolumeSnapshots` follow this lifecycle:
`VolumeSnapshotContent` objects are resources in the cluster and `VolumeSnapshot` objects are requests for those resources. The interaction between `VolumeSnapshotContent` and `VolumeSnapshot` objects follows this lifecycle:

### Provisioning Volume Snapshot

There are two ways snapshots may be provisioned: statically or dynamically.

This comment has been minimized.

Copy link
@msau42

msau42 Oct 3, 2019

Member

I would like to change the wording in both places


### Provisioning Volume Snapshot

There are two ways snapshots may be provisioned: statically or dynamically.

#### Static
A cluster administrator creates a number of `VolumeSnapshotContents`. They carry the details of the real storage which is available for use by cluster users. They exist in the Kubernetes API and are available for consumption.
A cluster administrator creates a number of `VolumeSnapshotContent` resources describing snapshots on the underlying storage that are available for use by cluster users. They exist in the Kubernetes API and are available for consumption.

This comment has been minimized.

Copy link
@msau42

msau42 Oct 3, 2019

Member

Emphasize that these are pre-existing snapshots that they want to import into Kubernetes.

When none of the static `VolumeSnapshotContents` the administrator created matches a user's `VolumeSnapshot`,
the cluster may try to dynamically provision a volume snapshot specially for the `VolumeSnapshot` object.
This provisioning is based on `VolumeSnapshotClasses`: the `VolumeSnapshot` must request a
This provisioning is based on `VolumeSnapshotClass` resources: the `VolumeSnapshot` must request a

This comment has been minimized.

Copy link
@msau42

msau42 Oct 3, 2019

Member

How about:
"Dynamic provisioning requires an administrator to create and configure a VolumeSnapshotClass. A VolumeSnapshot specifies the VolumeSnapshotClass by setting the VolumeSnapshotClassName field."


VolumeSnapshots will remain unbound indefinitely if a matching VolumeSnapshotContent does not exist. VolumeSnapshots will be bound as matching VolumeSnapshotContents become available.
Volume snapshots will remain unbound indefinitely if a matching `VolumeSnapshotContent` does not exist. Volume snapshots will be bound as matching `VolumeSnapshotContent` resources become available.

This comment has been minimized.

Copy link
@msau42

msau42 Oct 3, 2019

Member

I think this sentence can be removed after rewording the above

to VolumeSnapshots that request no particular class.
A `VolumeSnapshotContent` of a particular class can only be bound to volume snapshots requesting
that class. A `VolumeSnapshotContent` with no `snapshotClassName` has no class and can only be bound
to volume snapshots that request no particular class.

This comment has been minimized.

Copy link
@msau42

msau42 Oct 3, 2019

Member

"request an empty class"

@zacharysarah

This comment has been minimized.

Copy link
Contributor

zacharysarah commented Oct 10, 2019

@draghuram Please feel free to /reopen this PR when you're ready to continue with feedback.

/close

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 10, 2019

@zacharysarah: Closed this PR.

In response to this:

@draghuram Please feel free to /reopen this PR when you're ready to continue with feedback.

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.