-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
cleanup, formatting and styling of volumes concept #24077
cleanup, formatting and styling of volumes concept #24077
Conversation
Deploy preview for kubernetes-io-master-staging ready! Built with commit 3f5d599 https://deploy-preview-24077--kubernetes-io-master-staging.netlify.app |
75bade6
to
1deb0a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this effort.
When reviewing the PR, I was feeling more and more impatient. It is boring, tedious task. Maybe you have the same feelings as well when revising this and then proposing this change.
Currently we got 3 options:
Pod
,Container
,secretRef
,volume
-- This capitalization is a strict requirement, whenever we refer to a resource type, its field names or command line flags etc. We'd better use coding style for them for visual differentiation.- Pod, Container, Secret, Deployment -- we still see these spelling styles everywhere.
- pod, container, secret, deployment -- this is everywhere as well.
It would be a huge pain if an author/reviewer has to switch between three styles, no matter what kinds of guidelines we put there. Maybe we should drop one of these styles.
Option 1 is required for document accuracy, we have to keep it. Between option 2 and 3, I'd vote for option 2 because option 3 is obviously causing a lot of misunderstandings, e.g. "sharing a secret", "scale a deployment".
crashes, the files in the container are lost. The kubelet restarts the container | ||
but with a clean state. Second, when running containers together | ||
in a pod it is often necessary to share files between those containers. The | ||
Kubernetes volume abstraction solves both of these problems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree to this change. "volume abstraction" is generic enough and cause no ambiguity in this context. Also we are not yet touching the Volume
resource type in this paragragh.
image and volumes. The [Docker image](https://docs.docker.com/userguide/dockerimages/) | ||
is at the root of the filesystem hierarchy. Volumes mount at the specified paths within | ||
the image. Volumes can not mount onto other volumes or have hard links to | ||
other volumes. Each container in the pod must independently specify where to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should say "Each Container
in the Pod
must ...` because we are talking about the resource again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think this is referring to the Kubernetes Container resource or containers in general?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd write container lowercase. Even if this is primarily describing a field inside a Pod spec, container
is lowercased there as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is sentence is confusing. I've gone over this a few times.
I will update and capitalize as a resource because the sentence is describing how to "specify" the Container and Pod objects.
I also agree that the name of the field is called container
but if this styling is used I would update the sentence to identify the "field to set".
fsType: ext4 | ||
``` | ||
|
||
#### CSI Migration | ||
#### CSIMigrationAWS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree to this change, although CSIMigrationAWS
is the name of a feature gate. Using camel case in (sub)titles looks weird to me. The title should be a phrase or sub-sentence IMHO.
If the intent is to make sure each (sub)title has a unique anchor, we may want to put #### CSI Migration {#csi-migration-aws}
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree with you. I need to look at the the text that follows and make sure the feature gate name
is referenced (feature gate page). There should be unique headings for each section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the headings. Let me know what you think. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are the headings now?
It mounts a directory and writes the requested data in plain text files. | ||
|
||
{{< note >}} | ||
A Container using Downward API as a [subPath](#using-subpath) volume mount will not | ||
A container using Downward API as a [`subPath`](#using-subpath) volume mount will not | ||
receive Downward API updates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Downward API" should be "downward".
If we use "Downward API", we may need to provide a glossary for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this needs more cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this. Not sure if correct.
downwardAPI
is a field and DownwardAPIVolumeSource
is a resource.
any reason, the data in the `emptyDir` is deleted forever. | ||
|
||
{{< note >}} | ||
A Container crashing does *NOT* remove a Pod from a node, so the data in an `emptyDir` volume is safe across Container crashes. | ||
A Container crashing does *NOT* remove a Pod from a node, so the data in an `emptyDir` volume is safe across container crashes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Container crashing does *NOT* remove a Pod from a node, so the data in an `emptyDir` volume is safe across container crashes. | |
A container crashing does *NOT* remove a pod from a node, so the data in an `emptyDir` volume is safe across container crashes. |
@@ -398,8 +394,8 @@ See the [Flocker example](https://github.com/kubernetes/examples/tree/{{< param | |||
### gcePersistentDisk {#gcepersistentdisk} | |||
|
|||
A `gcePersistentDisk` volume mounts a Google Compute Engine (GCE) | |||
[Persistent Disk](https://cloud.google.com/compute/docs/disks) into your Pod. Unlike | |||
`emptyDir`, which is erased when a Pod is removed, the contents of a PD are | |||
[persistent disk](https://cloud.google.com/compute/docs/disks)(PD) into your Pod. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
1deb0a3
to
12353df
Compare
a3becc5
to
ae178a4
Compare
[EBS volume](https://aws.amazon.com/ebs/) into your pod. Unlike | ||
`emptyDir`, which is erased when a pod is removed, the contents of an EBS | ||
volume are preserved and the volume is merely unmounted. This means that an | ||
EBS volume can be pre-populated with data, and that data can be transferred between pods. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed "handed off" to transferred. Is this an accurate change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mainly about letting a replacement Pod pick up where a previous Pod left off, but you could use it for other patterns (eg a Job that does some work and then triggers a single-pod Deployment that will use the same EBS volume).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. My goal is to come up with a replacement phrase for "handed off".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shared? or used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I will change to "shared". This phrase is used throughout the page.
/cc @kubernetes/sig-storage-pr-reviews |
@tengqm :
If the project persists in capitalizing words in apparent contravention to conventional English usage, there will continue to be confusion and misunderstanding on this point. Fundamentally, we are trying to solve with typography what would be better addressed with plain language. As discussed in issue #20862, there is a short list of Pascal-cased terms (Pod, Container, Secret, etc.) that are very difficult to interpret as being proper vs. common nouns. This makes it very difficult for editors such as myself, who are not experts in Kubernetes, to make the distinction between a correctly capitalized resource name and an erroneously capitalized common noun. This problem will not go away by requesting people to capitalize by default. The only reliable solution to the capitalization problem for single-word Pascal-cased resources is to identify the resource in running text, which is expressly forbidden in the Kubernetes style guide . To make the distinction clear, writers must be allowed to identify resource names as resource names by declaring the resource's type in running text. I can easily tell the difference between a Container resource vs. any old container if the word "resource" (or object) is permitted after the word "Container." Precluded from having this important context, I cannot tell a Container from a container or a Pod from a pod. Until ground down or driven mad, most editors will flag such uses for clarification (wasting time) or simply "correct" the problem, thus eliminating the desired distinction. |
Hi @wabernatScality . Thanks for your feedback. Not: I think there is ambiguity writing "pod" versus "Pod": "pod" representing general usage (what are the common examples?) and "Pod" representing state of an object (creating, running, lifecycle, destroying, has containers, has spec, status ...). |
I am proposing that the style guide not enjoin writers in its examples from adding a resource identifier after a proper-named resource, particularly when the proper name is a single-word Pascal-cased name. Because PersistentVolumeClaim, for example, is unambiguously in Pascal case, there's no need to follow it with an identifying noun, and the existing rule makes some sense. However, when the object is a Pascal-cased word with no unusual capitalization inside the word (e.g., "Pod," "Job," "Secret," etc.) following the Pascal-cased word with the resource type becomes an essential aid to the reader and the editor to disambiguate the term. In precis, I am proposing: a) lifting the style guide's explicit prohibition on identifying named resources by type, and b) suggesting that writers actively use such identifiers when documenting "ambiguously Pascal-cased" single-word resources to make it clear that the capitalization is for a named resource. |
Pod, Job, Container, ReplicaSet, Node, PersistentVolumeClaim, Event, Secret, Ingress, Volume, Deployment, ... are some of the k8s resources. |
I addressed the issue of localization with Tim in issue #20862. Clear English should not be an issue for translators. The problem I have with monolithically capitalizing resource terms without sensitivity to context is that it substitutes one confusion for another and does not bring your readers clarity. I would not describe a Container of nuts in the desk at my Job that I keep Secret from my boss, because those are not Kubernetes resources. Your writing needs to admit of the possibility of proper and common nouns for unique and non-unique objects. The reason I'm asking you to consider this change is so that as an editor I can spot the difference between, for example, a cron job and a Job resource, or a Secret object and a secret key. Capitalization alone does not enable either me or a lay reader to distinguish these items if they are not followed by an explicit descriptor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big page to tackle, so 🎉 kudos 🎉 for cracking on here @kbhawkey
There's a bit of a discrepancy between CSI (the English abbreviation for “Container Storage Interface”) vs csi
(the volume type for using a CSI driver in a Pod). It's a similar story with FlexVolume vs flexVolume
.
That's something I recommend fixing.
I've also added over sixty bits of inline feedback that are quite a bit less important, some explicitly marked as nits. I'm absolutely fine with moving this PR forward without dealing with those less-vital details; I mentioned what I spotted and added my thoughts anyway in case that's useful.
drivers, but the functionality is very limited for now (e.g. as of Docker 1.7 | ||
only one volume driver is allowed per Container and there is no way to pass | ||
only one volume driver is allowed per container and there is no way to pass | ||
parameters to volumes). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also feels out of date. Fine to fix that in a different PR - this PR is focused on formatting fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I wanted to remove this but left in place. Hope someone from Storage would agree to remove or update.
|
||
Pods interact with FlexVolume drivers through the `flexvolume` in-tree plugin. | ||
More details can be found [here](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-storage/flexvolume.md). | ||
Pods interact with `FlexVolume` drivers through the `flexvolume` in-tree plugin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pods interact with `FlexVolume` drivers through the `flexvolume` in-tree plugin. | |
Pods interact with FlexVolume drivers through the `flexVolume` in-tree volume type. |
volume mounts anything there, the Container with `HostToContainer` mount | ||
propagation will see it. | ||
Similarly, if any Pod with `Bidirectional` mount propagation to the same | ||
volume mounts anything there, the `Container` with `HostToContainer` mount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd write:
volume mounts anything there, the `Container` with `HostToContainer` mount | |
volume mounts anything there, the container with `HostToContainer` mount |
or: container
|
||
A typical use case for this mode is a Pod with a FlexVolume or CSI driver or | ||
a Pod that needs to mount something on the host using a `hostPath` volume. | ||
A typical use case for this mode is a Pod with a `FlexVolume` or `CSI` driver or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd put:
A typical use case for this mode is a Pod with a `FlexVolume` or `CSI` driver or | |
A typical use case for this mode is a Pod using a `flexVolume` or `csi` volume, or |
{{< caution >}} | ||
`Bidirectional` mount propagation can be dangerous. It can damage | ||
the host operating system and therefore it is allowed only in privileged | ||
containers. Familiarity with Linux kernel behavior is strongly recommended. | ||
In addition, any volume mounts created by containers in pods must be destroyed | ||
(unmounted) by the containers on termination. | ||
{{< /caution >}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might merit a {{< warning >}}
. (OK to save that tweak for a later PR).
In terms of what capitalization style to apply: follow the style guide if you can't decide. If you can decide then let's go with that, feel free to add a note to reviewers if you think it might not be obvious why you've selected an approach. Whilst that style guide is very much a living document, this PR is not the place to discuss revisions to the style guide. |
I apologize for the thread-jack. |
/assign @sftim |
LGTM |
Also relevant to issue #4503 |
Possibly. I will ping Sig Storage. |
I'd like to start a conversation about the style guide and capitalization, based on the conversation in https://github.com/kubernetes/website/pull/24077/files, Specifically, [this comment](kubernetes#24077 (comment)) from @wabernatScality . Currently, the style guide explicitly prohibits usage of "object" -- such as "Pod object" or "PodList object" Judicious use of "object" could improve clarity. It's a hint to the reader that the distinction of being an API object is important. Right now, the guidelines are overly reliant on capitalization to allow readers to make that distinction. Capitalization is a weak way to signal information to the reader. This information may be lost entirely due to assistive technology, differences in visual abilities, or translation. I look forward to the feedback :)
I'd like to start a conversation about the style guide and capitalization, based on the conversation in https://github.com/kubernetes/website/pull/24077/files, Specifically, [this comment](kubernetes#24077 (comment)) from @wabernatScality . Currently, the style guide explicitly prohibits usage of "object" -- such as "Pod object" or "PodList object" Judicious use of "object" could improve clarity. It's a hint to the reader that the distinction of being an API object is important. Right now, the guidelines are overly reliant on capitalization to allow readers to make that distinction. Capitalization is a weak way to signal information to the reader. This information may be lost entirely due to assistive technology, differences in visual abilities, or translation. I look forward to the feedback :)
drivers, but the functionality is somewhat limited. | ||
|
||
Kubernetes supports many types of volumes. A {{< glossary_tooltip term_id="pod" text="Pod" >}} | ||
can use any number of volume types simultaneously. The lifetime of a Kubernetes volume |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The note about volume lifetime is not correct. Ephemeral volume types have a lifetime of a pod, but persistent volumes have a lifetime beyond the pod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @msau42 .
Thanks for the feedback. Initially, I created this PR to clean up the formatting and styling
of the page content. I was also experimenting with adapting some new docs style guidelines.
I went a bit further by modifying some of the text (grammar, readability), but
did not delve too deeply into the content. I'll look again at the page and see if I can make more content changes
otherwise, I can log an issue to review the page in more detail. How does that sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me see if I can updated correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it is still relevant to include the paragraph about Docker volumes (as a comparison)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think leaving content updates to a future pr is fine. I think the Docker volumes comparison may need to be updated as well. The biggest difference I think that's relevant is Docker volumes write data to the host and do not persist if the pod is restarted on a different node.
We welcome additional contributions. | ||
## Types of Volumes {#volume-types} | ||
|
||
Kubernetes supports several types of volumes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there another place we can see the TOC? I think it's convenient to have a condensed list of all the plugins up front and be able to skip to the relevant detailed section depending on what you're interested in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the TOC for the page (which gets automatically generated from the headings)
is located in the right hand side of the page. The in-page table of contents should contain
each plugin. Is this change okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes thanks for explaining!
[EBS volume](https://aws.amazon.com/ebs/) into your pod. Unlike | ||
`emptyDir`, which is erased when a pod is removed, the contents of an EBS | ||
volume are preserved and the volume is merely unmounted. This means that an | ||
EBS volume can be pre-populated with data, and that data can be transferred between pods. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shared? or used?
An `awsElasticBlockStore` volume mounts an Amazon Web Services (AWS) | ||
[EBS volume](https://aws.amazon.com/ebs/) into your pod. Unlike | ||
`emptyDir`, which is erased when a pod is removed, the contents of an EBS | ||
volume are preserved and the volume is merely unmounted. This means that an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe instead of preserved, use persisted, to reflect our persistentvolumes concept
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I will change. I did not attempt to change the description for each plugin.
|
||
{{< feature-state for_k8s_version="v1.17" state="beta" >}} | ||
|
||
The CSI Migration feature for awsElasticBlockStore, when enabled, shims all plugin operations | ||
The `CSIMigration` feature for `awsElasticBlockStore`, when enabled, shims all plugin operations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think "shims" is a clear word to describe this? or would something like "route" or "redirects" be more descriptive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not really. I think the docs could be improved here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the use of "shim" in the page. PTAL.
{{< feature-state for_k8s_version="v1.17" state="alpha" >}} | ||
|
||
To turn off the awsElasticBlockStore storage plugin from being loaded by controller manager and kubelet, you need to set this feature flag to true. This requires `ebs.csi.aws.com` Container Storage Interface (CSI) driver being installed on all worker nodes. | ||
To disable the `awsElasticBlockStore` storage plugin from being loaded by the controller manager | ||
and the kubelet, set the `CSIMigrationAWSComplete` flag to `true`. This feature requires the `ebs.csi.aws.com` Container Storage Interface (CSI) driver installed on all worker nodes. | ||
|
||
### azureDisk {#azuredisk} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the doc seems to be inconsistent about which volume types are persistent and which are ephemeral. Would it be more clear if we broke up this list of volume types into two sections, ephemeral and persistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
For more information on how to develop a CSI driver, refer to the [kubernetes-csi | ||
documentation](https://kubernetes-csi.github.io/docs/) | ||
For more information on how to develop a CSI driver, refer to the | ||
[kubernetes-csi documentation](https://kubernetes-csi.github.io/docs/) | ||
|
||
#### Migrating to CSI drivers from in-tree plugins | ||
|
||
{{< feature-state for_k8s_version="v1.14" state="alpha" >}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has been beta since 1.17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I will update.
For the content changes, I'd be happy (and prefer) to put corrections into a separate PR. If there are no regressions then this should be OK to merge. |
@kbhawkey it sounds as if (once rebased) this is basically good-to-go. |
5bd7c73
to
3f5d599
Compare
/lgtm Thanks for working on this! |
LGTM label has been added. Git tree hash: a31348a6d26d06dd7d2460e89ad68d09a0ac0823
|
/approve Nice one @kbhawkey |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sftim 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 |
Pod
. Is it possible for readers to switch betweenthe different formats in a sentence. What about "container" and "Container". Are there fuzzy areas where both formats could be used?
[Preview]