Fix PVC ModifyVolume conditions

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)

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 {

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{}

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,