Skip to content

Commit 42bb9bf

Browse files
sunnylovestiramisusunnylovestiramisu
committed
Enable Rollback Support of VAC with Infeasible Errors
1 parent 26a69fc commit 42bb9bf

File tree

8 files changed

+254
-40
lines changed

8 files changed

+254
-40
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: 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, &emptyString /*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: 41 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,44 @@ 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+
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+
76115
func (ctrl *modifyController) updateConditionBasedOnError(pvc *v1.PersistentVolumeClaim, err error) (*v1.PersistentVolumeClaim, error) {
77116
newPVC := pvc.DeepCopy()
78117
pvcCondition := v1.PersistentVolumeClaimCondition{
@@ -96,7 +135,7 @@ func (ctrl *modifyController) updateConditionBasedOnError(pvc *v1.PersistentVolu
96135
return updatedPVC, nil
97136
}
98137

99-
// markControllerModifyVolumeStatus will mark ModifyVolumeStatus as completed in the PVC
138+
// markControllerModifyVolumeCompleted will mark ModifyVolumeStatus as completed in the PVC
100139
// and update CurrentVolumeAttributesClassName, clear the conditions
101140
func (ctrl *modifyController) markControllerModifyVolumeCompleted(pvc *v1.PersistentVolumeClaim, pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error) {
102141
modifiedVacName := pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName
@@ -131,7 +170,7 @@ func (ctrl *modifyController) markControllerModifyVolumeCompleted(pvc *v1.Persis
131170
return updatedPVC, updatedPV, nil
132171
}
133172

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

‎pkg/modifycontroller/modify_status_test.go

Lines changed: 5 additions & 2 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"
@@ -397,10 +398,12 @@ func createTestPV(capacityGB int, pvcName, pvcNamespace string, pvcUID types.UID
397398
VolumeHandle: "foo",
398399
},
399400
},
400-
VolumeMode: volumeMode,
401-
VolumeAttributesClassName: &vacName,
401+
VolumeMode: volumeMode,
402402
},
403403
}
404+
if vacName != "" {
405+
pv.Spec.VolumeAttributesClassName = &vacName
406+
}
404407
if len(pvcName) > 0 {
405408
pv.Spec.ClaimRef = &v1.ObjectReference{
406409
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.markCotrollerModifyVolumeRollbackCompeleted(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)