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

storage: GenericEphemeralVolume #22438

Merged
merged 3 commits into from
Jul 15, 2020

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Jul 9, 2020

This is the initial documentation for one new feature:

The feature should get included in 1.19, its implementation is approved and just needs to be merged.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 9, 2020
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Jul 9, 2020

Deploy preview for kubernetes-io-vnext-staging processing.

Building with commit 4401fd8

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/5f0f2eb584370e000742a261

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 9, 2020
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Jul 9, 2020
@savitharaghunathan
Copy link
Member

/milestone 1.19
/assign

@k8s-ci-robot k8s-ci-robot added this to the 1.19 milestone Jul 9, 2020
@pohly pohly force-pushed the generic-ephemeral-volumes branch from ee31153 to 1f1fa07 Compare July 10, 2020 15:47
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 10, 2020
@pohly pohly changed the title WIP: storage: GenericEphemeralVolume storage: GenericEphemeralVolume Jul 10, 2020
@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 Jul 10, 2020
@pohly
Copy link
Contributor Author

pohly commented Jul 10, 2020

Pinging:

@pohly
Copy link
Contributor Author

pohly commented Jul 10, 2020

The URL for "Storage Capacity Tracking" assumes that PR #21634 gets merged.

The URLs for the 1.19 API are tentative: I believe they will be correct once the API gets published, but currently it isn't.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @pohly

Here's some feedback. Any comment I've labelled as a nit is nice-to-fix, but it's OK to merge this without addressing it.

content/en/docs/concepts/storage/ephemeral-volumes.md Outdated Show resolved Hide resolved
content/en/docs/concepts/storage/ephemeral-volumes.md Outdated Show resolved Hide resolved
content/en/docs/concepts/storage/ephemeral-volumes.md Outdated Show resolved Hide resolved
content/en/docs/concepts/storage/ephemeral-volumes.md Outdated Show resolved Hide resolved
content/en/docs/concepts/storage/ephemeral-volumes.md Outdated Show resolved Hide resolved
@@ -1355,37 +1358,9 @@ as usual, without any CSI specific changes.

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

This feature allows CSI volumes to be directly embedded in the Pod specification instead of a PersistentVolume. Volumes specified in this way are ephemeral and do not persist across Pod restarts.
This feature allows CSI volumes to be directly embedded in the Pod specification instead of a PersistentVolume. Volumes specified in this way are ephemeral and do not persist across Pod restarts. See [the ephemeral volume page](/docs/concepts/storage/ephemeral-volumes/#csi-ephemeral-volume) for more information.
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
This feature allows CSI volumes to be directly embedded in the Pod specification instead of a PersistentVolume. Volumes specified in this way are ephemeral and do not persist across Pod restarts. See [the ephemeral volume page](/docs/concepts/storage/ephemeral-volumes/#csi-ephemeral-volume) for more information.
You can directly configure CSI volume claims within the Pod specification. Volumes specified in this way are ephemeral and do not persist across Pod restarts. See [Ephemeral Volumes](/docs/concepts/storage/ephemeral-volumes/#csi-ephemeral-volume) for more information.

I'm saying volume claim because “ephemeral PersistentVolumeClaim” doesn't really make sense.

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 this case "CSI volumes" makes more sense than "CSI volume claims" because there is nothing resembling a "claim" in the API. The rest makes sense, changed.

content/en/docs/concepts/storage/ephemeral-volumes.md Outdated Show resolved Hide resolved
content/en/docs/concepts/storage/ephemeral-volumes.md Outdated Show resolved Hide resolved

Kubernetes supports several different kinds of ephemeral volumes for
different purposes:
- [emptyDir]((/docs/concepts/volumes/#emptydir): a directory on the root disk or
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the backing storage emptyDir have to live on the node's root filesystem? (I don't believe so).

I'd explain it as “an amount of node-local ephemeral storage, empty at Pod startup”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actual storage is limited to the root disk. The only alternatives are RAM. I was trying to point this out because it's the difference compared to CSI.

Let's change it into "empty at Pod startup, with storage provided locally from the root disk or RAM"

Copy link
Contributor

Choose a reason for hiding this comment

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

The kubelet has a base directory for other locally stored data (/var/lib/kubelet by default).

Typically, /var/lib/kubelet is on the system root filesystem, and the kubelet is designed with that layout in mind.

(adapted from https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#local-ephemeral-storage)

I don't think “root disk” is correct. First because the root filesystem isn't always backed by durable storage, and second because /var/lib/kubelet doesn't have to live on that filesystem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"root disk" is a simplification. How about "with storage provided locally from the kubelet base directory (usually the root disk) or from memory"?

I'm also not sure about "provided locally from" - perhaps "coming locally from"?

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've switched to "with storage coming locally from the kubelet base directory (usually the root disk) or RAM"

- pohly
title: Ephemeral Volumes
content_type: concept
weight: 20
Copy link
Contributor

Choose a reason for hiding this comment

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

This weight interposes Ephemeral Volumes between the Volumes concept page and the Persistent Volumes concept page. I don't think that's what you want here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cut-and-paste without thinking about what this field means... I'm torn between placing it directly after "persistent volumes" and adding it at the end, after all the concepts linked to in the document itself have been introduced.

Let me add it at the end.

@pohly pohly force-pushed the generic-ephemeral-volumes branch 2 times, most recently from 934c42f to f5f21d7 Compare July 13, 2020 09:38
@pohly
Copy link
Contributor Author

pohly commented Jul 13, 2020

@sftim thanks a lot for the detailed and constructive review. It's very appreciated!

Perhaps you can also work your review magic on #21634?

I just pushed a version which fixes some of the things I learned about in this review here (URL format, weight).

@divya-mohan0209
Copy link
Contributor

@pohly : Just checking in to see whether you've received a signoff from a docs & a technical (if applicable) perspective? If you haven't yet, please could you get them done at the earliest for a successful merge?

Other applications expect some read-only input data to be present in
files, like configuration data or secret keys.

_Ephemeral volumes_ are designed for these use cases. Because volumes
Copy link
Member

Choose a reason for hiding this comment

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

I think it's important to mention somehwere in the intro that ephemeral volumes follow the pod's lifetime and get created and deleted along with the pod.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced "created anew for each pod" with "follow the pod's lifetime and get created and deleted along with the pod".

Or rather, Pod (capital case)... I also fixed that elsewhere.

They are managed by kubelet on each node.

CSI ephemeral volumes *must* be provided by third-party CSI storage
drivers. Generic ephemeral volumes *can* be provided by third-party
Copy link
Member

Choose a reason for hiding this comment

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

Start "Generic ephemeral volumes on a separate line" since we're talking about a different feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.


CSI ephemeral volumes *must* be provided by third-party CSI storage
drivers. Generic ephemeral volumes *can* be provided by third-party
CSI storage drivers, but also by any other storage driver that
Copy link
Member

Choose a reason for hiding this comment

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

The same storage drivers that support CSI ephemeral volumes may not be necessarily able to support generic ephemeral volumes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added: "Some CSI drivers are written specifically for CSI
ephemeral volumes and do not support dynamic provisioning: those then
cannot be used for generic ephemeral volumes."

- Volumes can have a fixed size that pods are not able to exceed.
- Volumes may have some initial data, depending on the driver and
parameters.
- Typical operations on volumes are supported, including
Copy link
Member

Choose a reason for hiding this comment

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

assuming that the driver supports them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

are allowed inside a volume source of the pod. Labels, annotations and
the whole set of fields for a PersistentVolumeClaim are supported. When such a Pod gets
created, the ephemeral volume controller then creates an actual PersistentVolumeClaim
object in the same namespace as the pod.
Copy link
Member

Choose a reason for hiding this comment

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

And gets deleted when the Pod is deleted.

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've added "... and ensures that the PersistentVolumeClaim gets deleted when the Pod gets deleted." How that works specifically is then explained later - I didn't want to repeat that explanation.

IMHO explaining the steps in chronological order makes sense.


### PersistentVolumeClaim naming

Naming of the additional PVCs is deterministic: the name is
Copy link
Member

Choose a reason for hiding this comment

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

additional -> ephemeral? although "ephemeral PVC" is a bit of an oxymoron

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's why I avoided it 😢

Replaced with "automatically created".


Enabling the GenericEphemeralVolume feature allows users to create
PVCs indirectly if they can create pods, even if they do not have
permission to create them directly. Cluster administrators must be
Copy link
Member

Choose a reason for hiding this comment

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

them -> PVCs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

referenced in a Pod via a `PersistentVolumeClaim` object.
A `csi` volume can be used in a pod in three different ways:
- through a reference to a [`persistentVolumeClaim`](#persistentvolumeclaim)
- with a [generic ephemeral volume](/docs/concepts/storage/ephemeral-volumes/#generic-ephemeral-volume)
Copy link
Member

Choose a reason for hiding this comment

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

should we note that these are alpha/beta respsectively?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

@@ -1291,8 +1291,11 @@ Once a CSI compatible volume driver is deployed on a Kubernetes cluster, users
may use the `csi` volume type to attach, mount, etc. the volumes exposed by the
Copy link
Member

Choose a reason for hiding this comment

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

Should we link to the new ephemeral-volumes page somewhere at the top of this page? (and link to the persistent volumes page in the same area)

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 "background" section may need a more thorough rewrite. For example, it still says:

A Kubernetes volume, on the other hand, has an explicit lifetime - the same as
the Pod that encloses it.

That became obsolete when persistent volumes were introduced, which was a while ago...

Let me punt on that for now for the sake of getting this PR merged before the docs deadline. I've created #22505 to track it.

This is the initial documentation for one new feature:
- kubernetes/enhancements#1698

A new page gets created for different ephemeral volumes because the
relationship between them needs to be explained.

Co-authored-by: Tim Bannister <tim@scalefactory.com>
@pohly pohly force-pushed the generic-ephemeral-volumes branch from f5f21d7 to 5981f4f Compare July 14, 2020 08:33
Copy link
Contributor Author

@pohly pohly left a comment

Choose a reason for hiding this comment

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

Pushed an updated version.

Other applications expect some read-only input data to be present in
files, like configuration data or secret keys.

_Ephemeral volumes_ are designed for these use cases. Because volumes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced "created anew for each pod" with "follow the pod's lifetime and get created and deleted along with the pod".

Or rather, Pod (capital case)... I also fixed that elsewhere.

They are managed by kubelet on each node.

CSI ephemeral volumes *must* be provided by third-party CSI storage
drivers. Generic ephemeral volumes *can* be provided by third-party
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.


CSI ephemeral volumes *must* be provided by third-party CSI storage
drivers. Generic ephemeral volumes *can* be provided by third-party
CSI storage drivers, but also by any other storage driver that
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added: "Some CSI drivers are written specifically for CSI
ephemeral volumes and do not support dynamic provisioning: those then
cannot be used for generic ephemeral volumes."

CSI ephemeral volumes *must* be provided by third-party CSI storage
drivers. Generic ephemeral volumes *can* be provided by third-party
CSI storage drivers, but also by any other storage driver that
supports dynamic provisioning. These drivers can offer functionality
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"These drivers" became a bit ambiguous. I replaced it with "The advantage of using third-party drivers is that they can..."

- Volumes can have a fixed size that pods are not able to exceed.
- Volumes may have some initial data, depending on the driver and
parameters.
- Typical operations on volumes are supported, including
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

using a StorageClass with a reclaim policy of `retain`: the storage outlives the Pod,
and in this case you need to ensure that volume clean up happens separately.

Once these PVCs exist, they can be used like any other PVC. In
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced "Once" with "While" - it's more accurate and hints that this can change...


### PersistentVolumeClaim naming

Naming of the additional PVCs is deterministic: the name is
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's why I avoided it 😢

Replaced with "automatically created".


Enabling the GenericEphemeralVolume feature allows users to create
PVCs indirectly if they can create pods, even if they do not have
permission to create them directly. Cluster administrators must be
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

@@ -1291,8 +1291,11 @@ Once a CSI compatible volume driver is deployed on a Kubernetes cluster, users
may use the `csi` volume type to attach, mount, etc. the volumes exposed by the
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 "background" section may need a more thorough rewrite. For example, it still says:

A Kubernetes volume, on the other hand, has an explicit lifetime - the same as
the Pod that encloses it.

That became obsolete when persistent volumes were introduced, which was a while ago...

Let me punt on that for now for the sake of getting this PR merged before the docs deadline. I've created #22505 to track it.

referenced in a Pod via a `PersistentVolumeClaim` object.
A `csi` volume can be used in a pod in three different ways:
- through a reference to a [`persistentVolumeClaim`](#persistentvolumeclaim)
- with a [generic ephemeral volume](/docs/concepts/storage/ephemeral-volumes/#generic-ephemeral-volume)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

I'd actually be happy to see this merge, we can fix the one bit of dodgy Markdown in a follow-up good first issue PR.

Preview at https://deploy-preview-22438--kubernetes-io-vnext-staging.netlify.app/docs/concepts/storage/ephemeral-volumes/ otherwise looks OK.

content/en/docs/concepts/storage/ephemeral-volumes.md Outdated Show resolved Hide resolved
@sftim
Copy link
Contributor

sftim commented Jul 14, 2020

/sig storage
@kubernetes/sig-storage-pr-reviews is this OK from a technical accuracy point of view?

@k8s-ci-robot k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jul 14, 2020
Co-authored-by: Tim Bannister <tim@scalefactory.com>
@pohly
Copy link
Contributor Author

pohly commented Jul 14, 2020

I'd actually be happy to see this merge, we can fix the one bit of dodgy Markdown in a follow-up good first issue PR.

I took your commit, thanks for the fix.

@msau42 wants to check the technical side, but it might still take a few more hours before she has time for it.

Same with #21634

@msau42
Copy link
Member

msau42 commented Jul 14, 2020

Tech LGTM.

There were some comments on other PRs about not linking to keps or enhancement issues.

Comment on lines +259 to +273
### Ephemeral volumes managed by kubelet

See [local ephemeral storage](/docs/concepts/configuration/manage-resources-containers/#local-ephemeral-storage).

### CSI ephemeral volumes

- For more information on the design, see the [Ephemeral Inline CSI
volumes KEP](https://github.com/kubernetes/enhancements/blob/ad6021b3d61a49040a3f835e12c8bb5424db2bbb/keps/sig-storage/20190122-csi-inline-volumes.md).
- For more information on further development of this feature, see the [enhancement tracking issue #596](https://github.com/kubernetes/enhancements/issues/596).

### Generic ephemeral volumes

- For more information on the design, see the
[Generic ephemeral inline volumes KEP](https://github.com/kubernetes/enhancements/blob/master/keps/sig-storage/1698-generic-ephemeral-volumes/README.md).
- For more information on further development of this feature, see the [enhancement tracking issue #1698](https://github.com/kubernetes/enhancements/issues/1698).
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a little tidier (for example, we usually and intentionally don't link to KEPs other than for historical context), but I'm happy to log a separate issue to track tidying that up after this merges. It's a minor detail for sure.

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 think that for alpha and beta features, easy access to both the KEP and the enhancement issue are important: the KEP explains "why" the feature is what it is and in particular, what other alternatives were considered. The enhancement issue answers the questions "when will this graduate" and "what is still changing with this feature".

I can remove it in a follow-up PR if this explanation is not convincing enough. But for now, let's merge.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm

content/en/docs/concepts/storage/ephemeral-volumes.md Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 14, 2020
@sftim
Copy link
Contributor

sftim commented Jul 14, 2020

(not sure what GitHub is playing at there, but it does LGTM)

Co-authored-by: Tim Bannister <tim@scalefactory.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2020
@pohly
Copy link
Contributor Author

pohly commented Jul 15, 2020

New changes are detected. LGTM label has been removed.

The only change was the formatting fix from @sftim, so his and @msau42's LGTM still apply.

@savitharaghunathan
Copy link
Member

@sftim's comment on formatting has been addressed.

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: savitharaghunathan

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 Jul 15, 2020
@k8s-ci-robot k8s-ci-robot merged commit 3f7af5d into kubernetes:dev-1.19 Jul 15, 2020
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