Skip to content

Commit 98ffaa2

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 98ffaa2

File tree

5 files changed

+74
-57
lines changed

5 files changed

+74
-57
lines changed

pkg/modifycontroller/controller.go

Lines changed: 8 additions & 10 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.
@@ -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: 7 additions & 1 deletion
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
@@ -181,6 +181,12 @@ func TestSyncPVC(t *testing.T) {
181181
pv: basePV,
182182
callCSIModify: true,
183183
},
184+
{
185+
name: "Should NOT modify when rollback to empty VACName",
186+
pvc: createTestPVC(pvcName, "" /*vacName*/, "" /*curVacName*/, testVac /*targetVacName*/),
187+
pv: basePV,
188+
callCSIModify: true,
189+
},
184190
{
185191
name: "Should NOT modify if PVC managed by another CSI Driver",
186192
pvc: basePVC,

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: 52 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,69 @@ 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)
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) (*storagev1beta1.VolumeAttributesClass, error) {
84+
pvcSpecVacName := *pvc.Spec.VolumeAttributesClassName
85+
vac, err := ctrl.vacLister.Get(pvcSpecVacName)
86+
// Check if pvcSpecVac is valid and exist
87+
if err != nil {
88+
if apierrors.IsNotFound(err) {
89+
ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, "VAC "+pvcSpecVacName+" does not exist.")
90+
}
91+
return nil, fmt.Errorf("get VAC with vac name %s in VACInformer cache failed: %w", pvcSpecVacName, err)
92+
}
93+
return vac, nil
7994
}
8095

8196
// func validateVACAndModifyVolumeWithTarget validate the VAC. The function sets pvc.Status.ModifyVolumeStatus
8297
// to Pending if VAC does not exist and proceeds to trigger ModifyVolume if VAC exists
8398
func (ctrl *modifyController) validateVACAndModifyVolumeWithTarget(
8499
pvc *v1.PersistentVolumeClaim,
85100
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)
101+
102+
vac, err := ctrl.getTargetVAC(pvc)
103+
if err != nil {
105104
// Mark pvc.Status.ModifyVolumeStatus as pending
106105
pvc, err = ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumePending, nil)
107106
return pvc, pv, err, false
108107
}
108+
109+
// Mark pvc.Status.ModifyVolumeStatus as in progress
110+
pvc, err = ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumeInProgress, nil)
111+
if err != nil {
112+
return pvc, pv, err, false
113+
}
114+
// Record an event to indicate that external resizer is modifying this volume.
115+
ctrl.eventRecorder.Event(pvc, v1.EventTypeNormal, util.VolumeModify,
116+
fmt.Sprintf("external resizer is modifying volume %s with vac %s", pvc.Name, vac.Name))
117+
return ctrl.controllerModifyVolumeWithTarget(pvc, pv, vac)
109118
}
110119

111120
// func controllerModifyVolumeWithTarget trigger the CSI ControllerModifyVolume API call
@@ -114,11 +123,11 @@ func (ctrl *modifyController) controllerModifyVolumeWithTarget(
114123
pvc *v1.PersistentVolumeClaim,
115124
pv *v1.PersistentVolume,
116125
vacObj *storagev1beta1.VolumeAttributesClass,
117-
pvcSpecVacName *string) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) {
126+
) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) {
118127
var err error
119128
pvc, pv, err = ctrl.callModifyVolumeOnPlugin(pvc, pv, vacObj)
120129
if err == nil {
121-
klog.V(4).Infof("Update volumeAttributesClass of PV %q to %s succeeded", pv.Name, *pvcSpecVacName)
130+
klog.V(4).Infof("Update volumeAttributesClass of PV %q to %s succeeded", pv.Name, vacObj.Name)
122131
// Record an event to indicate that modify operation is successful.
123132
ctrl.eventRecorder.Eventf(pvc, v1.EventTypeNormal, util.VolumeModifySuccess, fmt.Sprintf("external resizer modified volume %s with vac %s successfully", pvc.Name, vacObj.Name))
124133
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)