Add support for attacher/detacher interface in Flex volume #41804

chakri-nelluri · 2017-02-21T14:03:40Z

Add support for attacher/detacher interface in Flex volume
This change breaks backward compatibility and requires to be release noted.

Flex volume plugin is updated to support attach/detach interfaces. It broke backward compatibility. Please update your drivers and implement the new callouts.

k8s-ci-robot · 2017-02-21T14:03:41Z

Hi @chakri-nelluri. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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. I understand the commands that are listed here.

k8s-reviewable · 2017-02-21T14:04:47Z

This change is

chakri-nelluri · 2017-02-21T14:05:05Z

@saad-ali @rootfs @MikaelCluseau PTAL

If the plugin is not installed on the master node(controller-manager to be exact), the infra is generating the following events.

 1m            1m              1       kubelet, 127.0.0.1                      Warning         FailedMount             Unable to mount volumes for pod "nginx-nfs_default(ad616cd7-f823-11e6-95e2-2c600c7bf1df)": timeout expired waiting for volumes to attach/mount for pod "default"/"nginx-nfs". list of unattached/unmounted volumes=[test]
  1m            1m              1       kubelet, 127.0.0.1                      Warning         FailedSync              Error syncing pod, skipping: timeout expired waiting for volumes to attach/mount for pod "default"/"nginx-nfs". list of unattached/unmounted volumes=[test]

rootfs · 2017-02-21T14:29:51Z

@k8s-bot ok to test

rootfs · 2017-02-21T14:30:44Z

@kubernetes/sig-storage-pr-reviews

rootfs · 2017-02-21T14:31:52Z

pkg/volume/flexvolume/util.go

@@ -0,0 +1,111 @@
+/*
+Copyright 2016 The Kubernetes Authors.


rootfs · 2017-02-21T14:33:43Z

pkg/volume/flexvolume/util.go

+	"k8s.io/kubernetes/pkg/volume"
+)
+
+func addSecretsToOptions(options map[string]string, spec *volume.Spec, namespace string, host volume.VolumeHost) error {


you can reuse GetSecretForPV

rootfs · 2017-02-21T14:35:34Z

pkg/volume/flexvolume/util.go

+	return
+}
+
+func prepareForMount(mounter mount.Interface, deviceMountPath string) (alreadyMounted bool, err error) {


you used named return but alreadyMounted and err are not used.

rootfs · 2017-02-21T14:36:42Z

pkg/volume/flexvolume/util.go

+// Mounts the device at the given path.
+// It is expected that prepareForMount has been called before.
+func doMount(mounter mount.Interface, devicePath, deviceMountPath, fsType string, options []string) error {
+	err := mounter.Mount(devicePath, deviceMountPath, fsType, options)


what if the device needs a format? Can FormatAndMount be called?

rootfs · 2017-02-21T14:39:04Z

pkg/volume/flexvolume/plugin.go

+	plugin.unsupportedCommands = append(plugin.unsupportedCommands, commands...)
+}
+
+// Returns true iff the given command is know to be unsupported.


rootfs · 2017-02-21T14:56:22Z

pkg/volume/flexvolume/driver-call.go

+	// Option keys
+	optionFSType    = "kubernetes.io/fsType"
+	optionReadWrite = "kubernetes.io/readwrite"
+	optionKeySecret = "kubernetes.io/secret"


any selinux keys?

rootfs · 2017-02-21T14:58:17Z

pkg/volume/flexvolume/mounter.go

+
+	call.AppendSpec(f.spec, f.plugin.host, extraOptions)
+
+	_, err = call.Run()


if fsGroup is present and !readOnly, call SetVolumeOwnership

chakri-nelluri · 2017-02-21T21:10:33Z

Review status: 0 of 31 files reviewed at latest revision, 7 unresolved discussions.

pkg/volume/flexvolume/driver-call.go, line 51 at r1 (raw file):

Previously, rootfs (Huamin Chen) wrote…

any selinux keys?

What is the mechanism to get selinux keys?

pkg/volume/flexvolume/mounter.go, line 80 at r1 (raw file):

Previously, rootfs (Huamin Chen) wrote…

if fsGroup is present and !readOnly, call SetVolumeOwnership

Done

pkg/volume/flexvolume/plugin.go, line 176 at r1 (raw file):

Previously, rootfs (Huamin Chen) wrote…

->known

Done

pkg/volume/flexvolume/util.go, line 2 at r1 (raw file):

Previously, rootfs (Huamin Chen) wrote…

2017

Done for all files.

pkg/volume/flexvolume/util.go, line 31 at r1 (raw file):

GetSecretForPV
Done

pkg/volume/flexvolume/util.go, line 66 at r1 (raw file):

named return but alreadyMounted and err are not used.
Fixed

pkg/volume/flexvolume/util.go, line 86 at r1 (raw file):

Previously, rootfs (Huamin Chen) wrote…

what if the device needs a format? Can FormatAndMount be called?

This call just bind mounts or mount an existing filesystem. So formatandmount is offloaded to the plugin using this interface. But FormatAndMount is also missing from MountDevice. Added it in mountDevice.

Comments from Reviewable

chakri-nelluri · 2017-02-21T21:43:55Z

Thanks @rootfs. Addressed the review comments. PTAL when you get a chance.

mcluseau · 2017-02-21T22:11:31Z

LGTM ;)

jsafrane · 2017-02-22T13:52:50Z

pkg/util/exec/exec.go

@@ -48,6 +49,8 @@ type Cmd interface {
 	SetDir(dir string)
 	SetStdin(in io.Reader)
 	SetStdout(out io.Writer)
+	// Stops the command (if it is running)
+	Stop()


Looking at the implementation below, the process does not need to be stopped when this function returns, it can wait for SIGKILL. Put there at least a comment that it's expected behavior.

jsafrane · 2017-02-22T13:58:43Z

pkg/volume/flexvolume/attacher-defaults.go

+
+// Attach is part of the volume.Attacher interface
+func (a *attacherDefaults) Attach(spec *volume.Spec, hostName types.NodeName) (string, error) {
+	glog.Warning(logPrefix(a.plugin), "using default Attach for volume ", spec.Name, ", host ", hostName)


is this default attacher supposed to be used? If so, please lower the log level, warning is quite offensive here if it is part of normal operation. +update it in all default implementations below.

If it should not be used at all then return an error instead.

Warning is desired because of the way that Flex is implemented. It implements the attacher interface but individual drivers may or may not implement attacher methods. Therefore, we want it to be very clear when Flex decides to use a default method.

jsafrane · 2017-02-22T15:35:00Z

Please add a release note. Follow the guide in new pr template at https://raw.githubusercontent.com/kubernetes/kubernetes/master/.github/PULL_REQUEST_TEMPLATE.md. This ```release-note section needs to be in the top comment of this PR.

chakri-nelluri · 2017-02-22T19:58:52Z

Thanks @jsafrane. Will add it using the template.

Review status: 0 of 31 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

pkg/util/exec/exec.go, line 53 at r2 (raw file):

Previously, jsafrane (Jan Šafránek) wrote…

Looking at the implementation below, the process does not need to be stopped when this function returns, it can wait for SIGKILL. Put there at least a comment that it's expected behavior.

Added the details.

pkg/volume/flexvolume/attacher-defaults.go, line 36 at r2 (raw file):

Previously, jsafrane (Jan Šafránek) wrote…

is this default attacher supposed to be used? If so, please lower the log level, warning is quite offensive here if it is part of normal operation. +update it in all default implementations below.

If it should not be used at all then return an error instead.

Miss from debugging. Fixed the log level.

Comments from Reviewable

jsafrane · 2017-02-23T08:51:32Z

pkg/volume/flexvolume/plugin-defaults.go

+}
+
+func (plugin *pluginDefaults) GetVolumeName(spec *volume.Spec) (string, error) {
+	glog.Warning(logPrefix((*flexVolumePlugin)(plugin)), "using default GetVolumeName for volume ", spec.Name)


leftover warning

chakri-nelluri · 2017-02-24T19:13:04Z

Review status: 0 of 33 files reviewed at latest revision, 21 unresolved discussions, some commit checks failed.

pkg/volume/flexvolume/attacher.go, line 106 at r4 (raw file):

Previously, MikaelCluseau (Mikaël Cluseau) wrote…

I should probably be called "areAttached" then, too?

Yep. If we change it.

Comments from Reviewable

saad-ali · 2017-02-24T23:55:23Z

Almost there. A couple of comments. Once those are addressed, please squash commits, and modify the release-note to be more descriptive. Indicate that Flex volume driver interface is being modified to expose new attach/detach hooks, that these are different from previous releases because... and they will require Flex Driver authors to update their code accordingly.

chakri-nelluri · 2017-02-25T01:16:17Z

Review status: 0 of 33 files reviewed at latest revision, 21 unresolved discussions, some commit checks failed.

pkg/volume/flexvolume/attacher-defaults.go, line 36 at r2 (raw file):

Previously, saad-ali (Saad Ali) wrote…

Warning is desired because of the way that Flex is implemented. It implements the attacher interface but individual drivers may or may not implement attacher methods. Therefore, we want it to be very clear when Flex decides to use a default method.

Ack.

pkg/volume/flexvolume/attacher.go, line 106 at r4 (raw file):

Previously, saad-ali (Saad Ali) wrote…

I'll let you and @MikaelCluseau decide, if it is too difficult to implement.

It is too difficult to implement for driver writers. As per isAttached vs areAttached, I would keep it isAttached as it is a per volume call out. :)

pkg/volume/flexvolume/detacher.go, line 94 at r4 (raw file):

Previously, saad-ali (Saad Ali) wrote…

nit: @chakri-nelluri Message should be something like "Skipped removing directory because directory does not exist" (the lack of directory does not indicate that unmount was skipped) and probably an Info is sufficient instead of warning.

Oops. Somehow this change is missing. I removed the log altogether.

Comments from Reviewable

chakri-nelluri · 2017-02-25T01:18:51Z

Thanks @saad-ali. Addressed the review comments.

saad-ali · 2017-02-25T01:25:40Z

/lgtm
/approve

k8s-github-robot · 2017-02-25T01:25:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

The following people have approved this PR: chakri-nelluri, saad-ali

Needs approval from an approver in each of these OWNERS Files:

examples/OWNERS
pkg/OWNERS
~~pkg/volume/flexvolume/OWNERS~~ [chakri-nelluri,saad-ali]

We suggest the following people:
cc @brendandburns
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

saad-ali · 2017-02-25T01:27:18Z

Please remember to:

Modify the release-note to be more descriptive. Indicate that Flex volume driver interface is being modified to expose new attach/detach hooks, that these are different from previous releases because... and they will require Flex Driver authors to update their code accordingly.

chakri-nelluri · 2017-02-25T01:32:03Z

Thanks @saad-ali and
of course @MikaelCluseau most of the code changes are from his previous PR.

k8s-github-robot · 2017-02-27T12:10:24Z

Automatic merge from submit-queue (batch tested with PRs 41116, 41804, 42104, 42111, 42120)

klausenbusk · 2017-03-13T01:26:45Z

It seems like the README was forgotten in this PR, it still shows the old design.

chakri-nelluri · 2017-03-13T21:37:15Z

@klausenbusk I am working on updating the documentation. It should be in tomorrow.

klausenbusk · 2017-03-14T11:54:57Z

@klausenbusk I am working on updating the documentation. It should be in tomorrow.

That sounds great :) I'm properly going to update https://github.com/tonyzou/flexvolumes DigitalOcean plugin for 1.6 support.

klausenbusk · 2017-03-20T06:50:24Z

Another question, shouldn't we also pass secret to the detach function? or should the API key be stored outside k8s?
This is properly a new issue, I'm just wondering if this is intentional or?

mcluseau · 2017-03-20T08:51:29Z

@klausenbusk secrets are namespaced while volumes aren't so, with the redesign, it was chosen to drop this feature. Some background is here: #26926 (comment).
[edit] I mean, secrets aren't supported at the attach/detach level, but are available for the mount call (because then the pod is known, giving us a namespace).

chakri-nelluri · 2017-03-22T19:38:50Z

@klausenbusk Updated documentation is at https://github.com/kubernetes/community/tree/master/contributors/devel/flexvolume.md.

Ping me if you have any questions.

klausenbusk · 2017-03-29T23:41:39Z

Ping me if you have any questions.

I couldn't get my K8s 1.6 cluster to work (kube-dns issues), so I'm "stuck" on 1.5.4 for a little while (properly until the CoreOS guys update their scripts).

I have been wondering about one thing.
My k8s cluster is running on DigitalOcean, and they "only" allow 5 "block storage disk" per node. Can the current k8s flexvolume code handle that somehow? Ex. can my flexvolume plugin say "Disk limit reached, please find another node"? I'm properly gonna open a new issue for that, I just wanna hear what you think, before I do that.

I think it is very unlikely that I end in a situation where 5 disk is attached to a single droplet, but someone is properly gonna hit the limit at some point.

mcluseau · 2017-03-30T03:23:40Z

@klausenbusk I think it's more a scheduler issue; I think I've seen something about that for GCE or AWS limit at 16 volumes/node.

mcluseau · 2017-03-30T03:26:25Z

yep, it's #7835 -> #19880 ; I think you can open an issue about the DigitalOcean case, but it's not the responsibility of the volume plugin so there's no interface to say that (for now at least).

klausenbusk · 2017-03-30T10:02:49Z

yep, it's #7835 -> #19880 ; I think you can open an issue about the DigitalOcean case, but it's not the responsibility of the volume plugin so there's no interface to say that (for now at least).

Thanks, I did a little more searching and stumbled on: #24317 which links to https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#opaque-integer-resources-alpha-feature which could solve the issue. There is also KUBE_MAX_PD_VOLS, I'm not exactly sure how it works, if I set it to 5 will it work then?

chakri-nelluri · 2017-03-30T22:58:32Z

@klausenbusk Check if you can leverage #13580 to integrate with. There is a new bind proposal for scheduler #41447, trying to address the limitations.

klausenbusk · 2017-11-14T23:47:41Z

@klausenbusk Check if you can leverage #13580 to integrate with. There is a new bind proposal for scheduler #41447, trying to address the limitations.

~~I'm wondering if it would make sense to extend the MaxPDVolumeCountChecker to support flexvolume plugins?~~

Edit: See #55738

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 21, 2017

k8s-github-robot assigned deads2k Feb 21, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Feb 21, 2017

deads2k assigned ingvagabund and unassigned deads2k Feb 21, 2017

ingvagabund assigned jsafrane and unassigned ingvagabund Feb 21, 2017

rootfs reviewed Feb 21, 2017

View reviewed changes

pkg/volume/flexvolume/util.go Outdated

@@ -0,0 +1,111 @@

/*

Copyright 2016 The Kubernetes Authors.

Copy link

Contributor

rootfs Feb 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2017

rootfs reviewed Feb 21, 2017

View reviewed changes

jsafrane reviewed Feb 22, 2017

View reviewed changes

jsafrane assigned rootfs Feb 22, 2017

k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Feb 22, 2017

jsafrane reviewed Feb 23, 2017

View reviewed changes

Add support for attacher/detacher interface in Flex volume

0d2af70

chakri-nelluri force-pushed the flex branch from 9035bf0 to 0d2af70 Compare February 25, 2017 01:18

saad-ali approved these changes Feb 25, 2017

View reviewed changes

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

saad-ali added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 25, 2017

k8s-github-robot merged commit d1f5331 into kubernetes:master Feb 27, 2017

codenrhoden mentioned this pull request Mar 20, 2017

k8s 1.6 Flex Volume support rexray/rexray#777

Closed

klausenbusk mentioned this pull request Mar 31, 2017

Feature: Provide "Scheduler extender" for k8s rexray/rexray#795

Open

klausenbusk mentioned this pull request Aug 27, 2017

flexVolume: LVM example broken per #47400 #51407

Closed


		call.AppendSpec(f.spec, f.plugin.host, extraOptions)

		_, err = call.Run()

Add support for attacher/detacher interface in Flex volume #41804

Add support for attacher/detacher interface in Flex volume #41804

Conversation

chakri-nelluri commented Feb 21, 2017 • edited

k8s-ci-robot commented Feb 21, 2017

k8s-reviewable commented Feb 21, 2017

chakri-nelluri commented Feb 21, 2017

rootfs commented Feb 21, 2017

rootfs commented Feb 21, 2017

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

chakri-nelluri commented Feb 21, 2017

chakri-nelluri commented Feb 21, 2017

mcluseau commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented Feb 22, 2017

chakri-nelluri commented Feb 22, 2017

Choose a reason for hiding this comment

chakri-nelluri commented Feb 24, 2017

saad-ali commented Feb 24, 2017

chakri-nelluri commented Feb 25, 2017

chakri-nelluri commented Feb 25, 2017

saad-ali commented Feb 25, 2017

k8s-github-robot commented Feb 25, 2017

saad-ali commented Feb 25, 2017

chakri-nelluri commented Feb 25, 2017

k8s-github-robot commented Feb 27, 2017

klausenbusk commented Mar 13, 2017 • edited

chakri-nelluri commented Mar 13, 2017

klausenbusk commented Mar 14, 2017

klausenbusk commented Mar 20, 2017

mcluseau commented Mar 20, 2017 • edited

chakri-nelluri commented Mar 22, 2017

klausenbusk commented Mar 29, 2017

mcluseau commented Mar 30, 2017

mcluseau commented Mar 30, 2017

klausenbusk commented Mar 30, 2017

chakri-nelluri commented Mar 30, 2017 • edited

klausenbusk commented Nov 14, 2017 • edited

chakri-nelluri commented Feb 21, 2017 •

edited

klausenbusk commented Mar 13, 2017 •

edited

mcluseau commented Mar 20, 2017 •

edited

chakri-nelluri commented Mar 30, 2017 •

edited

klausenbusk commented Nov 14, 2017 •

edited