Skip to content

Commit 1f7a07b

Browse files
huww98huww98
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. - Treat empty VAC name like refering to an special VAC with empty parameters, to support the use case of rolling back from non-nil VAC name to nil. We only allow this when no modify has been successfully applied, to avoid by-passing the quota system.
1 parent 5bfedda commit 1f7a07b

File tree

5 files changed

+77
-82
lines changed

5 files changed

+77
-82
lines changed

‎pkg/modifycontroller/controller.go

Lines changed: 2 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -149,33 +149,7 @@ func (ctrl *modifyController) addPVC(obj interface{}) {
149149
}
150150

151151
func (ctrl *modifyController) updatePVC(oldObj, newObj interface{}) {
152-
oldPVC, ok := oldObj.(*v1.PersistentVolumeClaim)
153-
if !ok || oldPVC == nil {
154-
return
155-
}
156-
157-
newPVC, ok := newObj.(*v1.PersistentVolumeClaim)
158-
if !ok || newPVC == nil {
159-
return
160-
}
161-
162-
// 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 {
169-
_, err := ctrl.pvLister.Get(oldPVC.Spec.VolumeName)
170-
if err != nil {
171-
klog.Errorf("Get PV %q of pvc %q in PVInformer cache failed: %v", oldPVC.Spec.VolumeName, klog.KObj(oldPVC), err)
172-
return
173-
}
174-
// Handle modify volume by adding to the claimQueue to avoid race conditions
175-
ctrl.addPVC(newObj)
176-
} else {
177-
klog.V(4).InfoS("No need to modify PVC", "PVC", klog.KObj(newPVC))
178-
}
152+
ctrl.addPVC(newObj)
179153
}
180154

181155
func (ctrl *modifyController) deletePVC(obj interface{}) {
@@ -274,8 +248,7 @@ func (ctrl *modifyController) syncPVC(key string) error {
274248
return nil
275249
}
276250

277-
vacName := pvc.Spec.VolumeAttributesClassName
278-
if vacName != nil && *vacName != "" && pvc.Status.Phase == v1.ClaimBound {
251+
if pvc.Status.Phase == v1.ClaimBound {
279252
_, _, err, _ := ctrl.modify(pvc, pv)
280253
if err != nil {
281254
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 execute ModifyVolume operation 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: 61 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,10 @@ import (
2626
"google.golang.org/grpc/codes"
2727
"google.golang.org/grpc/status"
2828
v1 "k8s.io/api/core/v1"
29-
storagev1beta1 "k8s.io/api/storage/v1beta1"
3029
apierrors "k8s.io/apimachinery/pkg/api/errors"
3130
"k8s.io/client-go/tools/cache"
3231
"k8s.io/klog/v2"
32+
"k8s.io/utils/ptr"
3333
)
3434

3535
const (
@@ -40,8 +40,6 @@ const (
4040

4141
// The return value bool is only used as a sentinel value when function returns without actually performing modification
4242
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
4543
pvcKey, err := cache.MetaNamespaceKeyFunc(pvc)
4644
if err != nil {
4745
return pvc, pv, err, false
@@ -53,74 +51,89 @@ func (ctrl *modifyController) modify(pvc *v1.PersistentVolumeClaim, pv *v1.Persi
5351
return pvc, pv, delayModificationErr, false
5452
}
5553

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)
54+
pvcSpecVacName := ptr.Deref(pvc.Spec.VolumeAttributesClassName, "")
55+
curVacName := ptr.Deref(pvc.Status.CurrentVolumeAttributesClassName, "")
56+
if pvc.Status.ModifyVolumeStatus == nil && pvcSpecVacName == curVacName {
57+
// No modification required
58+
return pvc, pv, nil, false
59+
}
60+
61+
if pvcSpecVacName == "" && curVacName != "" {
62+
klog.V(4).InfoS("Can only set VAC to empty for rollback", "PV", klog.KObj(pv))
63+
return pvc, pv, nil, false
64+
}
65+
66+
// Check if PVC in uncertain state
67+
_, inUncertainState := ctrl.uncertainPVCs.Load(pvcKey)
68+
if inUncertainState {
69+
pvcSpecVacName, parameters, err := ctrl.getTargetParameters(pvc)
70+
if err != nil {
71+
return pvc, pv, err, false
7472
}
73+
return ctrl.controllerModifyVolumeWithTarget(pvc, pv, parameters, pvcSpecVacName)
7574
}
7675

77-
// No modification required
78-
return pvc, pv, nil, false
76+
return ctrl.validateVACAndModifyVolumeWithTarget(pvc, pv)
77+
}
78+
79+
func (ctrl *modifyController) getTargetParameters(pvc *v1.PersistentVolumeClaim) (pvcSpecVacName string, parameters map[string]string, err error) {
80+
if pvc.Spec.VolumeAttributesClassName == nil || *pvc.Spec.VolumeAttributesClassName == "" {
81+
pvcSpecVacName = "[nil]"
82+
} else {
83+
pvcSpecVacName = *pvc.Spec.VolumeAttributesClassName
84+
vac, err := ctrl.vacLister.Get(pvcSpecVacName)
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 "+pvcSpecVacName+" does not exist.")
89+
}
90+
klog.Errorf("Get VAC with vac name %s in VACInformer cache failed: %v", pvcSpecVacName, err)
91+
return "", nil, err
92+
}
93+
parameters = vac.Parameters
94+
}
95+
return pvcSpecVacName, parameters, nil
7996
}
8097

8198
// func validateVACAndModifyVolumeWithTarget validate the VAC. The function sets pvc.Status.ModifyVolumeStatus
8299
// to Pending if VAC does not exist and proceeds to trigger ModifyVolume if VAC exists
83100
func (ctrl *modifyController) validateVACAndModifyVolumeWithTarget(
84101
pvc *v1.PersistentVolumeClaim,
85102
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 {
103+
104+
pvcSpecVacName, parameters, err := ctrl.getTargetParameters(pvc)
105+
if err != nil {
101106
if apierrors.IsNotFound(err) {
102-
ctrl.eventRecorder.Eventf(pvc, v1.EventTypeWarning, util.VolumeModifyFailed, "VAC "+*pvcSpecVacName+" does not exist.")
107+
// Mark pvc.Status.ModifyVolumeStatus as pending
108+
pvc, err = ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumePending, nil)
103109
}
104-
klog.Errorf("Get VAC with vac name %s in VACInformer cache failed: %v", *pvcSpecVacName, err)
105-
// Mark pvc.Status.ModifyVolumeStatus as pending
106-
pvc, err = ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumePending, nil)
107110
return pvc, pv, err, false
108111
}
112+
113+
// Mark pvc.Status.ModifyVolumeStatus as in progress
114+
pvc, err = ctrl.markControllerModifyVolumeStatus(pvc, v1.PersistentVolumeClaimModifyVolumeInProgress, nil)
115+
if err != nil {
116+
return pvc, pv, err, false
117+
}
118+
// Record an event to indicate that external resizer is modifying this volume.
119+
ctrl.eventRecorder.Event(pvc, v1.EventTypeNormal, util.VolumeModify,
120+
fmt.Sprintf("external resizer is modifying volume %s with vac %s", pvc.Name, pvcSpecVacName))
121+
return ctrl.controllerModifyVolumeWithTarget(pvc, pv, parameters, pvcSpecVacName)
109122
}
110123

111124
// func controllerModifyVolumeWithTarget trigger the CSI ControllerModifyVolume API call
112125
// and handle both success and error scenarios
113126
func (ctrl *modifyController) controllerModifyVolumeWithTarget(
114127
pvc *v1.PersistentVolumeClaim,
115128
pv *v1.PersistentVolume,
116-
vacObj *storagev1beta1.VolumeAttributesClass,
117-
pvcSpecVacName *string) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) {
129+
parameters map[string]string,
130+
pvcSpecVacName string) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error, bool) {
118131
var err error
119-
pvc, pv, err = ctrl.callModifyVolumeOnPlugin(pvc, pv, vacObj)
132+
pvc, pv, err = ctrl.callModifyVolumeOnPlugin(pvc, pv, parameters)
120133
if err == nil {
121-
klog.V(4).Infof("Update volumeAttributesClass of PV %q to %s succeeded", pv.Name, *pvcSpecVacName)
134+
klog.V(4).Infof("Update volumeAttributesClass of PV %q to %s succeeded", pv.Name, pvcSpecVacName)
122135
// Record an event to indicate that modify operation is successful.
123-
ctrl.eventRecorder.Eventf(pvc, v1.EventTypeNormal, util.VolumeModifySuccess, fmt.Sprintf("external resizer modified volume %s with vac %s successfully", pvc.Name, vacObj.Name))
136+
ctrl.eventRecorder.Eventf(pvc, v1.EventTypeNormal, util.VolumeModifySuccess, fmt.Sprintf("external resizer modified volume %s with vac %s successfully", pvc.Name, pvcSpecVacName))
124137
return pvc, pv, nil, true
125138
} else {
126139
errStatus, ok := status.FromError(err)
@@ -161,8 +174,7 @@ func (ctrl *modifyController) controllerModifyVolumeWithTarget(
161174
func (ctrl *modifyController) callModifyVolumeOnPlugin(
162175
pvc *v1.PersistentVolumeClaim,
163176
pv *v1.PersistentVolume,
164-
vac *storagev1beta1.VolumeAttributesClass) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error) {
165-
parameters := vac.Parameters
177+
parameters map[string]string) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error) {
166178
if ctrl.extraModifyMetadata {
167179
if len(parameters) == 0 {
168180
parameters = make(map[string]string, 3)

‎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)