csi-inline-volumes.md: prepare for beta #1172

pohly · 2019-07-26T12:38:16Z

This adds the implementation history and checks which of the proposed
tests are implemented.

/cc @vladimirvivien @kfox111 @msau42

pohly · 2019-07-26T12:38:45Z

What else might be needed in the KEP update if we want to go beta in 1.16?

pohly · 2019-07-26T12:39:18Z

/cc @kfox1111

k8s-ci-robot · 2019-07-26T12:39:18Z

@pohly: GitHub didn't allow me to request PR reviews from the following users: kfox1111.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @kfox1111

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.

pohly · 2019-07-26T12:42:01Z

@vladimirvivien, @kfox1111: can you help identify more tests that have already been implemented?

kfox1111 · 2019-07-26T15:47:36Z

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

-* Two pods accessing the same ephemeral inline volumes
-* Single pod referencing two distinct inline volume request from the same driver 
+* Pod spec with an ephemeral inline volume request can be mounted/unmounted: https://github.com/pohly/kubernetes/blob/4bc5d065c919fc239e2c8b40e6a96e409ca011fd/test/e2e/storage/csi_mock_volume.go#L356-L371, https://github.com/pohly/kubernetes/blob/4bc5d065c919fc239e2c8b40e6a96e409ca011fd/test/e2e/storage/testsuites/ephemeral.go#L110-L115
+* Two pods accessing the same ephemeral inline volume


How about:
Two pods configured the same (.spec.volumes.name, driver, args) get different volumes.

What is the definition of "different volumes"?

I changed the test case description to "two pods accessing an ephemeral inline volume which has the same attributes in both pods" and the corresponding test creates a second pod, but I don't have any sanity testing for "different volumes".

@kfox1111 we can still work on the test cases later. This shouldn't block merging this KEP.

For example, say you made pod A that has a hostpath ephemeral volume named 'foo' and a command [/bin/sh, -c, 'hostname > /ephemeralmountpoint/hello.txt']

You could wait until you saw that pod go up and the hello.txt get created.

You could then create a second pod, on the same host with a volume named 'foo' configured the same way with:
command [/bin/sh, -c, 'while true; do sleep 1000; done']
And see if you can read /ephemeralmountpoint/hello.txt out of the second container?

So "different volumes" basically means "changes not propagated". Yes, that can be tested.

In the meantime, are you okay with merging this PR?

Now that I started to think about it... who guarantees that two pods with the same (as in volume attributes) volume end up with separate volumes? The entire semantic of those volumes is left to the CSI driver. For example, a CSI driver might take no attributes and connect all pods to the same NFS share. That wouldn't be a very useful driver, but it wouldn't be invalid.

I think the test that I have written so far is useful (covers a potential corner case), but we can't make it more strict and keep it generic at the same time.

the ephemeral volumeHandle generation mechanism ensures that two different pods with the same exact config on the same host, or the same pod with the same config but different volume names each are different volumeHandles.

For example, very psudocody:

kind: Deployment spec: replicas: 2 spec: nodeSelector: samenode volumes: - name: foo csi: driver: hostpath.k8s.io

Creates two different volumes, one per pod. Each pod's volumeHandle is different.

This one also creates two different backing volumes. Two different volumeHandles:

kind: Pod spec: volumes: - name: foo csi: driver: hostpath.k8s.io - name: bar csi: driver: hostpath.k8s.io

In each case, with the ephemeral hostpath driver backing it, it would be the equivalent behavior of using emptyDir: {} instead of csi. No sharing between volumes.

Your technically right, the volume physically being different could be an implemntation detail of the driver. So it could share a read only filesystem and do some careful reference counting. But the above two tests are really the ones I'm interested in making sure work. If they work, other drivers will work properly. Its basically, does the ephemeral volumeHandle generation work properly all the way to the driver. Then the driver can do what it wants with that.

So you want to test volume handle generation, not the behavior of the resulting volumes.

pohly · 2019-07-30T07:30:26Z

@msau42 can you approve and merge this now?

pohly · 2019-07-30T07:31:20Z

/assign @saad-ali

Saad, please merge. The discussion with @kfox1111 about testing isn't blocking the KEP.

kfox1111 · 2019-07-30T15:35:01Z

This looks good to merge to me. We're still debating the test implementation, not that the tests do/don't need to be done.

msau42 · 2019-07-30T15:37:12Z

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

+  cannot determine the mode in its `NodePublishVolume` implementation
+
+1.16:


Can you note that this is still in progress for 1.16?

I linked to the two PRs instead, with comments about "merged" and "pending".

msau42 · 2019-07-30T15:38:07Z

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

+* CSI Kubelet cleans up ephemeral volume paths once pod goes away: TODO
+* Apply PodSecurity settings for allowed CSI drivers: TODO
+


Also add a line for adding an ephemeral driver to the external test suite

Added this:

Enable testing of an external ephemeral CSI driver: https://github.com/kubernetes/kubernetes/pull/79983/files#diff-e5fc8d9911130b421b74b1ebc273f458

Enable testing of the csi-host-path-driver in ephemeral mode in Kubernetes-CSI Prow jobs and Kubernetes itself: TODO

This adds the implementation history and checks which of the proposed tests are implemented.

msau42 · 2019-07-30T16:57:14Z

/lgtm

saad-ali

/approve
/lgtm

k8s-ci-robot · 2019-07-30T18:15:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, pohly, 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:

~~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

k8s-ci-robot requested review from msau42 and vladimirvivien July 26, 2019 12:38

pohly force-pushed the ephemeral-inline-volume-beta branch from 67d1638 to 9767794 Compare July 26, 2019 14:33

kfox1111 reviewed Jul 26, 2019

View reviewed changes

pohly force-pushed the ephemeral-inline-volume-beta branch from 9767794 to 994536e Compare July 29, 2019 15:50

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

msau42 reviewed Jul 30, 2019

View reviewed changes

csi-inline-volumes.md: prepare for beta

c079b27

This adds the implementation history and checks which of the proposed tests are implemented.

pohly force-pushed the ephemeral-inline-volume-beta branch from 994536e to c079b27 Compare July 30, 2019 16:10

k8s-ci-robot assigned msau42 Jul 30, 2019

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

saad-ali approved these changes Jul 30, 2019

View reviewed changes

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

k8s-ci-robot merged commit cc36040 into kubernetes:master Jul 30, 2019

saad-ali mentioned this pull request Jul 28, 2022

CSI Inline Volume Support #596

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

csi-inline-volumes.md: prepare for beta #1172

csi-inline-volumes.md: prepare for beta #1172

pohly commented Jul 26, 2019

pohly commented Jul 26, 2019

pohly commented Jul 26, 2019

k8s-ci-robot commented Jul 26, 2019

pohly commented Jul 26, 2019

kfox1111 Jul 26, 2019

pohly Jul 29, 2019

pohly Jul 29, 2019

kfox1111 Jul 29, 2019

pohly Jul 29, 2019

pohly Jul 29, 2019

kfox1111 Jul 29, 2019

kfox1111 Jul 29, 2019

pohly Jul 30, 2019

kfox1111 Jul 30, 2019

pohly commented Jul 30, 2019

pohly commented Jul 30, 2019

kfox1111 commented Jul 30, 2019

msau42 Jul 30, 2019

pohly Jul 30, 2019

msau42 Jul 30, 2019

pohly Jul 30, 2019

msau42 commented Jul 30, 2019

saad-ali left a comment

k8s-ci-robot commented Jul 30, 2019

		cannot determine the mode in its `NodePublishVolume` implementation

		1.16:

		* CSI Kubelet cleans up ephemeral volume paths once pod goes away: TODO
		* Apply PodSecurity settings for allowed CSI drivers: TODO

csi-inline-volumes.md: prepare for beta #1172

csi-inline-volumes.md: prepare for beta #1172

Conversation

pohly commented Jul 26, 2019

pohly commented Jul 26, 2019

pohly commented Jul 26, 2019

k8s-ci-robot commented Jul 26, 2019

pohly commented Jul 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Jul 30, 2019

pohly commented Jul 30, 2019

kfox1111 commented Jul 30, 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

msau42 commented Jul 30, 2019

saad-ali left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 30, 2019