Double check PVC if not found in syncVolume #67062

cofyc · 2018-08-07T07:07:42Z

What this PR does / why we need it:

Double check PVC if not found in syncVolume.

If PV is bound by external PV binder (e.g. kube-scheduler), it's possible on heavy load that corresponding PVC is not synced to controller local cache yet.

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 #66287

Special notes for your reviewer:

Release note:

PVC may not be synced to controller local cache in time if PV is bound by external PV binder (e.g. kube-scheduler), double check if PVC is not found to prevent reclaiming PV wrongly.

wenlxie · 2018-08-07T07:13:27Z

pkg/controller/volume/persistentvolume/pv_controller.go

+			// that corresponding PVC is not synced in controller yet. So we
+			// double-check PVC in apiserver to make sure we will not reclaim a
+			// PV due to API delay.
+			obj, err = ctrl.kubeClient.CoreV1().PersistentVolumeClaims(volume.Spec.ClaimRef.Namespace).Get(volume.Spec.ClaimRef.Name, metav1.GetOptions{})


Is it better to get the storage-class and then check whether volumeBindingMode=WaitForFirstConsumer before get the pvc from api-server?

It should be ok to check PVC in apiserver only if storage class volumeBindingMode is WaitForFirstConsumer normally, but the cause of problem is cache out of sync, checking class (in cache) here add some complexity.
@msau42 @jsafrane
What do you think?

One scenario need to concede is if there are lots of PVCs are deleted, then there will be many requests send to api server here.

cofyc · 2018-08-07T07:49:45Z

/assign @msau42
@kubernetes/sig-storage-pr-reviews

cofyc · 2018-08-08T02:12:27Z

/test pull-kubernetes-e2e-gce-100-performance

jsafrane · 2018-08-10T14:27:25Z

pkg/controller/volume/persistentvolume/pv_controller.go

+				// that corresponding PVC is not synced in controller yet. So
+				// we double-check PVC in apiserver to make sure we will not
+				// reclaim a PV due to API delay.
+				obj, err = ctrl.kubeClient.CoreV1().PersistentVolumeClaims(volume.Spec.ClaimRef.Namespace).Get(volume.Spec.ClaimRef.Name, metav1.GetOptions{})


In what order does scheduler write bound PV and PVC? If it writes PV first, this code might be executed before the scheduler even writes the PVC.

Reading #66287, it probably does not matter. Still I'd like to understand the issue before approving this PR.

The scheduler only writes PV.ClaimRef. It doesn't not modify PVC (in the preprovisioned case)

I like the idea of only checking this of some special scheduler annotation is set. Otherwise, this will trigger an extra API call for regular volumes too.

Although eventually when almost everything moves to late binding, then it makes no difference at that point.

I am not sure about annBoundByScheduler - if I have an external PV binder, then I still experience the issue. IMO, it should just check annBoundByController (the controller has seen the PVC so it must be in its cache).

Right now the scheduler is already setting annBoundByController. I need to think about if it is ok to remove that annotation from scheduler.

Regardless, I'm not sure how useful it is to add a scheduler annotation because eventually almost everything should be going through the scheduler, and like @jsafrane pointed out, any other external component that is binding PVs could hit this too.

I think we cannot remove annBoundByController from the scheduler. The annotation determines whether or not we automatically try to rollback the binding in case of some error.

I see. Controller in annBoundByController means all possible PV binders, not only PV controller itself. IIUC, if we want to optimize for external PV binders, we need to add a additional annotation for all external PV binders, but that requires upgrading external PV binders.

msau42 · 2018-08-11T01:25:30Z

One thing that bothered me: why is this different than the case where a user manually creates a PV with ClaimRef set to a non-existent PVC. The answer is that when the user sets ClaimRef, pvc.UID is not set. But in the case of the scheduler prebinding, pvc.UID is set. So that's why PV controller thinks that PVC previously existed and got deleted.

So another possible solution may be to not set pvc.UID in the case of scheduler prebinding. However, then we run into the issue where the user might actually delete the PVC and then this half bound PV is stuck forever and unavailable to other PVCs.

cofyc · 2018-08-11T06:47:52Z

/test pull-kubernetes-e2e-gce

msau42 · 2018-08-13T16:15:20Z

pkg/controller/volume/persistentvolume/pv_controller.go

+			// Note that only non-released and non-failed volumes will be
+			// updated to Released state when PVC does not eixst.
+			if volume.Status.Phase != v1.VolumeReleased && volume.Status.Phase != v1.VolumeFailed {
+				obj, err = ctrl.kubeClient.CoreV1().PersistentVolumeClaims(volume.Spec.ClaimRef.Namespace).Get(volume.Spec.ClaimRef.Name, metav1.GetOptions{})


So another theory as to what could be happening here. PV controller's pvc cache is different from the informer cache. Whenever a PVC informer event comes in, the pvc gets added to a queue, and the queue is processed by a single claimWorker, which will then add it to the PVC cache. So if there are many PVC events all at once, then it's possible that the "create pvc event" is stuck in the queue, even though the informer actually saw the update.

So one more thing we could add here is to check the informer cache directly before checking the API server. That could avoid the extra API call in this scenario, although will not avoid the api call on a normal PVC delete.

Thanks! I hadn't thought about it, not familiar with PV controller.

cofyc · 2018-08-15T07:27:21Z

@msau42 @jsafrane
PR has been updated (and squashed too), see https://github.com/kubernetes/kubernetes/pull/67062/files.

msau42

lgtm

msau42 · 2018-08-15T16:39:03Z

pkg/controller/volume/persistentvolume/pv_controller.go

@@ -123,6 +123,8 @@ const annBindCompleted = "pv.kubernetes.io/bind-completed"
 // the binding (PV->PVC or PVC->PV) was installed by the controller.  The
 // absence of this annotation means the binding was done by the user (i.e.
 // pre-bound). Value of this annotation does not matter.
+// Exteranl PV binders must bind PV the same way as PV controller, otherwise PV


nit: External

If PV is bound by external PV binder (e.g. kube-scheduler), it's possible on heavy load that corresponding PVC is not synced to controller local cache yet.

cofyc · 2018-08-16T03:02:24Z

/test pull-kubernetes-integration

jsafrane · 2018-08-17T10:38:34Z

/lgtm

k8s-ci-robot · 2018-08-17T10:38:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cofyc, jsafrane

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/persistentvolume/OWNERS~~ [jsafrane]

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

k8s-github-robot · 2018-08-17T18:58:05Z

Automatic merge from submit-queue (batch tested with PRs 67062, 67169, 67539, 67504, 66876). If you want to cherry-pick this change to another branch, please follow the instructions here.

cofyc · 2018-08-18T03:50:06Z

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

release-1.11: #67557
release-1.10: #67558
release-1.9: #67559

…upstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #67062: Double check PVC if not found in syncVolume. Cherry pick of #67062 on release-1.10. #67062: Double check PVC if not found in syncVolume.

…upstream-release-1.11 Automatic merge from submit-queue. Automated cherry pick of #67062: Double check PVC if not found in syncVolume. Cherry pick of #67062 on release-1.11. #67062: Double check PVC if not found in syncVolume.

…upstream-release-1.9 Automated cherry pick of #67062: Double check PVC if not found in syncVolume.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 7, 2018

k8s-ci-robot requested review from jsafrane and saad-ali August 7, 2018 07:07

cofyc mentioned this pull request Aug 7, 2018

Scheduler and pv_controller info not sync caused volume reclaimed #66287

Closed

wenlxie reviewed Aug 7, 2018

View reviewed changes

cofyc force-pushed the fix66287 branch from 6b365a9 to 76a6cb3 Compare August 7, 2018 07:29

k8s-ci-robot assigned msau42 Aug 7, 2018

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

cofyc force-pushed the fix66287 branch from 76a6cb3 to e1bcbd0 Compare August 7, 2018 07:52

jsafrane reviewed Aug 10, 2018

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 11, 2018

msau42 reviewed Aug 13, 2018

View reviewed changes

cofyc force-pushed the fix66287 branch from 3508f1e to c1a0154 Compare August 15, 2018 07:21

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 15, 2018

cofyc force-pushed the fix66287 branch from c1a0154 to 3b6b91e Compare August 15, 2018 07:31

cofyc changed the title ~~Double check PVC in apiserver to make sure we will not reclaim a PV due to API delay.~~ Double check PVC if not found in syncVolume Aug 15, 2018

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 15, 2018

msau42 reviewed Aug 15, 2018

View reviewed changes

Double check PVC if not found in syncVolume.

493502d

If PV is bound by external PV binder (e.g. kube-scheduler), it's possible on heavy load that corresponding PVC is not synced to controller local cache yet.

cofyc force-pushed the fix66287 branch from 3b6b91e to 493502d Compare August 16, 2018 02:33

k8s-ci-robot assigned jsafrane Aug 17, 2018

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

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

k8s-github-robot merged commit 027d0fc into kubernetes:master Aug 17, 2018

k8s-ci-robot added a commit that referenced this pull request Sep 20, 2018

Merge pull request #67559 from cofyc/automated-cherry-pick-of-#67062-…

06fe9b3

…upstream-release-1.9 Automated cherry pick of #67062: Double check PVC if not found in syncVolume.

wenlxie mentioned this pull request Nov 21, 2018

fix delaybinding claim should provision issue kubernetes-sigs/sig-storage-lib-external-provisioner#4

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Double check PVC if not found in syncVolume #67062

Double check PVC if not found in syncVolume #67062

cofyc commented Aug 7, 2018 •

edited

Loading

wenlxie Aug 7, 2018 •

edited

Loading

cofyc Aug 7, 2018 •

edited

Loading

wenlxie Aug 7, 2018 •

edited

Loading

cofyc commented Aug 7, 2018

cofyc commented Aug 8, 2018

jsafrane Aug 10, 2018

jsafrane Aug 10, 2018

msau42 Aug 11, 2018

msau42 Aug 11, 2018

msau42 Aug 11, 2018

jsafrane Aug 13, 2018

msau42 Aug 13, 2018

msau42 Aug 13, 2018

msau42 Aug 13, 2018

cofyc Aug 15, 2018

msau42 commented Aug 11, 2018

cofyc commented Aug 11, 2018

msau42 Aug 13, 2018

cofyc Aug 15, 2018

cofyc commented Aug 15, 2018 •

edited

Loading

msau42 left a comment

msau42 Aug 15, 2018

cofyc Aug 16, 2018

cofyc commented Aug 16, 2018

jsafrane commented Aug 17, 2018

k8s-ci-robot commented Aug 17, 2018

k8s-github-robot commented Aug 17, 2018

cofyc commented Aug 18, 2018 •

edited

Loading

Double check PVC if not found in syncVolume #67062

Double check PVC if not found in syncVolume #67062

Conversation

cofyc commented Aug 7, 2018 • edited Loading

wenlxie Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

cofyc Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

wenlxie Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

cofyc commented Aug 7, 2018

cofyc commented Aug 8, 2018

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

msau42 commented Aug 11, 2018

cofyc commented Aug 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc commented Aug 15, 2018 • edited Loading

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

cofyc commented Aug 16, 2018

jsafrane commented Aug 17, 2018

k8s-ci-robot commented Aug 17, 2018

k8s-github-robot commented Aug 17, 2018

cofyc commented Aug 18, 2018 • edited Loading

cofyc commented Aug 7, 2018 •

edited

Loading

wenlxie Aug 7, 2018 •

edited

Loading

cofyc Aug 7, 2018 •

edited

Loading

wenlxie Aug 7, 2018 •

edited

Loading

cofyc commented Aug 15, 2018 •

edited

Loading

cofyc commented Aug 18, 2018 •

edited

Loading