Enable Attach/Detach Controller #26351

saad-ali · 2016-05-26T09:11:33Z

This PR contains changes to enable the attach/detach controller introduced in #25457 (proposed in #20262).

Specifically it:

Introduces a new enable-controller-attach-detach kubelet flag to enable control by attach/detach controller. Default enabled.
Removes all references SafeToDetach annotation from controller.
Adds the new VolumesInUse field to the Node Status API object.
Modifies the controller to use VolumesInUse instead of SafeToDetach annotation to gate detachment.
Modifies kubelet to set VolumesInUse before Mount and after Unmount.
- There is a bug in the node-problem-detector binary that causes VolumesInUse to get reset to nil every 30 seconds. Issue Node-Problem-Detector should Patch NodeStatus not Update node-problem-detector#9 (comment) opened to fix that.
- There is a bug here in the mount/unmount code that prevents resetting `VolumeInUse in some cases, this will be fixed by mount/unmount refactor.
Have controller process detaches before attaches so that volumes referenced by pods that are rescheduled to a different node are detached first.
Fix misc bugs in controller.
Modify GCE attacher to: remove retries, remove mutex, and not fail if volume is already attached or already detached.

Fixes #14642, #19953

Kubernetes v1.3 introduces a new Attach/Detach Controller. This controller manages attaching and detaching of volumes on-behalf of nodes. 

This ensures that attachment and detachment of volumes is independent of any single nodes’ availability. Meaning, if a node or kubelet becomes unavailable for any reason, the volumes attached to that node will be detached so they are free to be attached to other nodes.

Specifically the new controller watches the API server for scheduled pods. It processes each pod and ensures that any volumes that implement the volume Attacher interface are attached to the node their pod is scheduled to.

When a pod is deleted, the controller waits for the volume to be safely unmounted by kubelet. It does this by waiting for the volume to no longer be present in the nodes Node.Status.VolumesInUse list. If the volume is not safely unmounted by kubelet within a pre-configured duration (3 minutes in Kubernetes v1.3), the controller unilaterally detaches the volume (this prevents volumes from getting stranded on nodes that become unavailable).

In order to remain backwards compatible, the new controller only manages attach/detach of volumes that are scheduled to nodes that opt-in to controller management. Nodes running v1.3 or higher of Kubernetes opt-in to controller management by default by setting the "volumes.kubernetes.io/controller-managed-attach-detach" annotation on the Node object on startup. This behavior is gated by a new kubelet flag, "enable-controller-attach-detach,” (default true).

In order to safely upgrade an existing Kubernetes cluster without interruption of volume attach/detach logic:
* First upgrade the master to Kubernetes v1.3.
    * This will start the new attach/detach controller.
    * The new controller will initially ignore volumes for all nodes since they lack the "volumes.kubernetes.io/controller-managed-attach-detach" annotation.
* Then upgrade nodes to Kubernetes v1.3.
    * As nodes are upgraded, they will automatically, by default, opt-in to attach/detach controller management, which will cause the controller to start managing attaches/detaches for volumes that get scheduled to those nodes.

Temporarily disable node-problem-detector to unblock #26351

saad-ali · 2016-05-27T18:24:54Z

Ran all the PD tests back to back overnight with these changes, and zero failures. Woo hoo :)

k8s-github-robot · 2016-05-30T00:44:31Z

@saad-ali
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

saad-ali · 2016-05-30T00:45:40Z

@k8s-merge-robot you're drunk, go home

saad-ali · 2016-05-30T00:47:01Z

@k8s-bot unit test this issue: #26256

To make merge-bot happy

pmorie · 2016-06-01T19:33:28Z

@saad-ali seems like this should have a release note to me, agree?

pmorie · 2016-06-01T19:35:12Z

There is a bug here in the mount/unmount code that prevents resetting `VolumeInUse in some cases, this will be fixed by mount/unmount refactor.

Link?

pmorie · 2016-06-01T19:39:23Z

pkg/controller/volume/attach_detach_controller.go

 			if err != nil {
 				glog.V(10).Infof(
-					"Failed to delete volume %q for pod %q/%q from desiredStateOfWorld. GetUniqueVolumeName failed with %v",
+					"Failed to delete volume %q for pod %q/%q from desiredStateOfWorld. GenerateUniqueDeviceName failed with %v",


Nit: log messages are supposed to start with lowercase letters -- no need to fix now, perhaps in a follow up.

My understanding is that rule is for error strings, since they may be concatenated at a higher level before being printed. But log messages are basically the end of the line so it is ok to capitalize in a way that makes it user readable.

GoLang guidence (https://github.com/golang/go/wiki/CodeReviewComments#error-strings) seems to align with this: This does not apply to logging, which is implicitly line-oriented and not combined inside other messages.

saad-ali · 2016-06-02T00:47:39Z

@saad-ali seems like this should have a release note to me, agree?

Yep, added release not to PR.

Link?

No issue to track it since all the kubelet code for mount/unmount/attach/detach is being rewritten. Mondo PR based on this PR is pending. It removes this note and fixes the issue.

saad-ali · 2016-06-02T00:49:47Z

Thanks Paul! Feedback addressed, PTAL

Will squash commits on LGTM

pmorie · 2016-06-02T19:14:17Z

pkg/controller/volume/cache/actual_state_of_world.go

@@ -83,6 +84,11 @@ type ActualStateOfWorld interface {
 	// reflecting which volumes are attached to which nodes based on the
 	// current actual state of the world.
 	GetAttachedVolumes() []AttachedVolume


Does anything actually use the global list of attached volumes now?

Yes, this list is used by the reconciler

This PR contains Kubelet changes to enable attach/detach controller control. * It introduces a new "enable-controller-attach-detach" kubelet flag to enable control by controller. Default enabled. * It removes all references "SafeToDetach" annoation from controller. * It adds the new VolumesInUse field to the Node Status API object. * It modifies the controller to use VolumesInUse instead of SafeToDetach annotation to gate detachment. * There is a bug in node-problem-detector that causes VolumesInUse to get reset every 30 seconds. Issue kubernetes/node-problem-detector#9 opened to fix that.

saad-ali · 2016-06-02T23:48:58Z

Thanks Paul. Added comments to test. PTAL

saad-ali · 2016-06-03T00:39:53Z

@k8s-bot unit test this issue: #25845

k8s-bot · 2016-06-03T00:40:48Z

GCE e2e build/test passed for commit 9dbe943.

pmorie · 2016-06-03T04:58:31Z

pkg/controller/volume/cache/actual_state_of_world_test.go

+	attachedVolumes := asw.GetAttachedVolumesForNode(nodeName)
+
+	// Assert
+	if len(attachedVolumes) != 1 {


for stuff like this i like to use this syntax:

if l := len(attachedVolumes); l != 1 { t.Fatalf("bla bla %v", l) }

..but you can do that in a follow up if you so choose.

pmorie · 2016-06-03T05:06:32Z

LGTM, looking forward to reading the next chapter of this tale.

k8s-github-robot · 2016-06-03T05:43:20Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-06-03T06:27:34Z

GCE e2e build/test passed for commit 9dbe943.

k8s-github-robot · 2016-06-03T06:30:32Z

Automatic merge from submit-queue

googlebot added the cla: yes label May 26, 2016

k8s-github-robot assigned bgrant0607 May 26, 2016

Random-Liu mentioned this pull request May 26, 2016

Node-Problem-Detector should Patch NodeStatus not Update kubernetes/node-problem-detector#9

Closed

saad-ali force-pushed the attachDetachControllerKubeletChanges branch from 9d1e678 to 57da768 Compare May 26, 2016 18:31

Random-Liu added a commit to Random-Liu/kubernetes that referenced this pull request May 26, 2016

Temporarily disable node-problem-detector to unblock kubernetes#26351

e5320c6

Random-Liu mentioned this pull request May 26, 2016

Temporarily disable node-problem-detector to unblock #26351 #26384

Merged

saad-ali force-pushed the attachDetachControllerKubeletChanges branch from 57da768 to 87ccfdc Compare May 26, 2016 22:20

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2016

alex-mohr added a commit that referenced this pull request May 26, 2016

Merge pull request #26384 from Random-Liu/disable-node-problem-detector

1dfa068

Temporarily disable node-problem-detector to unblock #26351

saad-ali force-pushed the attachDetachControllerKubeletChanges branch 2 times, most recently from bb27c79 to cacfb2a Compare May 27, 2016 08:05

saad-ali assigned thockin and unassigned bgrant0607 May 27, 2016

saad-ali added this to the v1.3 milestone May 27, 2016

saad-ali added the priority/backlog Higher priority than priority/awaiting-more-evidence. label May 27, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 28, 2016

saad-ali force-pushed the attachDetachControllerKubeletChanges branch from cacfb2a to e3b064c Compare May 29, 2016 23:36

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2016

pmorie reviewed Jun 1, 2016
View reviewed changes

saad-ali force-pushed the attachDetachControllerKubeletChanges branch from e3b064c to 0f92bb4 Compare June 2, 2016 00:48

pmorie mentioned this pull request Jun 2, 2016

Volume attach/detach in clouds without deterministic attach paths #26679

Closed

pmorie reviewed Jun 2, 2016
View reviewed changes

saad-ali force-pushed the attachDetachControllerKubeletChanges branch from 0f92bb4 to 9dbe943 Compare June 2, 2016 23:47

pmorie reviewed Jun 3, 2016
View reviewed changes

pmorie added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 3, 2016

k8s-github-robot merged commit 75ef1ca into kubernetes:master Jun 3, 2016

saad-ali mentioned this pull request Jun 3, 2016

e2e flake: kubelet should be able to delete 10 pods per node in 1m0s #23591

Closed

ianlewis mentioned this pull request Jun 4, 2016

detach EBS volumes when node restarted #26847

Closed

goltermann mentioned this pull request Jun 6, 2016

GCE PD Volumes already attached to a node fail with "Error 400: The disk resource is already being used by" node #19953

Closed

jsafrane mentioned this pull request Jun 7, 2016

[k8s.io] Dynamic provisioning [k8s.io] DynamicProvisioner should create and delete persistent volumes {Kubernetes e2e suite} #26682

Closed

jingxu97 mentioned this pull request Jun 8, 2016

[k8s.io] Services should serve a basic endpoint from pods [Conformance] {Kubernetes e2e suite} #26678

Closed

saad-ali mentioned this pull request Jun 19, 2016

Detailed Design for Volume Attach/Detach Controller #20262

Closed

saad-ali changed the title ~~Attach/Detach Controller Kubelet Changes~~ Enable Attach/Detach Controller Jun 19, 2016

saad-ali mentioned this pull request Jun 19, 2016

EBS volume mount failures due to "... already attached to an instance" are not retried #18785

Closed

spinolacastro mentioned this pull request Jun 25, 2016

Ceph : Unable to mount volumes for pod ... : rbd: image ext4-0002 is locked by other nodes openshift/origin#7983

Closed

saad-ali mentioned this pull request Jul 1, 2016

Kubernetes Volume System Redesign Proposal #18333

Closed

cvle mentioned this pull request Jul 23, 2016

Remove compute-rw scope from GCE nodes #8074

Closed

saad-ali deleted the attachDetachControllerKubeletChanges branch August 15, 2016 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Attach/Detach Controller #26351

Enable Attach/Detach Controller #26351

saad-ali commented May 26, 2016 •

edited by david-mcmahon

Loading

saad-ali commented May 27, 2016

k8s-github-robot commented May 30, 2016

saad-ali commented May 30, 2016

saad-ali commented May 30, 2016

pmorie commented Jun 1, 2016

pmorie commented Jun 1, 2016

pmorie Jun 1, 2016

saad-ali Jun 1, 2016

saad-ali commented Jun 2, 2016

saad-ali commented Jun 2, 2016

pmorie Jun 2, 2016

saad-ali Jun 2, 2016

saad-ali commented Jun 2, 2016

saad-ali commented Jun 3, 2016

k8s-bot commented Jun 3, 2016

pmorie Jun 3, 2016

pmorie commented Jun 3, 2016

k8s-github-robot commented Jun 3, 2016

k8s-bot commented Jun 3, 2016

k8s-github-robot commented Jun 3, 2016

Enable Attach/Detach Controller #26351

Enable Attach/Detach Controller #26351

Conversation

saad-ali commented May 26, 2016 • edited by david-mcmahon Loading

saad-ali commented May 27, 2016

k8s-github-robot commented May 30, 2016

saad-ali commented May 30, 2016

saad-ali commented May 30, 2016

pmorie commented Jun 1, 2016

pmorie commented Jun 1, 2016

pmorie Jun 1, 2016

Choose a reason for hiding this comment

saad-ali Jun 1, 2016

Choose a reason for hiding this comment

saad-ali commented Jun 2, 2016

saad-ali commented Jun 2, 2016

pmorie Jun 2, 2016

Choose a reason for hiding this comment

saad-ali Jun 2, 2016

Choose a reason for hiding this comment

saad-ali commented Jun 2, 2016

saad-ali commented Jun 3, 2016

k8s-bot commented Jun 3, 2016

pmorie Jun 3, 2016

Choose a reason for hiding this comment

pmorie commented Jun 3, 2016

k8s-github-robot commented Jun 3, 2016

k8s-bot commented Jun 3, 2016

k8s-github-robot commented Jun 3, 2016

saad-ali commented May 26, 2016 •

edited by david-mcmahon

Loading