CSI inline volume support KEP with suggested updates #790

vladimirvivien · 2019-01-30T19:20:42Z

This is to add changes suggested by @liggitt after KEP merged.
These are mostly cosmetic with design updates that still keep the KEP status implementable.

Original KEP - #716

vladimirvivien · 2019-01-30T19:21:02Z

/sig storage

vladimirvivien · 2019-01-30T19:25:10Z

/assign @saad-ali

PTAL

vladimirvivien · 2019-01-30T19:37:57Z

/hold

keps/sig-storage/20190122-csi-inline-volumes.md

vladimirvivien · 2019-01-31T23:57:26Z

/rm hold

vladimirvivien · 2019-01-31T23:58:12Z

/hold cancel

keps/sig-storage/20190122-csi-inline-volumes.md

vladimirvivien · 2019-02-01T21:35:18Z

@kfox1111 @saad-ali PTAL

keps/sig-storage/20190122-csi-inline-volumes.md

liggitt · 2019-02-01T21:56:30Z

keps/sig-storage/20190122-csi-inline-volumes.md

 * The Kubelet and external CSI components must ensure secret references can only be used with the namespace from inline pod spec.
+* Persistent inline volumes will not support secrets during attach/detach operations (see above for detail)


we should limit the secret fields to ones we intend to support (drop ControllerPublishSecretRef) in the inline CSIVolumeSource typedef above

Yes. Agreed!

liggitt · 2019-02-01T21:56:58Z

this flow makes a lot more sense to me, thanks for the update

kfox1111

lgtm

keps/sig-storage/20190122-csi-inline-volumes.md

liggitt · 2019-02-05T17:57:32Z

The external CSI A/D controller copies whole VolumeSource from Pod into VolumeAttachment. This allows external CSI attacher to detach volumes for deleted pods without keeping any internal database of attached VolumeSources.

It's worth calling out that when multiple pods with the same node + driver + volumeHandle exist, only the volumeSource of the first pod encountered by the attacher controller is effective in specifying parameters used by the attacher. Since that can result in extremely hard-to-understand behavior, it's important to make sure the approvers understand/agree with that outcome.

kfox1111 · 2019-02-21T20:26:55Z

keps/sig-storage/20190122-csi-inline-volumes.md

-* The driver can support PV/PVC originated volume (normal operation).
-* If the volume originates from a pod spec, it is assumed that its lifecycle is `persistent`.
+## Ephemeral inline volume proposal
+A CSI driver may be able to support either PV/PVC-originated or pod spec originated volumes. When a volume definition is embedded inside a pod spec, it is considered to be an `ephemeral inline` volume request and can only participate in *mount/unmount* volume operation calls.  Ephemeral inline volume requests have the following characteristics: 


Previously we added a flag in CSIDriver CRD to ensure that the driver was capable of operating properly as an ephemeral driver. I think this is still important.

What would the flag be used to gate?

it would prevent calling mount/unmount on a CSI driver for an inline ephemeral volume source, if the driver did not support ephemeral volumes

otherwise we're assuming a persistent volume CSI driver will fail in a safe/performant/reliable way when asked to mount a bogus (generated) volumeHandle for an inline ephemeral volumesource, which doesn't seem great.

Ok to summarize, it would be used to gate the auto-generation of a volume handle during mount/unmount. And this is to protect persistent drivers, not to protect ephemeral drivers.

And this is to protect persistent drivers, not to protect ephemeral drivers.

protection would go both ways, I'd imagine (ephemeral drivers wouldn't be eligible for being called to satisfy storageclass or PV-related things)

@msau42 OK. The flag will be a string where the current supported value is "ephemeral". Future implementation may define a new value. A value of "persistent" does not make sense right now since the code will not support that feature.

I will update the KEP again to reflect that and the code.

We still do want persistent right? Persistent supports provision, attach, resize, mount, etc, but does not support volume handle generation. Ephemeral only supports mount/unmount and volume handle generation.

@msau42
It sounded like here (kubernetes/kubernetes#74086 (comment)) that persistent inline CSI would not be supported at all. Maybe I am missing something. Let me know what you think. If you think we will come back (beta) to support persistent inline CSI, then I will word the KEP.

Persistent inline is not supported. However, it may still be useful to distinguish between persistent and ephemeral drivers in general due to their different lifecycles

@msau42 I see.
OK. I see your point. I will rename the flag to reflect that distinction CSIDriver.Mode = {ephemeral|persistent}. Thanks for clarifying.

kfox1111 · 2019-02-21T20:30:39Z

Just the one issue I think.

msau42 · 2019-02-21T21:43:15Z

keps/sig-storage/20190122-csi-inline-volumes.md

-* The driver can support PV/PVC originated volume (normal operation).
-* If the volume originates from a pod spec, it is assumed that its lifecycle is `persistent`.
+## Ephemeral inline volume proposal
+A CSI driver may be able to support either PV/PVC-originated or pod spec originated volumes. When a volume definition is embedded inside a pod spec, it is considered to be an `ephemeral inline` volume request and can only participate in *mount/unmount* volume operation calls.  Ephemeral inline volume requests have the following characteristics: 


What would the flag be used to gate?

keps/sig-storage/20190122-csi-inline-volumes.md

liggitt · 2019-02-21T21:57:54Z

keps/sig-storage/20190122-csi-inline-volumes.md

-* Two pods accessing the same inline persistent volumes
-* CSI attacher code invokes driver operation during attach for persistent volumes
-* CSI detacher code invokes driver operation during detach for persistent volumes
+* Two pods accessing the same ephemeral inline volumes


not sure what "the same ephemeral inline volumes" means... the volumes are per pod

Say you create a deployment with:

volumes: - name: foo csi: driver: myephemeraldriver

Two pods could land on the same node. If pod A writes to the foo volume, and pod B reads from it, it really should be a different volume between the two, even though they are defined the same in the pod. We should check to ensure that is the case.

ah, verifying an identical inline definition results in distinct volume instances. sounds good.

a single pod with two inline ephemeral volumes with identical definitions (other than name) might be easier to test (guaranteed to be on the same node) and test uniqueness down to the subpod level.

I think both test cases are important to have. One validates that the volumehandle is unique per volume name. And the other validates that the volumehandle is unique per pod.

You can use pod affinity to make sure two pods are on the same node.

Yeah, both tests are important. While writing the KEP we found problems in the KEP for each case uniquely. The KEP has it fixed now, but should have tests. :)

Will update language to indicate what's being tested

verifying an identical inline definition results in distinct volume instances

a single pod with two inline ephemeral volumes with identical definitions (other than name)

vladimirvivien · 2019-02-22T16:12:24Z

@msau42 @liggitt @kfox1111 PTAL

vladimirvivien · 2019-02-22T16:13:59Z

@jsafrane PTAL

msau42 · 2019-02-22T19:15:07Z

lgtm. Will let @kfox1111 and @liggitt review

vladimirvivien · 2019-02-22T22:45:45Z

@liggitt @saad-ali PTAL, Looking for a lgtm/approval so this can be merged.

vladimirvivien · 2019-02-23T01:58:11Z

@msau42 @kfox1111 PTAL

msau42 · 2019-02-23T02:08:54Z

keps/sig-storage/20190122-csi-inline-volumes.md

@@ -151,12 +151,20 @@ type CSIVolumeSource struct {
 ```

 ### Driver inline mode
-To indicate that a driver is setup to handle ephemeral inline volume requests, the existing `CSIDriver` object will be updated to include boolean attribute `InlineMode`.  When `CSIDriver.InlineMode == true` the followings are assumed:
+To indicate that the driver will support ephemeral inline volume requests, the existing `CSIDriver` object will be extended to include attribute `InlineMode`.  Currently the only InlineMode that will be supported is the `ephemeral` mode.  


Sorry I think we should go back to the field from the original proposal that had both "ephemeral" and "persistent". It makes it easier to add runtime validation when we make all the CSI calls.

I tend to agree. I wasn't anticipating this much of a rework in response to Tim's suggestion to defer persistent inline volumes

Yes, that's we'll do - #790 (comment)

The rework is more design then code. The code will be simpler once we agree on the KEP.

vladimirvivien · 2019-02-23T15:53:55Z

Thanks for all of the reviews.

krmayankk · 2019-02-23T18:11:58Z

keps/sig-storage/20190122-csi-inline-volumes.md

 creation-date: 2019-01-22
 status: implementable
 ---

-# Inline CSI volumes
+# Ephemeral inline CSI volumes


nit: Inline ?

vladimirvivien · 2019-02-25T16:51:34Z

@msau42 @liggitt @kfox1111 PTAL

kfox1111 · 2019-02-25T17:00:56Z

keps/sig-storage/20190122-csi-inline-volumes.md

+* The driver will not receive separate gRPC calls for provisioning, attaching, detaching, and deleting of volumes.
+* The driver is responsible for implementing steps to ensure the volume is created and made avaiable to the pod during mount call
+* The driver is responsible for implementing steps to delete and clean up any volume and resources during the unmount call.
+


Maybe two more while we're at it:

The mount could happen multiple times for the same volumeHandle. The first should allocate all backing volume data.

The unmount could happen multiple times for the same volumeHandle. The first should remove any backing volume data. It will be called at a minimum one time. No volume accesses will be made after the first call.

@kfox1111 I will highlight your points.

kfox1111 · 2019-02-25T17:01:04Z

keps/sig-storage/20190122-csi-inline-volumes.md

+* Ensure volumeHandle conforms to resource naming format
+* CSIVolumeSource info persists in CSI json file during mount/unmount
+* Ensure Kubelet skips attach/detach when `CSIDriver.InlineMode = true`
+* Ensure Kubelet skips inline logic when `CSIDriver.InlineMode = false` 


Still a couple of references to InlineMode here.

msau42 · 2019-02-25T19:41:38Z

keps/sig-storage/20190122-csi-inline-volumes.md


+When `CSIDriver.Mode == <not specified>` or when `CSIDriver.Mode == persistent`, the driver will function as normal supporting only PV/PVC-requested volumes and


Should Mode be defaulted to persistent?

Mode defaults to <unspecified> so we don't force reconfiguration of existing deployed drivers. So unspecified === persistent.

msau42 · 2019-02-25T19:44:08Z

keps/sig-storage/20190122-csi-inline-volumes.md

+* Kubelet will auto-generate a volumeHandle based on `podUID` and `pod.spec.volume[x].name` (see above for detail).
+* CSI driver will receive mount-like calls (NodePublish) with generated paths and generated volumeHandle.
+
+Since ephemeral volume requests will participate in only the mount/unmount volume operation phase, CSI drivers are responsible for implementing all necessary operations during that phase (i.e. create, mount, unmount, delete, etc).  For instance, a driver would be responsible for provisioning any new volume resource during the `NodePublishStage` and for tearing down these resources during the `NodeUnpublish` call.


nit: NodePublish stage. To not confuse it with NodeStage

msau42 · 2019-02-25T19:44:45Z

lgtm, just some minor comments.

msau42 · 2019-02-26T22:59:33Z

/lgtm

k8s-ci-robot · 2019-02-26T22:59:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, msau42, saad-ali, vladimirvivien

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:

~~keps/sig-storage/OWNERS~~ [saad-ali]

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

kfox1111 · 2019-02-26T23:15:49Z

Great work everyone! :)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 30, 2019

k8s-ci-robot requested review from childsb and saad-ali January 30, 2019 19:20

k8s-ci-robot assigned saad-ali Jan 30, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 30, 2019

liggitt reviewed Jan 30, 2019

View reviewed changes

keps/sig-storage/20190122-csi-inline-volumes.md Outdated Show resolved Hide resolved

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 31, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 31, 2019

kfox1111 reviewed Feb 1, 2019

View reviewed changes

keps/sig-storage/20190122-csi-inline-volumes.md Outdated Show resolved Hide resolved

kfox1111 reviewed Feb 1, 2019

View reviewed changes

keps/sig-storage/20190122-csi-inline-volumes.md Outdated Show resolved Hide resolved

keps/sig-storage/20190122-csi-inline-volumes.md Outdated Show resolved Hide resolved

keps/sig-storage/20190122-csi-inline-volumes.md Outdated Show resolved Hide resolved

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 Feb 1, 2019

liggitt reviewed Feb 1, 2019

View reviewed changes

kfox1111 reviewed Feb 1, 2019

View reviewed changes

liggitt reviewed Feb 1, 2019

View reviewed changes

keps/sig-storage/20190122-csi-inline-volumes.md Outdated Show resolved Hide resolved

liggitt mentioned this pull request Feb 4, 2019

Webhook admission KEP updates #806

Merged

kfox1111 reviewed Feb 21, 2019

View reviewed changes

msau42 reviewed Feb 21, 2019

View reviewed changes

liggitt reviewed Feb 21, 2019

View reviewed changes

keps/sig-storage/20190122-csi-inline-volumes.md Outdated Show resolved Hide resolved

liggitt reviewed Feb 21, 2019

View reviewed changes

kfox1111 mentioned this pull request Feb 22, 2019

REQUEST: New membership for kfox1111 kubernetes/org#518

Closed

6 tasks

msau42 mentioned this pull request Feb 22, 2019

CSI Inline Volume Support #596

Closed

vladimirvivien mentioned this pull request Feb 22, 2019

CSI Inline Volume Implementation kubernetes/kubernetes#74086

Merged

msau42 mentioned this pull request Feb 22, 2019

CSI: support for in-line volumes in pods. kubernetes/community#2273

Closed

msau42 reviewed Feb 23, 2019

View reviewed changes

krmayankk reviewed Feb 23, 2019

View reviewed changes

kfox1111 reviewed Feb 25, 2019

View reviewed changes

msau42 reviewed Feb 25, 2019

View reviewed changes

kfox1111 mentioned this pull request Feb 25, 2019

Make CSINodeInfo and CSIDriver Core APIs kubernetes/kubernetes#73883

Merged

CSI inline volume support for only ephemeral mode

a506dc2

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2019

k8s-ci-robot merged commit fc820ae into kubernetes:master Feb 26, 2019

justaugustus removed the sig/pm label Apr 19, 2020

		* The Kubelet and external CSI components must ensure secret references can only be used with the namespace from inline pod spec.
		* Persistent inline volumes will not support secrets during attach/detach operations (see above for detail)


		When `CSIDriver.Mode == <not specified>` or when `CSIDriver.Mode == persistent`, the driver will function as normal supporting only PV/PVC-requested volumes and

CSI inline volume support KEP with suggested updates #790

CSI inline volume support KEP with suggested updates #790

Conversation

vladimirvivien commented Jan 30, 2019 • edited Loading

vladimirvivien commented Jan 30, 2019

vladimirvivien commented Jan 30, 2019

vladimirvivien commented Jan 30, 2019

vladimirvivien commented Jan 31, 2019

vladimirvivien commented Jan 31, 2019

vladimirvivien commented Feb 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Feb 1, 2019

kfox1111 left a comment

Choose a reason for hiding this comment

liggitt commented Feb 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfox1111 commented Feb 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirvivien commented Feb 22, 2019

vladimirvivien commented Feb 22, 2019

msau42 commented Feb 22, 2019

vladimirvivien commented Feb 22, 2019

vladimirvivien commented Feb 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirvivien commented Feb 23, 2019

Choose a reason for hiding this comment

vladimirvivien commented Feb 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirvivien Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Feb 25, 2019

msau42 commented Feb 26, 2019

k8s-ci-robot commented Feb 26, 2019

kfox1111 commented Feb 26, 2019

vladimirvivien commented Jan 30, 2019 •

edited

Loading

liggitt commented Feb 5, 2019 •

edited

Loading

liggitt Feb 21, 2019 •

edited

Loading

vladimirvivien Feb 25, 2019 •

edited

Loading