Attach Detach Controller Business Logic #25457

saad-ali · 2016-05-11T05:25:58Z

This PR adds the meat of the attach/detach controller proposed in #20262.

The PR splits the in-memory cache into a desired and actual state of the world.

saad-ali · 2016-05-11T05:29:27Z

This PR is not yet complete. But I wanted to give you a chance to start looking at it.

What's left:

~~Add reconciliation loop between desired and actual state of world.~~
~~Fix detach code path to not require deviceMountPath.~~
~~Add logic to check if volume is safe to detach before detaching.~~
~~Add node population logic (check if annotation present and add to expected state of world data structure).~~

ghost · 2016-05-12T19:14:02Z

cmd/kube-controller-manager/app/controllermanager.go

+			pvInformer,
+			ResyncPeriod(s)(),
+			cloud,
+			ProbeAttachableVolumePlugins(s.VolumeConfiguration.FlexVolumePluginDir))


This feels a bit awkward.. perhaps pass the whole VolumeConfig struct and fish out the settings needed for each plugin in ProbeAttachableVolumePlugins. It is only FlexVolume now but may expand in the future

Ack, changed.

saad-ali · 2016-05-13T08:28:05Z

Ready to review.

ghost · 2016-05-13T20:09:39Z

cmd/kube-controller-manager/app/plugins.go

+	allPlugins = append(allPlugins, gce_pd.ProbeVolumePlugins()...)
+	allPlugins = append(allPlugins, git_repo.ProbeVolumePlugins()...)
+	allPlugins = append(allPlugins, host_path.ProbeVolumePlugins(volume.VolumeConfig{})...)
+	allPlugins = append(allPlugins, nfs.ProbeVolumePlugins(volume.VolumeConfig{})...)


So it looks like nfs and probably hostPath expect a non-empty configuration... it shouldn't really matter anyways because they are not attachable which makes me think: do we need to probe all plugins here or only the ones which have the potential of being attachable ? If someone wants to make NFS attachable in the future they can add a line here and properly construct a configuration object.

I could go either way. I hate this code, the plugin initialization mechanism needs to be redesigned... someday. I'll reduce this to: gce, aws, cinder, and flex.

thockin · 2016-05-24T21:39:32Z

pkg/controller/volume/attach_detach_controller.go

 		selfCreatedPodInformer = true
 	}
 	if nodeInformer == nil {
 		nodeInformer = informers.CreateSharedNodeIndexInformer(kubeClient, resyncPeriod)
 		selfCreatedNodeInformer = true
 	}
+	if pvcInformer == nil {


Is there a legitimate reason to ever pass nil to this function? I dislike this pattern, and I'd rather not have the fallback

Agreed it's ugly. It would be passed in nil for testing, but we could move this responsibility to the test. Will remove it.

thockin · 2016-05-25T05:30:56Z

probably should squash before commit

saad-ali · 2016-05-25T05:42:00Z

probably should squash before commit

Yep, will squash on LGTM. Just wanted to make it easy to see the diffs for code review.

thockin · 2016-05-25T05:48:29Z

LGTM - just make sure not to lose track of the follow-up work.

saad-ali · 2016-05-25T05:54:36Z

LGTM - just make sure not to lose track of the follow-up work.

I've got a list =)

Split controller cache into actual and desired state of world. Controller will only operate on volumes scheduled to nodes that have the "volumes.kubernetes.io/controller-managed-attach" annotation.

saad-ali · 2016-05-26T03:06:41Z

Setting priority to P2 to bump in the submit queue, other 1.3 work is dependent on this.

k8s-github-robot · 2016-05-26T06:46:11Z

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

k8s-bot · 2016-05-26T07:27:22Z

GCE e2e build/test passed for commit 92500a2.

k8s-github-robot · 2016-05-26T07:41:54Z

Automatic merge from submit-queue

saad-ali assigned ghost May 11, 2016

saad-ali added the release-note-none Denotes a PR that doesn't merit a release note. label May 11, 2016

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

googlebot added the cla: yes label May 11, 2016

saad-ali force-pushed the expectedStateOfWorldDataStructure branch from 98f03e6 to d583987 Compare May 11, 2016 19:48

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

saad-ali force-pushed the expectedStateOfWorldDataStructure branch from d583987 to 62cc762 Compare May 11, 2016 19:59

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

saad-ali force-pushed the expectedStateOfWorldDataStructure branch from 62cc762 to 4e7b676 Compare May 12, 2016 03:59

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/old-docs and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 12, 2016

saad-ali force-pushed the expectedStateOfWorldDataStructure branch 2 times, most recently from fcf1e70 to cc104eb Compare May 12, 2016 08:01

ghost reviewed May 12, 2016
View reviewed changes

saad-ali force-pushed the expectedStateOfWorldDataStructure branch 2 times, most recently from e9fff2d to 918e1e2 Compare May 13, 2016 01:14

saad-ali changed the title ~~[WIP] Attach Detach Controller Business Logic~~ Attach Detach Controller Business Logic May 13, 2016

ghost reviewed May 13, 2016
View reviewed changes

thockin reviewed May 24, 2016
View reviewed changes

saad-ali force-pushed the expectedStateOfWorldDataStructure branch from 6303cf9 to 523a541 Compare May 25, 2016 03:59

saad-ali mentioned this pull request May 25, 2016

Persistent Volume Controller should use Shared Informer #26247

Closed

thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2016

Attach detach controller business logic added

92500a2

Split controller cache into actual and desired state of world. Controller will only operate on volumes scheduled to nodes that have the "volumes.kubernetes.io/controller-managed-attach" annotation.

saad-ali force-pushed the expectedStateOfWorldDataStructure branch from 523a541 to 92500a2 Compare May 25, 2016 06:02

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2016

saad-ali added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. labels May 25, 2016

k8s-github-robot merged commit bda0dc8 into kubernetes:master May 26, 2016

saad-ali mentioned this pull request Jun 1, 2016

Problem rescheduling POD with GCE PD disk attached #14642

Closed

ianlewis mentioned this pull request Jun 4, 2016

detach EBS volumes when node restarted #26847

Closed

saad-ali mentioned this pull request Jun 19, 2016

Enable Attach/Detach Controller #26351

Merged

saad-ali deleted the expectedStateOfWorldDataStructure branch August 15, 2016 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attach Detach Controller Business Logic #25457

Attach Detach Controller Business Logic #25457

saad-ali commented May 11, 2016

saad-ali commented May 11, 2016 •

edited

Loading

ghost May 12, 2016 •

edited by ghost

Loading

saad-ali May 12, 2016

saad-ali commented May 13, 2016

ghost May 13, 2016

saad-ali May 13, 2016

thockin May 24, 2016

saad-ali May 24, 2016

saad-ali May 25, 2016

thockin commented May 25, 2016

saad-ali commented May 25, 2016

thockin commented May 25, 2016

saad-ali commented May 25, 2016

saad-ali commented May 26, 2016

k8s-github-robot commented May 26, 2016

k8s-bot commented May 26, 2016

k8s-github-robot commented May 26, 2016

Attach Detach Controller Business Logic #25457

Attach Detach Controller Business Logic #25457

Conversation

saad-ali commented May 11, 2016

saad-ali commented May 11, 2016 • edited Loading

ghost May 12, 2016 • edited by ghost Loading

Choose a reason for hiding this comment

saad-ali May 12, 2016

Choose a reason for hiding this comment

saad-ali commented May 13, 2016

ghost May 13, 2016

Choose a reason for hiding this comment

saad-ali May 13, 2016

Choose a reason for hiding this comment

thockin May 24, 2016

Choose a reason for hiding this comment

saad-ali May 24, 2016

Choose a reason for hiding this comment

saad-ali May 25, 2016

Choose a reason for hiding this comment

thockin commented May 25, 2016

saad-ali commented May 25, 2016

thockin commented May 25, 2016

saad-ali commented May 25, 2016

saad-ali commented May 26, 2016

k8s-github-robot commented May 26, 2016

k8s-bot commented May 26, 2016

k8s-github-robot commented May 26, 2016

saad-ali commented May 11, 2016 •

edited

Loading

ghost May 12, 2016 •

edited by ghost

Loading