From db19140e0d1120520fbb10d5c372c089a0ae87dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20M=C3=B6ller?= Date: Fri, 26 Apr 2024 15:55:06 +0200 Subject: [PATCH] fix: do not emit VolumeResizeFailed on conflict --- pkg/controller/controller.go | 13 +-- pkg/controller/controller_test.go | 171 +++++++++++++++++++----------- pkg/csi/mock_client.go | 10 +- 3 files changed, 121 insertions(+), 73 deletions(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index cbbff143..754dbd40 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -22,6 +22,7 @@ import ( "time" "github.com/kubernetes-csi/external-resizer/pkg/features" + "k8s.io/apimachinery/pkg/api/errors" "github.com/kubernetes-csi/external-resizer/pkg/resizer" "github.com/kubernetes-csi/external-resizer/pkg/util" @@ -438,7 +439,7 @@ func (ctrl *resizeController) resizePVC(pvc *v1.PersistentVolumeClaim, pv *v1.Pe return ctrl.markPVCResizeFinished(pvc, newSize) }() - if err != nil { + if err != nil && !errors.IsConflict(err) /* ignore conflicts as they should be silently retried */ { // Record an event to indicate that resize operation is failed. ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeResizeFailed, err.Error()) } @@ -466,7 +467,7 @@ func (ctrl *resizeController) resizeVolume( if inUseError(err) { ctrl.usedPVCs.addPVCWithInUseError(pvc) } - return newSize, fsResizeRequired, fmt.Errorf("resize volume %q by resizer %q failed: %v", pv.Name, ctrl.name, err) + return newSize, fsResizeRequired, fmt.Errorf("resize volume %q by resizer %q failed: %w", pv.Name, ctrl.name, err) } klog.V(4).InfoS("Resize volume succeeded start to update PV's capacity", "PV", klog.KObj(pv)) @@ -492,12 +493,12 @@ func (ctrl *resizeController) markPVCAsFSResizeRequired(pvc *v1.PersistentVolume updatedPVC, err := util.PatchClaim(ctrl.kubeClient, pvc, newPVC, true /* addResourceVersionCheck */) if err != nil { - return fmt.Errorf("Mark PVC %q as file system resize required failed: %v", klog.KObj(pvc), err) + return fmt.Errorf("Mark PVC %q as file system resize required failed: %w", klog.KObj(pvc), err) } err = ctrl.claims.Update(updatedPVC) if err != nil { - return fmt.Errorf("error updating PVC %s in local cache: %v", klog.KObj(newPVC), err) + return fmt.Errorf("error updating PVC %s in local cache: %w", klog.KObj(newPVC), err) } klog.V(4).InfoS("Mark PVC as file system resize required", "PVC", klog.KObj(pvc)) @@ -539,12 +540,12 @@ func (ctrl *resizeController) markPVCResizeFinished( updatedPVC, err := util.PatchClaim(ctrl.kubeClient, pvc, newPVC, true /* addResourceVersionCheck */) if err != nil { - return fmt.Errorf("Mark PVC %q as resize finished failed: %v", klog.KObj(pvc), err) + return fmt.Errorf("Mark PVC %q as resize finished failed: %w", klog.KObj(pvc), err) } err = ctrl.claims.Update(updatedPVC) if err != nil { - return fmt.Errorf("error updating PVC %s in local cache: %v", klog.KObj(newPVC), err) + return fmt.Errorf("error updating PVC %s in local cache: %w", klog.KObj(newPVC), err) } klog.V(4).InfoS("Resize PVC finished", "PVC", klog.KObj(pvc)) diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 887350fa..ba57bac7 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -2,10 +2,14 @@ package controller import ( "context" + "fmt" "testing" "time" "github.com/kubernetes-csi/external-resizer/pkg/features" + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/tools/record" "k8s.io/client-go/util/workqueue" @@ -324,88 +328,131 @@ func TestResizePVC(t *testing.T) { PVC *v1.PersistentVolumeClaim PV *v1.PersistentVolume - NodeResize bool - expansionFailure bool - expectFailure bool + NodeResize bool + expansionError error + expectFailure bool + expectedEvents []string }{ { Name: "Resize PVC with FS resize", PVC: createPVC(2, 1), PV: createPV(1, "testPVC", defaultNS, "foobar", &fsVolumeMode), NodeResize: true, + expectedEvents: []string{ + "Normal Resizing External resizer is resizing volume testPV", + "Normal FileSystemResizeRequired Require file system resize of volume on node", + }, + }, + { + Name: "Resize PVC with FS resize failure", + PVC: createPVC(2, 1), + PV: createPV(1, "testPVC", defaultNS, "foobar", &fsVolumeMode), + NodeResize: true, + expansionError: fmt.Errorf("expansion failed"), + expectFailure: true, + expectedEvents: []string{ + "Normal Resizing External resizer is resizing volume testPV", + "Warning VolumeResizeFailed resize volume \"testPV\" by resizer \"mock\" failed: expansion failed", + }, }, { - Name: "Resize PVC with FS resize failure", - PVC: createPVC(2, 1), - PV: createPV(1, "testPVC", defaultNS, "foobar", &fsVolumeMode), - NodeResize: true, - expansionFailure: true, - expectFailure: true, + Name: "Resize PVC with resource version conflict should not emit event", + PVC: createPVC(2, 1), + PV: createPV(1, "testPVC", defaultNS, "foobar", &fsVolumeMode), + NodeResize: true, + expansionError: errors.NewConflict( + schema.GroupResource{ + Group: v1.GroupName, + Resource: "persistentvolumeclaims", + }, + "testPVC", + fmt.Errorf("the object has been modified; "+ + "please apply your changes to the latest version and try again"), + ), + expectFailure: true, + expectedEvents: []string{ + "Normal Resizing External resizer is resizing volume testPV", + }, }, } { - client := csi.NewMockClient("mock", test.NodeResize, true, false, true, true) - if test.expansionFailure { - client.SetExpansionFailed() - } - driverName, _ := client.GetDriverName(context.TODO()) + t.Run(test.Name, func(t *testing.T) { + client := csi.NewMockClient("mock", test.NodeResize, true, false, true, true) + if test.expansionError != nil { + client.SetExpansionError(test.expansionError) + } + driverName, _ := client.GetDriverName(context.TODO()) - var expectedCap resource.Quantity - initialObjects := []runtime.Object{} - if test.PVC != nil { - initialObjects = append(initialObjects, test.PVC) - expectedCap = test.PVC.Status.Capacity[v1.ResourceStorage] - } - if test.PV != nil { - test.PV.Spec.PersistentVolumeSource.CSI.Driver = driverName - initialObjects = append(initialObjects, test.PV) - } + var expectedCap resource.Quantity + var initialObjects []runtime.Object + if test.PVC != nil { + initialObjects = append(initialObjects, test.PVC) + expectedCap = test.PVC.Status.Capacity[v1.ResourceStorage] + } + if test.PV != nil { + test.PV.Spec.PersistentVolumeSource.CSI.Driver = driverName + initialObjects = append(initialObjects, test.PV) + } - kubeClient, informerFactory := fakeK8s(initialObjects) - pvInformer := informerFactory.Core().V1().PersistentVolumes() - pvcInformer := informerFactory.Core().V1().PersistentVolumeClaims() - podInformer := informerFactory.Core().V1().Pods() + kubeClient, informerFactory := fakeK8s(initialObjects) + pvInformer := informerFactory.Core().V1().PersistentVolumes() + pvcInformer := informerFactory.Core().V1().PersistentVolumeClaims() + podInformer := informerFactory.Core().V1().Pods() - csiResizer, err := resizer.NewResizerFromClient(client, 15*time.Second, kubeClient, driverName) - if err != nil { - t.Fatalf("Test %s: Unable to create resizer: %v", test.Name, err) - } + csiResizer, err := resizer.NewResizerFromClient(client, 15*time.Second, kubeClient, driverName) + if err != nil { + t.Fatalf("Unable to create resizer: %v", err) + } - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AnnotateFsResize, true)() - controller := NewResizeController(driverName, csiResizer, kubeClient, time.Second, informerFactory, workqueue.DefaultControllerRateLimiter(), true /* disableVolumeInUseErrorHandler*/) + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AnnotateFsResize, true)() + controller := NewResizeController(driverName, csiResizer, kubeClient, time.Second, informerFactory, workqueue.DefaultControllerRateLimiter(), true /* disableVolumeInUseErrorHandler*/) - ctrlInstance, _ := controller.(*resizeController) + ctrlInstance, _ := controller.(*resizeController) - stopCh := make(chan struct{}) - informerFactory.Start(stopCh) + ctrlInstance.eventRecorder = record.NewFakeRecorder(10) - 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) - default: - t.Fatalf("Test %s: Unknown initalObject type: %+v", test.Name, obj) + stopCh := make(chan struct{}) + informerFactory.Start(stopCh) + + 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) + default: + t.Fatalf("Unknown initalObject type: %+v", obj) + } } - } - err = ctrlInstance.resizePVC(test.PVC, test.PV) - if test.expectFailure && err == nil { - t.Errorf("for %s expected error got nothing", test.Name) - continue - } - if !test.expectFailure { - if err != nil { - t.Errorf("for %s, unexpected error: %v", test.Name, err) - // check if pre resize capacity annotation gets properly populated after resize - } else if test.PV != nil && test.NodeResize && !expectedCap.IsZero() { - volObj, _, _ := ctrlInstance.volumes.GetByKey("testPV") - pv := volObj.(*v1.PersistentVolume) - checkPreResizeCap(t, test.Name, pv, expectedCap.String()) + err = ctrlInstance.resizePVC(test.PVC, test.PV) + if test.expectFailure && err == nil { + t.Errorf("expected error but got nothing") + return } - } + if !test.expectFailure { + if err != nil { + t.Errorf("unexpected error: %v", err) + // check if pre resize capacity annotation gets properly populated after resize + } else if test.PV != nil && test.NodeResize && !expectedCap.IsZero() { + volObj, _, _ := ctrlInstance.volumes.GetByKey("testPV") + pv := volObj.(*v1.PersistentVolume) + checkPreResizeCap(t, test.Name, pv, expectedCap.String()) + } + } + + events, expectedEvents := ctrlInstance.eventRecorder.(*record.FakeRecorder).Events, test.expectedEvents + if len(events) != len(expectedEvents) { + t.Errorf("expected %d events, got %d", len(expectedEvents), len(events)) + return + } + for i, expectedEvent := range expectedEvents { + if event := <-events; expectedEvent != event { + t.Errorf("expected event %s, got %s", expectedEvents[i], event) + } + } + }) } } diff --git a/pkg/csi/mock_client.go b/pkg/csi/mock_client.go index 57e85299..27c6524d 100644 --- a/pkg/csi/mock_client.go +++ b/pkg/csi/mock_client.go @@ -35,7 +35,7 @@ type MockClient struct { supportsControllerSingleNodeMultiWriter bool expandCalled atomic.Int32 modifyCalled atomic.Int32 - expansionFailed bool + expansionError error modifyFailed bool checkMigratedLabel bool usedSecrets atomic.Pointer[map[string]string] @@ -66,8 +66,8 @@ func (c *MockClient) SupportsControllerSingleNodeMultiWriter(context.Context) (b return c.supportsControllerSingleNodeMultiWriter, nil } -func (c *MockClient) SetExpansionFailed() { - c.expansionFailed = true +func (c *MockClient) SetExpansionError(err error) { + c.expansionError = err } func (c *MockClient) SetModifyFailed() { @@ -85,9 +85,9 @@ func (c *MockClient) Expand( secrets map[string]string, capability *csi.VolumeCapability) (int64, bool, error) { // TODO: Determine whether the operation succeeds or fails by parameters. - if c.expansionFailed { + if c.expansionError != nil { c.expandCalled.Add(1) - return requestBytes, c.supportsNodeResize, fmt.Errorf("expansion failed") + return requestBytes, c.supportsNodeResize, c.expansionError } if c.checkMigratedLabel { additionalInfo := ctx.Value(connection.AdditionalInfoKey)