We need to ensure that we are not reading stale PV objects #96

gnufied · 2020-08-10T20:30:46Z

So as we don't end up resizing same object more than once

Fixes flakes associated with resizing.

/kind bug

Ensure that we do not resize recently resized volumes

gnufied · 2020-08-10T20:31:25Z

This fixes flake coming from - https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-resizer/82/pull-kubernetes-csi-external-resizer-1-16-on-kubernetes-1-16/1292898043053477890

gnufied · 2020-08-10T20:31:32Z

/assign @msau42

msau42 · 2020-08-10T20:36:00Z

pkg/controller/controller.go

@@ -341,6 +341,15 @@ func (ctrl *resizeController) pvNeedResize(pvc *v1.PersistentVolumeClaim, pv *v1
 		return false
 	}

+	// lets make sure that we are reading latest PV object so as any PV updates that might have happened


What happens if you call resize twice on the same PV?

in general once kubelet finishes resizing then all resizing related conditions are removed from PVC. But what is happening in e2e is - we take down the pod and then we expand the PVC and we wait for "FileSystemResizePending" condition on PVC, but if resize is called again in resize controller then that condition might get replaced by "Resizing" condition and hence e2e may flake.

Does that mean we should keep an internal cache and update it after we make an API update, similar to what we do in pv controller? That way, we don't need to do a GET request on every loop, which will limit our scalibility.

Also the reason I think this will eliminate this kind of flakes is because - the previous operation on PVC can't finish without writing the object to etcd. The only reason we would return true from here is if informers haven't been updated yet, but if we did read from apiserver directly, this problem should go away.

Alternatively we will have to maintain local cache like provisioner/snapshotter etc.

I was thinking about local caching but I am not sure if it is worth doing in this case. The PV size comparison is always done after PVC size comparsion, so in this case PVC does have a volume expansion pending (like we know pvc.Status.Capacity < pvc.Spec.Capacity). So this API call should only be made twice per PVC size change.

Spawning 10 threads to resize PVCs in a loop for 120s. Each thread moves onto the next resize operation once the PVC enters FileSystemResizePending condition. Dummy vSphere CSI driver that returns success for ControllerExpandVolume calls.

Without this change, using image quay.io/k8scsi/csi-resizer:v1.0.0-rc2:
Ran 134 operations with throughput of 1.127 ops/sec

With this change:
Ran 88 operations with a throughput of 0.79 ops/sec

So if I understand correctly, this PR overall reduced number of API requests from external-resizer?

Quite the opposite unfortunately :) With the rc image, i can get 134 calls to ControllerExpandVolume but that drops to 88 with this PR, the reason being that each ControllerExpandVolume request takes longer to process.

@jsafrane pointed out that we could do same thing @RaunakShah did in provisioner - kubernetes-sigs/sig-storage-lib-external-provisioner#83 . lets see if this helps.

gnufied · 2020-08-17T23:58:11Z

/retest

gnufied · 2020-08-18T02:21:45Z

/retest

gnufied · 2020-08-18T02:34:20Z

@msau42 can you PTAL. I have moved the PR to use cache.Store object and hence should not incur API cost.

pkg/controller/controller.go

msau42 · 2020-08-19T00:42:33Z

pkg/controller/controller.go

+
+	pvc, ok := pvcObject.(*v1.PersistentVolumeClaim)
+	if !ok {
+		klog.Errorf("expected PVC got: %v", pvcObject)


Can we let the caller log the error? Ditto throughout

msau42 · 2020-08-19T00:44:55Z

pkg/controller/controller.go

@@ -407,7 +434,7 @@ func (ctrl *resizeController) resizePVC(pvc *v1.PersistentVolumeClaim, pv *v1.Pe
 		ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeResizeFailed, err.Error())
 	}

-	return err
+	return ctrl.claims.Update(updatedPVC)


We already update it in resizeVolume. Do we need to update it here too? Would it be simpler if we just always update it after immediately doing a successful api update?

We did not update PVC in resizeVolume function. We updated PV in there.

Same comment applies. Can we update the cache immediately after the update/patch operation, instead of the callers needing to realize that the function updated the api?

I have moved code that updates the cache to be in same place where PV or PVC object is updated.

thanks! I think this looks cleaner

gnufied · 2020-08-19T16:46:28Z

/retest

msau42 · 2020-08-19T18:56:56Z

pkg/controller/controller.go

@@ -407,7 +434,7 @@ func (ctrl *resizeController) resizePVC(pvc *v1.PersistentVolumeClaim, pv *v1.Pe
 		ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeResizeFailed, err.Error())
 	}

-	return err
+	return ctrl.claims.Update(updatedPVC)


Same comment applies. Can we update the cache immediately after the update/patch operation, instead of the callers needing to realize that the function updated the api?

msau42 · 2020-08-19T18:57:27Z

pkg/controller/controller.go

@@ -375,6 +387,12 @@ func (ctrl *resizeController) resizePVC(pvc *v1.PersistentVolumeClaim, pv *v1.Pe
 		pvc = updatedPVC
 	}

+	err := ctrl.claims.Update(pvc)
+	if err != nil {
+		klog.Errorf("error updating PVC %s in cache: %v", util.PVCKey(pvc), err)


Can we return this error and let the caller log?

msau42 · 2020-08-19T18:57:49Z

pkg/controller/controller.go

 		klog.Errorf("Update capacity of PV %q to %s failed: %v", pv.Name, newSize.String(), err)
 		return newSize, fsResizeRequired, err
 	}
+	err = ctrl.volumes.Update(updatedPV)
+	if err != nil {
+		klog.Errorf("error updating PV %s: %v", updatedPV.Name, err)


Can we return the error and let the caller log?

msau42 · 2020-08-19T18:58:02Z

pkg/controller/controller.go

 	newPVC := pvc.DeepCopy()
 	newPVC.Status.Capacity[v1.ResourceStorage] = newSize
 	newPVC.Status.Conditions = util.MergeResizeConditionsOfPVC(pvc.Status.Conditions, []v1.PersistentVolumeClaimCondition{})
-	if _, err := util.PatchPVCStatus(pvc, newPVC, ctrl.kubeClient); err != nil {
+	updatedPVC, err := util.PatchPVCStatus(pvc, newPVC, ctrl.kubeClient)
+	if err != nil {
 		klog.Errorf("Mark PVC %q as resize finished failed: %v", util.PVCKey(pvc), err)


return error

pkg/controller/controller.go

msau42 · 2020-08-19T21:44:38Z

pkg/controller/controller.go

 		// if this error was a in-use error then it must be tracked so as we don't retry without
 		// first verifying if volume is in-use
 		if inUseError(err) {
 			ctrl.usedPVCs.addPVCWithInUseError(pvc)
 		}
-		return newSize, fsResizeRequired, fmt.Errorf("resize volume %s failed: %v", pv.Name, err)
+		return newSize, fsResizeRequired, fmt.Errorf("Resize volume %q by resizer %q failed: %v", pv.Name, ctrl.name, err)


I think the convention is for error messages to be lower case

msau42 · 2020-08-20T00:13:57Z

pkg/controller/controller.go

@@ -406,8 +417,7 @@ func (ctrl *resizeController) resizePVC(pvc *v1.PersistentVolumeClaim, pv *v1.Pe
 		// Record an event to indicate that resize operation is failed.
 		ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeResizeFailed, err.Error())
 	}
-
-	return err
+	return nil


Why is this changed to nil?

not sure how this happeend. Added a unit test to ensure this does not happen. good catch.

msau42 · 2020-08-20T00:16:28Z

pkg/controller/controller.go

+	if err != nil {
+		return newSize, fsResizeRequired, fmt.Errorf("Update capacity of PV %q to %s failed: %v", pv.Name, newSize.String(), err)
+	}
+	err = ctrl.volumes.Update(updatedPV)


I was thinking to do the cache update in the util method itself, but I guess we don't want to pass ctrl in. Can we create another helper function here?

made a new helper function that updates the PV with api-server and updates local cache.

Sorry to be picky, can we also do the same for the other patch + updates? The main goal I'm trying to achieve is that developers can use the util methods to make api updates and don't have to remember to also update the cache afterwards.

We do have now helpers functions on controller type which do the same. They patch the object and then they update the cache. Functions - updatePVCapacity, markPVCAsFSResizeRequired and markPVCResizeFinished both update the object with apiserver and then update the object in the cache.

Did you wanted both patching and updating in cache to be done within util functions which aren't part of controller type? In that case - we will have to pass controller instance to those functions and in which case these functions may as well be on controller type (which is what I have done).

I was thinking even one more level down, like patchStatus so that we only do the patch + update in one place.

I was thinking about doing that but that means PatchPVCStatus function can't really belong to util package because it has to also update the controller's cache. I hear what you are saying but I am not sure if it is worth doing given the size of resize controller which has pretty small surface area of PV and PVC updates.

Okay I moved patchClaim and patchPersistentVolume functions from util into controller, so as it can patch the object and update the cache. PTAL

So as we don't end up resizing same object more than once

RaunakShah · 2020-08-20T16:51:57Z

pkg/controller/controller.go

+	}
+	err = ctrl.claims.Update(updatedPVC)
+	if err != nil {
+		return nil, fmt.Errorf("error updating PVC %s in local cache: %v", util.PVCKey(newPVC), err)


Do we want to fail the entire operation if the local cache fails to update?

In the worst case when updating local cache failed and the informer cache did not update itself (from PVC events) while operation was retried - the test will flake.

So, it should be fine I think.

msau42 · 2020-08-21T19:19:40Z

/lgtm
/approve

k8s-ci-robot · 2020-08-21T19:19:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, msau42

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:

~~OWNERS~~ [gnufied,msau42]

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

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 10, 2020

k8s-ci-robot requested review from mlmhl and saad-ali August 10, 2020 20:30

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 10, 2020

k8s-ci-robot assigned msau42 Aug 10, 2020

msau42 reviewed Aug 10, 2020

View reviewed changes

gnufied force-pushed the read-pv-from-apiserver branch from ccf4ef5 to 97eda0d Compare August 17, 2020 21:18

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 17, 2020

gnufied force-pushed the read-pv-from-apiserver branch from 97eda0d to a5c04c5 Compare August 17, 2020 21:41

msau42 reviewed Aug 19, 2020

View reviewed changes

gnufied force-pushed the read-pv-from-apiserver branch from a5c04c5 to ce0524f Compare August 19, 2020 15:26

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 19, 2020

msau42 reviewed Aug 19, 2020

View reviewed changes

gnufied force-pushed the read-pv-from-apiserver branch from ce0524f to 0325630 Compare August 19, 2020 20:35

msau42 reviewed Aug 20, 2020

View reviewed changes

We need to ensure that we are not reading stale PV objects

e1ec0ac

So as we don't end up resizing same object more than once

gnufied force-pushed the read-pv-from-apiserver branch from 0325630 to e1ec0ac Compare August 20, 2020 16:06

RaunakShah reviewed Aug 20, 2020

View reviewed changes

Move functions for updating PV and PVC into controller

a1746e8

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

k8s-ci-robot merged commit dddcb17 into kubernetes-csi:master Aug 21, 2020

We need to ensure that we are not reading stale PV objects #96

We need to ensure that we are not reading stale PV objects #96

Conversation

gnufied commented Aug 10, 2020

gnufied commented Aug 10, 2020

gnufied commented Aug 10, 2020

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

gnufied commented Aug 17, 2020

gnufied commented Aug 18, 2020

gnufied commented Aug 18, 2020

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

gnufied commented Aug 19, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Aug 21, 2020

k8s-ci-robot commented Aug 21, 2020