diff --git a/pkg/controller/expand_and_recover.go b/pkg/controller/expand_and_recover.go index 8fcb585c9..8890dcb3b 100644 --- a/pkg/controller/expand_and_recover.go +++ b/pkg/controller/expand_and_recover.go @@ -222,7 +222,7 @@ func (ctrl *resizeController) callResizeOnPlugin( if util.IsFinalError(err) { var markExpansionFailedError error ctrl.finalErrorPVCs.Insert(pvcKey) - if util.IsInfeasibleError(err) { + if util.IsResizeInfeasibleError(err) { pvc, markExpansionFailedError = ctrl.markControllerExpansionInfeasible(pvc, err) if markExpansionFailedError != nil { return pvc, pv, fmt.Errorf("resizing failed in controller with %v but failed to update PVC %s with: %v", err, klog.KObj(pvc), markExpansionFailedError) diff --git a/pkg/modifycontroller/controller_test.go b/pkg/modifycontroller/controller_test.go index 9ed3e077c..ac2b90219 100644 --- a/pkg/modifycontroller/controller_test.go +++ b/pkg/modifycontroller/controller_test.go @@ -4,13 +4,14 @@ import ( "context" "errors" "fmt" + "testing" + "time" + "github.com/kubernetes-csi/external-resizer/pkg/util" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/cache" - "testing" - "time" "github.com/kubernetes-csi/external-resizer/pkg/features" @@ -27,7 +28,7 @@ import ( ) func TestController(t *testing.T) { - basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/) + basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/) basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac) firstTimePV := basePV.DeepCopy() firstTimePV.Spec.VolumeAttributesClassName = nil @@ -44,7 +45,7 @@ func TestController(t *testing.T) { }{ { name: "Modify called", - pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/), + pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/), pv: basePV, vacExists: true, callCSIModify: true, @@ -104,14 +105,14 @@ func TestModifyPVC(t *testing.T) { }{ { name: "Modify succeeded", - pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/), + pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/), pv: basePV, modifyFailure: false, expectFailure: false, }, { name: "Modify failed", - pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/), + pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/), pv: basePV, modifyFailure: true, expectFailure: true, @@ -145,16 +146,16 @@ func TestModifyPVC(t *testing.T) { } func TestSyncPVC(t *testing.T) { - basePVC := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/) + basePVC := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/) basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac) otherDriverPV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac) otherDriverPV.Spec.PersistentVolumeSource.CSI.Driver = "some-other-driver" - unboundPVC := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/) + unboundPVC := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/) unboundPVC.Status.Phase = v1.ClaimPending - pvcWithUncreatedPV := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/) + pvcWithUncreatedPV := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/) pvcWithUncreatedPV.Spec.VolumeName = "" nonCSIPVC := &v1.PersistentVolumeClaim{ @@ -196,7 +197,7 @@ func TestSyncPVC(t *testing.T) { }, { name: "Should NOT modify if PVC has empty Spec.VACName", - pvc: createTestPVC(pvcName, "" /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/), + pvc: createTestPVC(pvcName, "" /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/), pv: basePV, callCSIModify: false, }, @@ -247,7 +248,7 @@ func TestSyncPVC(t *testing.T) { // TestInfeasibleRetry tests that sidecar doesn't spam plugin upon infeasible error code (e.g. invalid VAC parameter) func TestInfeasibleRetry(t *testing.T) { - basePVC := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/) + basePVC := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/) basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac) tests := []struct { diff --git a/pkg/modifycontroller/modify_status.go b/pkg/modifycontroller/modify_status.go index 4a12d034a..40aa06b7e 100644 --- a/pkg/modifycontroller/modify_status.go +++ b/pkg/modifycontroller/modify_status.go @@ -18,13 +18,106 @@ package modifycontroller import ( "fmt" + "k8s.io/client-go/tools/cache" + "github.com/kubernetes-csi/external-resizer/pkg/util" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/tools/cache" ) -// markControllerModifyVolumeStatus will mark ModifyVolumeStatus other than completed in the PVC +// TODO godoc comments +// markControllerModifyVolumePending will mark PVC.Status.ModifyVolumeStatus Pending and clear ModifyVolume conditions +func (ctrl *modifyController) markControllerModifyVolumePending(pvc *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaim, error) { + newPVC := pvc.DeepCopy() + if newPVC.Status.ModifyVolumeStatus == nil { + newPVC.Status.ModifyVolumeStatus = &v1.ModifyVolumeStatus{} + } + newPVC.Status.ModifyVolumeStatus.Status = v1.PersistentVolumeClaimModifyVolumePending + + // There should not be ModifyVolume related conditions when ModifyVolume is pending + newPVC.Status.Conditions = util.MergeModifyVolumeConditionsOfPVC(newPVC.Status.Conditions, + nil) + + updatedPVC, err := util.PatchClaim(ctrl.kubeClient, pvc, newPVC, true /* addResourceVersionCheck */) + if err != nil { + return pvc, fmt.Errorf("failed to mark PVC %q as ModifyVolumePending, errored with: %w", pvc.Name, err) + } + return updatedPVC, nil +} + +// markControllerModifyVolumeModifying marks. Ensure PVC has ModifyingVolume AND ModifyVolumeErr conditions. +// if pluginErr isn't nil, ensure +func (ctrl *modifyController) markControllerModifyVolumeFailed(pvc *v1.PersistentVolumeClaim, + pluginErr error) (*v1.PersistentVolumeClaim, error) { + pvcKey, keyErr := cache.MetaNamespaceKeyFunc(pvc) + if keyErr != nil { + return pvc, keyErr + } + + newPVC := pvc.DeepCopy() + if newPVC.Status.ModifyVolumeStatus == nil { + newPVC.Status.ModifyVolumeStatus = &v1.ModifyVolumeStatus{} + } + + switch { + // Failure case 1: Infeasible CSI RPC Error + // Plugin will never be able to modify volume. We will: + // - Set ModifyVolumeStatus to Infeasible + // - Mark PVC to be retried at slower interval + // - Ensure PVC is NOT marked as uncertain (because we're certain plugin didn't modify volume) + case util.IsModifyInfeasibleError(pluginErr): + newPVC.Status.ModifyVolumeStatus.Status = v1.PersistentVolumeClaimModifyVolumeInfeasible + ctrl.markForSlowRetry(pvc, pvcKey) + ctrl.removePVCFromModifyVolumeUncertainCache(pvcKey) + // Failure case 2: Non-Infeasible, Final CSI RPC Error + // Plugin did NOT modify volume YET. We will: + // - Ensure ModifyVolumeStatus is InProgress + // - Ensure PVC is NOT marked as uncertain (because we're certain plugin didn't modify volume) + case util.IsFinalError(pluginErr) && !util.IsModifyInfeasibleError(pluginErr): + newPVC.Status.ModifyVolumeStatus.Status = v1.PersistentVolumeClaimModifyVolumeInProgress + ctrl.removePVCFromModifyVolumeUncertainCache(pvcKey) + // Failure case 3: Non-Final CSI RPC Error + // Plugin MAY have modified volume. We will: + // - Ensure ModifyVolumeStatus is InProgress + // - Ensure PVC is marked as uncertain + case !util.IsFinalError(pluginErr): + newPVC.Status.ModifyVolumeStatus.Status = v1.PersistentVolumeClaimModifyVolumeInProgress + ctrl.uncertainPVCs[pvcKey] = *pvc + } + + // Update PVC's Condition to indicate modify volume error AND modifying volume + // We keep any non ModifyVolume conditions on PVC + newPVC.Status.Conditions = util.MergeModifyVolumeConditionsOfPVC(newPVC.Status.Conditions, + []v1.PersistentVolumeClaimCondition{pvcModifyingVolumeCondition(), pvcModifyVolumeErrorCondition(pluginErr)}) + + updatedPVC, err := util.PatchClaim(ctrl.kubeClient, pvc, newPVC, true /* addResourceVersionCheck */) + if err != nil { + return pvc, fmt.Errorf("failed to patch PVC %q after modification failure, errored with: %v", pvc.Name, err) + } + return updatedPVC, nil +} + +// markControllerModifyVolumeFirstAttempt +func (ctrl *modifyController) markControllerModifyVolumeFirstAttempt(pvc *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaim, error) { + newPVC := pvc.DeepCopy() + if newPVC.Status.ModifyVolumeStatus == nil { + newPVC.Status.ModifyVolumeStatus = &v1.ModifyVolumeStatus{} + } + newPVC.Status.ModifyVolumeStatus.Status = v1.PersistentVolumeClaimModifyVolumeInProgress + newPVC.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName = *pvc.Spec.VolumeAttributesClassName + + // Update PVC's Condition to indicate modification; Keep any non ModifyVolume conditions on PVC + newPVC.Status.Conditions = util.MergeModifyVolumeConditionsOfPVC(newPVC.Status.Conditions, + []v1.PersistentVolumeClaimCondition{pvcModifyingVolumeCondition()}) + + updatedPVC, err := util.PatchClaim(ctrl.kubeClient, pvc, newPVC, true /* addResourceVersionCheck */) + if err != nil { + return pvc, fmt.Errorf("failed to patch PVC %q as modifying, errored with: %v", pvc.Name, err) + } + return updatedPVC, nil +} + +// markControllerModifyVolumeStatus will mark in the PVC and update conditions func (ctrl *modifyController) markControllerModifyVolumeStatus( pvc *v1.PersistentVolumeClaim, modifyVolumeStatus v1.PersistentVolumeClaimModifyVolumeStatus, @@ -60,43 +153,43 @@ func (ctrl *modifyController) markControllerModifyVolumeStatus( if err != nil { return pvc, fmt.Errorf("mark PVC %q as modify volume failed, errored with: %v", pvc.Name, err) } - // Remove this PVC from the uncertain cache since the status is known now - if modifyVolumeStatus == v1.PersistentVolumeClaimModifyVolumeInfeasible { - pvcKey, err := cache.MetaNamespaceKeyFunc(pvc) - if err != nil { - return pvc, err - } - - ctrl.removePVCFromModifyVolumeUncertainCache(pvcKey) - ctrl.markForSlowRetry(pvc, pvcKey) - } return updatedPVC, nil } -func (ctrl *modifyController) updateConditionBasedOnError(pvc *v1.PersistentVolumeClaim, err error) (*v1.PersistentVolumeClaim, error) { +func (ctrl *modifyController) markControllerModifyVolumeRollbackCompleted( + pvc *v1.PersistentVolumeClaim, + pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error) { + // Update PVC newPVC := pvc.DeepCopy() - pvcCondition := v1.PersistentVolumeClaimCondition{ - Type: v1.PersistentVolumeClaimVolumeModifyVolumeError, - Status: v1.ConditionTrue, - LastTransitionTime: metav1.Now(), - Message: "ModifyVolume failed with error. Waiting for retry.", - } + // Update ModifyVolumeStatus to completed + newPVC.Status.ModifyVolumeStatus = nil + + // Rollback CurrentVolumeAttributesClassName + newPVC.Status.CurrentVolumeAttributesClassName = pvc.Spec.VolumeAttributesClassName + + // Clear all the conditions related to modify volume + newPVC.Status.Conditions = clearModifyVolumeConditions(newPVC.Status.Conditions) + + // Update PV + newPV := pv.DeepCopy() + newPV.Spec.VolumeAttributesClassName = pvc.Spec.VolumeAttributesClassName + + // Update PV before PVC to avoid PV not getting updated but PVC did + updatedPV, err := util.PatchPersistentVolume(ctrl.kubeClient, pv, newPV) if err != nil { - pvcCondition.Message = "ModifyVolume failed with error: " + err.Error() + ". Waiting for retry." + return pvc, pv, fmt.Errorf("update pv.Spec.VolumeAttributesClassName for PVC %q failed, errored with: %v", pvc.Name, err) } - newPVC.Status.Conditions = util.MergeModifyVolumeConditionsOfPVC(newPVC.Status.Conditions, - []v1.PersistentVolumeClaimCondition{pvcCondition}) - - updatedPVC, err := util.PatchClaim(ctrl.kubeClient, pvc, newPVC, false /* addResourceVersionCheck */) + updatedPVC, err := util.PatchClaim(ctrl.kubeClient, pvc, newPVC, true /* addResourceVersionCheck */) if err != nil { - return pvc, fmt.Errorf("mark PVC %q as controller expansion failed, errored with: %v", pvc.Name, err) + return pvc, pv, fmt.Errorf("mark PVC %q as ModifyVolumeCompleted failed, errored with: %v", pvc.Name, err) } - return updatedPVC, nil + + return updatedPVC, updatedPV, nil } -// markControllerModifyVolumeStatus will mark ModifyVolumeStatus as completed in the PVC +// markControllerModifyVolumeCompleted will mark ModifyVolumeStatus as completed in the PVC // and update CurrentVolumeAttributesClassName, clear the conditions func (ctrl *modifyController) markControllerModifyVolumeCompleted(pvc *v1.PersistentVolumeClaim, pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error) { modifiedVacName := pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName @@ -131,7 +224,30 @@ func (ctrl *modifyController) markControllerModifyVolumeCompleted(pvc *v1.Persis return updatedPVC, updatedPV, nil } -// markControllerModifyVolumeStatus clears all the conditions related to modify volume and only +func pvcModifyingVolumeCondition() v1.PersistentVolumeClaimCondition { + return v1.PersistentVolumeClaimCondition{ + Type: v1.PersistentVolumeClaimVolumeModifyingVolume, + Status: v1.ConditionTrue, + LastTransitionTime: metav1.Now(), + Message: "ModifyVolume operation in progress.", + } +} + +func pvcModifyVolumeErrorCondition(err error) v1.PersistentVolumeClaimCondition { + msg := "ModifyVolume failed with error. Waiting for retry." + if err != nil { + msg = "ModifyVolume failed with error: " + err.Error() + ". Waiting for retry." + } + + return v1.PersistentVolumeClaimCondition{ + Type: v1.PersistentVolumeClaimVolumeModifyVolumeError, + Status: v1.ConditionTrue, + LastTransitionTime: metav1.Now(), + Message: msg, + } +} + +// clearModifyVolumeConditions clears all the conditions related to modify volume and only // leave other condition types func clearModifyVolumeConditions(conditions []v1.PersistentVolumeClaimCondition) []v1.PersistentVolumeClaimCondition { knownConditions := []v1.PersistentVolumeClaimCondition{} diff --git a/pkg/modifycontroller/modify_status_test.go b/pkg/modifycontroller/modify_status_test.go index 932245a65..8a8743ec1 100644 --- a/pkg/modifycontroller/modify_status_test.go +++ b/pkg/modifycontroller/modify_status_test.go @@ -397,10 +397,12 @@ func createTestPV(capacityGB int, pvcName, pvcNamespace string, pvcUID types.UID VolumeHandle: "foo", }, }, - VolumeMode: volumeMode, - VolumeAttributesClassName: &vacName, + VolumeMode: volumeMode, }, } + if vacName != "" { + pv.Spec.VolumeAttributesClassName = &vacName + } if len(pvcName) > 0 { pv.Spec.ClaimRef = &v1.ObjectReference{ Namespace: pvcNamespace, diff --git a/pkg/modifycontroller/modify_volume.go b/pkg/modifycontroller/modify_volume.go index 6823dba16..b0d8d62fa 100644 --- a/pkg/modifycontroller/modify_volume.go +++ b/pkg/modifycontroller/modify_volume.go @@ -20,8 +20,6 @@ import ( "fmt" "github.com/kubernetes-csi/external-resizer/pkg/util" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" v1 "k8s.io/api/core/v1" storagev1beta1 "k8s.io/api/storage/v1beta1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -37,122 +35,113 @@ const ( // The return value bool is only used as a sentinel value when function returns without actually performing modification func (ctrl *modifyController) modify(pvc *v1.PersistentVolumeClaim, pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) { - pvcSpecVacName := pvc.Spec.VolumeAttributesClassName - curVacName := pvc.Status.CurrentVolumeAttributesClassName - pvcKey, err := cache.MetaNamespaceKeyFunc(pvc) - if err != nil { - return pvc, pv, err, false + pvcSpecVACName := pvc.Spec.VolumeAttributesClassName + + if !isModificationNeeded(pvc) { + return pvc, pv, nil, false } // Requeue PVC if modification recently failed with infeasible error. - delayModificationErr := ctrl.delayModificationIfRecentlyInfeasible(pvc, pvcKey) - if delayModificationErr != nil { - return pvc, pv, delayModificationErr, false + if recentlyInfeasibleErr := ctrl.delayModificationIfRecentlyInfeasible(pvc); recentlyInfeasibleErr != nil { + return pvc, pv, recentlyInfeasibleErr, false } - if pvcSpecVacName != nil && curVacName == nil { - // First time adding VAC to a PVC - return ctrl.validateVACAndModifyVolumeWithTarget(pvc, pv) - } else if pvcSpecVacName != nil && curVacName != nil && *pvcSpecVacName != *curVacName { - // Check if PVC in uncertain state - _, inUncertainState := ctrl.uncertainPVCs[pvcKey] - if !inUncertainState { - klog.V(3).InfoS("previous operation on the PVC failed with a final error, retrying") - return ctrl.validateVACAndModifyVolumeWithTarget(pvc, pv) - } else { - vac, err := ctrl.vacLister.Get(*pvcSpecVacName) - if err != nil { - if apierrors.IsNotFound(err) { - ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, "VAC "+*pvcSpecVacName+" does not exist.") - } - return pvc, pv, err, false - } - return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac, pvcSpecVacName) + // Validate VAC exists + vac, err := ctrl.vacLister.Get(*pvcSpecVACName) + if err != nil { + if apierrors.IsNotFound(err) { + ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, "VAC "+*pvcSpecVACName+" does not exist.") + } + klog.Errorf("Get VAC with vac name %s in VACInformer cache failed: %v", *pvcSpecVACName, err) + pvc, statusErr := ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumePending, nil) + if statusErr != nil { + return pvc, pv, statusErr, false } + return pvc, pv, nil, false } - // No modification required - return pvc, pv, nil, false + // If we haven't attempted modification to current VAC yet, + // set ModifyVolumeInProgress and clear any ModifyVolume conditions from outdated modifications + if isFirstTimeModifyingVacInPvcSpec(pvc) { + pvc, err = ctrl.markControllerModifyVolumeFirstAttempt(pvc) + if err != nil { + return pvc, pv, err, false + } + } + + // Call plugin + ctrl.eventRecorder.Event(pvc, v1.EventTypeNormal, util.VolumeModify, + fmt.Sprintf("external resizer is modifying volume %s with vac %s", pvc.Name, *pvcSpecVACName)) + return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac) +} + +func isModificationNeeded(pvc *v1.PersistentVolumeClaim) bool { + pvcSpecVacName := pvc.Spec.VolumeAttributesClassName + currentVacName := pvc.Status.CurrentVolumeAttributesClassName + + return (currentVacName == nil && pvcSpecVacName != nil) || + (pvcSpecVacName != nil && *currentVacName != *pvcSpecVacName) +} + +func isFirstTimeModifyingVacInPvcSpec(pvc *v1.PersistentVolumeClaim) bool { + pvcSpecVacName := pvc.Spec.VolumeAttributesClassName + + return pvc.Status.ModifyVolumeStatus == nil || // Never attempted modification + // ModifyVolumeStatus referencing outdated modification + (pvcSpecVacName != nil && *pvcSpecVacName != pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName) + } -// func validateVACAndModifyVolumeWithTarget validate the VAC. The function sets pvc.Status.ModifyVolumeStatus -// to Pending if VAC does not exist and proceeds to trigger ModifyVolume if VAC exists -func (ctrl *modifyController) validateVACAndModifyVolumeWithTarget( +func isVacRolledBack(pvc *v1.PersistentVolumeClaim) bool { + pvcSpecVacName := pvc.Spec.VolumeAttributesClassName + curVacName := pvc.Status.CurrentVolumeAttributesClassName + targetVacName := pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName + // Case 1: rollback to nil + // Case 2: rollback to previous VAC + return (pvcSpecVacName == nil && curVacName == nil && targetVacName != "") || + (pvcSpecVacName != nil && curVacName != nil && + *pvcSpecVacName == *curVacName && targetVacName != *curVacName) +} + +func currentModificationInfeasible(pvc *v1.PersistentVolumeClaim) bool { + return pvc.Status.ModifyVolumeStatus != nil && pvc.Status.ModifyVolumeStatus.Status == v1.PersistentVolumeClaimModifyVolumeInfeasible +} + +func (ctrl *modifyController) validateVACAndRollback( pvc *v1.PersistentVolumeClaim, pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) { - // The controller only triggers ModifyVolume if pvcSpecVacName is not nil nor empty - pvcSpecVacName := pvc.Spec.VolumeAttributesClassName - // Check if pvcSpecVac is valid and exist - vac, err := ctrl.vacLister.Get(*pvcSpecVacName) - if err == nil { - // Mark pvc.Status.ModifyVolumeStatus as in progress - pvc, err = ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumeInProgress, nil) - if err != nil { - return pvc, pv, err, false - } - // Record an event to indicate that external resizer is modifying this volume. - ctrl.eventRecorder.Event(pvc, v1.EventTypeNormal, util.VolumeModify, - fmt.Sprintf("external resizer is modifying volume %s with vac %s", pvc.Name, *pvcSpecVacName)) - return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac, pvcSpecVacName) - } else { - if apierrors.IsNotFound(err) { - ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, "VAC "+*pvcSpecVacName+" does not exist.") - } - klog.Errorf("Get VAC with vac name %s in VACInformer cache failed: %v", *pvcSpecVacName, err) - // Mark pvc.Status.ModifyVolumeStatus as pending - pvc, err = ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumePending, nil) - return pvc, pv, err, false + // The controller does not trigger ModifyVolume because it is only + // for rolling back infeasible errors + // Record an event to indicate that external resizer is rolling back this volume. + rollbackVACName := "nil" + if pvc.Spec.VolumeAttributesClassName != nil { + rollbackVACName = *pvc.Spec.VolumeAttributesClassName } + ctrl.eventRecorder.Event(pvc, v1.EventTypeNormal, util.VolumeModify, + fmt.Sprintf("external resizer is rolling back volume %s with infeasible error to VAC %s", pvc.Name, rollbackVACName)) + // Mark pvc.Status.ModifyVolumeStatus as completed + pvc, pv, err := ctrl.markControllerModifyVolumeRollbackCompleted(pvc, pv) + if err != nil { + return pvc, pv, fmt.Errorf("rollback volume %s modification with error: %v ", pvc.Name, err), false + } + return pvc, pv, nil, false } // func controllerModifyVolumeWithTarget trigger the CSI ControllerModifyVolume API call // and handle both success and error scenarios -func (ctrl *modifyController) controllerModifyVolumeWithTarget( - pvc *v1.PersistentVolumeClaim, - pv *v1.PersistentVolume, - vacObj *storagev1beta1.VolumeAttributesClass, - pvcSpecVacName *string) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) { - var err error - pvc, pv, err = ctrl.callModifyVolumeOnPlugin(pvc, pv, vacObj) - if err == nil { - klog.V(4).Infof("Update volumeAttributesClass of PV %q to %s succeeded", pv.Name, *pvcSpecVacName) - // Record an event to indicate that modify operation is successful. +func (ctrl *modifyController) controllerModifyVolumeWithTarget(pvc *v1.PersistentVolumeClaim, pv *v1.PersistentVolume, vacObj *storagev1beta1.VolumeAttributesClass) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) { + var pluginErr error + pvc, pv, pluginErr = ctrl.callModifyVolumeOnPlugin(pvc, pv, vacObj) + if pluginErr == nil { + klog.V(4).Infof("Update volumeAttributesClass of PV %q to %s succeeded", pv.Name, *pv.Spec.VolumeAttributesClassName) ctrl.eventRecorder.Eventf(pvc, v1.EventTypeNormal, util.VolumeModifySuccess, fmt.Sprintf("external resizer modified volume %s with vac %s successfully ", pvc.Name, vacObj.Name)) return pvc, pv, nil, true - } else { - errStatus, ok := status.FromError(err) - if ok { - pvc, updateConditionErr := ctrl.updateConditionBasedOnError(pvc, err) - if updateConditionErr != nil { - return nil, nil, err, false - } - pvcKey, keyErr := cache.MetaNamespaceKeyFunc(pvc) - if keyErr != nil { - return pvc, pv, keyErr, false - } - if !util.IsFinalError(keyErr) { - // update conditions and cache pvc as uncertain - ctrl.uncertainPVCs[pvcKey] = *pvc - } else { - // Only InvalidArgument can be set to Infeasible state - // Final errors other than InvalidArgument will still be in InProgress state - if errStatus.Code() == codes.InvalidArgument { - // Mark pvc.Status.ModifyVolumeStatus as infeasible - pvc, markModifyVolumeInfeasibleError := ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumeInfeasible, err) - if markModifyVolumeInfeasibleError != nil { - return pvc, pv, markModifyVolumeInfeasibleError, false - } - ctrl.markForSlowRetry(pvc, pvcKey) - } - ctrl.removePVCFromModifyVolumeUncertainCache(pvcKey) - } - } else { - return pvc, pv, fmt.Errorf("cannot get error status from modify volume err: %v ", err), false - } - // Record an event to indicate that modify operation is failed. - ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, err.Error()) - return pvc, pv, err, false } + + // Update PVC and record an event to indicate that modify operation is failed. + pvc, markErr := ctrl.markControllerModifyVolumeFailed(pvc, pluginErr) + ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, pluginErr.Error()) + return pvc, pv, markErr, false } func (ctrl *modifyController) callModifyVolumeOnPlugin( @@ -179,7 +168,12 @@ func (ctrl *modifyController) callModifyVolumeOnPlugin( // func delayModificationIfRecentlyInfeasible returns a delayRetryError if PVC modification recently failed with // infeasible error -func (ctrl *modifyController) delayModificationIfRecentlyInfeasible(pvc *v1.PersistentVolumeClaim, pvcKey string) error { +func (ctrl *modifyController) delayModificationIfRecentlyInfeasible(pvc *v1.PersistentVolumeClaim) error { + pvcKey, err := cache.MetaNamespaceKeyFunc(pvc) + if err != nil { + return err + } + // Do not delay modification if PVC updated with new VAC s := pvc.Status.ModifyVolumeStatus if s == nil || pvc.Spec.VolumeAttributesClassName == nil || s.TargetVolumeAttributesClassName != *pvc.Spec.VolumeAttributesClassName { diff --git a/pkg/modifycontroller/modify_volume_test.go b/pkg/modifycontroller/modify_volume_test.go index 25a8abbe8..b4a9db33c 100644 --- a/pkg/modifycontroller/modify_volume_test.go +++ b/pkg/modifycontroller/modify_volume_test.go @@ -35,7 +35,7 @@ var ( ) func TestModify(t *testing.T) { - basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/) + basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/) basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac) var tests = []struct { @@ -61,7 +61,7 @@ func TestModify(t *testing.T) { }, { name: "vac does not exist, no modification and set ModifyVolumeStatus to pending", - pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/), + pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/), pv: basePV, expectModifyCall: false, expectedModifyVolumeStatus: &v1.ModifyVolumeStatus{ @@ -73,7 +73,7 @@ func TestModify(t *testing.T) { }, { name: "modify volume success", - pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/), + pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/), pv: basePV, vacExists: true, expectModifyCall: true, @@ -83,7 +83,7 @@ func TestModify(t *testing.T) { }, { name: "modify volume success with extra metadata", - pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/), + pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/), pv: basePV, vacExists: true, expectModifyCall: true, @@ -130,16 +130,20 @@ func TestModify(t *testing.T) { actualCurrentVolumeAttributesClassName := pvc.Status.CurrentVolumeAttributesClassName - if diff := cmp.Diff(*test.expectedCurrentVolumeAttributesClassName, *actualCurrentVolumeAttributesClassName); diff != "" { - t.Errorf("expected CurrentVolumeAttributesClassName to be %v, got %v", *test.expectedCurrentVolumeAttributesClassName, *actualCurrentVolumeAttributesClassName) + if test.expectedCurrentVolumeAttributesClassName != nil && actualCurrentVolumeAttributesClassName != nil { + if diff := cmp.Diff(*test.expectedCurrentVolumeAttributesClassName, *actualCurrentVolumeAttributesClassName); diff != "" { + t.Errorf("expected CurrentVolumeAttributesClassName to be %v, got %v", *test.expectedCurrentVolumeAttributesClassName, *actualCurrentVolumeAttributesClassName) + } } actualPVVolumeAttributesClassName := pv.Spec.VolumeAttributesClassName - if diff := cmp.Diff(*test.expectedPVVolumeAttributesClassName, *actualPVVolumeAttributesClassName); diff != "" { - t.Errorf("expected VolumeAttributesClassName of pv to be %v, got %v", *test.expectedPVVolumeAttributesClassName, *actualPVVolumeAttributesClassName) + if test.expectedPVVolumeAttributesClassName != nil && actualPVVolumeAttributesClassName != nil { + if diff := cmp.Diff(*test.expectedPVVolumeAttributesClassName, *actualPVVolumeAttributesClassName); diff != "" { + t.Errorf("expected VolumeAttributesClassName of pv to be %v, got %v", *test.expectedPVVolumeAttributesClassName, *actualPVVolumeAttributesClassName) + } } - if test.withExtraMetadata { + if test.withExtraMetadata && test.expectedPVVolumeAttributesClassName != nil { vacObj, err := ctrlInstance.vacLister.Get(*test.expectedPVVolumeAttributesClassName) if err != nil { t.Errorf("failed to get VAC: %v", err) @@ -154,7 +158,7 @@ func TestModify(t *testing.T) { } } -func createTestPVC(pvcName string, vacName string, curVacName string, targetVacName string) *v1.PersistentVolumeClaim { +func createTestPVC(pvcName string, vacName string, curVacName string, targetVacName string, modifyVolumeStatus v1.PersistentVolumeClaimModifyVolumeStatus) *v1.PersistentVolumeClaim { pvc := &v1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{Name: pvcName, Namespace: pvcNamespace}, Spec: v1.PersistentVolumeClaimSpec{ @@ -167,21 +171,25 @@ func createTestPVC(pvcName string, vacName string, curVacName string, targetVacN v1.ResourceName(v1.ResourceStorage): resource.MustParse("2Gi"), }, }, - VolumeAttributesClassName: &vacName, - VolumeName: pvName, + VolumeName: pvName, }, Status: v1.PersistentVolumeClaimStatus{ Phase: v1.ClaimBound, Capacity: v1.ResourceList{ v1.ResourceStorage: resource.MustParse("2Gi"), }, - CurrentVolumeAttributesClassName: &curVacName, ModifyVolumeStatus: &v1.ModifyVolumeStatus{ TargetVolumeAttributesClassName: targetVacName, - Status: "", + Status: modifyVolumeStatus, }, }, } + if vacName != "" { + pvc.Spec.VolumeAttributesClassName = &vacName + } + if curVacName != "" { + pvc.Status.CurrentVolumeAttributesClassName = &curVacName + } return pvc } diff --git a/pkg/util/util.go b/pkg/util/util.go index 18b80d95f..037e92de7 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -258,10 +258,10 @@ func IsFinalError(err error) bool { return true } -// IsInfeasibleError returns true for grpc errors that are considered terminal in a way +// IsResizeInfeasibleError returns true for grpc errors that are considered terminal in a way // that they indicate CSI operation as infeasible. // This function is a subset of final errors. All infeasible errors are also final errors -func IsInfeasibleError(err error) bool { +func IsResizeInfeasibleError(err error) bool { st, ok := status.FromError(err) if !ok { // This is not gRPC error. The operation must have failed before gRPC @@ -279,3 +279,23 @@ func IsInfeasibleError(err error) bool { // even start or failed. It is for sure not in progress. return false } + +// IsModifyInfeasibleError returns true for ControllerModifyVolume CSI grpc errors that are considered terminal in a way +// that they indicate CSI operation as infeasible. +// This function is a subset of final errors. All infeasible errors are also final errors +func IsModifyInfeasibleError(err error) bool { + st, ok := status.FromError(err) + if !ok { + // This is not gRPC error. The operation must have failed before gRPC + // method was called, otherwise we would get gRPC error. + // We don't know if any previous volume operation is in progress, be on the safe side. + return false + } + switch st.Code() { + case codes.InvalidArgument: + return true + } + // All other errors mean that operation either did not + // even start or failed. It is for sure not in progress. + return false +}