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

persistent and ephemeral csi volumes #79983

Merged

Conversation

@pohly
Copy link
Contributor

commented Jul 10, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

/assign @vladimirvivien
/sig-storage

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

/sig storage
/priority important

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

This comment has been minimized.

Copy link
@cwdsuzhou

cwdsuzhou Jul 10, 2019

Member

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

This comment has been minimized.

Copy link
@pohly

pohly Jul 10, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@pohly

pohly Jul 10, 2019

Author Contributor

How about VolumeSource, with persistent and ephemeral as values?

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

This comment has been minimized.

Copy link
@kfox1111

kfox1111 Jul 10, 2019

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?

This comment has been minimized.

Copy link
@pohly

pohly Jul 11, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@kfox1111

kfox1111 Jul 11, 2019

Works for me.

This comment has been minimized.

Copy link
@pohly

pohly Jul 11, 2019

Author Contributor

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

@kfox1111
Copy link

left a comment

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

@pohly pohly force-pushed the pohly:persistent-and-ephemeral-csi-volumes branch from 80b6fe7 to fc451cb Jul 11, 2019

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/L labels Jul 11, 2019

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

This comment has been minimized.

Copy link
@cwdsuzhou

cwdsuzhou Jul 12, 2019

Member

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

This comment has been minimized.

Copy link
@pohly

pohly Jul 12, 2019

Author Contributor

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

@pohly pohly force-pushed the pohly:persistent-and-ephemeral-csi-volumes branch from fc451cb to a32af6b Jul 12, 2019

@k8s-ci-robot k8s-ci-robot added size/L and removed size/XL labels Jul 12, 2019

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

/retest

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

@cwdsuzhou is it okay now?

@@ -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),

This comment has been minimized.

Copy link
@pohly

pohly Jul 17, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@kfox1111

kfox1111 Jul 17, 2019

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.

This comment has been minimized.

Copy link
@pohly

pohly Jul 19, 2019

Author Contributor

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

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

This comment has been minimized.

Copy link
@pohly

pohly Jul 17, 2019

Author Contributor

/hold

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

This comment has been minimized.

Copy link
@pohly

pohly Jul 19, 2019

Author Contributor

Done.

@pohly
Copy link
Contributor Author

left a comment

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.

@@ -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"

This comment has been minimized.

Copy link
@pohly

pohly Jul 24, 2019

Author Contributor

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

// 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 pohly force-pushed the pohly:persistent-and-ephemeral-csi-volumes branch from aa8999a to 564e628 Jul 24, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jul 24, 2019

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

/retest

@pohly pohly force-pushed the pohly:persistent-and-ephemeral-csi-volumes branch from 564e628 to dc5ecfa Jul 24, 2019

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

/retest

@pohly pohly referenced this pull request Jul 25, 2019

Open

ephemeral mode check #80568

@pohly pohly force-pushed the pohly:persistent-and-ephemeral-csi-volumes branch from dc5ecfa to 93a0afe Jul 25, 2019

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

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 pohly force-pushed the pohly:persistent-and-ephemeral-csi-volumes branch from 93a0afe to 817a43d Jul 25, 2019

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

/hold cancel

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

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

pohly added some commits Jul 9, 2019

CSI: allow drivers that can handle persistent and ephemeral volumes
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: #79624
e2e storage: csi-mock tests for ephemeral inline volumes
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.
e2e: remove unused ShortName from external driver definition
The name was meant to be used as shorter replacement for potentially
long CSI driver names, but was never used in practice.

@pohly pohly force-pushed the pohly:persistent-and-ephemeral-csi-volumes branch from 817a43d to 4bc5d06 Jul 25, 2019

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

/test pull-kubernetes-integration

@msau42

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

[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:

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 merged commit a375050 into kubernetes:master Jul 25, 2019

23 checks passed

cla/linuxfoundation pohly authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.