Skip to content

Commit 8b9a874

Browse files
sunnylovestiramisusunnylovestiramisu
committed
Enable Rollback Support of VAC with Infeasible Errors
1 parent 2e1c582 commit 8b9a874

File tree

5 files changed

+128
-29
lines changed

5 files changed

+128
-29
lines changed

‎pkg/modifycontroller/controller_test.go

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,14 @@ import (
44
"context"
55
"errors"
66
"fmt"
7+
"testing"
8+
"time"
9+
710
"github.com/kubernetes-csi/external-resizer/pkg/util"
811
"google.golang.org/grpc/codes"
912
"google.golang.org/grpc/status"
1013
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1114
"k8s.io/client-go/tools/cache"
12-
"testing"
13-
"time"
1415

1516
"github.com/kubernetes-csi/external-resizer/pkg/features"
1617

@@ -27,7 +28,7 @@ import (
2728
)
2829

2930
func TestController(t *testing.T) {
30-
basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/)
31+
basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/)
3132
basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
3233
firstTimePV := basePV.DeepCopy()
3334
firstTimePV.Spec.VolumeAttributesClassName = nil
@@ -44,7 +45,7 @@ func TestController(t *testing.T) {
4445
}{
4546
{
4647
name: "Modify called",
47-
pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/),
48+
pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/),
4849
pv: basePV,
4950
vacExists: true,
5051
callCSIModify: true,
@@ -104,14 +105,14 @@ func TestModifyPVC(t *testing.T) {
104105
}{
105106
{
106107
name: "Modify succeeded",
107-
pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/),
108+
pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/),
108109
pv: basePV,
109110
modifyFailure: false,
110111
expectFailure: false,
111112
},
112113
{
113114
name: "Modify failed",
114-
pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/),
115+
pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/),
115116
pv: basePV,
116117
modifyFailure: true,
117118
expectFailure: true,
@@ -145,16 +146,16 @@ func TestModifyPVC(t *testing.T) {
145146
}
146147

147148
func TestSyncPVC(t *testing.T) {
148-
basePVC := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/)
149+
basePVC := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/)
149150
basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
150151

151152
otherDriverPV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
152153
otherDriverPV.Spec.PersistentVolumeSource.CSI.Driver = "some-other-driver"
153154

154-
unboundPVC := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/)
155+
unboundPVC := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/)
155156
unboundPVC.Status.Phase = v1.ClaimPending
156157

157-
pvcWithUncreatedPV := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/)
158+
pvcWithUncreatedPV := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/)
158159
pvcWithUncreatedPV.Spec.VolumeName = ""
159160

160161
nonCSIPVC := &v1.PersistentVolumeClaim{
@@ -196,7 +197,7 @@ func TestSyncPVC(t *testing.T) {
196197
},
197198
{
198199
name: "Should NOT modify if PVC has empty Spec.VACName",
199-
pvc: createTestPVC(pvcName, "" /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/),
200+
pvc: createTestPVC(pvcName, "" /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/),
200201
pv: basePV,
201202
callCSIModify: false,
202203
},
@@ -247,7 +248,7 @@ func TestSyncPVC(t *testing.T) {
247248

248249
// TestInfeasibleRetry tests that sidecar doesn't spam plugin upon infeasible error code (e.g. invalid VAC parameter)
249250
func TestInfeasibleRetry(t *testing.T) {
250-
basePVC := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/)
251+
basePVC := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/)
251252
basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
252253

253254
tests := []struct {

‎pkg/modifycontroller/modify_status.go

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package modifycontroller
1818

1919
import (
2020
"fmt"
21+
2122
"github.com/kubernetes-csi/external-resizer/pkg/util"
2223
v1 "k8s.io/api/core/v1"
2324
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -73,6 +74,47 @@ func (ctrl *modifyController) markControllerModifyVolumeStatus(
7374
return updatedPVC, nil
7475
}
7576

77+
func (ctrl *modifyController) markCotrollerModifyVolumeRollbackCompeleted(
78+
pvc *v1.PersistentVolumeClaim,
79+
pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error) {
80+
// Update PVC
81+
newPVC := pvc.DeepCopy()
82+
83+
// Update ModifyVolumeStatus to completed
84+
newPVC.Status.ModifyVolumeStatus = nil
85+
86+
// Rollback CurrentVolumeAttributesClassName
87+
newPVC.Status.CurrentVolumeAttributesClassName = pvc.Spec.VolumeAttributesClassName
88+
89+
// Clear all the conditions related to modify volume
90+
newPVC.Status.Conditions = clearModifyVolumeConditions(newPVC.Status.Conditions)
91+
92+
// Update PV
93+
newPV := pv.DeepCopy()
94+
newPV.Spec.VolumeAttributesClassName = pvc.Spec.VolumeAttributesClassName
95+
96+
// Update PV before PVC to avoid PV not getting updated but PVC did
97+
updatedPV, err := util.PatchPersistentVolume(ctrl.kubeClient, pv, newPV)
98+
if err != nil {
99+
return pvc, pv, fmt.Errorf("update pv.Spec.VolumeAttributesClassName for PVC %q failed, errored with: %v", pvc.Name, err)
100+
}
101+
102+
updatedPVC, err := util.PatchClaim(ctrl.kubeClient, pvc, newPVC, false /* addResourceVersionCheck */)
103+
if err != nil {
104+
return pvc, pv, fmt.Errorf("mark PVC %q as ModifyVolumeCompleted failed, errored with: %v", pvc.Name, err)
105+
}
106+
107+
pvcKey, err := cache.MetaNamespaceKeyFunc(pvc)
108+
if err != nil {
109+
return pvc, pv, err
110+
}
111+
112+
ctrl.removePVCFromModifyVolumeUncertainCache(pvcKey)
113+
ctrl.markForSlowRetry(pvc, pvcKey)
114+
115+
return updatedPVC, updatedPV, nil
116+
}
117+
76118
func (ctrl *modifyController) updateConditionBasedOnError(pvc *v1.PersistentVolumeClaim, err error) (*v1.PersistentVolumeClaim, error) {
77119
newPVC := pvc.DeepCopy()
78120
pvcCondition := v1.PersistentVolumeClaimCondition{
@@ -96,7 +138,7 @@ func (ctrl *modifyController) updateConditionBasedOnError(pvc *v1.PersistentVolu
96138
return updatedPVC, nil
97139
}
98140

99-
// markControllerModifyVolumeStatus will mark ModifyVolumeStatus as completed in the PVC
141+
// markControllerModifyVolumeCompleted will mark ModifyVolumeStatus as completed in the PVC
100142
// and update CurrentVolumeAttributesClassName, clear the conditions
101143
func (ctrl *modifyController) markControllerModifyVolumeCompleted(pvc *v1.PersistentVolumeClaim, pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error) {
102144
modifiedVacName := pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName
@@ -131,7 +173,7 @@ func (ctrl *modifyController) markControllerModifyVolumeCompleted(pvc *v1.Persis
131173
return updatedPVC, updatedPV, nil
132174
}
133175

134-
// markControllerModifyVolumeStatus clears all the conditions related to modify volume and only
176+
// clearModifyVolumeConditions clears all the conditions related to modify volume and only
135177
// leave other condition types
136178
func clearModifyVolumeConditions(conditions []v1.PersistentVolumeClaimCondition) []v1.PersistentVolumeClaimCondition {
137179
knownConditions := []v1.PersistentVolumeClaimCondition{}

‎pkg/modifycontroller/modify_status_test.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -397,10 +397,12 @@ func createTestPV(capacityGB int, pvcName, pvcNamespace string, pvcUID types.UID
397397
VolumeHandle: "foo",
398398
},
399399
},
400-
VolumeMode: volumeMode,
401-
VolumeAttributesClassName: &vacName,
400+
VolumeMode: volumeMode,
402401
},
403402
}
403+
if vacName != "" {
404+
pv.Spec.VolumeAttributesClassName = &vacName
405+
}
404406
if len(pvcName) > 0 {
405407
pv.Spec.ClaimRef = &v1.ObjectReference{
406408
Namespace: pvcNamespace,

‎pkg/modifycontroller/modify_volume.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,16 @@ func (ctrl *modifyController) modify(pvc *v1.PersistentVolumeClaim, pv *v1.Persi
6767
}
6868
}
6969

70+
// Rollback infeasible errors for recovery
71+
if pvc.Status.ModifyVolumeStatus != nil && pvc.Status.ModifyVolumeStatus.Status == v1.PersistentVolumeClaimModifyVolumeInfeasible {
72+
targetVacName := pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName
73+
// Case 1: rollback to nil
74+
// Case 2: rollback to previous VAC
75+
if (pvcSpecVacName == nil && curVacName == nil && targetVacName != "") || (pvcSpecVacName != nil && curVacName != nil && *pvcSpecVacName == *curVacName && targetVacName != *curVacName) {
76+
return ctrl.validateVACAndRollback(pvc, pv)
77+
}
78+
}
79+
7080
// No modification required
7181
return pvc, pv, nil, false
7282
}
@@ -98,6 +108,22 @@ func (ctrl *modifyController) validateVACAndModifyVolumeWithTarget(
98108
}
99109
}
100110

111+
func (ctrl *modifyController) validateVACAndRollback(
112+
pvc *v1.PersistentVolumeClaim,
113+
pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) {
114+
// The controller does not triggers ModifyVolume because it is only
115+
// for rollbacking infeasible errors
116+
// Record an event to indicate that external resizer is rolling back this volume.
117+
ctrl.eventRecorder.Event(pvc, v1.EventTypeNormal, util.VolumeModify,
118+
fmt.Sprintf("external resizer is rolling back volume %s with infeasible error", pvc.Name))
119+
// Mark pvc.Status.ModifyVolumeStatus as completed
120+
pvc, pv, err := ctrl.markCotrollerModifyVolumeRollbackCompeleted(pvc, pv)
121+
if err != nil {
122+
return pvc, pv, fmt.Errorf("rollback volume %s modification with error: %v ", pvc.Name, err), false
123+
}
124+
return pvc, pv, nil, false
125+
}
126+
101127
// func controllerModifyVolumeWithTarget trigger the CSI ControllerModifyVolume API call
102128
// and handle both success and error scenarios
103129
func (ctrl *modifyController) controllerModifyVolumeWithTarget(

‎pkg/modifycontroller/modify_volume_test.go

Lines changed: 42 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ var (
3535
)
3636

3737
func TestModify(t *testing.T) {
38-
basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/)
38+
basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/)
3939
basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
4040

4141
var tests = []struct {
@@ -61,7 +61,7 @@ func TestModify(t *testing.T) {
6161
},
6262
{
6363
name: "vac does not exist, no modification and set ModifyVolumeStatus to pending",
64-
pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/),
64+
pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/),
6565
pv: basePV,
6666
expectModifyCall: false,
6767
expectedModifyVolumeStatus: &v1.ModifyVolumeStatus{
@@ -73,7 +73,7 @@ func TestModify(t *testing.T) {
7373
},
7474
{
7575
name: "modify volume success",
76-
pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/),
76+
pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/),
7777
pv: basePV,
7878
vacExists: true,
7979
expectModifyCall: true,
@@ -83,7 +83,7 @@ func TestModify(t *testing.T) {
8383
},
8484
{
8585
name: "modify volume success with extra metadata",
86-
pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/),
86+
pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/),
8787
pv: basePV,
8888
vacExists: true,
8989
expectModifyCall: true,
@@ -98,6 +98,26 @@ func TestModify(t *testing.T) {
9898
"csi.storage.k8s.io/pv/name": "testPV",
9999
},
100100
},
101+
{
102+
name: "modify volume rollback succeeds for infeasible errors",
103+
pvc: createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, targetVac /*targetVacName*/, v1.PersistentVolumeClaimModifyVolumeInfeasible),
104+
pv: basePV,
105+
vacExists: true,
106+
expectModifyCall: false,
107+
expectedModifyVolumeStatus: nil,
108+
expectedCurrentVolumeAttributesClassName: &testVac,
109+
expectedPVVolumeAttributesClassName: &testVac,
110+
},
111+
{
112+
name: "modify volume rollback to nil succeeds for infeasible errors",
113+
pvc: createTestPVC(pvcName, "" /*vacName*/, "" /*curVacName*/, targetVac /*targetVacName*/, v1.PersistentVolumeClaimModifyVolumeInfeasible),
114+
pv: createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, ""),
115+
vacExists: true,
116+
expectModifyCall: false,
117+
expectedModifyVolumeStatus: nil,
118+
expectedCurrentVolumeAttributesClassName: nil,
119+
expectedPVVolumeAttributesClassName: nil,
120+
},
101121
}
102122

103123
for i := range tests {
@@ -130,16 +150,20 @@ func TestModify(t *testing.T) {
130150

131151
actualCurrentVolumeAttributesClassName := pvc.Status.CurrentVolumeAttributesClassName
132152

133-
if diff := cmp.Diff(*test.expectedCurrentVolumeAttributesClassName, *actualCurrentVolumeAttributesClassName); diff != "" {
134-
t.Errorf("expected CurrentVolumeAttributesClassName to be %v, got %v", *test.expectedCurrentVolumeAttributesClassName, *actualCurrentVolumeAttributesClassName)
153+
if test.expectedCurrentVolumeAttributesClassName != nil && actualCurrentVolumeAttributesClassName != nil {
154+
if diff := cmp.Diff(*test.expectedCurrentVolumeAttributesClassName, *actualCurrentVolumeAttributesClassName); diff != "" {
155+
t.Errorf("expected CurrentVolumeAttributesClassName to be %v, got %v", *test.expectedCurrentVolumeAttributesClassName, *actualCurrentVolumeAttributesClassName)
156+
}
135157
}
136158

137159
actualPVVolumeAttributesClassName := pv.Spec.VolumeAttributesClassName
138-
if diff := cmp.Diff(*test.expectedPVVolumeAttributesClassName, *actualPVVolumeAttributesClassName); diff != "" {
139-
t.Errorf("expected VolumeAttributesClassName of pv to be %v, got %v", *test.expectedPVVolumeAttributesClassName, *actualPVVolumeAttributesClassName)
160+
if test.expectedPVVolumeAttributesClassName != nil && actualPVVolumeAttributesClassName != nil {
161+
if diff := cmp.Diff(*test.expectedPVVolumeAttributesClassName, *actualPVVolumeAttributesClassName); diff != "" {
162+
t.Errorf("expected VolumeAttributesClassName of pv to be %v, got %v", *test.expectedPVVolumeAttributesClassName, *actualPVVolumeAttributesClassName)
163+
}
140164
}
141165

142-
if test.withExtraMetadata {
166+
if test.withExtraMetadata && test.expectedPVVolumeAttributesClassName != nil {
143167
vacObj, err := ctrlInstance.vacLister.Get(*test.expectedPVVolumeAttributesClassName)
144168
if err != nil {
145169
t.Errorf("failed to get VAC: %v", err)
@@ -154,7 +178,7 @@ func TestModify(t *testing.T) {
154178
}
155179
}
156180

157-
func createTestPVC(pvcName string, vacName string, curVacName string, targetVacName string) *v1.PersistentVolumeClaim {
181+
func createTestPVC(pvcName string, vacName string, curVacName string, targetVacName string, modifyVolumeStatus v1.PersistentVolumeClaimModifyVolumeStatus) *v1.PersistentVolumeClaim {
158182
pvc := &v1.PersistentVolumeClaim{
159183
ObjectMeta: metav1.ObjectMeta{Name: pvcName, Namespace: pvcNamespace},
160184
Spec: v1.PersistentVolumeClaimSpec{
@@ -167,21 +191,25 @@ func createTestPVC(pvcName string, vacName string, curVacName string, targetVacN
167191
v1.ResourceName(v1.ResourceStorage): resource.MustParse("2Gi"),
168192
},
169193
},
170-
VolumeAttributesClassName: &vacName,
171-
VolumeName: pvName,
194+
VolumeName: pvName,
172195
},
173196
Status: v1.PersistentVolumeClaimStatus{
174197
Phase: v1.ClaimBound,
175198
Capacity: v1.ResourceList{
176199
v1.ResourceStorage: resource.MustParse("2Gi"),
177200
},
178-
CurrentVolumeAttributesClassName: &curVacName,
179201
ModifyVolumeStatus: &v1.ModifyVolumeStatus{
180202
TargetVolumeAttributesClassName: targetVacName,
181-
Status: "",
203+
Status: modifyVolumeStatus,
182204
},
183205
},
184206
}
207+
if vacName != "" {
208+
pvc.Spec.VolumeAttributesClassName = &vacName
209+
}
210+
if curVacName != "" {
211+
pvc.Status.CurrentVolumeAttributesClassName = &curVacName
212+
}
185213
return pvc
186214
}
187215

0 commit comments

Comments
 (0)