From ad7dfe60451936fe0b5fa5201c174f3ebbcf0c0c Mon Sep 17 00:00:00 2001 From: Grant Griffiths Date: Wed, 29 Sep 2021 19:22:43 -0700 Subject: [PATCH] Revert patch for empty finalizers Signed-off-by: Grant Griffiths --- pkg/common-controller/framework_test.go | 24 +++++++++----- pkg/common-controller/snapshot_controller.go | 32 ++++++++++++------- .../snapshot_finalizer_test.go | 10 +++++- 3 files changed, 46 insertions(+), 20 deletions(-) diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go index 673946c8..78860c07 100644 --- a/pkg/common-controller/framework_test.go +++ b/pkg/common-controller/framework_test.go @@ -326,9 +326,7 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O return true, snapshot, nil case action.Matches("patch", "volumesnapshots"): - snapshot := &crdv1.VolumeSnapshot{} action := action.(core.PatchAction) - // Check and bump object version storedSnapshot, found := r.snapshots[action.GetName()] if found { @@ -359,9 +357,10 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O } // Store the updated object to appropriate places. - r.snapshots[snapshot.Name] = storedSnapshot + r.snapshots[storedSnapshot.Name] = storedSnapshot r.changedObjects = append(r.changedObjects, storedSnapshot) r.changedSinceLastSync++ + klog.V(4).Infof("saved updated snapshot %s", storedSnapshot.Name) return true, storedSnapshot, nil @@ -1253,6 +1252,10 @@ func testAddSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapsh return ctrl.addSnapshotFinalizer(test.initialSnapshots[0], true, true) } +func testAddSingleSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error { + return ctrl.addSnapshotFinalizer(test.initialSnapshots[0], false, true) +} + func testRemoveSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error { return ctrl.removeSnapshotFinalizer(test.initialSnapshots[0], true, true) } @@ -1510,8 +1513,10 @@ func evaluateFinalizerTests(ctrl *csiSnapshotCommonController, reactor *snapshot if funcName == "testAddSnapshotFinalizer" { for _, snapshot := range reactor.snapshots { if test.initialSnapshots[0].Name == snapshot.Name { - if !utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) && utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) && - !utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) && utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) { + if !utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) && + utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) && + !utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) && + utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) { klog.V(4).Infof("test %q succeeded. Finalizers are added to snapshot %s", test.name, snapshot.Name) bHasSnapshotFinalizer = true } @@ -1519,15 +1524,18 @@ func evaluateFinalizerTests(ctrl *csiSnapshotCommonController, reactor *snapshot } } if test.expectSuccess && !bHasSnapshotFinalizer { - t.Errorf("Test %q: failed to add finalizer to Snapshot %s", test.name, test.initialSnapshots[0].Name) + t.Errorf("Test %q: failed to add finalizer to Snapshot %s. Finalizers: %s", test.name, test.initialSnapshots[0].Name, test.initialSnapshots[0].GetFinalizers()) } } bHasSnapshotFinalizer = true if funcName == "testRemoveSnapshotFinalizer" { for _, snapshot := range reactor.snapshots { if test.initialSnapshots[0].Name == snapshot.Name { - if utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) && !utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) && - utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) && !utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) { + if utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) && + !utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) && + utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) && + !utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) { + klog.V(4).Infof("test %q succeeded. SnapshotFinalizer is removed from Snapshot %s", test.name, snapshot.Name) bHasSnapshotFinalizer = false } diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index db346b4e..689e845b 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -1421,16 +1421,25 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo var updatedSnapshot *crdv1.VolumeSnapshot var err error - var patches []utils.PatchOp + // NOTE(ggriffiths): Must perform an update if no finalizers exist. + // Unable to find a patch that correctly updated the finalizers if none currently exist. if len(snapshot.ObjectMeta.Finalizers) == 0 { - // Replace finalizers with new array if there are no other finalizers - patches = append(patches, utils.PatchOp{ - Op: "add", - Path: "/metadata/finalizers", - Value: []string{utils.VolumeSnapshotContentFinalizer}, - }) + snapshotClone := snapshot.DeepCopy() + if addSourceFinalizer { + snapshotClone.ObjectMeta.Finalizers = append(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) + } + if addBoundFinalizer { + snapshotClone.ObjectMeta.Finalizers = append(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) + } + updatedSnapshot, err = ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{}) + if err != nil { + return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error()) + } } else { // Otherwise, perform a patch + var patches []utils.PatchOp + + // If finalizers exist already, add new ones to the end of the array if addSourceFinalizer { patches = append(patches, utils.PatchOp{ Op: "add", @@ -1445,10 +1454,11 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo Value: utils.VolumeSnapshotBoundFinalizer, }) } - } - updatedSnapshot, err = utils.PatchVolumeSnapshot(snapshot, patches, ctrl.clientset) - if err != nil { - return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error()) + + updatedSnapshot, err = utils.PatchVolumeSnapshot(snapshot, patches, ctrl.clientset) + if err != nil { + return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error()) + } } _, err = ctrl.storeSnapshotUpdate(updatedSnapshot) diff --git a/pkg/common-controller/snapshot_finalizer_test.go b/pkg/common-controller/snapshot_finalizer_test.go index ecaf0246..8e5a3f06 100644 --- a/pkg/common-controller/snapshot_finalizer_test.go +++ b/pkg/common-controller/snapshot_finalizer_test.go @@ -19,6 +19,7 @@ package common_controller import ( "testing" + "github.com/kubernetes-csi/external-snapshotter/v4/pkg/utils" v1 "k8s.io/api/core/v1" ) @@ -70,7 +71,14 @@ func TestSnapshotFinalizer(t *testing.T) { expectSuccess: true, }, { - name: "2-1 - successful remove Snapshot finalizer", + name: "2-2 - successful add single Snapshot finalizer with patch", + initialSnapshots: withSnapshotFinalizers(newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", classSilver, "", &False, nil, nil, nil, false, false, nil), utils.VolumeSnapshotBoundFinalizer), + initialClaims: newClaimArray("claim6-2", "pvc-uid6-2", "1Gi", "volume6-2", v1.ClaimBound, &classEmpty), + test: testAddSingleSnapshotFinalizer, + expectSuccess: true, + }, + { + name: "2-3 - successful remove Snapshot finalizer", initialSnapshots: newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", classSilver, "", &False, nil, nil, nil, false, true, nil), initialClaims: newClaimArray("claim6-2", "pvc-uid6-2", "1Gi", "volume6-2", v1.ClaimBound, &classEmpty), test: testRemoveSnapshotFinalizer,