Skip to content

Commit 19cef74

Browse files
committed
modify: support rollback
Support rollback to VAC A if modifying from A to B failed with a final error. This works just like we modifying it again to C on final error. The significant changes in the sync logic: - Always retry if pvc.Status.ModifyVolumeStatus is not nil, which means the last transation does not finish successfully. - Keep reconciling to the previous target if spec is rolled back to nil, until it succeeds or we get an infeasible error. Then we just leave it at its current state and stop reconciling for it, since user may not care about it now.
1 parent 677f582 commit 19cef74

File tree

5 files changed

+94
-65
lines changed

5 files changed

+94
-65
lines changed

pkg/modifycontroller/controller.go

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import (
3939
"k8s.io/client-go/tools/record"
4040
"k8s.io/client-go/util/workqueue"
4141
"k8s.io/klog/v2"
42+
"k8s.io/utils/ptr"
4243
)
4344

4445
// ModifyController watches PVCs and checks if they are requesting an modify operation.
@@ -128,7 +129,7 @@ func (ctrl *modifyController) initUncertainPVCs() error {
128129
return err
129130
}
130131
for _, pvc := range allPVCs {
131-
if pvc.Status.ModifyVolumeStatus != nil && (pvc.Status.ModifyVolumeStatus.Status == v1.PersistentVolumeClaimModifyVolumeInProgress || pvc.Status.ModifyVolumeStatus.Status == v1.PersistentVolumeClaimModifyVolumeInfeasible) {
132+
if pvc.Status.ModifyVolumeStatus != nil && (pvc.Status.ModifyVolumeStatus.Status == v1.PersistentVolumeClaimModifyVolumeInProgress) {
132133
pvcKey, err := cache.MetaNamespaceKeyFunc(pvc)
133134
if err != nil {
134135
return err
@@ -160,12 +161,11 @@ func (ctrl *modifyController) updatePVC(oldObj, newObj interface{}) {
160161
}
161162

162163
// Only trigger modify volume if the following conditions are met
163-
// 1. Non empty vac name
164-
// 2. oldVacName != newVacName
165-
// 3. PVC is in Bound state
166-
oldVacName := oldPVC.Spec.VolumeAttributesClassName
167-
newVacName := newPVC.Spec.VolumeAttributesClassName
168-
if newVacName != nil && *newVacName != "" && (oldVacName == nil || *newVacName != *oldVacName) && oldPVC.Status.Phase == v1.ClaimBound {
164+
// 1. VAC changed or modify finished (check pending modify request while we are modifying)
165+
// 2. PVC is in Bound state
166+
oldVacName := ptr.Deref(oldPVC.Spec.VolumeAttributesClassName, "")
167+
newVacName := ptr.Deref(newPVC.Spec.VolumeAttributesClassName, "")
168+
if (newVacName != oldVacName || newPVC.Status.ModifyVolumeStatus == nil) && newPVC.Status.Phase == v1.ClaimBound {
169169
_, err := ctrl.pvLister.Get(oldPVC.Spec.VolumeName)
170170
if err != nil {
171171
klog.Errorf("Get PV %q of pvc %q in PVInformer cache failed: %v", oldPVC.Spec.VolumeName, klog.KObj(oldPVC), err)
@@ -267,15 +267,13 @@ func (ctrl *modifyController) syncPVC(key string) error {
267267

268268
// Only trigger modify volume if the following conditions are met
269269
// 1. PV provisioned by CSI driver AND driver name matches local driver
270-
// 2. Non-empty vac name
271-
// 3. PVC is in Bound state
270+
// 2. PVC is in Bound state
272271
if pv.Spec.CSI == nil || pv.Spec.CSI.Driver != ctrl.name {
273272
klog.V(7).InfoS("Skipping PV provisioned by different driver", "PV", klog.KObj(pv))
274273
return nil
275274
}
276275

277-
vacName := pvc.Spec.VolumeAttributesClassName
278-
if vacName != nil && *vacName != "" && pvc.Status.Phase == v1.ClaimBound {
276+
if pvc.Status.Phase == v1.ClaimBound {
279277
_, _, err, _ := ctrl.modify(pvc, pv)
280278
if err != nil {
281279
return err

pkg/modifycontroller/controller_test.go

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,13 @@ import (
2222
)
2323

2424
func TestController(t *testing.T) {
25-
basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/)
25+
basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, "" /*targetVacName*/)
26+
basePVC.Status.ModifyVolumeStatus = nil
2627
basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
2728
firstTimePV := basePV.DeepCopy()
2829
firstTimePV.Spec.VolumeAttributesClassName = nil
2930
firstTimePVC := basePVC.DeepCopy()
3031
firstTimePVC.Status.CurrentVolumeAttributesClassName = nil
31-
firstTimePVC.Status.ModifyVolumeStatus = nil
3232

3333
tests := []struct {
3434
name string
@@ -150,6 +150,9 @@ func TestSyncPVC(t *testing.T) {
150150
pvcWithUncreatedPV := createTestPVC(pvcName, targetVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/)
151151
pvcWithUncreatedPV.Spec.VolumeName = ""
152152

153+
inprogressPVC := createTestPVC(pvcName, "" /*vacName*/, "" /*curVacName*/, testVac /*targetVacName*/)
154+
inprogressPVC.Status.ModifyVolumeStatus.Status = v1.PersistentVolumeClaimModifyVolumeInProgress
155+
153156
nonCSIPVC := &v1.PersistentVolumeClaim{
154157
ObjectMeta: metav1.ObjectMeta{Name: pvcName, Namespace: pvcNamespace},
155158
Spec: v1.PersistentVolumeClaimSpec{
@@ -181,17 +184,23 @@ func TestSyncPVC(t *testing.T) {
181184
pv: basePV,
182185
callCSIModify: true,
183186
},
187+
{
188+
name: "Should NOT modify when rollback to empty VACName",
189+
pvc: createTestPVC(pvcName, "" /*vacName*/, "" /*curVacName*/, testVac /*targetVacName*/),
190+
pv: basePV,
191+
callCSIModify: false,
192+
},
184193
{
185194
name: "Should NOT modify if PVC managed by another CSI Driver",
186195
pvc: basePVC,
187196
pv: otherDriverPV,
188197
callCSIModify: false,
189198
},
190199
{
191-
name: "Should NOT modify if PVC has empty Spec.VACName",
192-
pvc: createTestPVC(pvcName, "" /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/),
200+
name: "Should execute ModifyVolume for InProgress target if PVC has empty Spec.VACName",
201+
pvc: inprogressPVC,
193202
pv: basePV,
194-
callCSIModify: false,
203+
callCSIModify: true,
195204
},
196205
{
197206
name: "Should NOT modify if PVC not in bound state",

pkg/modifycontroller/modify_status.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,10 @@ func (ctrl *modifyController) updateConditionBasedOnError(pvc *v1.PersistentVolu
9090
// markControllerModifyVolumeStatus will mark ModifyVolumeStatus as completed in the PVC
9191
// and update CurrentVolumeAttributesClassName, clear the conditions
9292
func (ctrl *modifyController) markControllerModifyVolumeCompleted(pvc *v1.PersistentVolumeClaim, pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error) {
93-
modifiedVacName := pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName
93+
var modifiedVacName *string
94+
if pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName != "" {
95+
modifiedVacName = &pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName
96+
}
9497

9598
// Update PVC
9699
newPVC := pvc.DeepCopy()
@@ -99,14 +102,14 @@ func (ctrl *modifyController) markControllerModifyVolumeCompleted(pvc *v1.Persis
99102
newPVC.Status.ModifyVolumeStatus = nil
100103

101104
// Update CurrentVolumeAttributesClassName
102-
newPVC.Status.CurrentVolumeAttributesClassName = &modifiedVacName
105+
newPVC.Status.CurrentVolumeAttributesClassName = modifiedVacName
103106

104107
// Clear all the conditions related to modify volume
105108
newPVC.Status.Conditions = clearModifyVolumeConditions(newPVC.Status.Conditions)
106109

107110
// Update PV
108111
newPV := pv.DeepCopy()
109-
newPV.Spec.VolumeAttributesClassName = &modifiedVacName
112+
newPV.Spec.VolumeAttributesClassName = modifiedVacName
110113

111114
// Update PV before PVC to avoid PV not getting updated but PVC did
112115
updatedPV, err := util.PatchPersistentVolume(ctrl.kubeClient, pv, newPV)

pkg/modifycontroller/modify_volume.go

Lines changed: 60 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import (
3030
apierrors "k8s.io/apimachinery/pkg/api/errors"
3131
"k8s.io/client-go/tools/cache"
3232
"k8s.io/klog/v2"
33+
"k8s.io/utils/ptr"
3334
)
3435

3536
const (
@@ -40,8 +41,6 @@ const (
4041

4142
// The return value bool is only used as a sentinel value when function returns without actually performing modification
4243
func (ctrl *modifyController) modify(pvc *v1.PersistentVolumeClaim, pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) {
43-
pvcSpecVacName := pvc.Spec.VolumeAttributesClassName
44-
curVacName := pvc.Status.CurrentVolumeAttributesClassName
4544
pvcKey, err := cache.MetaNamespaceKeyFunc(pvc)
4645
if err != nil {
4746
return pvc, pv, err, false
@@ -53,60 +52,77 @@ func (ctrl *modifyController) modify(pvc *v1.PersistentVolumeClaim, pv *v1.Persi
5352
return pvc, pv, delayModificationErr, false
5453
}
5554

56-
if pvcSpecVacName != nil && curVacName == nil {
57-
// First time adding VAC to a PVC
58-
return ctrl.validateVACAndModifyVolumeWithTarget(pvc, pv)
59-
} else if pvcSpecVacName != nil && curVacName != nil && *pvcSpecVacName != *curVacName {
60-
// Check if PVC in uncertain state
61-
_, inUncertainState := ctrl.uncertainPVCs.Load(pvcKey)
62-
status := pvc.Status.ModifyVolumeStatus
63-
if !inUncertainState || status == nil {
64-
klog.V(3).InfoS("previous operation on the PVC succeeded or failed with a final error, retrying")
65-
return ctrl.validateVACAndModifyVolumeWithTarget(pvc, pv)
66-
} else {
67-
vac, err := ctrl.vacLister.Get(status.TargetVolumeAttributesClassName)
68-
if err != nil {
69-
if apierrors.IsNotFound(err) {
70-
ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, "VAC "+status.TargetVolumeAttributesClassName+" does not exist.")
71-
}
72-
return pvc, pv, err, false
73-
}
74-
return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac, pvcSpecVacName)
55+
pvcSpecVacName := ptr.Deref(pvc.Spec.VolumeAttributesClassName, "")
56+
curVacName := ptr.Deref(pvc.Status.CurrentVolumeAttributesClassName, "")
57+
status := pvc.Status.ModifyVolumeStatus
58+
59+
if status == nil && pvcSpecVacName == curVacName {
60+
// No modification required, already reached target state
61+
return pvc, pv, nil, false
62+
}
63+
64+
if pvcSpecVacName == "" &&
65+
(status == nil || status.Status != v1.PersistentVolumeClaimModifyVolumeInProgress) {
66+
// User don't care the target state, and we've reached a relatively stable state. Just keep it here.
67+
// Note: APIServer generally not allowing setting pvcSpecVacName to empty when curVacName is not empty.
68+
klog.V(4).InfoS("stop reconcile for rolled back PVC", "PV", klog.KObj(pv))
69+
// Don't try to revert anything here, because we only record the result of the last modification.
70+
// We don't know what happened before. User can switch between InProgress/Infeasible/Pending status
71+
// freely by modifying the spec.
72+
return pvc, pv, nil, false
73+
}
74+
75+
// Check if we should change our target
76+
_, inUncertainState := ctrl.uncertainPVCs.Load(pvcKey)
77+
if (status != nil && status.Status == v1.PersistentVolumeClaimModifyVolumeInProgress && inUncertainState) || pvcSpecVacName == "" {
78+
vac, err := ctrl.getTargetVAC(pvc, status.TargetVolumeAttributesClassName)
79+
if err != nil {
80+
return pvc, pv, err, false
7581
}
82+
return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac)
7683
}
7784

78-
// No modification required
79-
return pvc, pv, nil, false
85+
// If status != InProgress && inUncertainState, we either see a stall PVC, or the status was updated externally.
86+
// For stall PVC, we will get Conflict when marking InProgress.
87+
// For status updated externally, we respect the user choice and try the new target, as if it were not uncertain.
88+
// The in-memory uncertain state will be updated after the next ControllerModifyVolume call.
89+
return ctrl.validateVACAndModifyVolumeWithTarget(pvc, pv)
90+
}
91+
92+
func (ctrl *modifyController) getTargetVAC(pvc *v1.PersistentVolumeClaim, vacName string) (*storagev1beta1.VolumeAttributesClass, error) {
93+
vac, err := ctrl.vacLister.Get(vacName)
94+
// Check if pvcSpecVac is valid and exist
95+
if err != nil {
96+
if apierrors.IsNotFound(err) {
97+
ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, "VAC %q does not exist.", vacName)
98+
}
99+
return nil, fmt.Errorf("get VAC with vac name %s in VACInformer cache failed: %w", vacName, err)
100+
}
101+
return vac, nil
80102
}
81103

82104
// func validateVACAndModifyVolumeWithTarget validate the VAC. The function sets pvc.Status.ModifyVolumeStatus
83105
// to Pending if VAC does not exist and proceeds to trigger ModifyVolume if VAC exists
84106
func (ctrl *modifyController) validateVACAndModifyVolumeWithTarget(
85107
pvc *v1.PersistentVolumeClaim,
86108
pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) {
87-
// The controller only triggers ModifyVolume if pvcSpecVacName is not nil nor empty
88-
pvcSpecVacName := pvc.Spec.VolumeAttributesClassName
89-
// Check if pvcSpecVac is valid and exist
90-
vac, err := ctrl.vacLister.Get(*pvcSpecVacName)
91-
if err == nil {
92-
// Mark pvc.Status.ModifyVolumeStatus as in progress
93-
pvc, err = ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumeInProgress, nil)
94-
if err != nil {
95-
return pvc, pv, err, false
96-
}
97-
// Record an event to indicate that external resizer is modifying this volume.
98-
ctrl.eventRecorder.Event(pvc, v1.EventTypeNormal, util.VolumeModify,
99-
fmt.Sprintf("external resizer is modifying volume %s with vac %s", pvc.Name, *pvcSpecVacName))
100-
return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac, pvcSpecVacName)
101-
} else {
102-
if apierrors.IsNotFound(err) {
103-
ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, "VAC "+*pvcSpecVacName+" does not exist.")
104-
}
105-
klog.Errorf("Get VAC with vac name %s in VACInformer cache failed: %v", *pvcSpecVacName, err)
109+
110+
vac, err := ctrl.getTargetVAC(pvc, *pvc.Spec.VolumeAttributesClassName)
111+
if err != nil {
106112
// Mark pvc.Status.ModifyVolumeStatus as pending
107113
pvc, err = ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumePending, nil)
108114
return pvc, pv, err, false
109115
}
116+
117+
// Mark pvc.Status.ModifyVolumeStatus as in progress
118+
pvc, err = ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumeInProgress, nil)
119+
if err != nil {
120+
return pvc, pv, err, false
121+
}
122+
// Record an event to indicate that external resizer is modifying this volume.
123+
ctrl.eventRecorder.Event(pvc, v1.EventTypeNormal, util.VolumeModify,
124+
fmt.Sprintf("external resizer is modifying volume %s with vac %s", pvc.Name, vac.Name))
125+
return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac)
110126
}
111127

112128
// func controllerModifyVolumeWithTarget trigger the CSI ControllerModifyVolume API call
@@ -115,11 +131,11 @@ func (ctrl *modifyController) controllerModifyVolumeWithTarget(
115131
pvc *v1.PersistentVolumeClaim,
116132
pv *v1.PersistentVolume,
117133
vacObj *storagev1beta1.VolumeAttributesClass,
118-
pvcSpecVacName *string) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) {
134+
) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) {
119135
var err error
120136
pvc, pv, err = ctrl.callModifyVolumeOnPlugin(pvc, pv, vacObj)
121137
if err == nil {
122-
klog.V(4).Infof("Update volumeAttributesClass of PV %q to %s succeeded", pv.Name, *pvcSpecVacName)
138+
klog.V(4).Infof("Update volumeAttributesClass of PV %q to %s succeeded", pv.Name, vacObj.Name)
123139
// Record an event to indicate that modify operation is successful.
124140
ctrl.eventRecorder.Eventf(pvc, v1.EventTypeNormal, util.VolumeModifySuccess, fmt.Sprintf("external resizer modified volume %s with vac %s successfully", pvc.Name, vacObj.Name))
125141
return pvc, pv, nil, true

pkg/modifycontroller/modify_volume_test.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ var (
3333
)
3434

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

3939
var tests = []struct {
@@ -220,10 +220,13 @@ func createTestPVC(pvcName string, vacName string, curVacName string, targetVacN
220220
CurrentVolumeAttributesClassName: &curVacName,
221221
ModifyVolumeStatus: &v1.ModifyVolumeStatus{
222222
TargetVolumeAttributesClassName: targetVacName,
223-
Status: "",
223+
Status: v1.PersistentVolumeClaimModifyVolumeInfeasible,
224224
},
225225
},
226226
}
227+
if targetVacName == "" {
228+
pvc.Status.ModifyVolumeStatus = nil
229+
}
227230
return pvc
228231
}
229232

0 commit comments

Comments
 (0)