CSI: send pod information in NodePublishVolumeRequest #2439

jsafrane · 2018-07-31T12:41:36Z

This proposal adds possibility to send pod information in CSI NodePublishVolumeRequest call.

It depends on #2514

/sig storage

tamalsaha · 2018-07-31T14:51:16Z

contributors/design-proposals/storage/container-storage-interface-pod-information.md

+	*	`csi.kubernetes.io/pod.name`: name of the pod that wants the volume.
+	*	`csi.kubernetes.io/pod.namespace`: namespace of the pod that wants the volume.
+	*	`csi.kubernetes.io/pod.uid`: uid of the pod that wants the volume.
+	*	`csi.kubernetes.io/serviceAccount.name`: name of the service account under which the pod operates. Namespace is the same as `pod.namespace`.


Should be the annotation names be like x.y.z/w1-w2-w3 ?

Example: https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/aws/aws.go#L98

It's not annotation.

As I wrote, I follow names introduced in Flex, but I don't have strong opinion about the format. CSI itself does not describe how volume_attributes should look like.

kfox1111

In general, looks very good. :) A couple of things inline.

kfox1111 · 2018-07-31T15:51:55Z

contributors/design-proposals/storage/container-storage-interface-pod-information.md

+## Detailed design
+
+### CSI enhancement
+We don't need to change CSI protocol in any way. It allows kubelet to pass `pod.name`, `pod.uid` and `pod.spec.serviceAccountName` in [`NodePublish` call as `volume_attributes`]((https://github.com/container-storage-interface/spec/blob/master/spec.md#nodepublishvolume)). `NodePublish` is roughly equivalent to Flex `mount` call.


The NodeServer has two sets of calls that I think are applicable. NodePublishVolume/Unpublish, but also NodeStageVolume/Unstage https://github.com/container-storage-interface/spec/blob/master/spec.md#nodestagevolume. Those calls also need the additional info to function properly.

In case several pods use the same volume on the same node, NodeStageVolume is called only once. It would include just the first Pod. NodePublishVolume is called separately for each pod that uses the volume and CSI driver can do the right thing there.

In addition, Flex driver did not get any pod in mountdevice call (which is equivalent of NodeStageVolume in CSI and nobody complained.

Flex did not provide pod information in unmount (=NodeUnpublishVolume) and nobody complained. Do you have concrete use case when pod can be useful in NodeUnpublishVolume?

Oh, really? I thought stage was called once per pod, but mount was called per container mounting it? I guess I was mistaken. So I'll have to rewrite the image driver to have those semantics then. Thanks.

@jsafrane For inline CSI volume, what would be the equivalent of flexvolume's options? Currently we can only pass in volumeAttributes to csi as part of a PersistentVolume deployment. Is there a way to support volumeAttributes as part of inline CSI volume as part of the pod spec?

See kubernetes/enhancements#716

kfox1111 · 2018-07-31T15:56:07Z

contributors/design-proposals/storage/container-storage-interface-pod-information.md

+9. CSI volume plugin stores this flag for the CSI driver.
+
+When a volume is being mounted for a pod:
+


optional csi stage call here

I added NodeStageVolume to the example, with a note that it won't get any pod information.

Ok. Thanks. The clarity will help others writing drivers.

sarjeet2013 · 2018-07-31T21:18:13Z

contributors/design-proposals/storage/container-storage-interface-pod-information.md

+
+### CSI driver wants pod information in `NodePublish`
+1. CSI driver author provides documentation and templates that the driver need  pod information in `NodePublish`.
+2. Admin installs CSI driver with CSI Driver Registrar `--requires-publish-pod`, e.g. by applying helm template from CSI diver author.


s/diver/driver

Fixed, thanks

jsafrane · 2018-08-10T13:41:16Z

We're considering using cluster-global API object to hold "configuration" of CSI driver in Kubernetes, i.e. how Kubernetes talks to the driver. It should contain also a flag if kubelet should pass pod name/namespace/uid in NodePublish. We won't need new gRPC API in kubelet this way.

The design is it flux right now, stay tuned.

kfox1111 · 2018-08-13T16:38:13Z

hmm. cluster global info might allow for pod inline validation to happen earlier too. +1.

jsafrane · 2018-08-15T12:49:32Z

I reworked the proposal to use CSIDriver introduced in #2514. It's much shorter and easier now.

jsafrane · 2018-08-15T12:50:03Z

And I kept the original commit there, just in case we need to revert to it.

kfox1111

looks good to me. just a couple of typo's to fix.

kfox1111 · 2018-08-15T15:37:44Z

contributors/design-proposals/storage/container-storage-interface-pod-information.md

+## High-level design
+We decided to pass the pod information as `NodePublishVolumeRequest.volume_attributes`.
+
+* Kubernetes passes pod information only to CSI drivers that explicitly require that information in their [`CSIDriver` instance](https://github.com/kubernetes/community/pull/2523). These drivers are tightly coupled to Kubernetes and may not work or may require reconfiguration on other cloud orchestrators. It is expected (but not limited to) that these drivers will provider ephemeral volumes similar to Secrets or ConfigMap, extending Kubernetes secret or configuration sources.


typo at 'provider ephemeral'

fixed, thanks!

kfox1111 · 2018-08-15T15:39:01Z

contributors/design-proposals/storage/container-storage-interface-pod-information.md

+We decided to pass the pod information as `NodePublishVolumeRequest.volume_attributes`.
+
+* Kubernetes passes pod information only to CSI drivers that explicitly require that information in their [`CSIDriver` instance](https://github.com/kubernetes/community/pull/2523). These drivers are tightly coupled to Kubernetes and may not work or may require reconfiguration on other cloud orchestrators. It is expected (but not limited to) that these drivers will provider ephemeral volumes similar to Secrets or ConfigMap, extending Kubernetes secret or configuration sources.
+* Kubernetes will not pass pod information to CSI drivers that don't know or don't care about pods and service accounts. It is expected (but not limited to) that these drivers will provide real persistent storage. Such CSI driver would reject a CSI call with pod information as invalid. This is current behavior of Kubernete and it will be the default behavior.


Kubernete typo

kfox1111

can we add one more that I think will be useful?
csi.kubernetes.io/volume.name: name of the volume as specified in the spec.

ex:

volumes:
  - name: foo
    csi:
      driver: image

it would be "foo" in this case.

jsafrane · 2018-08-21T16:00:26Z

@kfox1111, do you have concrete use case for csi.kubernetes.io/volume.name? I am not against per se, but I'd like to know why can be this information useful.

jsafrane · 2018-08-21T16:07:08Z

contributors/design-proposals/storage/container-storage-interface-pod-information.md

+	*	`csi.kubernetes.io/pod.name`: name of the pod that wants the volume.
+	*	`csi.kubernetes.io/pod.namespace`: namespace of the pod that wants the volume.
+	*	`csi.kubernetes.io/pod.uid`: uid of the pod that wants the volume.
+	*	`csi.kubernetes.io/serviceAccount.name`: name of the service account under which the pod operates. Namespace of the service account is the same as `pod.namespace`.


The strings above define sort of API between Kubernetes and kubernetes-aware CSI drivers. @liggitt, does it need any special approval? It will be set to stone when we implement this proposal.

Background: volume_attributes is generic map[string]string, without any prescribed format of keys in CSI standard. I used the above keys because similar ones are already used in flex volumes. Now is the time to change the keys if we don't like them.

kfox1111 · 2018-08-21T16:11:48Z

At very least, volume.name + pod.id is a guaranteed unique identifier for the volume instance. So could be useful to drivers that are pod aware and want to track lifecycle? There may be other uses too. Haven't thought too much about it.

saad-ali

LGTM other then one comment.

saad-ali · 2018-08-27T16:38:09Z

contributors/design-proposals/storage/container-storage-interface-pod-information.md

+We don't need to change CSI protocol in any way. It allows kubelet to pass `pod.name`, `pod.uid` and `pod.spec.serviceAccountName` in [`NodePublish` call as `volume_attributes`]((https://github.com/container-storage-interface/spec/blob/master/spec.md#nodepublishvolume)). `NodePublish` is roughly equivalent to Flex `mount` call.
+
+The only thing we need to do is to **define** names of the `volume_attributes` keys that CSI drivers can expect:
+	*	`csi.kubernetes.io/pod.name`: name of the pod that wants the volume.


csi.storage.kubernetes.io to align with CSIDriver group name

Renamed to csi.storage.k8s.io/*

jsafrane · 2018-08-28T12:39:45Z

Renamed parameters to csi.storage.k8s.io/* and squashed.

jsafrane · 2018-08-28T12:42:49Z

And added feature gate CSIPodInfo.

jsafrane · 2018-08-28T12:50:35Z

Implementation: kubernetes/kubernetes#67945

@saad-ali

Automatic merge from submit-queue (batch tested with PRs 68171, 67945, 68233). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. CSI pod information in NodePublish **What this PR does / why we need it**: This is implementation of kubernetes/community#2439. It brings CSI closer to Flex volume features, CSI driver can (optionally) get name/namespace/UID/ServiceAccount of the pod that requires the CSI volume mounted. This allows CSI drivers to either do AAA before the volume is mounted or tailor content of volume to the pod. Work in progress: * contains #67803 to get `CSIDriver` API. Ignore the first commit. Related to #64984 #66362 (fixes only part of these issues) /sig storage cc: @saad-ali @vladimirvivien @verult @msau42 @gnufied **Release note**: ```release-note CSI NodePublish call can optionally contain information about the pod that requested the CSI volume. ```

fejta-bot · 2019-01-20T07:31:47Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

saad-ali · 2019-01-29T23:02:47Z

This was implemented as alpha in k8s 1.12 (kubernetes/kubernetes#67945). Merging the doc so that it can be updated.

/approve
/lgtm

k8s-ci-robot · 2019-01-29T23:02:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, saad-ali

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:

~~contributors/design-proposals/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

CSI: send pod information in NodePublishVolumeRequest

k8s-ci-robot requested review from childsb and saad-ali July 31, 2018 12:41

k8s-ci-robot added kind/design Categorizes issue or PR as related to design. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 31, 2018

This was referenced Jul 31, 2018

Pass Pod information in CSI calls kubernetes/enhancements#603

Closed

Implementation gaps between FlexVolume and CSI for node-local volumes kubernetes/kubernetes#64984

Closed

tamalsaha reviewed Jul 31, 2018

View reviewed changes

kfox1111 suggested changes Jul 31, 2018

View reviewed changes

sarjeet2013 reviewed Jul 31, 2018

View reviewed changes

jsafrane force-pushed the csi-pod-info branch from cdc574d to a1e786f Compare August 1, 2018 11:41

jsafrane force-pushed the csi-pod-info branch from a1e786f to 7dc18e4 Compare August 15, 2018 12:47

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 15, 2018

kfox1111 reviewed Aug 15, 2018

View reviewed changes

kfox1111 suggested changes Aug 15, 2018

View reviewed changes

kfox1111 mentioned this pull request Aug 16, 2018

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

Closed

jsafrane force-pushed the csi-pod-info branch from 7dc18e4 to f2a35ff Compare August 21, 2018 15:58

jsafrane commented Aug 21, 2018

View reviewed changes

saad-ali reviewed Aug 27, 2018

View reviewed changes

CSI: send pod information in NodePublishVolumeRequest

5068303

jsafrane force-pushed the csi-pod-info branch from f2a35ff to 333523e Compare August 28, 2018 12:38

jsafrane force-pushed the csi-pod-info branch 2 times, most recently from b97efb4 to 5068303 Compare August 28, 2018 12:42

jsafrane mentioned this pull request Aug 28, 2018

CSI pod information in NodePublish kubernetes/kubernetes#67945

Merged

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 20, 2019

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

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 29, 2019

k8s-ci-robot merged commit 7fadb45 into kubernetes:master Jan 29, 2019

MadhavJivrajani pushed a commit to MadhavJivrajani/community that referenced this pull request Nov 30, 2021

Merge pull request kubernetes#2439 from jsafrane/csi-pod-info

9386e70

CSI: send pod information in NodePublishVolumeRequest

		9. CSI volume plugin stores this flag for the CSI driver.

		When a volume is being mounted for a pod:

CSI: send pod information in NodePublishVolumeRequest #2439

CSI: send pod information in NodePublishVolumeRequest #2439

Conversation

jsafrane commented Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfox1111 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane Aug 1, 2018 • 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

Choose a reason for hiding this comment

jsafrane commented Aug 10, 2018

kfox1111 commented Aug 13, 2018

jsafrane commented Aug 15, 2018

jsafrane commented Aug 15, 2018

kfox1111 left a 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 left a comment • edited Loading

Choose a reason for hiding this comment

jsafrane commented Aug 21, 2018

Choose a reason for hiding this comment

kfox1111 commented Aug 21, 2018

saad-ali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented Aug 28, 2018

jsafrane commented Aug 28, 2018

jsafrane commented Aug 28, 2018

fejta-bot commented Jan 20, 2019

saad-ali commented Jan 29, 2019

k8s-ci-robot commented Jan 29, 2019

jsafrane commented Jul 31, 2018 •

edited

Loading

jsafrane Aug 1, 2018 •

edited

Loading

kfox1111 left a comment •

edited

Loading