Remove controller node plugin driver dependency for non-attachable fl… #47503

chakri-nelluri · 2017-06-14T07:11:21Z

…ex volume drivers (Ex: NFS).

What this PR does / why we need it:
Removes requirement to install flex volume drivers on master node for non-attachable drivers likes NFS.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #47109

Fixes issue w/Flex volume, introduced in 1.6.0, where drivers without an attacher would fail (node indefinitely waiting for attach). Drivers that don't implement attach should return `attach: false` on `init`.

k8s-ci-robot · 2017-06-14T07:11:29Z

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.

I understand the commands that are listed here.

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.

msau42 · 2017-06-14T17:44:39Z

/assign @saad-ali

cc @verult

msau42 · 2017-06-14T17:46:12Z

@k8s-bot ok to test

msau42 · 2017-06-14T17:50:09Z

pkg/volume/flexvolume/probe.go

+			ok, err := attachablePlugin.isAttachable()
+			if err != nil {
+				glog.Errorf("Unable to probe plugin: %s", attachablePlugin.driverName)
+				continue


This is going to require all existing flex drivers to implement the new probe call. Can we have a default instead?

If the capabilities call isn't implemented, isAttachable() will return true, and the old behavior stays.

Yes. This change doesn't require you to modify the existing plugins.

msau42 · 2017-06-14T18:05:47Z

cc @kubernetes/sig-storage-pr-reviews

verult · 2017-06-14T17:49:07Z

pkg/volume/flexvolume/plugin.go

+	flexVolumePlugin
+}
+
+var _ volume.AttachableVolumePlugin = &flexVolumeAttachablePlugin{}
 var _ volume.PersistentVolumePlugin = &flexVolumePlugin{}


The naming here might cause confusions (attachable vs persistent?) ?

These are not flex objects. These are infra objects and we are just declaring it.

verult · 2017-06-14T17:51:36Z

pkg/volume/flexvolume/plugin.go

+	}
+
+	// By default all plugins are attachable, unless they report otherwise.
+	cap, ok := res.CapabilityMap[attachCapability]


Looks like cap is a keyword. Should be OK here, though.

I want to rename it to capabilities, similar to annotations & labels. Will update the PR.

verult · 2017-06-14T18:00:38Z

pkg/volume/flexvolume/plugin.go

+
+	// By default all plugins are attachable, unless they report otherwise.
+	cap, ok := res.CapabilityMap[attachCapability]
+	if ok && !cap {


"&& !cap" isn't necessary here. The two return statements can also be collapsed into one.

yep.. remove !cap :) We still need two return statements.

verult · 2017-06-14T18:23:21Z

pkg/volume/flexvolume/probe.go

+			} else {
+				// Plugin does not support attach/detach, so return flexVolumePlugin which supports
+				// SetupAt & TearDown functionality
+				plugin := &flexVolumePlugin{


Could reuse the same instance in line 45.

verult · 2017-06-14T18:25:56Z

pkg/volume/flexvolume/probe.go

+			// Check whether the plugin is attachable.
+			ok, err := attachablePlugin.isAttachable()
+			if err != nil {
+				glog.Errorf("Unable to probe plugin: %s", attachablePlugin.driverName)


I recommend including the isAttachable() error in the message to give a reason why probe is not successful.

yep. Copy & paste typo. will fix it.

verult · 2017-06-14T18:27:13Z

pkg/volume/flexvolume/driver-call.go

@@ -198,6 +200,10 @@ type DriverStatus struct {
 	VolumeName string `json:"volumeName,omitempty"`
 	// Represents volume is attached on the node
 	Attached bool `json:"attached,omitempty"`
+	// Returns capabilites of the driver.


verult · 2017-06-14T18:41:48Z

pkg/volume/flexvolume/probe.go

+			ok, err := attachablePlugin.isAttachable()
+			if err != nil {
+				glog.Errorf("Unable to probe plugin: %s", attachablePlugin.driverName)
+				continue


If the capabilities call isn't implemented, isAttachable() will return true, and the old behavior stays.

verult · 2017-06-14T18:43:13Z

pkg/volume/flexvolume/attacher.go


-	return (*attacherDefaults)(a).GetDeviceMountPath(spec, mountsDir)
+	volumeName, err := a.plugin.GetVolumeName(spec)


GetVolumeName() is called before every makeGlobalMountPath(). Maybe move the call inside makeGlobalMountPath().

saad-ali · 2017-06-14T22:46:47Z

pkg/volume/flexvolume/probe.go

-				unsupportedCommands: []string{},
-			})
+
+			attachablePlugin := &flexVolumeAttachablePlugin{


nit: Factoring is a little strange here. Two suggestions:

How about introducing a constructor method NewFlexVolumePlugin(...) that takes all the flexVolumePlugin fields as a parameter. It would internally test for attachability and return either a flexVolumeAttachablePlugin or a flexVolumePlugin--that'll simplify the code here.

For testing attachability, instead of constructing a flexVolumePlugin object and calling isAttachable() on it, it would be cleaner to have IsAttachable(...) be a static method that takes all the flexVolumePlugin fields as a parameter, make the appropriate calls to the driver to determine attachability, and then return true/false.

Thanks for the feedback. Refactored it. PTAL.

saad-ali · 2017-06-14T22:54:17Z

pkg/volume/flexvolume/plugin.go

+		return false, err
+	}
+
+	// By default all plugins are attachable, unless they report otherwise.


How about making the default false, aka non-attachable? And if a plugin implements attach, then they must explicitly also set capability to true?

That is going to break existing plugins. That is the only reason. Doesn't benefit too much either-way.

To clarify it would break existing post-1.6 drivers right? Pre-1.6 drivers would continue to work as is if the default was false, right?

Yes. For Post 1.6 drivers. Users have to modify their drivers to work for 1.6+ anyways, because of change mount callout.

Ack. Ok fine to leave as is.

jsafrane · 2017-06-15T08:01:17Z

I don't understand how this PR "Removes requirement to install flex volume drivers on master node" - if there is no flex volume driver on the master, then controller-manager does not create a volume plugin for it and the situation is exactly the same as before this PR. Calling init and parsing its output won't change a thing, because there is nothing to call on the master.

chakri-nelluri · 2017-06-15T15:01:20Z

@jsafrane. This will fix it on Kubelet side. Now Kubelet won't wait for controller-manager to attach the volume for it.

chakri-nelluri · 2017-06-15T17:48:22Z

Thanks @saad-ali @verult @msau42. Rebased it to one commit.

saad-ali

/lgtm
/approve

saad-ali · 2017-06-15T17:57:54Z

pkg/volume/flexvolume/probe.go

-				runner:              exec.New(),
-				unsupportedCommands: []string{},
-			})
+			plugin, err := NewFlexVolumePlugin(pluginDir, f.Name())


Beautiful! Thanks for refactoring, so much cleaner!

saad-ali · 2017-06-15T18:04:41Z

/lgtm
/approve

I added a release note to original comment.

…ex volume drivers (Ex: NFS).

chakri-nelluri

Ack

chakri-nelluri · 2017-06-17T07:47:16Z

pkg/volume/flexvolume/plugin.go

@@ -71,21 +123,13 @@ func (plugin *flexVolumePlugin) GetVolumeName(spec *volume.Spec) (string, error)
 	call := plugin.NewDriverCall(getVolumeNameCmd)
 	call.AppendSpec(spec, plugin.host, nil)

-	_, err := call.Run()
+	status, err := call.Run()


Good Catch. Oops.. Mangled in rebase. Let me fix it.

saad-ali · 2017-06-19T18:00:58Z

/lgtm
/approve

saad-ali · 2017-06-19T18:01:15Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

verult · 2017-06-19T18:02:59Z

/approve

k8s-github-robot · 2017-06-19T18:06:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chakri-nelluri, saad-ali, verult
We suggest the following additional approver: zmerlynn

Assign the PR to them by writing /assign @zmerlynn in a comment when ready.

Associated issue: 47109

The full list of commands accepted by this bot can be found here.

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

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

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

marun · 2017-06-20T20:09:17Z

/test pull-kubernetes-e2e-gce-etcd3

saad-ali · 2017-06-20T20:52:14Z

/test pull-kubernetes-e2e-gce-etcd3

saad-ali · 2017-06-20T23:07:30Z

/test pull-kubernetes-e2e-gce-etcd3

saad-ali · 2017-06-20T23:07:34Z

This test suite is super flaky! @marun you may want to consider manual merge.

marun · 2017-06-21T19:03:42Z

/retest

dchen1107 · 2017-06-22T02:40:30Z

/retest

k8s-github-robot · 2017-06-22T04:12:15Z

Automatic merge from submit-queue (batch tested with PRs 47878, 47503, 47857)

feiskyer · 2017-08-14T06:49:28Z

Will this fix be cherry-picked to 1.7 branch? The milestone is v1.7 but there is no cherry-pick labels.

verult · 2017-08-16T23:10:55Z

Looks like it's already in the 1.7 branch:
https://github.com/kubernetes/kubernetes/blob/release-1.7/pkg/volume/flexvolume/driver-call.go#L207

Automatic merge from submit-queue Updating FlexVolume doc with capabilities addition. The following PR introduces a FlexVolume API change, so the documentation needs to be updated. kubernetes/kubernetes#47503

resouer · 2017-08-22T03:13:44Z

FYI: you need to use 1.7.4+ to include this fix.

kokhang · 2017-09-13T21:40:03Z

@verult, @resouer : just found out that non-attachable flexvolumes was broken on 1.6. is there any work around to make it work without having to set the enable-controller-attach-detach=false?

Also, if we set this flag, would that screw things up with other volume plugins?

verult · 2017-09-13T22:55:52Z

@kokhang as long as the driver exists on master, everything should work as expected.

If the flag is set, other plugins may run into attach race conditions between nodes. This was the main problem AttachDetachController solves.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 14, 2017

k8s-github-robot assigned jingxu97 and bprashanth Jun 14, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jun 14, 2017

k8s-ci-robot assigned saad-ali Jun 14, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 14, 2017

msau42 reviewed Jun 14, 2017

View reviewed changes

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jun 14, 2017

verult reviewed Jun 14, 2017

View reviewed changes

saad-ali reviewed Jun 14, 2017

View reviewed changes

saad-ali added this to the v1.7 milestone Jun 14, 2017

chakri-nelluri force-pushed the flexcap branch from cbb5a83 to 36ed1c9 Compare June 15, 2017 06:56

chakri-nelluri force-pushed the flexcap branch from 36ed1c9 to 7c0047f Compare June 15, 2017 17:47

saad-ali approved these changes Jun 15, 2017

View reviewed changes

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 Jun 15, 2017

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

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

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 15, 2017

chakri-nelluri force-pushed the flexcap branch from 61bcad6 to b97dcad Compare June 15, 2017 23:37

Remove controller node plugin driver dependency for non-attachable fl…

894b9b2

…ex volume drivers (Ex: NFS).

chakri-nelluri force-pushed the flexcap branch from b97dcad to 894b9b2 Compare June 17, 2017 18:34

chakri-nelluri commented Jun 18, 2017

View reviewed changes

saad-ali approved these changes Jun 19, 2017

View reviewed changes

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

saad-ali mentioned this pull request Jun 19, 2017

nfs flexvolume example not working #47109

Closed

dchen1107 assigned msau42 and saad-ali and unassigned saad-ali, bprashanth and jingxu97 Jun 22, 2017

k8s-github-robot merged commit d021db8 into kubernetes:master Jun 22, 2017

verult mentioned this pull request Jul 6, 2017

Updating FlexVolume doc with capabilities addition. kubernetes/community#786

Merged


		return (*attacherDefaults)(a).GetDeviceMountPath(spec, mountsDir)
		volumeName, err := a.plugin.GetVolumeName(spec)

Remove controller node plugin driver dependency for non-attachable fl… #47503

Remove controller node plugin driver dependency for non-attachable fl… #47503

Conversation

chakri-nelluri commented Jun 14, 2017 • edited by saad-ali

k8s-ci-robot commented Jun 14, 2017

msau42 commented Jun 14, 2017

msau42 commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Jun 14, 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

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

chakri-nelluri Jun 15, 2017 • edited

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

jsafrane commented Jun 15, 2017

chakri-nelluri commented Jun 15, 2017

chakri-nelluri commented Jun 15, 2017

saad-ali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Jun 15, 2017

chakri-nelluri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Jun 19, 2017

saad-ali commented Jun 19, 2017

verult commented Jun 19, 2017

k8s-github-robot commented Jun 19, 2017

marun commented Jun 20, 2017

saad-ali commented Jun 20, 2017

saad-ali commented Jun 20, 2017

saad-ali commented Jun 20, 2017

marun commented Jun 21, 2017

dchen1107 commented Jun 22, 2017

k8s-github-robot commented Jun 22, 2017

feiskyer commented Aug 14, 2017 • edited

verult commented Aug 16, 2017

resouer commented Aug 22, 2017

kokhang commented Sep 13, 2017

verult commented Sep 13, 2017

chakri-nelluri commented Jun 14, 2017 •

edited by saad-ali

chakri-nelluri Jun 15, 2017 •

edited

feiskyer commented Aug 14, 2017 •

edited