persistent and ephemeral csi volumes #79983

pohly · 2019-07-10T12:22:19Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

This is part of the effort to move CSI inline ephemeral volumes to beta.

Which issue(s) this PR fixes:
Related-to: #79624

Does this PR introduce a user-facing change?:

when PodInfoOnMount is enabled for a CSI driver, the new csi.storage.k8s.io/ephemeral parameter in the volume context allows a driver's NodePublishVolume implementation to determine on a case-by-case basis whether the volume is ephemeral or a normal persistent volume

pohly · 2019-07-10T12:22:52Z

/assign @vladimirvivien
/sig-storage

pohly · 2019-07-10T12:23:29Z

/sig storage
/priority important

cwdsuzhou · 2019-07-10T12:44:59Z

pkg/volume/csi/csi_mounter.go

@@ -60,7 +61,7 @@ type csiMountMgr struct {
 	k8s            kubernetes.Interface
 	plugin         *csiPlugin
 	driverName     csiDriverName
-	driverMode     driverMode
+	volumeMode     volumeMode


If volumeMode would be misleading or confused with pv.spec.volumeMode

Yes, naming is hard 😦

I also considered ephemeralMode or inlineMode, but both then don't make much sense when the value is persistent.

Any suggestions?

How about VolumeSource, with persistent and ephemeral as values?

It's then different from driverMode, but perhaps that's okay.

Yeah, naming things is hard. Slight preference to the previous name, driverMode, as that kind of matches the spec:
CSIDriver.Spec.Mode -> driverMode.

Or maybe csiDriverMode?

VolumeSource also could be misunderstood (see snapshotting...). Let's see how well CSIVolumeMode works...

Works for me.

I left "driverMode" unchanged, because as you said, it is derived from CSIDriver.Spec.Mode.

kfox1111

With my limited knowledge of how e2e works, and excluding the naming question, this all looks good to me. :)

cwdsuzhou · 2019-07-12T07:31:36Z

pkg/volume/csi/csi_plugin.go

+		if csiVolumeMode == ephemeralVolumeMode {
+			klog.V(5).Info(log("plugin.CanDeviceMount skipped ephemeral mode detected for spec %v", spec.Name()))
+			return false, nil
+		}


Would this make it more clear?

If you do not think so, ignore it.

if !inlineEnabled { return true, nil } driverMode, err := p.getDriverMode(spec) ... return true, nil

Yes, I think that's better and more consistent with usual Go style. I changed it, please have another look.

pohly · 2019-07-12T14:36:24Z

/retest

pohly · 2019-07-15T08:37:11Z

@cwdsuzhou is it okay now?

pohly · 2019-07-17T15:40:26Z

pkg/volume/csi/csi_mounter.go

@@ -323,6 +324,7 @@ func (c *csiMountMgr) podAttributes() (map[string]string, error) {
 		"csi.storage.k8s.io/pod.namespace":       c.pod.Namespace,
 		"csi.storage.k8s.io/pod.uid":             string(c.pod.UID),
 		"csi.storage.k8s.io/serviceAccount.name": c.pod.Spec.ServiceAccountName,
+		"csi.storage.k8s.io/ephemeral":           strconv.FormatBool(c.csiVolumeMode == ephemeralVolumeMode),


@kfox1111 should the presence of this new field in the PodInfo depend on the feature gate?

Hmm... That is a good question...

It shouldn't hurt anything to have it always defined as it is, but following the principal that nothing should change if the gate is disabled, it probably should be behind the gate, yeah.

I've made it optional, i.e. it only gets added when the feature gate is set.

pohly · 2019-07-17T17:20:54Z

test/e2e/storage/testsuites/ephemeral.go

+func InitEphemeralTestSuite() TestSuite {
+	return &ephemeralTestSuite{
+		tsInfo: TestSuiteInfo{
+			name: "ephemeral",


/hold

Currently ephemeral inline volumes are an alpha feature, so this whole suite must have a [Feature:CSIInlineVolume] tag. Will add it.

pohly · 2019-07-24T15:59:24Z

/hold cancel

pohly

These consts should be defined in the api. If the API is not there yet, then we should remember to remove these once we add it

The API still needs to be added. I've done that in a follow-up branch and also moved the constants.

pohly · 2019-07-24T16:13:21Z

test/e2e/storage/testpatterns/testpattern.go

@@ -44,6 +44,8 @@ var (
 	PreprovisionedPV TestVolType = "PreprovisionedPV"
 	// DynamicPV represents a volume type for dynamic provisioned Persistent Volume
 	DynamicPV TestVolType = "DynamicPV"
+	// CSIVolume represents a volume type that is defined inline and provided by a CSI driver.
+	CSIVolume TestVolType = "CSIVolume"


I can rename "CSIVolume" to "CSIInlineVolume". I chose "CSIVolume" because that's also the term used in "CSIVolumeSource" (

kubernetes/staging/src/k8s.io/api/core/v1/types.go

Lines 1706 to 1707 in 1dac5fd

// Represents a source location of a volume to mount, managed by an external CSI driver

type CSIVolumeSource struct {

), but I agree, the longer term is better and also used elsewhere (for example, in the feature gate).

The reason for the separate volume type is, as you said, that the characteristics of the volume are different. I don't know whether it makes a difference in practice, though.

pohly · 2019-07-24T18:31:39Z

/retest

pohly · 2019-07-25T06:43:10Z

/retest

pohly · 2019-07-25T13:39:07Z

I included another fix (problem found while enabling in-tree testing of ephemeral volumes with the CSI hostpath driver):

diff --git a/test/e2e/storage/testsuites/ephemeral.go b/test/e2e/storage/testsuites/ephemeral.go
index bc7b8b244e..c9e4152cae 100644
--- a/test/e2e/storage/testsuites/ephemeral.go
+++ b/test/e2e/storage/testsuites/ephemeral.go
@@ -92,7 +92,7 @@ func (p *ephemeralTestSuite) defineTests(driver TestDriver, pattern testpatterns
                l.testCase = &EphemeralTest{
                        Client:     l.config.Framework.ClientSet,
                        Namespace:  f.Namespace.Name,
-                       DriverName: dInfo.Name,
+                       DriverName: l.config.GetUniqueDriverName(),
                        Node:       framework.NodeSelection{Name: l.config.ClientNodeName},
                        GetVolumeAttributes: func(volumeNumber int) map[string]string {
                                return eDriver.GetVolumeAttributes(l.config, volumeNumber)

It worked when testing an external driver because the unique name is the same as the base name...

pohly · 2019-07-25T13:48:25Z

It worked when testing an external driver because the unique name is the same as the base name...

And using the generated name breaks testing an external driver... this needs some more work before it works for both.

/hold

pohly · 2019-07-25T14:20:14Z

Fixed this way:

diff --git a/test/e2e/storage/external/external.go b/test/e2e/storage/external/external.go
index 6ff28829de..cb2c95535b 100644
--- a/test/e2e/storage/external/external.go
+++ b/test/e2e/storage/external/external.go
@@ -148,9 +148,6 @@ type driverDefinition struct {
 	// the default file system are enabled.
 	DriverInfo testsuites.DriverInfo
 
-	// ShortName is used to create unique names for test cases and test resources.
-	ShortName string
-
 	// StorageClass must be set to enable dynamic provisioning tests.
 	// The default is to not run those tests.
 	StorageClass struct {
@@ -186,6 +183,8 @@ type driverDefinition struct {
 	// has to be defined to enable testing of inline ephemeral volumes.
 	// If a test needs more volumes than defined, some of the defined
 	// volumes will be used multiple times.
+	//
+	// DriverInfo.Name is used as name of the driver in the inline volume.
 	InlineVolumeAttributes []map[string]string
 
 	// ClaimSize defines the desired size of dynamically
@@ -301,6 +300,10 @@ func (d *driverDefinition) GetVolumeAttributes(config *testsuites.PerTestConfig,
 	return d.InlineVolumeAttributes[volumeNumber%len(d.InlineVolumeAttributes)]
 }
 
+func (d *driverDefinition) GetCSIDriverName(config *testsuites.PerTestConfig) string {
+	return d.DriverInfo.Name
+}
+
 func (d *driverDefinition) PrepareTest(f *framework.Framework) (*testsuites.PerTestConfig, func()) {
 	config := &testsuites.PerTestConfig{
 		Driver:         d,
diff --git a/test/e2e/storage/testsuites/ephemeral.go b/test/e2e/storage/testsuites/ephemeral.go
index c9e4152cae..ee9cfcbf71 100644
--- a/test/e2e/storage/testsuites/ephemeral.go
+++ b/test/e2e/storage/testsuites/ephemeral.go
@@ -92,7 +92,7 @@ func (p *ephemeralTestSuite) defineTests(driver TestDriver, pattern testpatterns
 		l.testCase = &EphemeralTest{
 			Client:     l.config.Framework.ClientSet,
 			Namespace:  f.Namespace.Name,
-			DriverName: l.config.GetUniqueDriverName(),
+			DriverName: eDriver.GetCSIDriverName(l.config),
 			Node:       framework.NodeSelection{Name: l.config.ClientNodeName},
 			GetVolumeAttributes: func(volumeNumber int) map[string]string {
 				return eDriver.GetVolumeAttributes(l.config, volumeNumber)
diff --git a/test/e2e/storage/testsuites/testdriver.go b/test/e2e/storage/testsuites/testdriver.go
index 74224ca8d4..4c05d2554f 100644
--- a/test/e2e/storage/testsuites/testdriver.go
+++ b/test/e2e/storage/testsuites/testdriver.go
@@ -111,6 +111,11 @@ type EphemeralTestDriver interface {
 	// all be the same or different, depending what the driver supports
 	// and/or wants to test.
 	GetVolumeAttributes(config *PerTestConfig, volumeNumber int) map[string]string
+
+	// GetCSIDriverName returns the name that was used when registering with
+	// kubelet. Depending on how the driver was deployed, this can be different
+	// from DriverInfo.Name.
+	GetCSIDriverName(config *PerTestConfig) string
 }
 
 // SnapshottableTestDriver represents an interface for a TestDriver that supports DynamicSnapshot

The removal of ShortName is unrelated and a separate commit, I just noticed while looking into the driver naming problem that it exists although not used anywhere.

pohly · 2019-07-25T14:20:29Z

/hold cancel

pohly · 2019-07-25T14:22:21Z

For reference, the in-tree usage of the new ephemeral testsuite test is part of PR #80568, currently in commit 4de69dcf9e759c9f096f83d0a444a3e091342f73.

The name was meant to be used as shorter replacement for potentially long CSI driver names, but was never used in practice.

The conceptual change is that the mode in which a volume gets handled is derived from it's spec, not from the ability of the driver. In practice, that is already how the code worked because it didn't actually look at CSIDriver.Spec.Mode at all. Therefore the code change itself is mostly just renaming "driver mode" to "volume mode". In some places (CanDeviceMount, CanAttach) the feature check that was used elsewhere seemed to be missing. Now their code path for ephemeral volumes are also only entered if that feature is enabled. The sanity check whether a CSI driver is being used correctly still needs to be implemented. Related-to: kubernetes#79624

The PodInfo tests can be extended to also cover the new csi.storage.k8s.io/ephemeral flag. However, the presence of that flag depends on whether inline volume support is enabled, so tests that run with and without the feature have to detect that at runtime. Other tests have a feature tag and thus can assume that they only run when that feature is enabled. However, we need a newer csi-mock driver before we can actually ask it to publish an ephemeral inline volume.

pohly · 2019-07-25T16:03:02Z

/test pull-kubernetes-integration

msau42 · 2019-07-25T20:11:23Z

/lgtm
/approve

k8s-ci-robot · 2019-07-25T20:11:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, pohly

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:

~~pkg/volume/csi/OWNERS~~ [msau42]
~~test/e2e/storage/OWNERS~~ [msau42]
~~test/e2e/testing-manifests/storage-csi/OWNERS~~ [msau42]

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 assigned vladimirvivien Jul 10, 2019

k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 10, 2019

k8s-ci-robot requested review from copejon and saad-ali July 10, 2019 12:24

pohly mentioned this pull request Jul 10, 2019

CSI Inline - Update CSIDriver to indicate driver mode #75222

Closed

cwdsuzhou reviewed Jul 10, 2019

View reviewed changes

kfox1111 reviewed Jul 10, 2019

View reviewed changes

pohly force-pushed the persistent-and-ephemeral-csi-volumes branch from 80b6fe7 to fc451cb Compare July 11, 2019 14:50

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 11, 2019

cwdsuzhou reviewed Jul 12, 2019

View reviewed changes

pohly force-pushed the persistent-and-ephemeral-csi-volumes branch from fc451cb to a32af6b Compare July 12, 2019 11:01

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 12, 2019

pohly commented Jul 17, 2019

View reviewed changes

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

pohly commented Jul 17, 2019

View reviewed changes

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

pohly commented Jul 24, 2019

View reviewed changes

pohly force-pushed the persistent-and-ephemeral-csi-volumes branch from aa8999a to 564e628 Compare July 24, 2019 17:33

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

pohly force-pushed the persistent-and-ephemeral-csi-volumes branch from 564e628 to dc5ecfa Compare July 24, 2019 20:31

pohly mentioned this pull request Jul 25, 2019

ephemeral mode check #80568

Merged

pohly force-pushed the persistent-and-ephemeral-csi-volumes branch from dc5ecfa to 93a0afe Compare July 25, 2019 13:37

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

pohly force-pushed the persistent-and-ephemeral-csi-volumes branch from 93a0afe to 817a43d Compare July 25, 2019 14:20

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

pohly added 4 commits July 25, 2019 16:45

e2e: remove unused ShortName from external driver definition

be9f909

The name was meant to be used as shorter replacement for potentially long CSI driver names, but was never used in practice.

updated Bazel files

4bc5d06

pohly force-pushed the persistent-and-ephemeral-csi-volumes branch from 817a43d to 4bc5d06 Compare July 25, 2019 14:46

k8s-ci-robot assigned msau42 Jul 25, 2019

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

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

k8s-ci-robot merged commit a375050 into kubernetes:master Jul 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

persistent and ephemeral csi volumes #79983

persistent and ephemeral csi volumes #79983

pohly commented Jul 10, 2019

pohly commented Jul 10, 2019

pohly commented Jul 10, 2019

cwdsuzhou Jul 10, 2019 •

edited

Loading

pohly Jul 10, 2019

pohly Jul 10, 2019

kfox1111 Jul 10, 2019

pohly Jul 11, 2019

kfox1111 Jul 11, 2019

pohly Jul 11, 2019

kfox1111 left a comment

cwdsuzhou Jul 12, 2019

pohly Jul 12, 2019

pohly commented Jul 12, 2019

pohly commented Jul 15, 2019

pohly Jul 17, 2019

kfox1111 Jul 17, 2019

pohly Jul 19, 2019

pohly Jul 17, 2019

pohly Jul 19, 2019

pohly commented Jul 24, 2019

pohly left a comment

pohly Jul 24, 2019

pohly commented Jul 24, 2019

pohly commented Jul 25, 2019

pohly commented Jul 25, 2019

pohly commented Jul 25, 2019

pohly commented Jul 25, 2019

pohly commented Jul 25, 2019

pohly commented Jul 25, 2019

pohly commented Jul 25, 2019

msau42 commented Jul 25, 2019

k8s-ci-robot commented Jul 25, 2019

	// Represents a source location of a volume to mount, managed by an external CSI driver
	type CSIVolumeSource struct {

persistent and ephemeral csi volumes #79983

persistent and ephemeral csi volumes #79983

Conversation

pohly commented Jul 10, 2019

pohly commented Jul 10, 2019

pohly commented Jul 10, 2019

cwdsuzhou Jul 10, 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

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

pohly commented Jul 12, 2019

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

pohly commented Jul 24, 2019

pohly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Jul 24, 2019

pohly commented Jul 25, 2019

pohly commented Jul 25, 2019

pohly commented Jul 25, 2019

pohly commented Jul 25, 2019

pohly commented Jul 25, 2019

pohly commented Jul 25, 2019

pohly commented Jul 25, 2019

msau42 commented Jul 25, 2019

k8s-ci-robot commented Jul 25, 2019

cwdsuzhou Jul 10, 2019 •

edited

Loading