Skip to content

Commit 56ff79c

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 5678513 commit 56ff79c

File tree

5 files changed

+79
-60
lines changed

5 files changed

+79
-60
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: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,12 @@ import (
2323

2424
func TestController(t *testing.T) {
2525
basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, testVac /*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+
rolledBackPVC := createTestPVC(pvcName, "" /*vacName*/, "" /*curVacName*/, testVac /*targetVacName*/)
154+
rolledBackPVC.Status.ModifyVolumeStatus.Status = v1.PersistentVolumeClaimModifyVolumeInfeasible
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: rolledBackPVC,
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",
200+
name: "Should execute ModifyVolume(target) if PVC has empty Spec.VACName",
192201
pvc: createTestPVC(pvcName, "" /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/),
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
@@ -89,7 +89,10 @@ func (ctrl *modifyController) updateConditionBasedOnError(pvc *v1.PersistentVolu
8989
// markControllerModifyVolumeStatus will mark ModifyVolumeStatus as completed in the PVC
9090
// and update CurrentVolumeAttributesClassName, clear the conditions
9191
func (ctrl *modifyController) markControllerModifyVolumeCompleted(pvc *v1.PersistentVolumeClaim, pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error) {
92-
modifiedVacName := pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName
92+
var modifiedVacName *string
93+
if pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName != "" {
94+
modifiedVacName = &pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName
95+
}
9396

9497
// Update PVC
9598
newPVC := pvc.DeepCopy()
@@ -98,14 +101,14 @@ func (ctrl *modifyController) markControllerModifyVolumeCompleted(pvc *v1.Persis
98101
newPVC.Status.ModifyVolumeStatus = nil
99102

100103
// Update CurrentVolumeAttributesClassName
101-
newPVC.Status.CurrentVolumeAttributesClassName = &modifiedVacName
104+
newPVC.Status.CurrentVolumeAttributesClassName = modifiedVacName
102105

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

106109
// Update PV
107110
newPV := pv.DeepCopy()
108-
newPV.Spec.VolumeAttributesClassName = &modifiedVacName
111+
newPV.Spec.VolumeAttributesClassName = modifiedVacName
109112

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

pkg/modifycontroller/modify_volume.go

Lines changed: 51 additions & 43 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,59 +52,68 @@ 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-
if !inUncertainState {
63-
klog.V(3).InfoS("previous operation on the PVC failed with a final error, retrying")
64-
return ctrl.validateVACAndModifyVolumeWithTarget(pvc, pv)
65-
} else {
66-
vac, err := ctrl.vacLister.Get(*pvcSpecVacName)
67-
if err != nil {
68-
if apierrors.IsNotFound(err) {
69-
ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, "VAC "+*pvcSpecVacName+" does not exist.")
70-
}
71-
return pvc, pv, err, false
72-
}
73-
return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac, pvcSpecVacName)
55+
pvcSpecVacName := ptr.Deref(pvc.Spec.VolumeAttributesClassName, "")
56+
curVacName := ptr.Deref(pvc.Status.CurrentVolumeAttributesClassName, "")
57+
if pvc.Status.ModifyVolumeStatus == nil && pvcSpecVacName == curVacName {
58+
// No modification required, already reached target state
59+
return pvc, pv, nil, false
60+
}
61+
62+
if pvcSpecVacName == "" &&
63+
(pvc.Status.ModifyVolumeStatus == nil || pvc.Status.ModifyVolumeStatus.Status == v1.PersistentVolumeClaimModifyVolumeInfeasible) {
64+
// User don't care the target state, and we've reached a relatively stable state. Just keep it here.
65+
// Note: APIServer generally not allowing setting pvcSpecVacName to empty when curVacName is not empty.
66+
klog.V(4).InfoS("stop reconcile for rolled back PVC", "PV", klog.KObj(pv))
67+
return pvc, pv, nil, false
68+
}
69+
70+
// Check if we should change our target
71+
_, inUncertainState := ctrl.uncertainPVCs.Load(pvcKey)
72+
if inUncertainState || pvcSpecVacName == "" {
73+
vac, err := ctrl.getTargetVAC(pvc, pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName)
74+
if err != nil {
75+
return pvc, pv, err, false
7476
}
77+
return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac)
7578
}
7679

77-
// No modification required
78-
return pvc, pv, nil, false
80+
return ctrl.validateVACAndModifyVolumeWithTarget(pvc, pv)
81+
}
82+
83+
func (ctrl *modifyController) getTargetVAC(pvc *v1.PersistentVolumeClaim, vacName string) (*storagev1beta1.VolumeAttributesClass, error) {
84+
vac, err := ctrl.vacLister.Get(vacName)
85+
// Check if pvcSpecVac is valid and exist
86+
if err != nil {
87+
if apierrors.IsNotFound(err) {
88+
ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, "VAC %q does not exist.", vacName)
89+
}
90+
return nil, fmt.Errorf("get VAC with vac name %s in VACInformer cache failed: %w", vacName, err)
91+
}
92+
return vac, nil
7993
}
8094

8195
// func validateVACAndModifyVolumeWithTarget validate the VAC. The function sets pvc.Status.ModifyVolumeStatus
8296
// to Pending if VAC does not exist and proceeds to trigger ModifyVolume if VAC exists
8397
func (ctrl *modifyController) validateVACAndModifyVolumeWithTarget(
8498
pvc *v1.PersistentVolumeClaim,
8599
pv *v1.PersistentVolume) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) {
86-
// The controller only triggers ModifyVolume if pvcSpecVacName is not nil nor empty
87-
pvcSpecVacName := pvc.Spec.VolumeAttributesClassName
88-
// Check if pvcSpecVac is valid and exist
89-
vac, err := ctrl.vacLister.Get(*pvcSpecVacName)
90-
if err == nil {
91-
// Mark pvc.Status.ModifyVolumeStatus as in progress
92-
pvc, err = ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumeInProgress, nil)
93-
if err != nil {
94-
return pvc, pv, err, false
95-
}
96-
// Record an event to indicate that external resizer is modifying this volume.
97-
ctrl.eventRecorder.Event(pvc, v1.EventTypeNormal, util.VolumeModify,
98-
fmt.Sprintf("external resizer is modifying volume %s with vac %s", pvc.Name, *pvcSpecVacName))
99-
return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac, pvcSpecVacName)
100-
} else {
101-
if apierrors.IsNotFound(err) {
102-
ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, "VAC "+*pvcSpecVacName+" does not exist.")
103-
}
104-
klog.Errorf("Get VAC with vac name %s in VACInformer cache failed: %v", *pvcSpecVacName, err)
100+
101+
vac, err := ctrl.getTargetVAC(pvc, *pvc.Spec.VolumeAttributesClassName)
102+
if err != nil {
105103
// Mark pvc.Status.ModifyVolumeStatus as pending
106104
pvc, err = ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumePending, nil)
107105
return pvc, pv, err, false
108106
}
107+
108+
// Mark pvc.Status.ModifyVolumeStatus as in progress
109+
pvc, err = ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumeInProgress, nil)
110+
if err != nil {
111+
return pvc, pv, err, false
112+
}
113+
// Record an event to indicate that external resizer is modifying this volume.
114+
ctrl.eventRecorder.Event(pvc, v1.EventTypeNormal, util.VolumeModify,
115+
fmt.Sprintf("external resizer is modifying volume %s with vac %s", pvc.Name, vac.Name))
116+
return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac)
109117
}
110118

111119
// func controllerModifyVolumeWithTarget trigger the CSI ControllerModifyVolume API call
@@ -114,11 +122,11 @@ func (ctrl *modifyController) controllerModifyVolumeWithTarget(
114122
pvc *v1.PersistentVolumeClaim,
115123
pv *v1.PersistentVolume,
116124
vacObj *storagev1beta1.VolumeAttributesClass,
117-
pvcSpecVacName *string) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) {
125+
) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) {
118126
var err error
119127
pvc, pv, err = ctrl.callModifyVolumeOnPlugin(pvc, pv, vacObj)
120128
if err == nil {
121-
klog.V(4).Infof("Update volumeAttributesClass of PV %q to %s succeeded", pv.Name, *pvcSpecVacName)
129+
klog.V(4).Infof("Update volumeAttributesClass of PV %q to %s succeeded", pv.Name, vacObj.Name)
122130
// Record an event to indicate that modify operation is successful.
123131
ctrl.eventRecorder.Eventf(pvc, v1.EventTypeNormal, util.VolumeModifySuccess, fmt.Sprintf("external resizer modified volume %s with vac %s successfully", pvc.Name, vacObj.Name))
124132
return pvc, pv, nil, true

pkg/modifycontroller/modify_volume_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ var (
3333

3434
func TestModify(t *testing.T) {
3535
basePVC := createTestPVC(pvcName, testVac /*vacName*/, testVac /*curVacName*/, testVac /*targetVacName*/)
36+
basePVC.Status.ModifyVolumeStatus = nil
3637
basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
3738

3839
var tests = []struct {

0 commit comments

Comments
 (0)