attachdetach controller: attach volumes immediately when Pod's PVCs are bound #66863

cofyc · 2018-08-01T12:35:14Z

What this PR does / why we need it:

Let attachdetach controller to attach volumes immediately when Pod's PVCs are bound.

Current attachdetach controller calls util.ProcessPodVolume to add pod volumes into desiredStateOfWorld on these events:

podAdd event
podUpdate event
podDelete event
periodical desiredStateOfWorldPopulator.findAndAddActivePod

But if a pod is created with PVCs not bound, no volumes will be added into desiredStateOfWorld because PVCs not bound. When pv controller binds PVCs successfully, attachdetach controller will not add pod volumes immediately because it does not watch on PVC events.

It will wait until a pod update event is triggered (normally will not happen because no new status will be reported by kubelet) or desiredStateOfWorldPopulator.findAndAddActivePod is called (maybe 0~3 minutes later, see timer configs).

In bad case, pod start time will be very long (~3 minutes + ~2 minutes (kubelet max exponential backoff)), for example: #64549 (comment).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #64549

Special notes for your reviewer:

Release note:

attachdetach controller attaches volumes immediately when Pod's PVCs are bound

gnufied · 2018-08-02T02:03:26Z

pkg/controller/volume/attachdetach/populator/desired_state_of_world_populator.go

+		// Skip unbound PVCs.
+		return
+	}
+	pods, err := dswp.podLister.List(labels.Everything())


Another way - perhaps we can solve this is, by implementing work queues in attach/detach controller. The problem here is - any random PVC update will cause iteration of all pods in the cluster. One can perhaps DOS the cluster by constantly updating the pvc?

Yes, you're right. I'll update it.

Because PVC does not contain Pod references, but we can index pods by PVC locally, then we don't need to iterate all pods every time. For example, daemon controller indexes pods based on their NodeName.

I think this will improve performance in large cluster.

cofyc · 2018-08-02T11:16:02Z

/assign @saad-ali
Ready to review, summary of changes:

Use queue to process PVCs on add/update events
Index pods by PVC key then we don't need to iterate to find pods
Add integration test for this feature

cc @jingxu97 @msau42

cofyc · 2018-08-02T14:16:52Z

/test pull-kubernetes-integration
/test pull-kubernetes-kubemark-e2e-gce-big
/test pull-kubernetes-e2e-kops-aws

redbaron · 2018-08-02T14:58:10Z

@cofyc , do you think it is cherry-pickable to 1.10, 1.11?

msau42 · 2018-08-02T15:46:05Z

@kubernetes/sig-storage-pr-reviews

gnufied

mostly looks good to me.

gnufied · 2018-08-02T17:46:20Z

pkg/controller/volume/attachdetach/attach_detach_controller.go

+		UpdateFunc: func(old, new interface{}) {
+			adc.enqueuePVC(new)
+		},
+		DeleteFunc: nil,


I dont think you need to specify DeleteFunc if you don't want to.

cofyc · 2018-08-03T02:34:13Z

@redbaron
Patch of attachdetach is easy to be cherry-picked, test part have some conflicts.
I'll try to cherry-pick it into old releases when it's accepted.

gnufied · 2018-08-03T19:36:55Z

@cofyc can you please squash your commits?

cofyc · 2018-08-04T04:12:51Z

@gnufied
Done. I split it into two parts:

attachdetach controller: 8db47db
integration test: ~~8336621~~ 7a9fde4

jingxu97 · 2018-08-06T17:17:09Z

test/integration/volume/attach_detach_test.go

+	waitForPodFuncInDSWP(t, ctrl.GetDesiredStateOfWorld(), 10*time.Second, "expected 1 pod in dsw", 1)
+	createPVForPVC(t, testClient, pvc)
+	waitForPodFuncInDSWP(t, ctrl.GetDesiredStateOfWorld(), 20*time.Second, "expected 2 pods in dsw after PVCs are bound", 2)
+	close(stopCh)


I wonder how well 10s and 20s setting will work? What is the typical delay under normal load, and heavy load?

attachdetach controller processes Pod's PVs to attach very fast, the delay should be almost same as apiserver watch event delay, because it does nothing except adding Pod's Volumes into DSWP (no api calls) on event.
In my local test, delay to attach PV is less than 100ms. The most delay is waiting PV controller to bound PVC, normally 4~5 seconds.
Perhaps we should increase timeout of waitForPodFuncInDSWP, and it will not increase execution time of test.
time.Sleep(10 * time.Second) is a hack to verify the second pod cannot be added into DSWP, because no object state or event can be checked to verify. 10 seconds should be enough here.

timeout of waitForPodFuncInDSWP is increased to 60 seconds now.

msau42

This is awesome! I just have two nits

msau42 · 2018-08-10T21:55:30Z

pkg/controller/volume/attachdetach/attach_detach_controller.go

+	}
+	pvc, err := adc.pvcLister.PersistentVolumeClaims(namespace).Get(name)
+	if apierrors.IsNotFound(err) {
+		glog.V(2).Infof("error getting pvc %q from informer: %v", key, err)


Should this really be log level 2?

Fixed, increased to log level 4.

msau42 · 2018-08-10T22:00:55Z

pkg/controller/volume/attachdetach/attach_detach_controller.go

+	if !ok {
+		return []string{}, nil
+	}
+	// We are only interested in active pods with nodeName set


Can you also add a comment that this cache is only used for attaching

saad-ali

Largely LGTM, a couple of questions.

saad-ali · 2018-08-10T23:41:04Z

pkg/controller/volume/attachdetach/attach_detach_controller.go

+func (adc *attachDetachController) enqueuePVC(obj interface{}) {
+	key, err := kcache.DeletionHandlingMetaNamespaceKeyFunc(obj)
+	if err != nil {
+		runtime.HandleError(fmt.Errorf("Couldn't get key for object %+v: %v", obj, err))


Just curious: Why runtime.HandleError? Why not just glog.Errorf?

I followed https://github.com/kubernetes/kubernetes/blob/v1.12.0-alpha.1/pkg/controller/daemon/daemon_controller.go#L316, and found sample-controller uses runtime.HandleError to log errors too. Perhaps because runtime.HandleError is dedicated to log non-user facing ignored errors and can be extended by adding error handlers.

saad-ali · 2018-08-10T23:47:10Z

pkg/controller/volume/attachdetach/attach_detach_controller.go

+		return nil
+	}
+
+	objs, err := adc.podIndexer.ByIndex(pvcKeyIndex, key)


Just to verify, if there are multiple pods using the same PVC, this will return all pods? If so make sure there is a test case for that.

Yes, test has been updated. Created more pvc notbound pods for testing in 74cfffc.

…re bound - Use queue to process PVCs on add/update events - Index pods by PVC key then we don't need to iterate to find pods

…re bound - Add integration test for this feature

cofyc · 2018-08-13T17:05:56Z

Squashed into two commits:

attachdetach controller: 5fafae1
integration test: 17ab4c3

msau42 · 2018-08-13T18:00:18Z

/test pull-kubernetes-integration

saad-ali · 2018-08-13T21:20:08Z

/lgtm

k8s-ci-robot · 2018-08-13T21:20:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cofyc, saad-ali

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:

~~pkg/controller/volume/attachdetach/OWNERS~~ [saad-ali]
~~test/OWNERS~~ [saad-ali]

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

redbaron · 2018-08-13T21:39:53Z

/test pull-kubernetes-integration

redbaron · 2018-08-14T18:02:32Z

/retest

k8s-github-robot · 2018-08-14T19:22:34Z

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

k8s-github-robot · 2018-08-14T20:46:35Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

cofyc · 2018-08-15T03:43:56Z

@redbaron

I've created cherry pick PRs for 1.9/1.10/1.11:

release-1.9: Automated cherry pick of #66863: attachdetach controller: attach volumes immediately when #67417
release-1.10: Automated cherry pick of #66863: attachdetach controller: attach volumes immediately when #67415
release-1.11: Automated cherry pick of #66863: attachdetach controller: attach volumes immediately when #67413

…upstream-release-1.11 Automatic merge from submit-queue. Automated cherry pick of #66863: attachdetach controller: attach volumes immediately when Cherry pick of #66863 on release-1.11. #66863: attachdetach controller: attach volumes immediately when

…upstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #66863: attachdetach controller: attach volumes immediately when Cherry pick of #66863 on release-1.10. #66863: attachdetach controller: attach volumes immediately when

…upstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #66863: attachdetach controller: attach volumes immediately when Cherry pick of #66863 on release-1.9. #66863: attachdetach controller: attach volumes immediately when

This PS moves to use k8s 1.10.8, which includes a couple of fixes for PVC mounts. * kubernetes/kubernetes#66863 Change-Id: Ica30950a8200f5755897b51fd2b4d24c69a10e61 Signed-off-by: Pete Birley <pete@port.direct>

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 1, 2018

k8s-ci-robot requested review from davidz627 and saad-ali August 1, 2018 12:35

cofyc changed the title ~~attachdetach controller: attach volumes immediately when Pod's PVCs are bound~~ WIP: attachdetach controller: attach volumes immediately when Pod's PVCs are bound Aug 1, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 1, 2018

cofyc force-pushed the fix64549 branch from f25d152 to 11d4224 Compare August 1, 2018 12:42

gnufied reviewed Aug 2, 2018

View reviewed changes

cofyc force-pushed the fix64549 branch from 11d4224 to e144382 Compare August 2, 2018 10:39

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 2, 2018

cofyc changed the title ~~WIP: attachdetach controller: attach volumes immediately when Pod's PVCs are bound~~ attachdetach controller: attach volumes immediately when Pod's PVCs are bound Aug 2, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2018

cofyc force-pushed the fix64549 branch from 65493f0 to 6cf9f41 Compare August 2, 2018 11:14

k8s-ci-robot assigned saad-ali Aug 2, 2018

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Aug 2, 2018

gnufied reviewed Aug 2, 2018

View reviewed changes

cofyc force-pushed the fix64549 branch from 42abce4 to 8336621 Compare August 4, 2018 04:07

jingxu97 reviewed Aug 6, 2018

View reviewed changes

cofyc force-pushed the fix64549 branch from 8336621 to 7a9fde4 Compare August 7, 2018 02:32

msau42 reviewed Aug 10, 2018

View reviewed changes

saad-ali reviewed Aug 10, 2018

View reviewed changes

cofyc added 2 commits August 14, 2018 01:03

attachdetach controller: attach volumes immediately when Pod's PVCs a…

5fafae1

…re bound - Use queue to process PVCs on add/update events - Index pods by PVC key then we don't need to iterate to find pods

attachdetach controller: attach volumes immediately when Pod's PVCs a…

17ab4c3

…re bound - Add integration test for this feature

cofyc force-pushed the fix64549 branch from 74cfffc to 17ab4c3 Compare August 13, 2018 17:04

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 13, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 13, 2018

k8s-github-robot merged commit cd786bd into kubernetes:master Aug 14, 2018

afrittoli mentioned this pull request Jan 19, 2019

Step using Kaniko build does not wait for PVC to be bound tektoncd/pipeline#403

Closed

cofyc deleted the fix64549 branch May 4, 2019 07:18

attachdetach controller: attach volumes immediately when Pod's PVCs are bound #66863

attachdetach controller: attach volumes immediately when Pod's PVCs are bound #66863

Conversation

cofyc commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

cofyc commented Aug 2, 2018

cofyc commented Aug 2, 2018

redbaron commented Aug 2, 2018

msau42 commented Aug 2, 2018

gnufied left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc commented Aug 3, 2018

gnufied commented Aug 3, 2018

cofyc commented Aug 4, 2018 • edited Loading

Choose a reason for hiding this comment

cofyc Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 left a 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

saad-ali left a 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

cofyc Aug 11, 2018 • edited Loading

Choose a reason for hiding this comment

cofyc commented Aug 13, 2018

msau42 commented Aug 13, 2018

saad-ali commented Aug 13, 2018

k8s-ci-robot commented Aug 13, 2018

redbaron commented Aug 13, 2018

redbaron commented Aug 14, 2018

k8s-github-robot commented Aug 14, 2018

k8s-github-robot commented Aug 14, 2018

cofyc commented Aug 15, 2018

cofyc Aug 2, 2018 •

edited

Loading

cofyc commented Aug 4, 2018 •

edited

Loading

cofyc Aug 7, 2018 •

edited

Loading

cofyc Aug 11, 2018 •

edited

Loading