From 4cdc2b71c3e25222c681ec1054062ae0bb75ad46 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Fri, 14 Jan 2022 07:53:59 -0500 Subject: [PATCH] Add more tests for pending expansion fix bug with switch..case statement --- pkg/controller/expand_and_recover.go | 38 +++++++++++++++-------- pkg/controller/expand_and_recover_test.go | 18 +++++++++++ 2 files changed, 43 insertions(+), 13 deletions(-) diff --git a/pkg/controller/expand_and_recover.go b/pkg/controller/expand_and_recover.go index bb5784c2f..8b151fb0b 100644 --- a/pkg/controller/expand_and_recover.go +++ b/pkg/controller/expand_and_recover.go @@ -31,7 +31,7 @@ func (ctrl *resizeController) expandAndRecover(pvc *v1.PersistentVolumeClaim, pv klog.V(4).Infof("No need to resize PV %q", pv.Name) return pvc, pv, nil, false } - resizeCalled := false + resizeNotCalled := false // if we are here that already means pvc.Spec.Size > pvc.Status.Size pvcSpecSize := pvc.Spec.Resources.Requests[v1.ResourceStorage] @@ -53,15 +53,26 @@ func (ctrl *resizeController) expandAndRecover(pvc *v1.PersistentVolumeClaim, pv if pvSize.Cmp(pvcSpecSize) < 0 { // PV is smaller than user requested size. In general some control-plane volume expansion // is necessary at this point but if we were expanding the PVC before we should let - // previous operation finished before starting expansion to new user requested size. + // previous operation finish before starting expansion to new user requested size. switch resizeStatus { - case v1.PersistentVolumeClaimControllerExpansionInProgress: - case v1.PersistentVolumeClaimNodeExpansionPending: - case v1.PersistentVolumeClaimNodeExpansionInProgress: - case v1.PersistentVolumeClaimNodeExpansionFailed: + case v1.PersistentVolumeClaimControllerExpansionInProgress, + v1.PersistentVolumeClaimNodeExpansionFailed: if allocatedSize != nil { newSize = *allocatedSize } + case v1.PersistentVolumeClaimNodeExpansionPending, + v1.PersistentVolumeClaimNodeExpansionInProgress: + if allocatedSize != nil { + newSize = *allocatedSize + } + // If PV is already greater or equal to whatever newSize is, then we should + // let previously issued node expansion finish before starting new one. + // This case is essentially an optimization on previous case, generally it is safe + // to let volume plugin receive the expansion call (expansion calls are idempotent) + // but having this check here *avoids* unnecessary RPC calls to plugin. + if pvSize.Cmp(newSize) >= 0 { + return pvc, pv, nil, resizeNotCalled + } default: newSize = pvcSpecSize } @@ -77,11 +88,11 @@ func (ctrl *resizeController) expandAndRecover(pvc *v1.PersistentVolumeClaim, pv // safe to do so. // 4. While expansion was still pending on the node, user reduced the pvc size. switch resizeStatus { - case v1.PersistentVolumeClaimNodeExpansionInProgress: - case v1.PersistentVolumeClaimNodeExpansionPending: + case v1.PersistentVolumeClaimNodeExpansionInProgress, + v1.PersistentVolumeClaimNodeExpansionPending: // we don't need to do any work. We could be here because of a spurious update event. // This is case #1 - return pvc, pv, nil, resizeCalled + return pvc, pv, nil, resizeNotCalled case v1.PersistentVolumeClaimNodeExpansionFailed: // This is case#3, we need to reset the pvc status in such a way that kubelet can safely retry volume // expansion. @@ -90,9 +101,9 @@ func (ctrl *resizeController) expandAndRecover(pvc *v1.PersistentVolumeClaim, pv } else { newSize = pvcSpecSize } - case v1.PersistentVolumeClaimControllerExpansionInProgress: - case v1.PersistentVolumeClaimControllerExpansionFailed: - case v1.PersistentVolumeClaimNoExpansionInProgress: + case v1.PersistentVolumeClaimControllerExpansionInProgress, + v1.PersistentVolumeClaimControllerExpansionFailed, + v1.PersistentVolumeClaimNoExpansionInProgress: // This is case#2 or it could also be case#4 when user manually shrunk the PVC // after expanding it. if allocatedSize != nil { @@ -112,7 +123,7 @@ func (ctrl *resizeController) expandAndRecover(pvc *v1.PersistentVolumeClaim, pv var err error pvc, err = ctrl.markControllerResizeInProgress(pvc, newSize) if err != nil { - return pvc, pv, fmt.Errorf("marking pvc %q as resizing failed: %v", util.PVCKey(pvc), err), resizeCalled + return pvc, pv, fmt.Errorf("marking pvc %q as resizing failed: %v", util.PVCKey(pvc), err), resizeNotCalled } // if pvc previously failed to expand because it can't be expanded when in-use @@ -138,6 +149,7 @@ func (ctrl *resizeController) expandAndRecover(pvc *v1.PersistentVolumeClaim, pv if err != nil { // Record an event to indicate that resize operation is failed. ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeResizeFailed, err.Error()) + return pvc, pv, err, true } klog.V(4).Infof("Update capacity of PV %q to %s succeeded", pv.Name, newSize.String()) diff --git a/pkg/controller/expand_and_recover_test.go b/pkg/controller/expand_and_recover_test.go index dc267b3b7..77b283805 100644 --- a/pkg/controller/expand_and_recover_test.go +++ b/pkg/controller/expand_and_recover_test.go @@ -75,6 +75,24 @@ func TestExpandAndRecover(t *testing.T) { expectedAllocatedSize: resource.MustParse("2G"), expectResizeCall: true, }, + { + name: "pvc.spec.size > pv.spec.size, recover_expansion=on, resize_status-node_expansion_pending", + pvc: getTestPVC("test-vol0", "10G", "1G", "3G", v1.PersistentVolumeClaimNodeExpansionPending), + pv: createPV(3, "claim01", defaultNS, "test-uid", &fsVolumeMode), + recoverFeatureGate: true, + expectedResizeStatus: v1.PersistentVolumeClaimNodeExpansionPending, + expectedAllocatedSize: resource.MustParse("3G"), + expectResizeCall: false, + }, + { + name: "pvc.spec.size > pv.spec.size, recover_expansion=on, resize_status-node_expansion_inprogress", + pvc: getTestPVC("test-vol0", "10G", "1G", "3G", v1.PersistentVolumeClaimNodeExpansionInProgress), + pv: createPV(3, "claim01", defaultNS, "test-uid", &fsVolumeMode), + recoverFeatureGate: true, + expectedResizeStatus: v1.PersistentVolumeClaimNodeExpansionInProgress, + expectedAllocatedSize: resource.MustParse("3G"), + expectResizeCall: false, + }, } for i := range tests { test := tests[i]