Cleanup conditions and update event message

Author: gnufied

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/modify_status.go

@@ -91,7 +91,12 @@ func (ctrl *modifyController) markCotrollerModifyVolumeRollbackCompeleted(
 
 	// Update PV
 	newPV := pv.DeepCopy()
-	newPV.Spec.VolumeAttributesClassName = pvc.Spec.VolumeAttributesClassName
+	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)

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,38 +53,34 @@ 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
+	}
+
+	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)
 		}
 	}
-
-	// Rollback infeasible errors for recovery
-	if pvc.Status.ModifyVolumeStatus != nil && pvc.Status.ModifyVolumeStatus.Status == v1.PersistentVolumeClaimModifyVolumeInfeasible {
-		targetVacName := pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName
-		// Case 1: rollback to nil
-		// Case 2: rollback to previous VAC
-		if (pvcSpecVacName == nil && curVacName == nil && targetVacName != "") || (pvcSpecVacName != nil && curVacName != nil && *pvcSpecVacName == *curVacName && targetVacName != *curVacName) {
-			return ctrl.validateVACAndRollback(pvc, pv)
-		}
-	}
-
-	// No modification required
 	return pvc, pv, nil, false
 }
 
@@ -123,8 +120,12 @@ func (ctrl *modifyController) validateVACAndRollback(
 	// 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", pvc.Name))
+		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.markCotrollerModifyVolumeRollbackCompeleted(pvc, pv)
 	if err != nil {

pkg/util/util.go

@@ -281,3 +281,18 @@ func IsInfeasibleError(err error) bool {
 	// even start or failed. It is for sure not in progress.
 	return false
 }
+
+func IsVacRolledBack(pvc *v1.PersistentVolumeClaim) bool {
+	pvcSpecVacName := pvc.Spec.VolumeAttributesClassName
+	curVacName := pvc.Status.CurrentVolumeAttributesClassName
+	targetVacName := pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName
+	// Case 1: rollback to nil or empty string
+	// Case 2: rollback to previous VAC
+	return ((pvcSpecVacName == nil || *pvcSpecVacName == "") && (curVacName == nil || *curVacName == "") && 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
+}