diff --git a/go.mod b/go.mod index 8c6e50078..e547a2486 100644 --- a/go.mod +++ b/go.mod @@ -6,8 +6,6 @@ require ( github.com/container-storage-interface/spec v1.11.0 github.com/google/go-cmp v0.7.0 github.com/kubernetes-csi/csi-lib-utils v0.22.0 - golang.org/x/oauth2 v0.27.0 // indirect - golang.org/x/term v0.31.0 // indirect google.golang.org/grpc v1.72.1 k8s.io/api v0.34.0 k8s.io/apimachinery v0.34.0 @@ -16,10 +14,9 @@ require ( k8s.io/component-base v0.34.0 k8s.io/csi-translation-lib v0.34.0 k8s.io/klog/v2 v2.130.1 + k8s.io/utils v0.0.0-20250604170112-4c0f3b243397 ) -require k8s.io/utils v0.0.0-20250604170112-4c0f3b243397 - require ( cel.dev/expr v0.24.0 // indirect github.com/NYTimes/gziphandler v1.1.1 // indirect @@ -87,8 +84,10 @@ require ( golang.org/x/crypto v0.37.0 // indirect golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 // indirect golang.org/x/net v0.39.0 // indirect + golang.org/x/oauth2 v0.27.0 // indirect golang.org/x/sync v0.13.0 // indirect golang.org/x/sys v0.32.0 // indirect + golang.org/x/term v0.31.0 // indirect golang.org/x/text v0.24.0 // indirect golang.org/x/time v0.9.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20250303144028-a0af3efb3deb // indirect diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 60afed470..6e52627d9 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -293,7 +293,7 @@ func (ctrl *resizeController) Run(workers int, ctx context.Context, wg *sync.Wai } if utilfeature.DefaultFeatureGate.Enabled(features.ReleaseLeaderElectionOnExit) { - for i := 0; i < workers; i++ { + for range workers { wg.Add(1) go func() { defer wg.Done() @@ -301,7 +301,7 @@ func (ctrl *resizeController) Run(workers int, ctx context.Context, wg *sync.Wai }() } } else { - for i := 0; i < workers; i++ { + for range workers { go wait.Until(ctrl.syncPVCs, 0, stopCh) } } diff --git a/pkg/controller/resize_status_test.go b/pkg/controller/resize_status_test.go index 67afc6844..a3ba48092 100644 --- a/pkg/controller/resize_status_test.go +++ b/pkg/controller/resize_status_test.go @@ -30,16 +30,16 @@ func TestResizeFunctions(t *testing.T) { }{ { name: "mark fs resize, with no other conditions", - pvc: basePVC.Get(), - expectedPVC: basePVC.WithStorageResourceStatus(v1.PersistentVolumeClaimNodeResizePending).Get(), + pvc: basePVC().Get(), + expectedPVC: basePVC().WithStorageResourceStatus(v1.PersistentVolumeClaimNodeResizePending).Get(), testFunc: func(pvc *v1.PersistentVolumeClaim, ctrl *resizeController, size resource.Quantity) (*v1.PersistentVolumeClaim, error) { return ctrl.markForPendingNodeExpansion(pvc) }, }, { name: "mark fs resize, when other resource statuses are present", - pvc: basePVC.WithResourceStatus(v1.ResourceCPU, v1.PersistentVolumeClaimControllerResizeInfeasible).Get(), - expectedPVC: basePVC.WithResourceStatus(v1.ResourceCPU, v1.PersistentVolumeClaimControllerResizeInfeasible). + pvc: basePVC().WithResourceStatus(v1.ResourceCPU, v1.PersistentVolumeClaimControllerResizeInfeasible).Get(), + expectedPVC: basePVC().WithResourceStatus(v1.ResourceCPU, v1.PersistentVolumeClaimControllerResizeInfeasible). WithStorageResourceStatus(v1.PersistentVolumeClaimNodeResizePending).Get(), testFunc: func(pvc *v1.PersistentVolumeClaim, ctrl *resizeController, _ resource.Quantity) (*v1.PersistentVolumeClaim, error) { return ctrl.markForPendingNodeExpansion(pvc) @@ -47,25 +47,25 @@ func TestResizeFunctions(t *testing.T) { }, { name: "mark controller resize in-progress", - pvc: basePVC.Get(), - expectedPVC: basePVC.WithStorageResourceStatus(v1.PersistentVolumeClaimControllerResizeInProgress).Get(), + pvc: basePVC().Get(), + expectedPVC: basePVC().WithStorageResourceStatus(v1.PersistentVolumeClaimControllerResizeInProgress).Get(), testFunc: func(pvc *v1.PersistentVolumeClaim, ctrl *resizeController, q resource.Quantity) (*v1.PersistentVolumeClaim, error) { return ctrl.markControllerResizeInProgress(pvc, q, true) }, }, { name: "mark controller resize failed", - pvc: basePVC.Get(), - expectedPVC: basePVC.WithStorageResourceStatus(v1.PersistentVolumeClaimControllerResizeInfeasible).Get(), + pvc: basePVC().Get(), + expectedPVC: basePVC().WithStorageResourceStatus(v1.PersistentVolumeClaimControllerResizeInfeasible).Get(), testFunc: func(pvc *v1.PersistentVolumeClaim, ctrl *resizeController, q resource.Quantity) (*v1.PersistentVolumeClaim, error) { return ctrl.markControllerExpansionInfeasible(pvc, fmt.Errorf("things failed")) }, }, { name: "mark resize finished", - pvc: basePVC.WithResourceStatus(v1.ResourceCPU, v1.PersistentVolumeClaimControllerResizeInfeasible). + pvc: basePVC().WithResourceStatus(v1.ResourceCPU, v1.PersistentVolumeClaimControllerResizeInfeasible). WithStorageResourceStatus(v1.PersistentVolumeClaimNodeResizePending).Get(), - expectedPVC: basePVC.WithResourceStatus(v1.ResourceCPU, v1.PersistentVolumeClaimControllerResizeInfeasible). + expectedPVC: basePVC().WithResourceStatus(v1.ResourceCPU, v1.PersistentVolumeClaimControllerResizeInfeasible). WithStorageResourceStatus("").Get(), testFunc: func(pvc *v1.PersistentVolumeClaim, ctrl *resizeController, q resource.Quantity) (*v1.PersistentVolumeClaim, error) { return ctrl.markOverallExpansionAsFinished(pvc, q) diff --git a/pkg/modifycontroller/controller.go b/pkg/modifycontroller/controller.go index 68f386d5e..d86c8d3a9 100644 --- a/pkg/modifycontroller/controller.go +++ b/pkg/modifycontroller/controller.go @@ -28,6 +28,7 @@ import ( "github.com/kubernetes-csi/csi-lib-utils/slowset" "github.com/kubernetes-csi/external-resizer/pkg/modifier" v1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/wait" utilfeature "k8s.io/apiserver/pkg/util/feature" @@ -41,6 +42,7 @@ import ( "k8s.io/client-go/tools/record" "k8s.io/client-go/util/workqueue" "k8s.io/klog/v2" + "k8s.io/utils/ptr" ) // ModifyController watches PVCs and checks if they are requesting an modify operation. @@ -63,8 +65,11 @@ type modifyController struct { vacLister storagev1listers.VolumeAttributesClassLister vacListerSynced cache.InformerSynced extraModifyMetadata bool - // the key of the map is {PVC_NAMESPACE}/{PVC_NAME} - uncertainPVCs map[string]v1.PersistentVolumeClaim + // uncertainPVCs tracks PVCs that failed with non-final errors. + // We must not change the target when retrying. + // All in-progress PVCs are added here on initialization. + // The key of the map is {PVC_NAMESPACE}/{PVC_NAME}, value is not important now. + uncertainPVCs sync.Map // slowSet tracks PVCs for which modification failed with infeasible error and should be retried at slower rate. slowSet *slowset.SlowSet } @@ -124,19 +129,18 @@ func NewModifyController( } func (ctrl *modifyController) initUncertainPVCs() error { - ctrl.uncertainPVCs = make(map[string]v1.PersistentVolumeClaim) allPVCs, err := ctrl.pvcLister.List(labels.Everything()) if err != nil { klog.Errorf("Failed to list pvcs when init uncertain pvcs: %v", err) return err } for _, pvc := range allPVCs { - if pvc.Status.ModifyVolumeStatus != nil && (pvc.Status.ModifyVolumeStatus.Status == v1.PersistentVolumeClaimModifyVolumeInProgress || pvc.Status.ModifyVolumeStatus.Status == v1.PersistentVolumeClaimModifyVolumeInfeasible) { + if pvc.Status.ModifyVolumeStatus != nil && (pvc.Status.ModifyVolumeStatus.Status == v1.PersistentVolumeClaimModifyVolumeInProgress) { pvcKey, err := cache.MetaNamespaceKeyFunc(pvc) if err != nil { return err } - ctrl.uncertainPVCs[pvcKey] = *pvc.DeepCopy() + ctrl.uncertainPVCs.Store(pvcKey, pvc) } } @@ -163,18 +167,18 @@ func (ctrl *modifyController) updatePVC(oldObj, newObj interface{}) { } // Only trigger modify volume if the following conditions are met - // 1. Non empty vac name - // 2. oldVacName != newVacName - // 3. PVC is in Bound state - oldVacName := oldPVC.Spec.VolumeAttributesClassName - newVacName := newPVC.Spec.VolumeAttributesClassName - if newVacName != nil && *newVacName != "" && (oldVacName == nil || *newVacName != *oldVacName) && oldPVC.Status.Phase == v1.ClaimBound { + // 1. VAC changed or modify finished (check pending modify request while we are modifying) + // 2. PVC is in Bound state + oldVacName := ptr.Deref(oldPVC.Spec.VolumeAttributesClassName, "") + newVacName := ptr.Deref(newPVC.Spec.VolumeAttributesClassName, "") + if (newVacName != oldVacName || newPVC.Status.ModifyVolumeStatus == nil) && newPVC.Status.Phase == v1.ClaimBound { _, err := ctrl.pvLister.Get(oldPVC.Spec.VolumeName) if err != nil { klog.Errorf("Get PV %q of pvc %q in PVInformer cache failed: %v", oldPVC.Spec.VolumeName, klog.KObj(oldPVC), err) return } // Handle modify volume by adding to the claimQueue to avoid race conditions + klog.V(4).InfoS("Enqueueing PVC for modify", "PVC", klog.KObj(newPVC)) ctrl.addPVC(newObj) } else { klog.V(4).InfoS("No need to modify PVC", "PVC", klog.KObj(newPVC)) @@ -190,10 +194,7 @@ func (ctrl *modifyController) deletePVC(obj interface{}) { } func (ctrl *modifyController) init(ctx context.Context) bool { - informersSyncd := []cache.InformerSynced{ctrl.pvListerSynced, ctrl.pvcListerSynced} - informersSyncd = append(informersSyncd, ctrl.vacListerSynced) - - if !cache.WaitForCacheSync(ctx.Done(), informersSyncd...) { + if !cache.WaitForCacheSync(ctx.Done(), ctrl.pvListerSynced, ctrl.pvcListerSynced, ctrl.vacListerSynced) { klog.ErrorS(nil, "Cannot sync pod, pv, pvc or vac caches") return false } @@ -224,7 +225,7 @@ func (ctrl *modifyController) Run( go ctrl.slowSet.Run(stopCh) if utilfeature.DefaultFeatureGate.Enabled(features.ReleaseLeaderElectionOnExit) { - for i := 0; i < workers; i++ { + for range workers { wg.Add(1) go func() { defer wg.Done() @@ -232,7 +233,7 @@ func (ctrl *modifyController) Run( }() } } else { - for i := 0; i < workers; i++ { + for range workers { go wait.Until(ctrl.sync, 0, stopCh) } } @@ -268,6 +269,10 @@ func (ctrl *modifyController) syncPVC(key string) error { pvc, err := ctrl.pvcLister.PersistentVolumeClaims(namespace).Get(name) if err != nil { + if apierrors.IsNotFound(err) { + klog.V(3).InfoS("PVC is deleted or does not exist", "PVC", klog.KRef(namespace, name)) + return nil + } return fmt.Errorf("getting PVC %s/%s failed: %v", namespace, name, err) } @@ -283,15 +288,13 @@ func (ctrl *modifyController) syncPVC(key string) error { // Only trigger modify volume if the following conditions are met // 1. PV provisioned by CSI driver AND driver name matches local driver - // 2. Non-empty vac name - // 3. PVC is in Bound state + // 2. PVC is in Bound state if pv.Spec.CSI == nil || pv.Spec.CSI.Driver != ctrl.name { klog.V(7).InfoS("Skipping PV provisioned by different driver", "PV", klog.KObj(pv)) return nil } - vacName := pvc.Spec.VolumeAttributesClassName - if vacName != nil && *vacName != "" && pvc.Status.Phase == v1.ClaimBound { + if pvc.Status.Phase == v1.ClaimBound { _, _, 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 94067d92c..08c1b17b7 100644 --- a/pkg/modifycontroller/controller_test.go +++ b/pkg/modifycontroller/controller_test.go @@ -1,9 +1,9 @@ package modifycontroller import ( - "context" "errors" "fmt" + "sync" "testing" "time" @@ -14,7 +14,6 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/status" v1 "k8s.io/api/core/v1" - storagev1 "k8s.io/api/storage/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" utilfeature "k8s.io/apiserver/pkg/util/feature" @@ -24,13 +23,13 @@ import ( ) func TestController(t *testing.T) { - basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/) + basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, "" /*targetVacName*/) + basePVC.Status.ModifyVolumeStatus = nil basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac) firstTimePV := basePV.DeepCopy() firstTimePV.Spec.VolumeAttributesClassName = nil firstTimePVC := basePVC.DeepCopy() firstTimePVC.Status.CurrentVolumeAttributesClassName = nil - firstTimePVC.Status.ModifyVolumeStatus = nil tests := []struct { name string @@ -152,6 +151,9 @@ func TestSyncPVC(t *testing.T) { pvcWithUncreatedPV := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/) pvcWithUncreatedPV.Spec.VolumeName = "" + inprogressPVC := createTestPVC(pvcName, "" /*vacName*/, "" /*curVacName*/, testVac /*targetVacName*/) + inprogressPVC.Status.ModifyVolumeStatus.Status = v1.PersistentVolumeClaimModifyVolumeInProgress + nonCSIPVC := &v1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{Name: pvcName, Namespace: pvcNamespace}, Spec: v1.PersistentVolumeClaimSpec{ @@ -183,6 +185,12 @@ func TestSyncPVC(t *testing.T) { pv: basePV, callCSIModify: true, }, + { + name: "Should NOT modify when rollback to empty VACName", + pvc: createTestPVC(pvcName, "" /*vacName*/, "" /*curVacName*/, testVac /*targetVacName*/), + pv: basePV, + callCSIModify: false, + }, { name: "Should NOT modify if PVC managed by another CSI Driver", pvc: basePVC, @@ -190,9 +198,14 @@ func TestSyncPVC(t *testing.T) { callCSIModify: false, }, { - name: "Should NOT modify if PVC has empty Spec.VACName", - pvc: createTestPVC(pvcName, "" /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/), + name: "Should execute ModifyVolume for InProgress target if PVC has empty Spec.VACName", + pvc: inprogressPVC, pv: basePV, + callCSIModify: true, + }, + { + name: "Should NOT modify if PVC deleted", + pvc: nil, callCSIModify: false, }, { @@ -219,7 +232,13 @@ func TestSyncPVC(t *testing.T) { t.Run(test.name, func(t *testing.T) { client := csi.NewMockClient(testDriverName, true, true, true, true, true) - initialObjects := []runtime.Object{test.pvc, test.pv, testVacObject, targetVacObject} + initialObjects := []runtime.Object{testVacObject, targetVacObject} + if test.pvc != nil { + initialObjects = append(initialObjects, test.pvc) + } + if test.pv != nil { + initialObjects = append(initialObjects, test.pv) + } ctrlInstance := setupFakeK8sEnvironment(t, client, initialObjects) err := ctrlInstance.syncPVC(pvcNamespace + "/" + pvcName) @@ -321,6 +340,67 @@ func TestInfeasibleRetry(t *testing.T) { } } +// Intended to catch any race conditions in the controller +func TestConcurrentSync(t *testing.T) { + cases := []struct { + name string + waitCount int + err error + }{ + // TODO: This case is flaky due to fake client lacks resourceVersion support. + // { + // name: "success", + // waitCount: 10, + // }, + { + name: "uncertain", + waitCount: 30, + err: nonFinalErr, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + client := csi.NewMockClient(testDriverName, true, true, true, true, true) + client.SetModifyError(tc.err) + + initialObjects := []runtime.Object{testVacObject, targetVacObject} + for i := range 10 { + initialObjects = append(initialObjects, + &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("foo-%d", i), Namespace: pvcNamespace}, + Spec: v1.PersistentVolumeClaimSpec{ + VolumeAttributesClassName: &testVac, + VolumeName: fmt.Sprintf("testPV-%d", i), + }, + Status: v1.PersistentVolumeClaimStatus{ + Phase: v1.ClaimBound, + }, + }, + &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("testPV-%d", i)}, + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{ + Driver: testDriverName, + VolumeHandle: fmt.Sprintf("foo-%d", i), + }, + }, + }, + }, + ) + } + ctrlInstance := setupFakeK8sEnvironment(t, client, initialObjects) + wg := sync.WaitGroup{} + t.Cleanup(wg.Wait) + go ctrlInstance.Run(3, t.Context(), &wg) + + for client.GetModifyCount() < tc.waitCount { + time.Sleep(20 * time.Millisecond) + } + }) + } +} + // setupFakeK8sEnvironment creates fake K8s environment and starts Informers and ModifyController func setupFakeK8sEnvironment(t *testing.T, client *csi.MockClient, initialObjects []runtime.Object) *modifyController { t.Helper() @@ -329,11 +409,9 @@ func setupFakeK8sEnvironment(t *testing.T, client *csi.MockClient, initialObject /* Create fake kubeClient, Informers, and ModifyController */ kubeClient, informerFactory := fakeK8s(initialObjects) - pvInformer := informerFactory.Core().V1().PersistentVolumes() - pvcInformer := informerFactory.Core().V1().PersistentVolumeClaims() - vacInformer := informerFactory.Storage().V1().VolumeAttributesClasses() - driverName, _ := client.GetDriverName(context.TODO()) + ctx := t.Context() + driverName, _ := client.GetDriverName(ctx) csiModifier, err := modifier.NewModifierFromClient(client, 15*time.Second, kubeClient, informerFactory, false, driverName) if err != nil { @@ -346,26 +424,10 @@ func setupFakeK8sEnvironment(t *testing.T, client *csi.MockClient, initialObject workqueue.DefaultTypedControllerRateLimiter[string]()) /* Start informers and ModifyController*/ - stopCh := make(chan struct{}) - informerFactory.Start(stopCh) - - go controller.Run(1, t.Context(), nil) - - /* Add initial objects to informer caches */ - for _, obj := range initialObjects { - switch obj.(type) { - case *v1.PersistentVolume: - pvInformer.Informer().GetStore().Add(obj) - case *v1.PersistentVolumeClaim: - pvcInformer.Informer().GetStore().Add(obj) - case *storagev1.VolumeAttributesClass: - vacInformer.Informer().GetStore().Add(obj) - default: - t.Fatalf("Test %s: Unknown initalObject type: %+v", t.Name(), obj) - } - } + informerFactory.Start(ctx.Done()) - ctrlInstance, _ := controller.(*modifyController) + ctrlInstance := controller.(*modifyController) + ctrlInstance.init(ctx) return ctrlInstance } diff --git a/pkg/modifycontroller/modify_status.go b/pkg/modifycontroller/modify_status.go index eb7c70320..c078b6482 100644 --- a/pkg/modifycontroller/modify_status.go +++ b/pkg/modifycontroller/modify_status.go @@ -18,11 +18,12 @@ package modifycontroller import ( "fmt" + "slices" "github.com/kubernetes-csi/external-resizer/pkg/util" + "google.golang.org/grpc/status" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/tools/cache" "k8s.io/utils/ptr" ) @@ -33,67 +34,49 @@ func (ctrl *modifyController) markControllerModifyVolumeStatus( err error) (*v1.PersistentVolumeClaim, error) { newPVC := pvc.DeepCopy() - newPVC.Status.ModifyVolumeStatus = &v1.ModifyVolumeStatus{ - Status: modifyVolumeStatus, - TargetVolumeAttributesClassName: ptr.Deref(pvc.Spec.VolumeAttributesClassName, ""), + if newPVC.Status.ModifyVolumeStatus == nil { + newPVC.Status.ModifyVolumeStatus = &v1.ModifyVolumeStatus{} } - // Update PVC's Condition to indicate modification - pvcCondition := v1.PersistentVolumeClaimCondition{ - Type: v1.PersistentVolumeClaimVolumeModifyingVolume, - Status: v1.ConditionTrue, - LastTransitionTime: metav1.Now(), - } - conditionMessage := "" - switch modifyVolumeStatus { - case v1.PersistentVolumeClaimModifyVolumeInProgress: - conditionMessage = "ModifyVolume operation in progress." - case v1.PersistentVolumeClaimModifyVolumeInfeasible: - conditionMessage = "ModifyVolume failed with error" + err.Error() + ". Waiting for retry." - } - pvcCondition.Message = conditionMessage + newPVC.Status.ModifyVolumeStatus.Status = modifyVolumeStatus + // Do not change conditions for pending modifications and keep existing conditions if modifyVolumeStatus != v1.PersistentVolumeClaimModifyVolumePending { - newPVC.Status.Conditions = util.MergeModifyVolumeConditionsOfPVC(newPVC.Status.Conditions, - []v1.PersistentVolumeClaimCondition{pvcCondition}) + now := metav1.Now() + conditions := []v1.PersistentVolumeClaimCondition{{ + Type: v1.PersistentVolumeClaimVolumeModifyingVolume, + Status: v1.ConditionTrue, + LastProbeTime: now, + }} + modifying := &conditions[0] + + targetVAC := newPVC.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName + if err == nil { + targetVAC := ptr.Deref(pvc.Spec.VolumeAttributesClassName, "") + newPVC.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName = targetVAC + modifying.Message = fmt.Sprintf("Modifying volume to %q is in progress.", targetVAC) + } else { + if util.IsFinalError(err) { + modifying.Message = fmt.Sprintf("Modifying volume to %q failed. Waiting for retry.", targetVAC) + } else { + modifying.Message = fmt.Sprintf("Modifying volume to %q is still in progress.", targetVAC) + } + + grpcStatus, _ := status.FromError(err) + conditions = append(conditions, v1.PersistentVolumeClaimCondition{ + Type: v1.PersistentVolumeClaimVolumeModifyVolumeError, + Status: v1.ConditionTrue, + Reason: grpcStatus.Code().String(), + Message: grpcStatus.Message(), + LastProbeTime: now, + }) + } + newPVC.Status.Conditions = util.MergePVCConditions(newPVC.Status.Conditions, conditions) } updatedPVC, err := util.PatchClaim(ctrl.kubeClient, pvc, newPVC, true /* addResourceVersionCheck */) 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) { - newPVC := pvc.DeepCopy() - pvcCondition := v1.PersistentVolumeClaimCondition{ - Type: v1.PersistentVolumeClaimVolumeModifyVolumeError, - Status: v1.ConditionTrue, - LastTransitionTime: metav1.Now(), - Message: "ModifyVolume failed with error. Waiting for retry.", - } - - if err != nil { - pvcCondition.Message = "ModifyVolume failed with error: " + err.Error() + ". Waiting for retry." - } - - newPVC.Status.Conditions = util.MergeModifyVolumeConditionsOfPVC(newPVC.Status.Conditions, - []v1.PersistentVolumeClaimCondition{pvcCondition}) - - 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 updatedPVC, nil } @@ -135,24 +118,20 @@ func (ctrl *modifyController) markControllerModifyVolumeCompleted(pvc *v1.Persis // markControllerModifyVolumeStatus clears all the conditions related to modify volume and only // leave other condition types func clearModifyVolumeConditions(conditions []v1.PersistentVolumeClaimCondition) []v1.PersistentVolumeClaimCondition { - knownConditions := []v1.PersistentVolumeClaimCondition{} - for _, value := range conditions { - // Only keep conditions that are not related to modify volume - if value.Type != v1.PersistentVolumeClaimVolumeModifyVolumeError && value.Type != v1.PersistentVolumeClaimVolumeModifyingVolume { - knownConditions = append(knownConditions, value) - } - } - return knownConditions + return slices.DeleteFunc(conditions, func(c v1.PersistentVolumeClaimCondition) bool { + return c.Type == v1.PersistentVolumeClaimVolumeModifyVolumeError || c.Type == v1.PersistentVolumeClaimVolumeModifyingVolume + }) } -// removePVCFromModifyVolumeUncertainCache removes the pvc from the uncertain cache -func (ctrl *modifyController) removePVCFromModifyVolumeUncertainCache(pvcKey string) { - if ctrl.uncertainPVCs == nil { - return - } - // Format of the key of the uncertainPVCs is NAMESPACE/NAME of the pvc - _, ok := ctrl.uncertainPVCs[pvcKey] - if ok { - delete(ctrl.uncertainPVCs, pvcKey) +// markRolledBack will clear the modifying conditions +func (ctrl *modifyController) markRolledBack(pvc *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaim, error) { + newPVC := pvc.DeepCopy() + newPVC.Status.Conditions = slices.DeleteFunc(newPVC.Status.Conditions, func(condition v1.PersistentVolumeClaimCondition) bool { + return condition.Type == v1.PersistentVolumeClaimVolumeModifyingVolume + }) + newPVC, err := util.PatchClaim(ctrl.kubeClient, pvc, newPVC, false /* addResourceVersionCheck */) + if err != nil { + return nil, fmt.Errorf("mark PVC %q as rolled back failed: %v", pvc.Name, err) } + return newPVC, nil } diff --git a/pkg/modifycontroller/modify_status_test.go b/pkg/modifycontroller/modify_status_test.go index c9e8b6ad0..4181882fc 100644 --- a/pkg/modifycontroller/modify_status_test.go +++ b/pkg/modifycontroller/modify_status_test.go @@ -14,13 +14,11 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/status" v1 "k8s.io/api/core/v1" - storagev1 "k8s.io/api/storage/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" utilfeature "k8s.io/apiserver/pkg/util/feature" - "k8s.io/client-go/tools/cache" "k8s.io/client-go/util/workqueue" featuregatetesting "k8s.io/component-base/featuregate/testing" ) @@ -38,22 +36,53 @@ var ( testDriverName = "mock" infeasibleErr = status.Errorf(codes.InvalidArgument, "Parameters in VolumeAttributesClass is invalid") finalErr = status.Errorf(codes.Internal, "Final error") - pvcConditionInProgress = v1.PersistentVolumeClaimCondition{ + nonFinalErr = status.Errorf(codes.Aborted, "Non-final error") + pvcConditionInProgress = []v1.PersistentVolumeClaimCondition{{ Type: v1.PersistentVolumeClaimVolumeModifyingVolume, Status: v1.ConditionTrue, - Message: "ModifyVolume operation in progress.", + Message: "Modifying volume to \"target-vac\" is in progress.", + }} + + pvcConditionInfeasible = []v1.PersistentVolumeClaimCondition{ + { + Type: v1.PersistentVolumeClaimVolumeModifyingVolume, + Status: v1.ConditionTrue, + Message: "Modifying volume to \"target-vac\" failed. Waiting for retry.", + }, + { + Type: v1.PersistentVolumeClaimVolumeModifyVolumeError, + Status: v1.ConditionTrue, + Reason: "InvalidArgument", + Message: "Parameters in VolumeAttributesClass is invalid", + }, } - pvcConditionInfeasible = v1.PersistentVolumeClaimCondition{ - Type: v1.PersistentVolumeClaimVolumeModifyingVolume, - Status: v1.ConditionTrue, - Message: "ModifyVolume failed with errorrpc error: code = InvalidArgument desc = Parameters in VolumeAttributesClass is invalid. Waiting for retry.", + pvcConditionUncertain = []v1.PersistentVolumeClaimCondition{ + { + Type: v1.PersistentVolumeClaimVolumeModifyingVolume, + Status: v1.ConditionTrue, + Message: "Modifying volume to \"target-vac\" is still in progress.", + }, + { + Type: v1.PersistentVolumeClaimVolumeModifyVolumeError, + Status: v1.ConditionTrue, + Reason: "Aborted", + Message: "Non-final error", + }, } - pvcConditionError = v1.PersistentVolumeClaimCondition{ - Type: v1.PersistentVolumeClaimVolumeModifyVolumeError, - Status: v1.ConditionTrue, - Message: "ModifyVolume failed with error. Waiting for retry.", + pvcConditionError = []v1.PersistentVolumeClaimCondition{ + { + Type: v1.PersistentVolumeClaimVolumeModifyingVolume, + Status: v1.ConditionTrue, + Message: "Modifying volume to \"target-vac\" failed. Waiting for retry.", + }, + { + Type: v1.PersistentVolumeClaimVolumeModifyVolumeError, + Status: v1.ConditionTrue, + Reason: "Internal", + Message: "Final error", + }, } ) @@ -70,19 +99,39 @@ func TestMarkControllerModifyVolumeStatus(t *testing.T) { }{ { name: "mark modify volume as in progress", - pvc: basePVC.Get(), - expectedPVC: basePVC.WithModifyVolumeStatus(v1.PersistentVolumeClaimModifyVolumeInProgress).Get(), - expectedConditions: []v1.PersistentVolumeClaimCondition{pvcConditionInProgress}, + pvc: basePVC().Get(), + expectedPVC: basePVC().WithModifyVolumeStatus(v1.PersistentVolumeClaimModifyVolumeInProgress).Get(), + expectedConditions: pvcConditionInProgress, expectedErr: nil, testFunc: func(pvc *v1.PersistentVolumeClaim, ctrl *modifyController) (*v1.PersistentVolumeClaim, error) { return ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumeInProgress, nil) }, }, + { + name: "mark modify volume as failed", + pvc: basePVC().WithConditions(pvcConditionInProgress).Get(), + expectedPVC: basePVC().WithModifyVolumeStatus(v1.PersistentVolumeClaimModifyVolumeInProgress).Get(), + expectedConditions: pvcConditionError, + expectedErr: nil, + testFunc: func(pvc *v1.PersistentVolumeClaim, ctrl *modifyController) (*v1.PersistentVolumeClaim, error) { + return ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumeInProgress, finalErr) + }, + }, + { + name: "mark modify volume as uncertain", + pvc: basePVC().WithConditions(pvcConditionInProgress).Get(), + expectedPVC: basePVC().WithModifyVolumeStatus(v1.PersistentVolumeClaimModifyVolumeInProgress).Get(), + expectedConditions: pvcConditionUncertain, + expectedErr: nil, + testFunc: func(pvc *v1.PersistentVolumeClaim, ctrl *modifyController) (*v1.PersistentVolumeClaim, error) { + return ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumeInProgress, nonFinalErr) + }, + }, { name: "mark modify volume as infeasible", - pvc: basePVC.Get(), - expectedPVC: basePVC.WithModifyVolumeStatus(v1.PersistentVolumeClaimModifyVolumeInfeasible).Get(), - expectedConditions: []v1.PersistentVolumeClaimCondition{pvcConditionInfeasible}, + pvc: basePVC().WithConditions(pvcConditionInProgress).Get(), + expectedPVC: basePVC().WithModifyVolumeStatus(v1.PersistentVolumeClaimModifyVolumeInfeasible).Get(), + expectedConditions: pvcConditionInfeasible, expectedErr: infeasibleErr, testFunc: func(pvc *v1.PersistentVolumeClaim, ctrl *modifyController) (*v1.PersistentVolumeClaim, error) { return ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumeInfeasible, infeasibleErr) @@ -90,9 +139,9 @@ func TestMarkControllerModifyVolumeStatus(t *testing.T) { }, { name: "mark modify volume as pending", - pvc: basePVC.Get(), - expectedPVC: basePVC.WithModifyVolumeStatus(v1.PersistentVolumeClaimModifyVolumePending).Get(), - expectedConditions: nil, + pvc: basePVC().WithConditions(pvcConditionInfeasible).Get(), + expectedPVC: basePVC().WithModifyVolumeStatus(v1.PersistentVolumeClaimModifyVolumePending).Get(), + expectedConditions: pvcConditionInfeasible, // not touched expectedErr: nil, testFunc: func(pvc *v1.PersistentVolumeClaim, ctrl *modifyController) (*v1.PersistentVolumeClaim, error) { return ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumePending, nil) @@ -144,65 +193,12 @@ func TestMarkControllerModifyVolumeStatus(t *testing.T) { } } -func TestUpdateConditionBasedOnError(t *testing.T) { - basePVC := testutil.MakeTestPVC([]v1.PersistentVolumeClaimCondition{}) - - tests := []struct { - name string - pvc *v1.PersistentVolumeClaim - expectedConditions []v1.PersistentVolumeClaimCondition - expectedErr error - }{ - { - name: "update condition based on error", - pvc: basePVC.Get(), - expectedConditions: []v1.PersistentVolumeClaimCondition{pvcConditionError}, - }, - } - - for _, test := range tests { - tc := test - t.Run(tc.name, func(t *testing.T) { - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeAttributesClass, true) - client := csi.NewMockClient("foo", true, true, true, true, true) - driverName, _ := client.GetDriverName(context.TODO()) - - pvc := test.pvc - - var initialObjects []runtime.Object - initialObjects = append(initialObjects, test.pvc) - - kubeClient, informerFactory := fakeK8s(initialObjects) - - csiModifier, err := modifier.NewModifierFromClient(client, 15*time.Second, kubeClient, informerFactory, false, driverName) - if err != nil { - t.Fatalf("Test %s: Unable to create modifier: %v", test.name, err) - } - controller := NewModifyController(driverName, - csiModifier, kubeClient, - time.Second, 2*time.Minute, false, informerFactory, - workqueue.DefaultTypedControllerRateLimiter[string]()) - - ctrlInstance, _ := controller.(*modifyController) - - pvc, err = ctrlInstance.updateConditionBasedOnError(tc.pvc, err) - if err != nil && !reflect.DeepEqual(tc.expectedErr, err) { - t.Errorf("Expected error to be %v but got %v", tc.expectedErr, err) - } - - if !testutil.CompareConditions(pvc.Status.Conditions, tc.expectedConditions) { - t.Errorf("expected conditions %+v got %+v", tc.expectedConditions, pvc.Status.Conditions) - } - }) - } -} - func TestMarkControllerModifyVolumeCompleted(t *testing.T) { basePVC := testutil.MakeTestPVC([]v1.PersistentVolumeClaimCondition{}) basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac) expectedPV := basePV.DeepCopy() expectedPV.Spec.VolumeAttributesClassName = &targetVac - expectedPVC := basePVC.WithCurrentVolumeAttributesClassName(targetVac).Get() + expectedPVC := basePVC().WithCurrentVolumeAttributesClassName(targetVac).Get() expectedPVC.Status.ModifyVolumeStatus = nil tests := []struct { @@ -215,14 +211,14 @@ func TestMarkControllerModifyVolumeCompleted(t *testing.T) { }{ { name: "update modify volume status to completed", - pvc: basePVC.Get(), + pvc: basePVC().Get(), pv: basePV, expectedPVC: expectedPVC, expectedPV: expectedPV, }, { name: "update modify volume status to completed, and clear conditions", - pvc: basePVC.WithConditions([]v1.PersistentVolumeClaimCondition{pvcConditionInProgress}).Get(), + pvc: basePVC().WithConditions(pvcConditionInProgress).Get(), pv: basePV, expectedPVC: expectedPVC, expectedPV: expectedPV, @@ -273,110 +269,6 @@ func TestMarkControllerModifyVolumeCompleted(t *testing.T) { } } -func TestRemovePVCFromModifyVolumeUncertainCache(t *testing.T) { - basePVC := testutil.MakeTestPVC([]v1.PersistentVolumeClaimCondition{}) - basePVC.WithModifyVolumeStatus(v1.PersistentVolumeClaimModifyVolumeInProgress) - secondPVC := testutil.GetTestPVC("test-vol0", "2G", "1G", "", "") - secondPVC.Status.Phase = v1.ClaimBound - secondPVC.Status.ModifyVolumeStatus = &v1.ModifyVolumeStatus{} - secondPVC.Status.ModifyVolumeStatus.Status = v1.PersistentVolumeClaimModifyVolumeInfeasible - - tests := []struct { - name string - pvc *v1.PersistentVolumeClaim - }{ - { - name: "should delete the target pvc but keep the others in the cache", - pvc: basePVC.Get(), - }, - } - - for _, test := range tests { - tc := test - t.Run(tc.name, func(t *testing.T) { - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeAttributesClass, true) - client := csi.NewMockClient("foo", true, true, true, true, true) - driverName, _ := client.GetDriverName(context.TODO()) - - var initialObjects []runtime.Object - initialObjects = append(initialObjects, test.pvc) - initialObjects = append(initialObjects, secondPVC) - - kubeClient, informerFactory := fakeK8s(initialObjects) - pvInformer := informerFactory.Core().V1().PersistentVolumes() - pvcInformer := informerFactory.Core().V1().PersistentVolumeClaims() - podInformer := informerFactory.Core().V1().Pods() - vacInformer := informerFactory.Storage().V1().VolumeAttributesClasses() - - csiModifier, err := modifier.NewModifierFromClient(client, 15*time.Second, kubeClient, informerFactory, false, driverName) - if err != nil { - t.Fatalf("Test %s: Unable to create modifier: %v", test.name, err) - } - controller := NewModifyController(driverName, - csiModifier, kubeClient, - time.Second, 2*time.Minute, false, informerFactory, - workqueue.DefaultTypedControllerRateLimiter[string]()) - - ctrlInstance, _ := controller.(*modifyController) - - stopCh := make(chan struct{}) - informerFactory.Start(stopCh) - - success := ctrlInstance.init(t.Context()) - if !success { - t.Fatal("failed to init controller") - } - - for _, obj := range initialObjects { - switch obj.(type) { - case *v1.PersistentVolume: - pvInformer.Informer().GetStore().Add(obj) - case *v1.PersistentVolumeClaim: - pvcInformer.Informer().GetStore().Add(obj) - case *v1.Pod: - podInformer.Informer().GetStore().Add(obj) - case *storagev1.VolumeAttributesClass: - vacInformer.Informer().GetStore().Add(obj) - default: - t.Fatalf("Test %s: Unknown initalObject type: %+v", test.name, obj) - } - } - - time.Sleep(time.Second * 2) - - pvcKey, err := cache.MetaNamespaceKeyFunc(tc.pvc) - if err != nil { - t.Errorf("failed to extract pvc key from pvc %v", tc.pvc) - } - ctrlInstance.removePVCFromModifyVolumeUncertainCache(pvcKey) - - deletedPVCKey, err := cache.MetaNamespaceKeyFunc(tc.pvc) - if err != nil { - t.Errorf("failed to extract pvc key from pvc %v", tc.pvc) - } - _, ok := ctrlInstance.uncertainPVCs[deletedPVCKey] - if ok { - t.Errorf("pvc %v should be deleted but it is still in the uncertainPVCs cache", tc.pvc) - } - if err != nil { - t.Errorf("err get pvc %v from uncertainPVCs: %v", tc.pvc, err) - } - - notDeletedPVCKey, err := cache.MetaNamespaceKeyFunc(secondPVC) - if err != nil { - t.Errorf("failed to extract pvc key from secondPVC %v", secondPVC) - } - _, ok = ctrlInstance.uncertainPVCs[notDeletedPVCKey] - if !ok { - t.Errorf("pvc %v should not be deleted, uncertainPVCs list %v", secondPVC, ctrlInstance.uncertainPVCs) - } - if err != nil { - t.Errorf("err get pvc %v from uncertainPVCs: %v", secondPVC, err) - } - }) - } -} - func createTestPV(capacityGB int, pvcName, pvcNamespace string, pvcUID types.UID, volumeMode *v1.PersistentVolumeMode, vacName string) *v1.PersistentVolume { capacity := testutil.QuantityGB(capacityGB) diff --git a/pkg/modifycontroller/modify_volume.go b/pkg/modifycontroller/modify_volume.go index 455e0c022..df05ec596 100644 --- a/pkg/modifycontroller/modify_volume.go +++ b/pkg/modifycontroller/modify_volume.go @@ -19,6 +19,7 @@ package modifycontroller import ( "fmt" "maps" + "slices" "time" "github.com/kubernetes-csi/csi-lib-utils/slowset" @@ -30,6 +31,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/client-go/tools/cache" "k8s.io/klog/v2" + "k8s.io/utils/ptr" ) const ( @@ -40,8 +42,6 @@ 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 pvcKey, err := cache.MetaNamespaceKeyFunc(pvc) if err != nil { return pvc, pv, err, false @@ -53,29 +53,71 @@ 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 - return ctrl.validateVACAndModifyVolumeWithTarget(pvc, pv) - } else if pvcSpecVacName != nil && curVacName != nil && *pvcSpecVacName != *curVacName { - // 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) - if err != nil { - if apierrors.IsNotFound(err) { - 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) + pvcSpecVacName := ptr.Deref(pvc.Spec.VolumeAttributesClassName, "") + curVacName := ptr.Deref(pvc.Status.CurrentVolumeAttributesClassName, "") + status := pvc.Status.ModifyVolumeStatus + + if status == nil && pvcSpecVacName == curVacName { + // No modification required, already reached target state + return pvc, pv, nil, false + } + + // Last modification failed with non-infeasible error + inProgress := status != nil && status.Status == v1.PersistentVolumeClaimModifyVolumeInProgress + + if pvcSpecVacName == "" && !inProgress { + // User don't care the target state, and we've reached a relatively stable state. Just keep it here. + // Note: APIServer generally not allowing setting pvcSpecVacName to empty when curVacName is not empty. + klog.V(4).InfoS("stop reconcile for rolled back PVC", "PV", klog.KObj(pv)) + pvc, err := ctrl.rolledBack(pvc) + return pvc, pv, err, false + } + + inUncertainState := false + if inProgress { + _, inUncertainState = ctrl.uncertainPVCs.Load(pvcKey) + } else { + // we either see a stall PVC, or the status was updated externally. + // For stall PVC, we will get Conflict error when marking InProgress. + // For status updated externally, we respect the user's choice and try the new target, as if it were not uncertain. + // `ctrl.uncertainPVCs` will be updated after the next ControllerModifyVolume call. + } + // Check if we should change our target + if inUncertainState || pvcSpecVacName == "" { + // No. Continue our previous modification + vac, err := ctrl.getTargetVAC(pvc, status.TargetVolumeAttributesClassName) + if err != nil { + return pvc, pv, err, false } + return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac) + } + + return ctrl.validateVACAndModifyVolumeWithTarget(pvc, pv) +} + +func (ctrl *modifyController) rolledBack(pvc *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaim, error) { + if slices.ContainsFunc(pvc.Status.Conditions, func(condition v1.PersistentVolumeClaimCondition) bool { + return condition.Type == v1.PersistentVolumeClaimVolumeModifyingVolume + }) { + ctrl.eventRecorder.Eventf(pvc, v1.EventTypeNormal, util.VolumeModifyCancelled, "Cancelled modify.") + return ctrl.markRolledBack(pvc) } + // Don't try to revert Status.ModifyVolumeStatus here, because we only record the result of the last modification. + // We don't know what happened before. User can switch between InProgress/Infeasible/Pending status + // freely by modifying the spec. + return pvc, nil +} - // No modification required - return pvc, pv, nil, false +func (ctrl *modifyController) getTargetVAC(pvc *v1.PersistentVolumeClaim, vacName string) (*storagev1.VolumeAttributesClass, error) { + vac, err := ctrl.vacLister.Get(vacName) + // Check if pvcSpecVac is valid and exist + if err != nil { + if apierrors.IsNotFound(err) { + ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, "VAC %q does not exist.", vacName) + } + return nil, fmt.Errorf("get VAC with vac name %s in VACInformer cache failed: %w", vacName, err) + } + return vac, nil } // func validateVACAndModifyVolumeWithTarget validate the VAC. The function sets pvc.Status.ModifyVolumeStatus @@ -83,29 +125,23 @@ func (ctrl *modifyController) modify(pvc *v1.PersistentVolumeClaim, pv *v1.Persi func (ctrl *modifyController) validateVACAndModifyVolumeWithTarget( pvc *v1.PersistentVolumeClaim, pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) { - // The controller only triggers ModifyVolume if pvcSpecVacName is not nil nor empty - pvcSpecVacName := pvc.Spec.VolumeAttributesClassName - // Check if pvcSpecVac is valid and exist - vac, err := ctrl.vacLister.Get(*pvcSpecVacName) - if err == nil { - // Mark pvc.Status.ModifyVolumeStatus as in progress - pvc, err = ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumeInProgress, nil) - if err != nil { - return pvc, pv, err, false - } - // Record an event to indicate that external resizer is modifying this volume. - ctrl.eventRecorder.Event(pvc, v1.EventTypeNormal, util.VolumeModify, - fmt.Sprintf("external resizer is modifying volume %s with vac %s", pvc.Name, *pvcSpecVacName)) - return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac, pvcSpecVacName) - } else { - if apierrors.IsNotFound(err) { - ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, "VAC "+*pvcSpecVacName+" does not exist.") - } - klog.Errorf("Get VAC with vac name %s in VACInformer cache failed: %v", *pvcSpecVacName, err) + + vac, err := ctrl.getTargetVAC(pvc, *pvc.Spec.VolumeAttributesClassName) + if err != nil { // Mark pvc.Status.ModifyVolumeStatus as pending pvc, err = ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumePending, nil) return pvc, pv, err, false } + + // Mark pvc.Status.ModifyVolumeStatus as in progress + pvc, err = ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumeInProgress, nil) + if err != nil { + return pvc, pv, err, false + } + // Record an event to indicate that external resizer is modifying this volume. + ctrl.eventRecorder.Event(pvc, v1.EventTypeNormal, util.VolumeModify, + fmt.Sprintf("external resizer is modifying volume %s with vac %s", pvc.Name, vac.Name)) + return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac) } // func controllerModifyVolumeWithTarget trigger the CSI ControllerModifyVolume API call @@ -114,46 +150,46 @@ func (ctrl *modifyController) controllerModifyVolumeWithTarget( pvc *v1.PersistentVolumeClaim, pv *v1.PersistentVolume, vacObj *storagev1.VolumeAttributesClass, - pvcSpecVacName *string) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) { +) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) { var err error pvc, pv, err = ctrl.callModifyVolumeOnPlugin(pvc, pv, vacObj) if err == nil { - klog.V(4).Infof("Update volumeAttributesClass of PV %q to %s succeeded", pv.Name, *pvcSpecVacName) + klog.V(4).Infof("Update volumeAttributesClass of PV %q to %s succeeded", pv.Name, vacObj.Name) // Record an event to indicate that modify operation is successful. - ctrl.eventRecorder.Eventf(pvc, v1.EventTypeNormal, util.VolumeModifySuccess, fmt.Sprintf("external resizer modified volume %s with vac %s successfully ", pvc.Name, vacObj.Name)) + ctrl.eventRecorder.Eventf(pvc, v1.EventTypeNormal, util.VolumeModifySuccess, fmt.Sprintf("external resizer modified volume %s with vac %s successfully", pvc.Name, vacObj.Name)) return pvc, pv, nil, true } else { errStatus, ok := status.FromError(err) + errMsg := err.Error() if ok { - pvc, updateConditionErr := ctrl.updateConditionBasedOnError(pvc, err) - if updateConditionErr != nil { - return nil, nil, err, false - } + targetStatus := v1.PersistentVolumeClaimModifyVolumeInProgress pvcKey, keyErr := cache.MetaNamespaceKeyFunc(pvc) if keyErr != nil { return pvc, pv, keyErr, false } - if !util.IsFinalError(keyErr) { + if !util.IsFinalError(err) { // update conditions and cache pvc as uncertain - ctrl.uncertainPVCs[pvcKey] = *pvc + ctrl.uncertainPVCs.Store(pvcKey, pvc) + errMsg += ". Still modifying to VAC " + vacObj.Name } else { // Only InvalidArgument can be set to Infeasible state // Final errors other than InvalidArgument will still be in InProgress state if errStatus.Code() == codes.InvalidArgument { - // Mark pvc.Status.ModifyVolumeStatus as infeasible - pvc, markModifyVolumeInfeasibleError := ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumeInfeasible, err) - if markModifyVolumeInfeasibleError != nil { - return pvc, pv, markModifyVolumeInfeasibleError, false - } - ctrl.markForSlowRetry(pvc, pvcKey) + targetStatus = v1.PersistentVolumeClaimModifyVolumeInfeasible } - ctrl.removePVCFromModifyVolumeUncertainCache(pvcKey) + ctrl.uncertainPVCs.Delete(pvcKey) + } + var markErr error + pvc, markErr = ctrl.markControllerModifyVolumeStatus(pvc, targetStatus, err) + if markErr != nil { + return pvc, pv, markErr, false } + ctrl.markForSlowRetry(pvc, pvcKey) } else { - return pvc, pv, fmt.Errorf("cannot get error status from modify volume err: %v ", err), false + return pvc, pv, fmt.Errorf("cannot get error status from modify volume err: %v", err), false } // Record an event to indicate that modify operation is failed. - ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, err.Error()) + ctrl.eventRecorder.Event(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, errMsg) return pvc, pv, err, false } } diff --git a/pkg/modifycontroller/modify_volume_test.go b/pkg/modifycontroller/modify_volume_test.go index d0e942f42..17d5e8867 100644 --- a/pkg/modifycontroller/modify_volume_test.go +++ b/pkg/modifycontroller/modify_volume_test.go @@ -1,6 +1,9 @@ package modifycontroller import ( + "errors" + "fmt" + "slices" "testing" "github.com/google/go-cmp/cmp" @@ -13,6 +16,7 @@ import ( "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" + "k8s.io/utils/ptr" ) var ( @@ -30,7 +34,7 @@ var ( ) func TestModify(t *testing.T) { - basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/) + basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, "" /*targetVacName*/) basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac) var tests = []struct { @@ -56,11 +60,11 @@ 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*/, "whatever" /*targetVacName*/), pv: basePV, expectModifyCall: false, expectedModifyVolumeStatus: &v1.ModifyVolumeStatus{ - TargetVolumeAttributesClassName: targetVac, + TargetVolumeAttributesClassName: "whatever", Status: v1.PersistentVolumeClaimModifyVolumePending, }, expectedCurrentVolumeAttributesClassName: &testVac, @@ -145,6 +149,87 @@ func TestModify(t *testing.T) { } } +func TestModifyUncertain(t *testing.T) { + basePVC := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, targetVac /*targetVacName*/) + basePVC.Status.ModifyVolumeStatus.Status = v1.PersistentVolumeClaimModifyVolumeInProgress + basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac) + + client := csi.NewMockClient(testDriverName, true, true, true, true, true) + initialObjects := []runtime.Object{testVacObject, targetVacObject, basePVC, basePV} + ctrlInstance := setupFakeK8sEnvironment(t, client, initialObjects) + + pvcKey := fmt.Sprintf("%s/%s", pvcNamespace, pvcName) + assertUncertain := func(uncertain bool) { + t.Helper() + _, ok := ctrlInstance.uncertainPVCs.Load(pvcKey) + if ok != uncertain { + t.Fatalf("expected uncertain state to be %v, got %v", uncertain, ok) + } + } + + // initialized to uncertain + assertUncertain(true) + + client.SetModifyError(finalErr) + pvc, pv, err, _ := ctrlInstance.modify(basePVC, basePV) + if !errors.Is(err, finalErr) { + t.Fatalf("expected error to be %v, got %v", finalErr, err) + } + // should clear uncertain state + assertUncertain(false) + + client.SetModifyError(nonFinalErr) + 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 TestCancel(t *testing.T) { + basePVC := createTestPVC(pvcName, "" /*vacName*/, testVac /*curVacName*/, targetVac /*targetVacName*/) + basePVC.Status.ModifyVolumeStatus.Status = v1.PersistentVolumeClaimModifyVolumeInProgress + basePVC.Status.Conditions = []v1.PersistentVolumeClaimCondition{ + { + Type: v1.PersistentVolumeClaimVolumeModifyingVolume, + Status: v1.ConditionTrue, + }, + } + basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac) + + client := csi.NewMockClient(testDriverName, true, true, true, true, true) + initialObjects := []runtime.Object{testVacObject, targetVacObject, basePVC, basePV} + ctrlInstance := setupFakeK8sEnvironment(t, client, initialObjects) + + client.SetModifyError(infeasibleErr) + // InProgress, so still tried + pvc, pv, err, _ := ctrlInstance.modify(basePVC, basePV) + if !errors.Is(err, infeasibleErr) { + t.Fatalf("expected error to be %v, got %v", finalErr, err) + } + // Got infeasibleErr error, should cancel + pvc, _, err, _ = ctrlInstance.modify(pvc, pv) + if err != nil { + t.Fatalf("expected modify cancelled, got %v", err) + } + if slices.ContainsFunc(pvc.Status.Conditions, func(c v1.PersistentVolumeClaimCondition) bool { + return c.Type == v1.PersistentVolumeClaimVolumeModifyingVolume + }) { + t.Fatalf("expected modify volume condition to be cleared, got %v", pvc.Status.Conditions) + } +} + func createTestPVC(pvcName string, vacName string, curVacName string, targetVacName string) *v1.PersistentVolumeClaim { pvc := &v1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{Name: pvcName, Namespace: pvcNamespace}, @@ -169,10 +254,13 @@ func createTestPVC(pvcName string, vacName string, curVacName string, targetVacN CurrentVolumeAttributesClassName: &curVacName, ModifyVolumeStatus: &v1.ModifyVolumeStatus{ TargetVolumeAttributesClassName: targetVacName, - Status: "", + Status: v1.PersistentVolumeClaimModifyVolumeInfeasible, }, }, } + if targetVacName == "" { + pvc.Status.ModifyVolumeStatus = nil + } return pvc } diff --git a/pkg/testutil/test_util.go b/pkg/testutil/test_util.go index 7ca0d1677..201714a61 100644 --- a/pkg/testutil/test_util.go +++ b/pkg/testutil/test_util.go @@ -54,7 +54,7 @@ type pvcModifier struct { pvc *v1.PersistentVolumeClaim } -func MakePVC(conditions []v1.PersistentVolumeClaimCondition) pvcModifier { +func MakePVC(conditions []v1.PersistentVolumeClaimCondition) func() pvcModifier { pvc := &v1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "resize"}, Spec: v1.PersistentVolumeClaimSpec{ @@ -76,10 +76,12 @@ func MakePVC(conditions []v1.PersistentVolumeClaimCondition) pvcModifier { }, }, } - return pvcModifier{pvc} + return func() pvcModifier { + return pvcModifier{pvc.DeepCopy()} + } } -func MakeTestPVC(conditions []v1.PersistentVolumeClaimCondition) pvcModifier { +func MakeTestPVC(conditions []v1.PersistentVolumeClaimCondition) func() pvcModifier { pvc := &v1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{Name: pvcName, Namespace: "modify"}, Spec: v1.PersistentVolumeClaimSpec{ @@ -106,7 +108,9 @@ func MakeTestPVC(conditions []v1.PersistentVolumeClaimCondition) pvcModifier { }, }, } - return pvcModifier{pvc} + return func() pvcModifier { + return pvcModifier{pvc.DeepCopy()} + } } func (m pvcModifier) WithModifyVolumeStatus(status v1.PersistentVolumeClaimModifyVolumeStatus) pvcModifier { @@ -139,7 +143,8 @@ func CompareConditions(realConditions, expectedConditions []v1.PersistentVolumeC } for i, condition := range realConditions { - if condition.Type != expectedConditions[i].Type || condition.Message != expectedConditions[i].Message || condition.Status != expectedConditions[i].Status { + if condition.Type != expectedConditions[i].Type || condition.Message != expectedConditions[i].Message || + condition.Status != expectedConditions[i].Status || condition.Reason != expectedConditions[i].Reason { return false } } @@ -147,7 +152,7 @@ func CompareConditions(realConditions, expectedConditions []v1.PersistentVolumeC } func (m pvcModifier) Get() *v1.PersistentVolumeClaim { - return m.pvc.DeepCopy() + return m.pvc } func (m pvcModifier) WithStorageResourceStatus(status v1.ClaimResourceStatus) pvcModifier { diff --git a/pkg/util/events.go b/pkg/util/events.go index c9f1fb514..d23f4955c 100644 --- a/pkg/util/events.go +++ b/pkg/util/events.go @@ -24,6 +24,7 @@ const ( VolumeModify = "VolumeModify" VolumeModifyFailed = "VolumeModifyFailed" VolumeModifySuccess = "VolumeModifySuccessful" + VolumeModifyCancelled = "VolumeModifyCanceled" FileSystemResizeRequired = "FileSystemResizeRequired" ) diff --git a/pkg/util/util.go b/pkg/util/util.go index 5bb0c10ca..f2202f6ea 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -43,11 +43,6 @@ var ( v1.PersistentVolumeClaimNodeResizeError: true, } - knownModifyVolumeConditions = map[v1.PersistentVolumeClaimConditionType]bool{ - v1.PersistentVolumeClaimVolumeModifyingVolume: true, - v1.PersistentVolumeClaimVolumeModifyVolumeError: true, - } - // AnnPreResizeCapacity annotation is added to a PV when expanding volume. // Its value is status capacity of the PVC prior to the volume expansion // Its value will be set by the external-resizer when it deems that filesystem resize is required after resizing volume. @@ -92,34 +87,42 @@ func MergeResizeConditionsOfPVC(oldConditions, newConditions []v1.PersistentVolu return resultConditions } -// MergeModifyVolumeConditionsOfPVC updates pvc with requested modify volume conditions -// leaving other conditions untouched. -func MergeModifyVolumeConditionsOfPVC(oldConditions, newConditions []v1.PersistentVolumeClaimCondition) []v1.PersistentVolumeClaimCondition { +// MergePVCConditions updates pvc with requested conditions +// leaving unmentioned conditions untouched. +// It is responsible for setting LastTransitionTime. +func MergePVCConditions(oldConditions, newConditions []v1.PersistentVolumeClaimCondition) []v1.PersistentVolumeClaimCondition { newConditionSet := make(map[v1.PersistentVolumeClaimConditionType]v1.PersistentVolumeClaimCondition, len(newConditions)) for _, condition := range newConditions { newConditionSet[condition.Type] = condition } resultConditions := []v1.PersistentVolumeClaimCondition{} - for _, condition := range oldConditions { - // If Condition is not modifyVolume type, we keep it. - if _, ok := knownModifyVolumeConditions[condition.Type]; !ok { - resultConditions = append(resultConditions, condition) + for _, old := range oldConditions { + // If Condition is not changed, we keep it. + new, ok := newConditionSet[old.Type] + if !ok { + resultConditions = append(resultConditions, old) continue } - if newCondition, ok := newConditionSet[condition.Type]; ok { - // Use the new condition to replace old condition with same type. - resultConditions = append(resultConditions, newCondition) - delete(newConditionSet, condition.Type) + // Use the new condition to replace old condition with same type. + // Update LastTransitionTime if Reason/Status changed. + if new.Reason != old.Reason || new.Status != old.Status { + new.LastTransitionTime = new.LastProbeTime + } else { + new.LastTransitionTime = old.LastTransitionTime } - // Drop other modify volume conditions that are not in the newConditionSet + resultConditions = append(resultConditions, new) + delete(newConditionSet, old.Type) } // Append remains modify volume conditions. - for _, condition := range newConditionSet { - resultConditions = append(resultConditions, condition) + // Iterate over slice for deterministic ordering. + for _, new := range newConditions { + if _, ok := newConditionSet[new.Type]; ok { + new.LastTransitionTime = new.LastProbeTime + resultConditions = append(resultConditions, new) + } } - return resultConditions } diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go index ebe91b8b8..0eed1e40f 100644 --- a/pkg/util/util_test.go +++ b/pkg/util/util_test.go @@ -4,44 +4,58 @@ import ( "encoding/json" "reflect" "testing" + "time" + "github.com/google/go-cmp/cmp" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) var ( + now = metav1.Now() + old = metav1.Date(2019, 1, 1, 0, 0, 0, 0, time.UTC) + pvcWithResizePendingCondition = v1.PersistentVolumeClaimCondition{ Type: v1.PersistentVolumeClaimFileSystemResizePending, Status: v1.ConditionTrue, - LastTransitionTime: metav1.Now(), + LastTransitionTime: now, Message: "Waiting for user to (re-)start a pod to finish file system resize of volume on node.", } pvcWithControllerResizeError = v1.PersistentVolumeClaimCondition{ Type: v1.PersistentVolumeClaimControllerResizeError, Status: v1.ConditionTrue, - LastTransitionTime: metav1.Now(), + LastTransitionTime: now, Message: "controller resize failed", } pvcWithControllerResizeError2 = v1.PersistentVolumeClaimCondition{ Type: v1.PersistentVolumeClaimControllerResizeError, Status: v1.ConditionTrue, - LastTransitionTime: metav1.Now(), + LastTransitionTime: now, Message: "controller resize failed with error2", } pvcWithModifyVolumeProgressCondition = v1.PersistentVolumeClaimCondition{ - Type: v1.PersistentVolumeClaimVolumeModifyingVolume, - Status: v1.ConditionTrue, - LastTransitionTime: metav1.Now(), - Message: "ModifyVolume operation in progress.", + Type: v1.PersistentVolumeClaimVolumeModifyingVolume, + Status: v1.ConditionTrue, + LastProbeTime: now, + Message: "ModifyVolume operation in progress.", } pvcConditionInfeasible = v1.PersistentVolumeClaimCondition{ - Type: v1.PersistentVolumeClaimVolumeModifyingVolume, - Status: v1.ConditionTrue, - LastTransitionTime: metav1.Now(), - Message: "ModifyVolume operation in progress.", + Type: v1.PersistentVolumeClaimVolumeModifyVolumeError, + Status: v1.ConditionTrue, + LastProbeTime: now, + Reason: "InvalidParameter", + Message: "invalid parameter", + } + + pvcConditionTimeout = v1.PersistentVolumeClaimCondition{ + Type: v1.PersistentVolumeClaimVolumeModifyVolumeError, + Status: v1.ConditionTrue, + LastProbeTime: now, + Reason: "DeadlineExceeded", + Message: "Progress: 10%", } ) @@ -120,7 +134,12 @@ func TestMergeResizeConditionsOfPVC(t *testing.T) { } } -func TestMergeModifyVolumeConditionsOfPVC(t *testing.T) { +func transit(condition v1.PersistentVolumeClaimCondition, t metav1.Time) v1.PersistentVolumeClaimCondition { + condition.LastTransitionTime = t + return condition +} + +func TestMergePVCConditions(t *testing.T) { tests := []struct { name string oldConditions []v1.PersistentVolumeClaimCondition @@ -128,31 +147,58 @@ func TestMergeModifyVolumeConditionsOfPVC(t *testing.T) { expectedConditions []v1.PersistentVolumeClaimCondition }{ { - name: "merge new modify volume condition with old resize condition", - oldConditions: []v1.PersistentVolumeClaimCondition{pvcWithResizePendingCondition}, - newConditions: []v1.PersistentVolumeClaimCondition{pvcWithModifyVolumeProgressCondition}, - expectedConditions: []v1.PersistentVolumeClaimCondition{pvcWithResizePendingCondition, pvcWithModifyVolumeProgressCondition}, + name: "merge new into irrelevant", + oldConditions: []v1.PersistentVolumeClaimCondition{pvcWithResizePendingCondition}, + newConditions: []v1.PersistentVolumeClaimCondition{pvcWithModifyVolumeProgressCondition}, + expectedConditions: []v1.PersistentVolumeClaimCondition{ + pvcWithResizePendingCondition, + transit(pvcWithModifyVolumeProgressCondition, now), + }, }, { - name: "merge new modify volume condition with old modify volume condition", - oldConditions: []v1.PersistentVolumeClaimCondition{pvcWithModifyVolumeProgressCondition}, - newConditions: []v1.PersistentVolumeClaimCondition{pvcConditionInfeasible}, - expectedConditions: []v1.PersistentVolumeClaimCondition{pvcConditionInfeasible}, + name: "no transition", + oldConditions: []v1.PersistentVolumeClaimCondition{ + transit(pvcWithModifyVolumeProgressCondition, old), + transit(pvcConditionInfeasible, old), + }, + newConditions: []v1.PersistentVolumeClaimCondition{pvcConditionInfeasible}, + expectedConditions: []v1.PersistentVolumeClaimCondition{ + transit(pvcWithModifyVolumeProgressCondition, old), + transit(pvcConditionInfeasible, old), + }, }, { - name: "merge empty condition with old modify volume condition", + name: "transit", + oldConditions: []v1.PersistentVolumeClaimCondition{ + transit(pvcWithModifyVolumeProgressCondition, old), + transit(pvcConditionInfeasible, old), + }, + newConditions: []v1.PersistentVolumeClaimCondition{pvcConditionTimeout}, + expectedConditions: []v1.PersistentVolumeClaimCondition{ + transit(pvcWithModifyVolumeProgressCondition, old), + transit(pvcConditionTimeout, now), + }, + }, + { + name: "merge empty", oldConditions: []v1.PersistentVolumeClaimCondition{pvcWithModifyVolumeProgressCondition}, newConditions: []v1.PersistentVolumeClaimCondition{}, - expectedConditions: []v1.PersistentVolumeClaimCondition{}, + expectedConditions: []v1.PersistentVolumeClaimCondition{pvcWithModifyVolumeProgressCondition}, }, { - name: "merge new condition with old empty volume condition", - oldConditions: []v1.PersistentVolumeClaimCondition{}, - newConditions: []v1.PersistentVolumeClaimCondition{pvcWithModifyVolumeProgressCondition}, - expectedConditions: []v1.PersistentVolumeClaimCondition{pvcWithModifyVolumeProgressCondition}, + name: "merge new into empty", + oldConditions: []v1.PersistentVolumeClaimCondition{}, + newConditions: []v1.PersistentVolumeClaimCondition{ + pvcWithModifyVolumeProgressCondition, + pvcConditionInfeasible, + }, + expectedConditions: []v1.PersistentVolumeClaimCondition{ + transit(pvcWithModifyVolumeProgressCondition, now), + transit(pvcConditionInfeasible, now), + }, }, { - name: "should not remove previous non-resize conditions", + name: "noop", oldConditions: []v1.PersistentVolumeClaimCondition{}, newConditions: []v1.PersistentVolumeClaimCondition{}, expectedConditions: []v1.PersistentVolumeClaimCondition{}, @@ -162,9 +208,9 @@ func TestMergeModifyVolumeConditionsOfPVC(t *testing.T) { for _, test := range tests { tc := test t.Run(tc.name, func(t *testing.T) { - resultConditions := MergeModifyVolumeConditionsOfPVC(tc.oldConditions, tc.newConditions) - if !reflect.DeepEqual(resultConditions, tc.expectedConditions) { - t.Errorf("expected conditions %+v got %+v", tc.expectedConditions, resultConditions) + resultConditions := MergePVCConditions(tc.oldConditions, tc.newConditions) + if diff := cmp.Diff(tc.expectedConditions, resultConditions); diff != "" { + t.Errorf("conditions diff (-expected, +got): %v", diff) } }) }