don't change target in uncertain state

We should keep retry the previously specified target.

Author: huww98

pkg/modifycontroller/modify_status.go

@@ -47,6 +47,7 @@ func (ctrl *modifyController) markControllerModifyVolumeStatus(
 	case v1.PersistentVolumeClaimModifyVolumeInProgress:
 		conditionMessage = "ModifyVolume operation in progress."
 	case v1.PersistentVolumeClaimModifyVolumeInfeasible:
+		newPVC.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName = pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName
 		conditionMessage = "ModifyVolume failed with error " + err.Error() + ". Waiting for retry."
 	}
 	pvcCondition.Message = conditionMessage

pkg/modifycontroller/modify_volume.go

@@ -58,14 +58,15 @@ func (ctrl *modifyController) modify(pvc *v1.PersistentVolumeClaim, pv *v1.Persi
 	} else if pvcSpecVacName != nil && curVacName != nil && *pvcSpecVacName != *curVacName {
 		// Check if PVC in uncertain state
 		_, inUncertainState := ctrl.uncertainPVCs.Load(pvcKey)
-		if !inUncertainState {
-			klog.V(3).InfoS("previous operation on the PVC failed with a final error, retrying")
+		status := pvc.Status.ModifyVolumeStatus
+		if !inUncertainState || status == nil {
+			klog.V(3).InfoS("previous operation on the PVC succeeded or failed with a final error, retrying")
 			return ctrl.validateVACAndModifyVolumeWithTarget(pvc, pv)
 		} else {
-			vac, err := ctrl.vacLister.Get(*pvcSpecVacName)
+			vac, err := ctrl.vacLister.Get(status.TargetVolumeAttributesClassName)
 			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 "+status.TargetVolumeAttributesClassName+" does not exist.")
 				}
 				return pvc, pv, err, false
 			}

pkg/modifycontroller/modify_volume_test.go

@@ -15,6 +15,7 @@ import (
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/fake"
+	"k8s.io/utils/ptr"
 )
 
 var (
@@ -185,12 +186,22 @@ func TestModifyUncertain(t *testing.T) {
 	assertUncertain(false)
 
 	client.SetModifyError(nonFinalErr)
-	_, _, err, _ = ctrlInstance.modify(pvc, pv)
+	pvc, pv, err, _ = ctrlInstance.modify(pvc, pv)
 	if !errors.Is(err, nonFinalErr) {
 		t.Fatalf("expected error to be %v, got %v", nonFinalErr, err)
 	}
 	// should enter uncertain state again
 	assertUncertain(true)
+
+	pvc.Spec.VolumeAttributesClassName = ptr.To("yet-another-vac")
+	pvc, _, err, _ = ctrlInstance.modify(pvc, pv)
+	if !errors.Is(err, nonFinalErr) {
+		t.Fatalf("expected error to be %v, got %v", nonFinalErr, err)
+	}
+	// target should not change, yet-another-vac should be ignored
+	if pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName != targetVac {
+		t.Fatalf("expected target to be %v, got %v", targetVac, pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName)
+	}
 }
 
 func createTestPVC(pvcName string, vacName string, curVacName string, targetVacName string) *v1.PersistentVolumeClaim {