Skip to content

Commit ad7dfe6

Browse files
author
Grant Griffiths
committed
Revert patch for empty finalizers
Signed-off-by: Grant Griffiths <[email protected]>
1 parent d14e2ee commit ad7dfe6

File tree

3 files changed

+46
-20
lines changed

3 files changed

+46
-20
lines changed

‎pkg/common-controller/framework_test.go‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -326,9 +326,7 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O
326326
return true, snapshot, nil
327327

328328
case action.Matches("patch", "volumesnapshots"):
329-
snapshot := &crdv1.VolumeSnapshot{}
330329
action := action.(core.PatchAction)
331-
332330
// Check and bump object version
333331
storedSnapshot, found := r.snapshots[action.GetName()]
334332
if found {
@@ -359,9 +357,10 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O
359357
}
360358

361359
// Store the updated object to appropriate places.
362-
r.snapshots[snapshot.Name] = storedSnapshot
360+
r.snapshots[storedSnapshot.Name] = storedSnapshot
363361
r.changedObjects = append(r.changedObjects, storedSnapshot)
364362
r.changedSinceLastSync++
363+
365364
klog.V(4).Infof("saved updated snapshot %s", storedSnapshot.Name)
366365
return true, storedSnapshot, nil
367366

@@ -1253,6 +1252,10 @@ func testAddSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapsh
12531252
return ctrl.addSnapshotFinalizer(test.initialSnapshots[0], true, true)
12541253
}
12551254

1255+
func testAddSingleSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
1256+
return ctrl.addSnapshotFinalizer(test.initialSnapshots[0], false, true)
1257+
}
1258+
12561259
func testRemoveSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
12571260
return ctrl.removeSnapshotFinalizer(test.initialSnapshots[0], true, true)
12581261
}
@@ -1510,24 +1513,29 @@ func evaluateFinalizerTests(ctrl *csiSnapshotCommonController, reactor *snapshot
15101513
if funcName == "testAddSnapshotFinalizer" {
15111514
for _, snapshot := range reactor.snapshots {
15121515
if test.initialSnapshots[0].Name == snapshot.Name {
1513-
if !utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) && utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
1514-
!utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) && utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) {
1516+
if !utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
1517+
utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
1518+
!utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) &&
1519+
utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) {
15151520
klog.V(4).Infof("test %q succeeded. Finalizers are added to snapshot %s", test.name, snapshot.Name)
15161521
bHasSnapshotFinalizer = true
15171522
}
15181523
break
15191524
}
15201525
}
15211526
if test.expectSuccess && !bHasSnapshotFinalizer {
1522-
t.Errorf("Test %q: failed to add finalizer to Snapshot %s", test.name, test.initialSnapshots[0].Name)
1527+
t.Errorf("Test %q: failed to add finalizer to Snapshot %s. Finalizers: %s", test.name, test.initialSnapshots[0].Name, test.initialSnapshots[0].GetFinalizers())
15231528
}
15241529
}
15251530
bHasSnapshotFinalizer = true
15261531
if funcName == "testRemoveSnapshotFinalizer" {
15271532
for _, snapshot := range reactor.snapshots {
15281533
if test.initialSnapshots[0].Name == snapshot.Name {
1529-
if utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) && !utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
1530-
utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) && !utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) {
1534+
if utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
1535+
!utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
1536+
utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) &&
1537+
!utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) {
1538+
15311539
klog.V(4).Infof("test %q succeeded. SnapshotFinalizer is removed from Snapshot %s", test.name, snapshot.Name)
15321540
bHasSnapshotFinalizer = false
15331541
}

‎pkg/common-controller/snapshot_controller.go‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1421,16 +1421,25 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo
14211421
var updatedSnapshot *crdv1.VolumeSnapshot
14221422
var err error
14231423

1424-
var patches []utils.PatchOp
1424+
// NOTE(ggriffiths): Must perform an update if no finalizers exist.
1425+
// Unable to find a patch that correctly updated the finalizers if none currently exist.
14251426
if len(snapshot.ObjectMeta.Finalizers) == 0 {
1426-
// Replace finalizers with new array if there are no other finalizers
1427-
patches = append(patches, utils.PatchOp{
1428-
Op: "add",
1429-
Path: "/metadata/finalizers",
1430-
Value: []string{utils.VolumeSnapshotContentFinalizer},
1431-
})
1427+
snapshotClone := snapshot.DeepCopy()
1428+
if addSourceFinalizer {
1429+
snapshotClone.ObjectMeta.Finalizers = append(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer)
1430+
}
1431+
if addBoundFinalizer {
1432+
snapshotClone.ObjectMeta.Finalizers = append(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer)
1433+
}
1434+
updatedSnapshot, err = ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
1435+
if err != nil {
1436+
return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
1437+
}
14321438
} else {
14331439
// Otherwise, perform a patch
1440+
var patches []utils.PatchOp
1441+
1442+
// If finalizers exist already, add new ones to the end of the array
14341443
if addSourceFinalizer {
14351444
patches = append(patches, utils.PatchOp{
14361445
Op: "add",
@@ -1445,10 +1454,11 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo
14451454
Value: utils.VolumeSnapshotBoundFinalizer,
14461455
})
14471456
}
1448-
}
1449-
updatedSnapshot, err = utils.PatchVolumeSnapshot(snapshot, patches, ctrl.clientset)
1450-
if err != nil {
1451-
return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
1457+
1458+
updatedSnapshot, err = utils.PatchVolumeSnapshot(snapshot, patches, ctrl.clientset)
1459+
if err != nil {
1460+
return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
1461+
}
14521462
}
14531463

14541464
_, err = ctrl.storeSnapshotUpdate(updatedSnapshot)

‎pkg/common-controller/snapshot_finalizer_test.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package common_controller
1919
import (
2020
"testing"
2121

22+
"github.com/kubernetes-csi/external-snapshotter/v4/pkg/utils"
2223
v1 "k8s.io/api/core/v1"
2324
)
2425

@@ -70,7 +71,14 @@ func TestSnapshotFinalizer(t *testing.T) {
7071
expectSuccess: true,
7172
},
7273
{
73-
name: "2-1 - successful remove Snapshot finalizer",
74+
name: "2-2 - successful add single Snapshot finalizer with patch",
75+
initialSnapshots: withSnapshotFinalizers(newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", classSilver, "", &False, nil, nil, nil, false, false, nil), utils.VolumeSnapshotBoundFinalizer),
76+
initialClaims: newClaimArray("claim6-2", "pvc-uid6-2", "1Gi", "volume6-2", v1.ClaimBound, &classEmpty),
77+
test: testAddSingleSnapshotFinalizer,
78+
expectSuccess: true,
79+
},
80+
{
81+
name: "2-3 - successful remove Snapshot finalizer",
7482
initialSnapshots: newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", classSilver, "", &False, nil, nil, nil, false, true, nil),
7583
initialClaims: newClaimArray("claim6-2", "pvc-uid6-2", "1Gi", "volume6-2", v1.ClaimBound, &classEmpty),
7684
test: testRemoveSnapshotFinalizer,

0 commit comments

Comments
 (0)