CSI MountDevice/UnmountDevice Implementation #60115

davidz627 · 2018-02-21T02:10:37Z

What this PR does / why we need it:
This PR Implements MountDevice and UnmountDevice for the CSI Plugin, the functions will call through to NodeStageVolume/NodeUnstageVolume for CSI plugins.

/sig storage

Implements MountDevice and UnmountDevice for the CSI Plugin, the functions will call through to NodeStageVolume/NodeUnstageVolume for CSI plugins.

davidz627 · 2018-02-21T02:11:23Z

/assign saad-ali

davidz627 · 2018-02-21T22:20:23Z

/assign vladimirvivien

saad-ali · 2018-02-22T23:48:21Z

pkg/volume/csi/csi_attacher.go

+		return "", fmt.Errorf("attacher.GetDeviceMountPath failed, driver name empty")
+	}
+
+	deviceMountPath := path.Join(c.plugin.host.GetPluginDir(csiSource.Driver), "mounts", csiSource.VolumeHandle)


Should this be {pluginDir}/csi/{csiDriverName}/mounts/{csiVolumeHandle}? Otherwise if someone names their plugin kubernetes.io/gce-pd they could stomp on the internal GCE PD directories right?

Also instead of "mounts" use mount.MountsInGlobalPDPath.

And maybe sanitize the volume and driver names (EscapeQualifiedNameForDisk or something like that)?

@davidz627 as a followup to Saad's, I would use the method defined here https://github.com/davidz627/kubernetes/blob/f8d0d3162ada15d8a28aed55431d3e66b7fec30f/pkg/volume/csi/csi_mounter.go#L441 as a central location to generate the device mount path. Make all adjustment there.

saad-ali · 2018-02-22T23:51:13Z

pkg/kubelet/kubelet_getters.go

+// getPluginDir returns the pluginName for the given pluginDir that
+// was created by getPluginDir
+func (kl *Kubelet) getPluginNameFromDir(pluginDir string) string {
+	return filepath.Base(pluginDir)


You've defined plugin path as:

deviceMountPath := path.Join(c.plugin.host.GetPluginDir(csiSource.Driver), "mounts", csiSource.VolumeHandle)

So wouldn't this just return the csiSource.VolumeHandle?

Oh it looks like getDriverAndVolNameFromDeviceMountPath(...) is called first that splits at / boundaries, and then getPluginNameFromDir(...) is called to finish everything off.

Why not just do GetPluginDir(...) inside getPluginNameFromDir(...) and move this logic there? That way you could eliminate this method all together. It doesn't look like it is used anywhere else.

overhauled to use pv.

saad-ali · 2018-02-23T00:06:40Z

pkg/volume/csi/csi_attacher.go

+	glog.V(4).Infof(log("attacher.MountDevice(%s, %s)", devicePath, deviceMountPath))
+
+	// Setup
+	ctx, cancel := grpctx.WithTimeout(grpctx.Background(), csiTimeout)


Move creating ctx down to right before you actually use it for the first time. No need to waste cycles if you end up not using it.

saad-ali · 2018-02-23T00:09:26Z

pkg/volume/csi/csi_attacher.go

+		return fmt.Errorf("attacher.MountDevice failed while getting node capabilities: %v", err)
+	}
+
+	if capabilities == nil {


Move this nil check inside hasStageUnstageCapability(...)

saad-ali · 2018-02-23T00:19:08Z

pkg/volume/csi/csi_attacher.go

+		return fmt.Errorf("attacher.MountDevice failed, deviceMountPath is empty")
+	}
+
+	mounted, err := isDirMounted(c.plugin, deviceMountPath)


Could you put this check first, and if it is false, then create a context, client, etc.?

saad-ali · 2018-02-23T00:19:57Z

pkg/volume/csi/csi_attacher.go

+	}
+
+	nodeName := string(c.plugin.host.GetNodeName())
+	fmt.Printf("voldid: %v, drivername: %v, nodeName: %v", csiSource.VolumeHandle, csiSource.Driver, nodeName)


Remember to remove debug statement or convert to glog with appropriate verbosity

saad-ali · 2018-02-23T00:21:09Z

pkg/volume/csi/csi_attacher.go

+
+	// probe driver
+	// TODO (vladimirvivien) move probe call where it is done only when it is needed.
+	if err := csi.NodeProbe(ctx, csiVersion); err != nil {


You can remove probe call altogether. It has been removed in the latest (HEAD) version of CSI.

saad-ali · 2018-02-23T00:29:06Z

pkg/volume/csi/csi_attacher.go

+	publishVolumeInfo := attachment.Status.AttachmentMetadata
+
+	// get volume attributes
+	// TODO: for alpha vol attributes are passed via PV.Annotations


Make sure to do a follow up PR to fix this, create a bug or something to remind you.

PR #58762 has the fix. Simply spec.VolumeAttribs as the parameter in call to NodeStageVolume

was in source.VolumeAttributes. lmk if this is not the correct one you were referring to. Looks right to me.

saad-ali · 2018-02-23T00:50:46Z

pkg/volume/csi/csi_attacher.go

+	return stageUnstageSet, nil
+}
+
+func getDriverAndVolNameFromDeviceMountPath(deviceMountPath string, plugin *csiPlugin) (string, string, error) {


Oh man this is a fun bit of code.

It would be helpful to document the format deviceMountPath (i.e. {pluginDir}/csi/etc so future maintainers would have an idea why this works.

vladimirvivien

Please take a look at comments.

vladimirvivien · 2018-02-23T17:54:03Z

pkg/volume/csi/csi_attacher.go

+		return "", fmt.Errorf("attacher.GetDeviceMountPath failed, driver name empty")
+	}
+
+	deviceMountPath := path.Join(c.plugin.host.GetPluginDir(csiSource.Driver), "mounts", csiSource.VolumeHandle)


@davidz627 as a followup to Saad's, I would use the method defined here https://github.com/davidz627/kubernetes/blob/f8d0d3162ada15d8a28aed55431d3e66b7fec30f/pkg/volume/csi/csi_mounter.go#L441 as a central location to generate the device mount path. Make all adjustment there.

vladimirvivien · 2018-02-23T20:17:28Z

pkg/volume/csi/csi_attacher.go

+	attachID := getAttachmentName(csiSource.VolumeHandle, csiSource.Driver, nodeName)
+
+	// ensure version is supported
+	if err := csi.AssertSupportedVersion(ctx, csiVersion); err != nil {


We dont have to asssertSupportedVersion anymore that if block can be removed.

removed for MountDevice. Still exists in SetUpAt and TearDownAt unless thats been changed in the last day or two.

vladimirvivien · 2018-02-23T20:29:21Z

pkg/volume/csi/csi_attacher.go

+	publishVolumeInfo := attachment.Status.AttachmentMetadata
+
+	// get volume attributes
+	// TODO: for alpha vol attributes are passed via PV.Annotations


PR #58762 has the fix. Simply spec.VolumeAttribs as the parameter in call to NodeStageVolume

vladimirvivien · 2018-02-23T21:18:01Z

pkg/volume/csi/csi_attacher.go

+	return stageUnstageSet, nil
+}
+
+func getDriverAndVolNameFromDeviceMountPath(deviceMountPath string, plugin *csiPlugin) (string, string, error) {


It would be helpful to document the format deviceMountPath (i.e. {pluginDir}/csi/etc so future maintainers would have an idea why this works.

vladimirvivien · 2018-02-23T21:24:08Z

pkg/volume/csi/csi_attacher.go

+	}
+	volID := strings.Join(splitted[len(splitted)-3:], "/")
+	dir := strings.Join(splitted[:len(splitted)-4], "/")
+	pluginName := plugin.host.GetPluginNameFromDir(dir)


Assuming this{pluginDir}/csi/{csiDriverName}/mounts/{csiVolumeHandle} is the format, then
strings.Join(splitted[len(splitted)-3:], "/") will return slice like {csiDriverName}/mounts/{csiVolumeHandle} . Uneasy about the hardcoded offsets, seems a bit brittle.

Also, consider using filepath.Base(), filepath.Dir(), filepath.Join() for more portable path nomenclatures.

I agree that the hardcoded offesets are brittle but I couldn't find a better way to do this without a hardcoded number of "Dir"'s anyway (3 of them). Because {csiVolumeHandle} itself is a 3 part object of form project/zone/name. That fact isn't really captured anywhere (maybe I should have a comment). Open to suggestions for better ways to do this.

I tried using Rel which ended up being more brittle because even small changes or differences to directory structure would completely destroy the resulting path.

overhauled. PTAL and review new versions of makeDeviceMountPath and getDriverAndVolNameFromDeviceMountPath.

davidz627 · 2018-02-24T01:08:21Z

@saad-ali @vladimirvivien Addressed all comments. Biggest change is in makeDeviceMountPath and getDriverAndVolNameFromDeviceMountPath. Please take a closer look there. Thanks!

davidz627 · 2018-02-24T01:13:41Z

@vladimirvivien the PV method in getDriverAndVolNameFromDeviceMountPath was created to be able to reliably retrieve the volume handle.

Since there are no character restrictions in the CSI spec on volume handles there were no guaranteed ways to parse the volume handle out of the DeviceMountPath even with sanitation.

davidz627 · 2018-02-24T02:06:17Z

/retest

davidz627 · 2018-02-25T20:09:53Z

/retest

saad-ali

Almost there!

saad-ali · 2018-02-25T21:52:29Z

pkg/volume/csi/csi_attacher.go

+}
+
+func getDriverAndVolNameFromDeviceMountPath(k8s kubernetes.Interface, deviceMountPath string) (string, string, error) {
+	// deviceMountPath structure: /var/lib/kubelet/plugins/kubernetes.io/csi/pv/{pvname}/globalmount


Maybe sanity check assert that path terminates in /globalmount or /globalmount/?

saad-ali · 2018-02-25T22:00:55Z

pkg/volume/csi/csi_client.go

+		return err
+	}
+
+	req := &csipb.NodeStageVolumeRequest{


Pass in secret (node_stage_secrets field). It is specified on the Kubernetes side in the NodeStageSecretRef in CSIPersistentVolumeSource.

Examples here:
#60118
#60382

saad-ali · 2018-02-25T22:02:37Z

pkg/volume/csi/csi_attacher.go

+		return fmt.Errorf("attacher.MountDevice failed while getting node capabilities: %v", err)
+	}
+
+	stageUnstageSet, err := hasStageUnstageCapability(capabilities)


Since you always call NodeGetCapabilities(...) before hasStageUnstageCapability(...). Maybe move NodeGetCapabilities(...) inside hasStageUnstageCapability(...)

vladimirvivien · 2018-02-26T16:12:32Z

@davidz627 taking a look right now.

davidz627 · 2018-02-26T19:37:50Z

/retest

saad-ali

/lgtm
/approve

vladimirvivien

Added some comments. Getting there.

vladimirvivien · 2018-02-26T20:17:43Z

pkg/volume/csi/csi_attacher.go

+	if file := filepath.Base(deviceMountPath); file != globalMountInGlobalPath {
+		return "", "", fmt.Errorf("getDriverAndVolNameFromDeviceMountPath failed, path did not end in %s", globalMountInGlobalPath)
+	}
+	// dir is now /var/lib/kubelet/plugins/kubernetes.io/csi/pv/{pvname}


Is {pvname} sanitized elsewhere (to remove path demarkation chars) ? If so, it probably need to return back to its original state. If no sanitization occurred, ignore.

I am not doing sanitation anywhere, so the name should be same in both the creation and the extraction.

vladimirvivien · 2018-02-26T20:28:08Z

pkg/volume/csi/csi_attacher.go

+	pvName := filepath.Base(dir)
+
+	// Get PV and check for errors
+	pv, err := k8s.CoreV1().PersistentVolumes().Get(pvName, meta.GetOptions{})


Do we need to worry about namespace here? If not, there maybe name collision.

PV's are not namespaced in Kubernetes.

vladimirvivien · 2018-02-26T20:29:02Z

pkg/volume/csi/csi_attacher.go

+	}
+
+	// Get VolumeHandle and PluginName from pv
+	csiSource := pv.Spec.CSI


Do a nil check here for pv.Spec.CSI

Done 5 lines above already

davidz627 · 2018-02-26T21:00:21Z

/retest

saad-ali · 2018-02-26T21:11:19Z

Will let @vladimirvivien review and LGTM

/lgtm cancel

saad-ali · 2018-02-26T21:11:49Z

/approve

davidz627 · 2018-02-26T21:51:50Z

/retest

…nstageVolume for CSI Volume Plugin. Added related unit tests. Vendored CSI Spec to HEAD

davidz627 · 2018-02-26T22:56:19Z

/retest

davidz627 · 2018-02-26T23:41:23Z

/retest

vladimirvivien · 2018-02-27T00:49:48Z

/LGTM

k8s-ci-robot · 2018-02-27T00:50:21Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~pkg/volume/csi/OWNERS~~ [saad-ali,vladimirvivien]

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

k8s-github-robot · 2018-02-27T17:11:49Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-27T17:38:39Z

Automatic merge from submit-queue (batch tested with PRs 60430, 60115, 58052, 60355, 60116). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from feiskyer, jingxu97, saad-ali, thockin and verult February 21, 2018 02:10

k8s-ci-robot assigned saad-ali Feb 21, 2018

davidz627 force-pushed the csiMountDevice branch from 10dfa38 to 1eb33d9 Compare February 21, 2018 22:19

k8s-ci-robot assigned vladimirvivien Feb 21, 2018

davidz627 force-pushed the csiMountDevice branch 3 times, most recently from 2f64af5 to f8d0d31 Compare February 22, 2018 01:20

saad-ali reviewed Feb 23, 2018

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2018

vladimirvivien reviewed Feb 23, 2018

View reviewed changes

davidz627 force-pushed the csiMountDevice branch from f8d0d31 to 870c949 Compare February 24, 2018 01:07

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 24, 2018

davidz627 force-pushed the csiMountDevice branch from 870c949 to 1773385 Compare February 24, 2018 02:04

davidz627 force-pushed the csiMountDevice branch from 1773385 to eeae057 Compare February 25, 2018 19:17

saad-ali reviewed Feb 25, 2018

View reviewed changes

davidz627 force-pushed the csiMountDevice branch from eeae057 to 1e6add3 Compare February 26, 2018 19:01

saad-ali approved these changes Feb 26, 2018

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 26, 2018

saad-ali added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. kind/feature Categorizes issue or PR as related to a new feature. labels Feb 26, 2018

saad-ali added this to the v1.10 milestone Feb 26, 2018

vladimirvivien reviewed Feb 26, 2018

View reviewed changes

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 26, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2018

Added MountDevice/UnmountDevice pass-through to NodeStageVolume/NodeU…

cbd1896

…nstageVolume for CSI Volume Plugin. Added related unit tests. Vendored CSI Spec to HEAD

davidz627 force-pushed the csiMountDevice branch from 1e6add3 to cbd1896 Compare February 26, 2018 22:51

davidz627 mentioned this pull request Feb 26, 2018

E2E Test for CSI Volume Plugin MountDevice/UnmountDevice #60461

Closed

saad-ali added the status/approved-for-milestone label Feb 27, 2018

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

k8s-github-robot merged commit 0a8e5f8 into kubernetes:master Feb 27, 2018

CSI MountDevice/UnmountDevice Implementation #60115

CSI MountDevice/UnmountDevice Implementation #60115

Conversation

davidz627 commented Feb 21, 2018

davidz627 commented Feb 21, 2018

davidz627 commented Feb 21, 2018

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirvivien 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

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

davidz627 commented Feb 24, 2018

davidz627 commented Feb 24, 2018

davidz627 commented Feb 24, 2018

davidz627 commented Feb 25, 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

Choose a reason for hiding this comment

vladimirvivien commented Feb 26, 2018

davidz627 commented Feb 26, 2018

saad-ali left a comment

Choose a reason for hiding this comment

vladimirvivien 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 commented Feb 26, 2018

saad-ali commented Feb 26, 2018

saad-ali commented Feb 26, 2018

davidz627 commented Feb 26, 2018

davidz627 commented Feb 26, 2018

davidz627 commented Feb 26, 2018

vladimirvivien commented Feb 27, 2018

k8s-ci-robot commented Feb 27, 2018

k8s-github-robot commented Feb 27, 2018

k8s-github-robot commented Feb 27, 2018