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

CSI MountDevice/UnmountDevice Implementation #60115

Merged
merged 1 commit into from Feb 27, 2018

Conversation

@davidz627
Collaborator

davidz627 commented Feb 21, 2018

Fixes #60114

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

This comment has been minimized.

Collaborator

davidz627 commented Feb 21, 2018

/assign saad-ali

@davidz627

This comment has been minimized.

Collaborator

davidz627 commented Feb 21, 2018

/assign vladimirvivien

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

This comment has been minimized.

@saad-ali

saad-ali Feb 23, 2018

Member

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.

This comment has been minimized.

@saad-ali

saad-ali Feb 23, 2018

Member

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

This comment has been minimized.

@vladimirvivien

vladimirvivien Feb 23, 2018

Member

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

This comment has been minimized.

@davidz627

davidz627 Feb 24, 2018

Collaborator

fixed

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

This comment has been minimized.

@saad-ali

saad-ali Feb 23, 2018

Member

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?

This comment has been minimized.

@saad-ali

saad-ali Feb 23, 2018

Member

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.

This comment has been minimized.

@davidz627

davidz627 Feb 24, 2018

Collaborator

overhauled to use pv.

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

This comment has been minimized.

@saad-ali

saad-ali Feb 23, 2018

Member

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.

This comment has been minimized.

@davidz627

davidz627 Feb 24, 2018

Collaborator

fixed

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

This comment has been minimized.

@saad-ali

saad-ali Feb 23, 2018

Member

Move this nil check inside hasStageUnstageCapability(...)

This comment has been minimized.

@davidz627

davidz627 Feb 24, 2018

Collaborator

fixed

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

This comment has been minimized.

@saad-ali

saad-ali Feb 23, 2018

Member

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

This comment has been minimized.

@davidz627

davidz627 Feb 24, 2018

Collaborator

fixed

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

This comment has been minimized.

@saad-ali

saad-ali Feb 23, 2018

Member

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

This comment has been minimized.

@davidz627

davidz627 Feb 24, 2018

Collaborator

fixed

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

This comment has been minimized.

@saad-ali

saad-ali Feb 23, 2018

Member

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

This comment has been minimized.

@davidz627

davidz627 Feb 24, 2018

Collaborator

fixed

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

This comment has been minimized.

@saad-ali

saad-ali Feb 23, 2018

Member

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

This comment has been minimized.

@vladimirvivien

vladimirvivien Feb 23, 2018

Member

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

This comment has been minimized.

@davidz627

davidz627 Feb 24, 2018

Collaborator

fixed

This comment has been minimized.

@davidz627

davidz627 Feb 24, 2018

Collaborator

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

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

This comment has been minimized.

@saad-ali

saad-ali Feb 23, 2018

Member

Oh man this is a fun bit of code.

This comment has been minimized.

@vladimirvivien

vladimirvivien Feb 23, 2018

Member

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.

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

This comment has been minimized.

@vladimirvivien

vladimirvivien Feb 23, 2018

Member

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

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

This comment has been minimized.

@vladimirvivien

vladimirvivien Feb 23, 2018

Member

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

This comment has been minimized.

@davidz627

davidz627 Feb 24, 2018

Collaborator

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

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

This comment has been minimized.

@vladimirvivien

vladimirvivien Feb 23, 2018

Member

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

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

This comment has been minimized.

@vladimirvivien

vladimirvivien Feb 23, 2018

Member

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

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

This comment has been minimized.

@vladimirvivien

vladimirvivien Feb 23, 2018

Member

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.

This comment has been minimized.

@davidz627

davidz627 Feb 23, 2018

Collaborator

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.

This comment has been minimized.

@davidz627

davidz627 Feb 24, 2018

Collaborator

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

@davidz627

This comment has been minimized.

Collaborator

davidz627 commented Feb 24, 2018

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

@davidz627

This comment has been minimized.

Collaborator

davidz627 commented Feb 24, 2018

@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

This comment has been minimized.

Collaborator

davidz627 commented Feb 24, 2018

/retest

1 similar comment
@davidz627

This comment has been minimized.

Collaborator

davidz627 commented Feb 25, 2018

/retest

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

This comment has been minimized.

@saad-ali

saad-ali Feb 25, 2018

Member

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

return err
}
req := &csipb.NodeStageVolumeRequest{

This comment has been minimized.

@saad-ali

saad-ali Feb 25, 2018

Member

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

Examples here:
#60118
#60382

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

This comment has been minimized.

@saad-ali

saad-ali Feb 25, 2018

Member

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

@vladimirvivien

This comment has been minimized.

Member

vladimirvivien commented Feb 26, 2018

@davidz627 taking a look right now.

@davidz627

This comment has been minimized.

Collaborator

davidz627 commented Feb 26, 2018

/retest

@saad-ali

/lgtm
/approve

@vladimirvivien

Added some comments. Getting there.

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}

This comment has been minimized.

@vladimirvivien

vladimirvivien Feb 26, 2018

Member

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.

This comment has been minimized.

@davidz627

davidz627 Feb 26, 2018

Collaborator

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

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

This comment has been minimized.

@vladimirvivien

vladimirvivien Feb 26, 2018

Member

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

This comment has been minimized.

@davidz627

davidz627 Feb 26, 2018

Collaborator

PV's are not namespaced in Kubernetes.

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

This comment has been minimized.

@vladimirvivien

vladimirvivien Feb 26, 2018

Member

Do a nil check here for pv.Spec.CSI

This comment has been minimized.

@davidz627

davidz627 Feb 26, 2018

Collaborator

Done 5 lines above already

@davidz627

This comment has been minimized.

Collaborator

davidz627 commented Feb 26, 2018

/retest

@saad-ali

This comment has been minimized.

Member

saad-ali commented Feb 26, 2018

Will let @vladimirvivien review and LGTM

/lgtm cancel

@saad-ali

This comment has been minimized.

Member

saad-ali commented Feb 26, 2018

/approve

@davidz627

This comment has been minimized.

Collaborator

davidz627 commented Feb 26, 2018

/retest

Added MountDevice/UnmountDevice pass-through to NodeStageVolume/NodeU…
…nstageVolume for CSI Volume Plugin. Added related unit tests. Vendored CSI Spec to HEAD
@davidz627

This comment has been minimized.

Collaborator

davidz627 commented Feb 26, 2018

/retest

@davidz627

This comment has been minimized.

Collaborator

davidz627 commented Feb 26, 2018

/retest

@vladimirvivien

This comment has been minimized.

Member

vladimirvivien commented Feb 27, 2018

/LGTM

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 27, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 27, 2018

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

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 27, 2018

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 27, 2018

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-merge-robot k8s-merge-robot merged commit 0a8e5f8 into kubernetes:master Feb 27, 2018

5 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
pull-kubernetes-e2e-gce Job triggered.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job triggered.
Details
pull-kubernetes-e2e-kops-aws Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce Job triggered.
Details
pull-kubernetes-node-e2e Job triggered.
Details
pull-kubernetes-unit Job triggered.
Details
pull-kubernetes-verify Job triggered.
Details
cla/linuxfoundation davidz627 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gke Skipped
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment