Skip to content

Commit

Permalink
fix: do not emit VolumeResizeFailed on conflict
Browse files Browse the repository at this point in the history
  • Loading branch information
@jakobmoellerdev
jakobmoellerdev committed May 13, 2024
1 parent 6dabe77 commit db19140
Show file tree
Hide file tree
Showing 3 changed files with 121 additions and 73 deletions.
13 changes: 7 additions & 6 deletions pkg/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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())
}
Expand Down Expand Up @@ -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))

Expand All @@ -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))
Expand Down Expand Up @@ -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))
Expand Down
171 changes: 109 additions & 62 deletions pkg/controller/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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)
}
}
})
}
}

Expand Down
10 changes: 5 additions & 5 deletions pkg/csi/mock_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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() {
Expand All @@ -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)
Expand Down

0 comments on commit db19140

Please sign in to comment.