diff --git a/pkg/modifycontroller/controller.go b/pkg/modifycontroller/controller.go index 1895a355..b3c67ad4 100644 --- a/pkg/modifycontroller/controller.go +++ b/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 diff --git a/pkg/modifycontroller/controller_test.go b/pkg/modifycontroller/controller_test.go index 3acd842e..b6aa01e5 100644 --- a/pkg/modifycontroller/controller_test.go +++ b/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 diff --git a/pkg/modifycontroller/modify_status.go b/pkg/modifycontroller/modify_status.go index eb7c7032..474a1791 100644 --- a/pkg/modifycontroller/modify_status.go +++ b/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{} diff --git a/pkg/modifycontroller/modify_status_test.go b/pkg/modifycontroller/modify_status_test.go index 50c7de27..fad705d4 100644 --- a/pkg/modifycontroller/modify_status_test.go +++ b/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, diff --git a/pkg/modifycontroller/modify_volume.go b/pkg/modifycontroller/modify_volume.go index 7523ab62..1f5dd2b7 100644 --- a/pkg/modifycontroller/modify_volume.go +++ b/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( diff --git a/pkg/modifycontroller/modify_volume_test.go b/pkg/modifycontroller/modify_volume_test.go index 97490c47..cf6d145c 100644 --- a/pkg/modifycontroller/modify_volume_test.go +++ b/pkg/modifycontroller/modify_volume_test.go @@ -35,8 +35,8 @@ var ( ) func TestModify(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) var tests = []struct { name string @@ -49,6 +49,7 @@ func TestModify(t *testing.T) { expectedPVVolumeAttributesClassName *string withExtraMetadata bool expectedVacParams map[string]string + addPVCtoUncertainCache bool }{ { name: "nothing to modify", @@ -61,7 +62,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*/, "" /*targetVacName*/), + pvc: createTestPVC(pvcName, &targetVac /*vacName*/, &testVac /*curVacName*/, "" /*targetVacName*/, "" /*modifyVolumeStatus*/), pv: basePV, expectModifyCall: false, expectedModifyVolumeStatus: &v1.ModifyVolumeStatus{ @@ -73,7 +74,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 +84,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, @@ -98,6 +99,61 @@ func TestModify(t *testing.T) { "csi.storage.k8s.io/pv/name": "testPV", }, }, + { + name: "modify volume rollback succeeds for infeasible errors", + pvc: createTestPVC(pvcName, &testVac /*vacName*/, &testVac /*curVacName*/, targetVac /*targetVacName*/, v1.PersistentVolumeClaimModifyVolumeInfeasible), + pv: basePV, + vacExists: true, + expectModifyCall: false, + expectedModifyVolumeStatus: nil, + expectedCurrentVolumeAttributesClassName: &testVac, + expectedPVVolumeAttributesClassName: &testVac, + }, + { + name: "modify volume rollback to nil succeeds for infeasible errors", + pvc: createTestPVC(pvcName, nil /*vacName*/, nil /*curVacName*/, targetVac /*targetVacName*/, v1.PersistentVolumeClaimModifyVolumeInfeasible), + pv: createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, nil), + vacExists: true, + expectModifyCall: false, + expectedModifyVolumeStatus: nil, + expectedCurrentVolumeAttributesClassName: nil, + expectedPVVolumeAttributesClassName: nil, + }, + { + name: "modify volume rollback to empty string succeeds for infeasible errors", + pvc: createTestPVC(pvcName, &emptyString /*vacName*/, nil /*curVacName*/, targetVac /*targetVacName*/, v1.PersistentVolumeClaimModifyVolumeInfeasible), + pv: createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, nil), + vacExists: true, + expectModifyCall: false, + expectedModifyVolumeStatus: nil, + expectedCurrentVolumeAttributesClassName: nil, + expectedPVVolumeAttributesClassName: nil, + }, + { + name: "pvc in uncertain cache, no modification", + pvc: createTestPVC(pvcName, &targetVac, &testVac, targetVac, v1.PersistentVolumeClaimModifyVolumeInProgress), + pv: basePV, + addPVCtoUncertainCache: true, + expectModifyCall: false, + expectedModifyVolumeStatus: &v1.ModifyVolumeStatus{ + TargetVolumeAttributesClassName: targetVac, + Status: v1.PersistentVolumeClaimModifyVolumeInProgress, + }, + expectedCurrentVolumeAttributesClassName: &testVac, + expectedPVVolumeAttributesClassName: &testVac, + }, + { + name: "pvc not in uncertain cache and no error, no modification", + pvc: createTestPVC(pvcName, &targetVac, &testVac, targetVac, v1.PersistentVolumeClaimModifyVolumeInProgress), + pv: basePV, + expectModifyCall: false, + expectedModifyVolumeStatus: &v1.ModifyVolumeStatus{ + TargetVolumeAttributesClassName: targetVac, + Status: v1.PersistentVolumeClaimModifyVolumeInProgress, + }, + expectedCurrentVolumeAttributesClassName: &testVac, + expectedPVVolumeAttributesClassName: &testVac, + }, } for i := range tests { @@ -111,6 +167,10 @@ func TestModify(t *testing.T) { } ctrlInstance := setupFakeK8sEnvironment(t, client, initialObjects) + if test.addPVCtoUncertainCache { + ctrlInstance.uncertainPVCs[pvcNamespace+"/"+pvcName] = *test.pvc + } + // Action pvc, pv, err, modifyCalled := ctrlInstance.modify(test.pvc, test.pv) // Verify @@ -129,16 +189,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) @@ -153,10 +217,11 @@ 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{ + VolumeAttributesClassName: vacName, AccessModes: []v1.PersistentVolumeAccessMode{ v1.ReadWriteOnce, v1.ReadOnlyMany, @@ -166,18 +231,17 @@ 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, + CurrentVolumeAttributesClassName: curVacName, + Phase: v1.ClaimBound, Capacity: v1.ResourceList{ v1.ResourceStorage: resource.MustParse("2Gi"), }, - CurrentVolumeAttributesClassName: &curVacName, ModifyVolumeStatus: &v1.ModifyVolumeStatus{ TargetVolumeAttributesClassName: targetVacName, - Status: "", + Status: modifyVolumeStatus, }, }, } diff --git a/pkg/util/util.go b/pkg/util/util.go index 5bb0c10c..fe8c3683 100644 --- a/pkg/util/util.go +++ b/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 +} diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go index ebe91b8b..9ca06441 100644 --- a/pkg/util/util_test.go +++ b/pkg/util/util_test.go @@ -169,3 +169,91 @@ func TestMergeModifyVolumeConditionsOfPVC(t *testing.T) { }) } } + +func TestIsVacRolledBack(t *testing.T) { + emptyString := "" + originalVacName := "original-vac" + targetVacName := "test" + noRollbackVacName := "no-rollback-vac" + tests := []struct { + name string + pvc *v1.PersistentVolumeClaim + expectedOutput bool + }{ + { + name: "rollback to nil", + pvc: &v1.PersistentVolumeClaim{ + Spec: v1.PersistentVolumeClaimSpec{ + VolumeAttributesClassName: nil, + }, + Status: v1.PersistentVolumeClaimStatus{ + CurrentVolumeAttributesClassName: nil, + ModifyVolumeStatus: &v1.ModifyVolumeStatus{ + Status: v1.PersistentVolumeClaimModifyVolumeInfeasible, + TargetVolumeAttributesClassName: targetVacName, + }, + }, + }, + expectedOutput: true, + }, + { + name: "rollback to empty string", + pvc: &v1.PersistentVolumeClaim{ + Spec: v1.PersistentVolumeClaimSpec{ + VolumeAttributesClassName: &emptyString, + }, + Status: v1.PersistentVolumeClaimStatus{ + CurrentVolumeAttributesClassName: nil, + ModifyVolumeStatus: &v1.ModifyVolumeStatus{ + Status: v1.PersistentVolumeClaimModifyVolumeInfeasible, + TargetVolumeAttributesClassName: targetVacName, + }, + }, + }, + expectedOutput: true, + }, + { + name: "rollback from VAC B to VAC A", + pvc: &v1.PersistentVolumeClaim{ + Spec: v1.PersistentVolumeClaimSpec{ + VolumeAttributesClassName: &originalVacName, + }, + Status: v1.PersistentVolumeClaimStatus{ + CurrentVolumeAttributesClassName: &originalVacName, + ModifyVolumeStatus: &v1.ModifyVolumeStatus{ + Status: v1.PersistentVolumeClaimModifyVolumeInfeasible, + TargetVolumeAttributesClassName: targetVacName, + }, + }, + }, + expectedOutput: true, + }, + { + name: "no rollback", + pvc: &v1.PersistentVolumeClaim{ + Spec: v1.PersistentVolumeClaimSpec{ + VolumeAttributesClassName: &noRollbackVacName, + }, + Status: v1.PersistentVolumeClaimStatus{ + CurrentVolumeAttributesClassName: &originalVacName, + ModifyVolumeStatus: &v1.ModifyVolumeStatus{ + Status: v1.PersistentVolumeClaimModifyVolumeInfeasible, + TargetVolumeAttributesClassName: targetVacName, + }, + }, + }, + expectedOutput: false, + }, + } + + for _, test := range tests { + tc := test + t.Run(tc.name, func(t *testing.T) { + actualOutput := IsVacRolledBack(tc.pvc) + if actualOutput != tc.expectedOutput { + t.Errorf("expected output %+v got %+v", tc.expectedOutput, actualOutput) + } + }) + } + +}