Enable Rollback Support of VAC with Infeasible Errors & Do Not Modify Volume if There is InProgress Modification

pkg/modifycontroller/controller.go

@@ -278,7 +278,7 @@ func (ctrl *modifyController) syncPVC(key string) error {
 	}
 
 	vacName := pvc.Spec.VolumeAttributesClassName
-	if vacName != nil && *vacName != "" && pvc.Status.Phase == v1.ClaimBound {
+	if (vacName != nil && *vacName != "" && pvc.Status.Phase == v1.ClaimBound) || (util.CurrentModificationInfeasible(pvc) && util.IsVacRolledBack(pvc)) {
 		_, _, err, _ := ctrl.modify(pvc, pv)
 		if err != nil {
 			return err

pkg/modifycontroller/controller_test.go

@@ -24,8 +24,8 @@ import (
 )
 
 func TestController(t *testing.T) {
-	basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/)
-	basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
+	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
 	firstTimePVC := basePVC.DeepCopy()
@@ -41,7 +41,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,
@@ -89,7 +89,7 @@ func TestController(t *testing.T) {
 }
 
 func TestModifyPVC(t *testing.T) {
-	basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
+	basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, &testVac)
 
 	tests := []struct {
 		name          string
@@ -100,14 +100,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,
@@ -140,16 +140,16 @@ func TestModifyPVC(t *testing.T) {
 }
 
 func TestSyncPVC(t *testing.T) {
-	basePVC := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/)
-	basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
+	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 := 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{
@@ -191,7 +191,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, &emptyString /*vacName*/, &testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/),
 			pv:            basePV,
 			callCSIModify: false,
 		},
@@ -241,8 +241,8 @@ 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*/)
-	basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
+	basePVC := createTestPVC(pvcName, &targetVac /*vacName*/, &testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/)
+	basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, &testVac)
 
 	tests := []struct {
 		name                        string

pkg/modifycontroller/modify_status.go

@@ -74,6 +74,44 @@ func (ctrl *modifyController) markControllerModifyVolumeStatus(
 	return updatedPVC, nil
 }
 
+func (ctrl *modifyController) markControllerModifyVolumeRollbackCompeleted(
+	pvc *v1.PersistentVolumeClaim,
+	pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error) {
+	// Update PVC
+	newPVC := pvc.DeepCopy()
+
+	// 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()
+	if pvc.Spec.VolumeAttributesClassName != nil && *pvc.Spec.VolumeAttributesClassName == "" {
+		// PV does not support empty string, set VolumeAttributesClassName to nil
+		newPV.Spec.VolumeAttributesClassName = nil
+	} else {
+		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 {
+		return pvc, pv, fmt.Errorf("update pv.Spec.VolumeAttributesClassName for PVC %q failed, errored with: %v", pvc.Name, err)
+	}
+
+	updatedPVC, err := util.PatchClaim(ctrl.kubeClient, pvc, newPVC, false /* addResourceVersionCheck */)
+	if err != nil {
+		return pvc, pv, fmt.Errorf("mark PVC %q as ModifyVolumeCompleted failed, errored with: %v", pvc.Name, err)
+	}
+
+	return updatedPVC, updatedPV, nil
+}
+
 func (ctrl *modifyController) updateConditionBasedOnError(pvc *v1.PersistentVolumeClaim, err error) (*v1.PersistentVolumeClaim, error) {
 	newPVC := pvc.DeepCopy()
 	pvcCondition := v1.PersistentVolumeClaimCondition{
@@ -92,12 +130,12 @@ func (ctrl *modifyController) updateConditionBasedOnError(pvc *v1.PersistentVolu
 
 	updatedPVC, err := util.PatchClaim(ctrl.kubeClient, pvc, newPVC, false /* addResourceVersionCheck */)
 	if err != nil {
-		return pvc, fmt.Errorf("mark PVC %q as controller expansion failed, errored with: %v", pvc.Name, err)
+		return pvc, fmt.Errorf("mark PVC %q as controller modification failed, errored with: %v", pvc.Name, err)
 	}
 	return updatedPVC, 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
@@ -132,7 +170,7 @@ func (ctrl *modifyController) markControllerModifyVolumeCompleted(pvc *v1.Persis
 	return updatedPVC, updatedPV, nil
 }
 
-// markControllerModifyVolumeStatus clears all the conditions related to modify volume and only
+// 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{}

pkg/modifycontroller/modify_status_test.go

@@ -33,6 +33,7 @@ const (
 
 var (
 	fsVolumeMode           = v1.PersistentVolumeFilesystem
+	emptyString            = ""
 	testVac                = "test-vac"
 	targetVac              = "target-vac"
 	testDriverName         = "mock"
@@ -199,7 +200,7 @@ func TestUpdateConditionBasedOnError(t *testing.T) {
 
 func TestMarkControllerModifyVolumeCompleted(t *testing.T) {
 	basePVC := testutil.MakeTestPVC([]v1.PersistentVolumeClaimCondition{})
-	basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
+	basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, &testVac)
 	expectedPV := basePV.DeepCopy()
 	expectedPV.Spec.VolumeAttributesClassName = &targetVac
 	expectedPVC := basePVC.WithCurrentVolumeAttributesClassName(targetVac).Get()
@@ -377,7 +378,7 @@ func TestRemovePVCFromModifyVolumeUncertainCache(t *testing.T) {
 	}
 }
 
-func createTestPV(capacityGB int, pvcName, pvcNamespace string, pvcUID types.UID, volumeMode *v1.PersistentVolumeMode, vacName string) *v1.PersistentVolume {
+func createTestPV(capacityGB int, pvcName, pvcNamespace string, pvcUID types.UID, volumeMode *v1.PersistentVolumeMode, vacName *string) *v1.PersistentVolume {
 	capacity := testutil.QuantityGB(capacityGB)
 
 	pv := &v1.PersistentVolume{
@@ -395,10 +396,11 @@ func createTestPV(capacityGB int, pvcName, pvcNamespace string, pvcUID types.UID
 					VolumeHandle: "foo",
 				},
 			},
+			VolumeAttributesClassName: vacName,
 			VolumeMode:                volumeMode,
-			VolumeAttributesClassName: &vacName,
 		},
 	}
+
 	if len(pvcName) > 0 {
 		pv.Spec.ClaimRef = &v1.ObjectReference{
 			Namespace: pvcNamespace,

pkg/modifycontroller/modify_volume.go

@@ -39,8 +39,9 @@ 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
+	pvcSpecVACName := pvc.Spec.VolumeAttributesClassName
+	currentVacName := pvc.Status.CurrentVolumeAttributesClassName
+
 	pvcKey, err := cache.MetaNamespaceKeyFunc(pvc)
 	if err != nil {
 		return pvc, pv, err, false
@@ -52,28 +53,52 @@ func (ctrl *modifyController) modify(pvc *v1.PersistentVolumeClaim, pv *v1.Persi
 		return pvc, pv, delayModificationErr, false
 	}
 
-	if pvcSpecVacName != nil && curVacName == nil {
-		// First time adding VAC to a PVC
+	if util.CurrentModificationInfeasible(pvc) && util.IsVacRolledBack(pvc) {
+		return ctrl.validateVACAndRollback(pvc, pv)
+	}
+
+	if pvcSpecVACName == nil {
+		return pvc, pv, nil, false
+	}
+
+	// If there are in-progress or non-final error VAC modifications, do not apply
+	// the next VAC until the current one applies or fails.
+	if pvc.Status.ModifyVolumeStatus != nil &&
+		pvc.Status.ModifyVolumeStatus.Status == v1.PersistentVolumeClaimModifyVolumeInProgress {
+		_, inUncertainCache := ctrl.uncertainPVCs[pvcKey]
+		hasModifyErrorCondition := false
+		for _, c := range pvc.Status.Conditions {
+			if c.Type == v1.PersistentVolumeClaimVolumeModifyVolumeError && c.Status == v1.ConditionTrue {
+				hasModifyErrorCondition = true
+				break
+			}
+		}
+		if inUncertainCache || !hasModifyErrorCondition {
+			klog.V(4).Infof("PVC %s is already being modified, skipping", pvcKey)
+			return pvc, pv, nil, false
+		}
+	}
+
+	switch {
+	case currentVacName == nil:
 		return ctrl.validateVACAndModifyVolumeWithTarget(pvc, pv)
-	} else if pvcSpecVacName != nil && curVacName != nil && *pvcSpecVacName != *curVacName {
+	case *currentVacName != *pvcSpecVACName:
 		// 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)
+			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.")
+					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)
+			return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac, pvcSpecVACName)
 		}
 	}
-
-	// No modification required
 	return pvc, pv, nil, false
 }
 
@@ -107,6 +132,26 @@ func (ctrl *modifyController) validateVACAndModifyVolumeWithTarget(
 	}
 }
 
+func (ctrl *modifyController) validateVACAndRollback(
+	pvc *v1.PersistentVolumeClaim,
+	pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) {
+	// The controller does not triggers ModifyVolume because it is only
+	// for rollbacking 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.markControllerModifyVolumeRollbackCompeleted(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(