Skip to content

Commit 5b33df4

Browse files
sunnylovestiramisusunnylovestiramisu
committed
Enable Rollback Support of VAC with Infeasible Errors
1 parent f28f38b commit 5b33df4

File tree

8 files changed

+250
-46
lines changed

8 files changed

+250
-46
lines changed

‎pkg/modifycontroller/controller.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -278,7 +278,7 @@ func (ctrl *modifyController) syncPVC(key string) error {
278278
}
279279

280280
vacName := pvc.Spec.VolumeAttributesClassName
281-
if vacName != nil && *vacName != "" && pvc.Status.Phase == v1.ClaimBound {
281+
if (vacName != nil && *vacName != "" && pvc.Status.Phase == v1.ClaimBound) || (util.CurrentModificationInfeasible(pvc) && util.IsVacRolledBack(pvc)) {
282282
_, _, err, _ := ctrl.modify(pvc, pv)
283283
if err != nil {
284284
return err

‎pkg/modifycontroller/controller_test.go‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@ import (
2424
)
2525

2626
func TestController(t *testing.T) {
27-
basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/)
28-
basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
27+
basePVC := createTestPVC(pvcName, &testVac /*vacName*/, &testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/)
28+
basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, &testVac)
2929
firstTimePV := basePV.DeepCopy()
3030
firstTimePV.Spec.VolumeAttributesClassName = nil
3131
firstTimePVC := basePVC.DeepCopy()
@@ -41,7 +41,7 @@ func TestController(t *testing.T) {
4141
}{
4242
{
4343
name: "Modify called",
44-
pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/),
44+
pvc: createTestPVC(pvcName, &targetVac /*vacName*/, &testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/),
4545
pv: basePV,
4646
vacExists: true,
4747
callCSIModify: true,
@@ -89,7 +89,7 @@ func TestController(t *testing.T) {
8989
}
9090

9191
func TestModifyPVC(t *testing.T) {
92-
basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
92+
basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, &testVac)
9393

9494
tests := []struct {
9595
name string
@@ -100,14 +100,14 @@ func TestModifyPVC(t *testing.T) {
100100
}{
101101
{
102102
name: "Modify succeeded",
103-
pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/),
103+
pvc: createTestPVC(pvcName, &targetVac /*vacName*/, &testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/),
104104
pv: basePV,
105105
modifyFailure: false,
106106
expectFailure: false,
107107
},
108108
{
109109
name: "Modify failed",
110-
pvc: createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/),
110+
pvc: createTestPVC(pvcName, &targetVac /*vacName*/, &testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/),
111111
pv: basePV,
112112
modifyFailure: true,
113113
expectFailure: true,
@@ -140,16 +140,16 @@ func TestModifyPVC(t *testing.T) {
140140
}
141141

142142
func TestSyncPVC(t *testing.T) {
143-
basePVC := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/)
144-
basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
143+
basePVC := createTestPVC(pvcName, &targetVac /*vacName*/, &testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/)
144+
basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, &testVac)
145145

146-
otherDriverPV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
146+
otherDriverPV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, &testVac)
147147
otherDriverPV.Spec.PersistentVolumeSource.CSI.Driver = "some-other-driver"
148148

149-
unboundPVC := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/)
149+
unboundPVC := createTestPVC(pvcName, &targetVac /*vacName*/, &testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/)
150150
unboundPVC.Status.Phase = v1.ClaimPending
151151

152-
pvcWithUncreatedPV := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/)
152+
pvcWithUncreatedPV := createTestPVC(pvcName, &targetVac /*vacName*/, &testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/)
153153
pvcWithUncreatedPV.Spec.VolumeName = ""
154154

155155
nonCSIPVC := &v1.PersistentVolumeClaim{
@@ -191,7 +191,7 @@ func TestSyncPVC(t *testing.T) {
191191
},
192192
{
193193
name: "Should NOT modify if PVC has empty Spec.VACName",
194-
pvc: createTestPVC(pvcName, "" /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/),
194+
pvc: createTestPVC(pvcName, &emptyString /*vacName*/, &testVac /*curVacName*/, testVac /*targetVacName*/, "" /*modifyVolumeStatus*/),
195195
pv: basePV,
196196
callCSIModify: false,
197197
},
@@ -241,8 +241,8 @@ func TestSyncPVC(t *testing.T) {
241241

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

247247
tests := []struct {
248248
name string

‎pkg/modifycontroller/modify_status.go‎

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,44 @@ func (ctrl *modifyController) markControllerModifyVolumeStatus(
7474
return updatedPVC, nil
7575
}
7676

77+
func (ctrl *modifyController) markControllerModifyVolumeRollbackCompeleted(
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+
if pvc.Spec.VolumeAttributesClassName != nil && *pvc.Spec.VolumeAttributesClassName == "" {
95+
// PV does not support empty string, set VolumeAttributesClassName to nil
96+
newPV.Spec.VolumeAttributesClassName = nil
97+
} else {
98+
newPV.Spec.VolumeAttributesClassName = pvc.Spec.VolumeAttributesClassName
99+
}
100+
101+
// Update PV before PVC to avoid PV not getting updated but PVC did
102+
updatedPV, err := util.PatchPersistentVolume(ctrl.kubeClient, pv, newPV)
103+
if err != nil {
104+
return pvc, pv, fmt.Errorf("update pv.Spec.VolumeAttributesClassName for PVC %q failed, errored with: %v", pvc.Name, err)
105+
}
106+
107+
updatedPVC, err := util.PatchClaim(ctrl.kubeClient, pvc, newPVC, false /* addResourceVersionCheck */)
108+
if err != nil {
109+
return pvc, pv, fmt.Errorf("mark PVC %q as ModifyVolumeCompleted failed, errored with: %v", pvc.Name, err)
110+
}
111+
112+
return updatedPVC, updatedPV, nil
113+
}
114+
77115
func (ctrl *modifyController) updateConditionBasedOnError(pvc *v1.PersistentVolumeClaim, err error) (*v1.PersistentVolumeClaim, error) {
78116
newPVC := pvc.DeepCopy()
79117
pvcCondition := v1.PersistentVolumeClaimCondition{
@@ -97,7 +135,7 @@ func (ctrl *modifyController) updateConditionBasedOnError(pvc *v1.PersistentVolu
97135
return updatedPVC, nil
98136
}
99137

100-
// markControllerModifyVolumeStatus will mark ModifyVolumeStatus as completed in the PVC
138+
// markControllerModifyVolumeCompleted will mark ModifyVolumeStatus as completed in the PVC
101139
// and update CurrentVolumeAttributesClassName, clear the conditions
102140
func (ctrl *modifyController) markControllerModifyVolumeCompleted(pvc *v1.PersistentVolumeClaim, pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error) {
103141
modifiedVacName := pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName
@@ -132,7 +170,7 @@ func (ctrl *modifyController) markControllerModifyVolumeCompleted(pvc *v1.Persis
132170
return updatedPVC, updatedPV, nil
133171
}
134172

135-
// markControllerModifyVolumeStatus clears all the conditions related to modify volume and only
173+
// clearModifyVolumeConditions clears all the conditions related to modify volume and only
136174
// leave other condition types
137175
func clearModifyVolumeConditions(conditions []v1.PersistentVolumeClaimCondition) []v1.PersistentVolumeClaimCondition {
138176
knownConditions := []v1.PersistentVolumeClaimCondition{}

‎pkg/modifycontroller/modify_status_test.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ const (
3333

3434
var (
3535
fsVolumeMode = v1.PersistentVolumeFilesystem
36+
emptyString = ""
3637
testVac = "test-vac"
3738
targetVac = "target-vac"
3839
testDriverName = "mock"
@@ -199,7 +200,7 @@ func TestUpdateConditionBasedOnError(t *testing.T) {
199200

200201
func TestMarkControllerModifyVolumeCompleted(t *testing.T) {
201202
basePVC := testutil.MakeTestPVC([]v1.PersistentVolumeClaimCondition{})
202-
basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
203+
basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, &testVac)
203204
expectedPV := basePV.DeepCopy()
204205
expectedPV.Spec.VolumeAttributesClassName = &targetVac
205206
expectedPVC := basePVC.WithCurrentVolumeAttributesClassName(targetVac).Get()
@@ -377,7 +378,7 @@ func TestRemovePVCFromModifyVolumeUncertainCache(t *testing.T) {
377378
}
378379
}
379380

380-
func createTestPV(capacityGB int, pvcName, pvcNamespace string, pvcUID types.UID, volumeMode *v1.PersistentVolumeMode, vacName string) *v1.PersistentVolume {
381+
func createTestPV(capacityGB int, pvcName, pvcNamespace string, pvcUID types.UID, volumeMode *v1.PersistentVolumeMode, vacName *string) *v1.PersistentVolume {
381382
capacity := testutil.QuantityGB(capacityGB)
382383

383384
pv := &v1.PersistentVolume{
@@ -395,10 +396,11 @@ func createTestPV(capacityGB int, pvcName, pvcNamespace string, pvcUID types.UID
395396
VolumeHandle: "foo",
396397
},
397398
},
399+
VolumeAttributesClassName: vacName,
398400
VolumeMode: volumeMode,
399-
VolumeAttributesClassName: &vacName,
400401
},
401402
}
403+
402404
if len(pvcName) > 0 {
403405
pv.Spec.ClaimRef = &v1.ObjectReference{
404406
Namespace: pvcNamespace,

‎pkg/modifycontroller/modify_volume.go‎

Lines changed: 37 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,9 @@ const (
3939

4040
// The return value bool is only used as a sentinel value when function returns without actually performing modification
4141
func (ctrl *modifyController) modify(pvc *v1.PersistentVolumeClaim, pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) {
42-
pvcSpecVacName := pvc.Spec.VolumeAttributesClassName
43-
curVacName := pvc.Status.CurrentVolumeAttributesClassName
42+
pvcSpecVACName := pvc.Spec.VolumeAttributesClassName
43+
currentVacName := pvc.Status.CurrentVolumeAttributesClassName
44+
4445
pvcKey, err := cache.MetaNamespaceKeyFunc(pvc)
4546
if err != nil {
4647
return pvc, pv, err, false
@@ -52,28 +53,34 @@ func (ctrl *modifyController) modify(pvc *v1.PersistentVolumeClaim, pv *v1.Persi
5253
return pvc, pv, delayModificationErr, false
5354
}
5455

55-
if pvcSpecVacName != nil && curVacName == nil {
56-
// First time adding VAC to a PVC
56+
if util.CurrentModificationInfeasible(pvc) && util.IsVacRolledBack(pvc) {
57+
return ctrl.validateVACAndRollback(pvc, pv)
58+
}
59+
60+
if pvcSpecVACName == nil {
61+
return pvc, pv, nil, false
62+
}
63+
64+
switch {
65+
case currentVacName == nil:
5766
return ctrl.validateVACAndModifyVolumeWithTarget(pvc, pv)
58-
} else if pvcSpecVacName != nil && curVacName != nil && *pvcSpecVacName != *curVacName {
67+
case *currentVacName != *pvcSpecVACName:
5968
// Check if PVC in uncertain state
6069
_, inUncertainState := ctrl.uncertainPVCs[pvcKey]
6170
if !inUncertainState {
6271
klog.V(3).InfoS("previous operation on the PVC failed with a final error, retrying")
6372
return ctrl.validateVACAndModifyVolumeWithTarget(pvc, pv)
6473
} else {
65-
vac, err := ctrl.vacLister.Get(*pvcSpecVacName)
74+
vac, err := ctrl.vacLister.Get(*pvcSpecVACName)
6675
if err != nil {
6776
if apierrors.IsNotFound(err) {
68-
ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, "VAC "+*pvcSpecVacName+" does not exist.")
77+
ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, "VAC "+*pvcSpecVACName+" does not exist.")
6978
}
7079
return pvc, pv, err, false
7180
}
72-
return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac, pvcSpecVacName)
81+
return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac, pvcSpecVACName)
7382
}
7483
}
75-
76-
// No modification required
7784
return pvc, pv, nil, false
7885
}
7986

@@ -107,6 +114,26 @@ func (ctrl *modifyController) validateVACAndModifyVolumeWithTarget(
107114
}
108115
}
109116

117+
func (ctrl *modifyController) validateVACAndRollback(
118+
pvc *v1.PersistentVolumeClaim,
119+
pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) {
120+
// The controller does not triggers ModifyVolume because it is only
121+
// for rollbacking infeasible errors
122+
// Record an event to indicate that external resizer is rolling back this volume.
123+
rollbackVACName := "nil"
124+
if pvc.Spec.VolumeAttributesClassName != nil {
125+
rollbackVACName = *pvc.Spec.VolumeAttributesClassName
126+
}
127+
ctrl.eventRecorder.Event(pvc, v1.EventTypeNormal, util.VolumeModify,
128+
fmt.Sprintf("external resizer is rolling back volume %s with infeasible error to VAC %s", pvc.Name, rollbackVACName))
129+
// Mark pvc.Status.ModifyVolumeStatus as completed
130+
pvc, pv, err := ctrl.markControllerModifyVolumeRollbackCompeleted(pvc, pv)
131+
if err != nil {
132+
return pvc, pv, fmt.Errorf("rollback volume %s modification with error: %v ", pvc.Name, err), false
133+
}
134+
return pvc, pv, nil, false
135+
}
136+
110137
// func controllerModifyVolumeWithTarget trigger the CSI ControllerModifyVolume API call
111138
// and handle both success and error scenarios
112139
func (ctrl *modifyController) controllerModifyVolumeWithTarget(

0 commit comments

Comments
 (0)