From ff71329d8cc08dca53194f8d4568ed53e69ecb82 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Wed, 12 Jul 2023 15:10:51 -0700 Subject: [PATCH] Update VolumeSnapshot and VolumeSnapshotContent using JSON patch fix unit tests operate on cloned copy revert to update call for removeSnapshotFinalizer fix content delete finalizer --- pkg/common-controller/snapshot_controller.go | 46 +++++++++++----- .../snapshot_finalizer_test.go | 3 +- pkg/sidecar-controller/snapshot_controller.go | 20 +++++-- .../snapshot_delete_test.go | 53 ++++++++++--------- 4 files changed, 76 insertions(+), 46 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 60b3cb062..8ae79dfae 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -1381,8 +1381,15 @@ func (ctrl *csiSnapshotCommonController) SetDefaultSnapshotClass(snapshot *crdv1 } klog.V(5).Infof("setDefaultSnapshotClass [%s]: default VolumeSnapshotClassName [%s]", snapshot.Name, defaultClasses[0].Name) snapshotClone := snapshot.DeepCopy() - snapshotClone.Spec.VolumeSnapshotClassName = &(defaultClasses[0].Name) - newSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{}) + patches := []utils.PatchOp{ + { + Op: "replace", + Path: "/spec/volumeSnapshotClassName", + Value: &(defaultClasses[0].Name), + }, + } + + newSnapshot, err := utils.PatchVolumeSnapshot(snapshotClone, patches, ctrl.clientset) if err != nil { klog.V(4).Infof("updating VolumeSnapshot[%s] default class failed %v", utils.SnapshotKey(snapshot), err) } @@ -1606,18 +1613,24 @@ func (ctrl *csiSnapshotCommonController) checkAndSetInvalidContentLabel(content return content, nil } + var patches []utils.PatchOp contentClone := content.DeepCopy() if hasLabel { // Need to remove the label - delete(contentClone.Labels, utils.VolumeSnapshotContentInvalidLabel) + patches = append(patches, utils.PatchOp{ + Op: "remove", + Path: "/metadata/labels/" + utils.VolumeSnapshotContentInvalidLabel, + }) + } else { // Snapshot content is invalid and does not have the label. Need to add the label - if contentClone.ObjectMeta.Labels == nil { - contentClone.ObjectMeta.Labels = make(map[string]string) - } - contentClone.ObjectMeta.Labels[utils.VolumeSnapshotContentInvalidLabel] = "" + patches = append(patches, utils.PatchOp{ + Op: "add", + Path: "/metadata/labels/" + utils.VolumeSnapshotContentInvalidLabel, + Value: "", + }) } - updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{}) + updatedContent, err := utils.PatchVolumeSnapshotContent(contentClone, patches, ctrl.clientset) if err != nil { return content, newControllerUpdateError(content.Name, err.Error()) } @@ -1647,19 +1660,24 @@ func (ctrl *csiSnapshotCommonController) checkAndSetInvalidSnapshotLabel(snapsho return snapshot, nil } + var patches []utils.PatchOp snapshotClone := snapshot.DeepCopy() if hasLabel { // Need to remove the label - delete(snapshotClone.Labels, utils.VolumeSnapshotInvalidLabel) + patches = append(patches, utils.PatchOp{ + Op: "remove", + Path: "/metadata/labels/" + utils.VolumeSnapshotInvalidLabel, + }) } else { // Snapshot is invalid and does not have the label. Need to add the label - if snapshotClone.ObjectMeta.Labels == nil { - snapshotClone.ObjectMeta.Labels = make(map[string]string) - } - snapshotClone.ObjectMeta.Labels[utils.VolumeSnapshotInvalidLabel] = "" + patches = append(patches, utils.PatchOp{ + Op: "add", + Path: "/metadata/labels/" + utils.VolumeSnapshotInvalidLabel, + Value: "", + }) } - updatedSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshot.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{}) + updatedSnapshot, err := utils.PatchVolumeSnapshot(snapshotClone, patches, ctrl.clientset) if err != nil { return snapshot, newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error()) } diff --git a/pkg/common-controller/snapshot_finalizer_test.go b/pkg/common-controller/snapshot_finalizer_test.go index d2d0cfd86..7d0be4191 100644 --- a/pkg/common-controller/snapshot_finalizer_test.go +++ b/pkg/common-controller/snapshot_finalizer_test.go @@ -17,10 +17,9 @@ limitations under the License. package common_controller import ( - "testing" - "github.com/kubernetes-csi/external-snapshotter/v6/pkg/utils" v1 "k8s.io/api/core/v1" + "testing" ) // Test single call to ensurePVCFinalizer, checkandRemovePVCFinalizer, addSnapshotFinalizer, removeSnapshotFinalizer diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index 0ccf2e53a..9d7c7aefd 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -542,10 +542,16 @@ func (ctrl csiSnapshotSideCarController) removeContentFinalizer(content *crdv1.V // the finalizer does not exit, return directly return nil } + var patches []utils.PatchOp contentClone := content.DeepCopy() - contentClone.ObjectMeta.Finalizers = utils.RemoveString(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer) + patches = append(patches, + utils.PatchOp{ + Op: "replace", + Path: "/metadata/finalizers", + Value: utils.RemoveString(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer), + }) - updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{}) + updatedContent, err := utils.PatchVolumeSnapshotContent(contentClone, patches, ctrl.clientset) if err != nil { return newControllerUpdateError(content.Name, err.Error()) } @@ -638,9 +644,15 @@ func (ctrl csiSnapshotSideCarController) removeAnnVolumeSnapshotBeingCreated(con return content, nil } contentClone := content.DeepCopy() - delete(contentClone.ObjectMeta.Annotations, utils.AnnVolumeSnapshotBeingCreated) + annotationPatchPath := strings.ReplaceAll(utils.AnnVolumeSnapshotBeingCreated, "/", "~1") + + var patches []utils.PatchOp + patches = append(patches, utils.PatchOp{ + Op: "remove", + Path: "/metadata/annotations/" + annotationPatchPath, + }) - updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{}) + updatedContent, err := utils.PatchVolumeSnapshotContent(contentClone, patches, ctrl.clientset) if err != nil { return content, newControllerUpdateError(content.Name, err.Error()) } diff --git a/pkg/sidecar-controller/snapshot_delete_test.go b/pkg/sidecar-controller/snapshot_delete_test.go index c4f322c83..66fc9de1e 100644 --- a/pkg/sidecar-controller/snapshot_delete_test.go +++ b/pkg/sidecar-controller/snapshot_delete_test.go @@ -29,14 +29,15 @@ import ( ) var ( - defaultSize int64 = 1000 - emptySize int64 - deletePolicy = crdv1.VolumeSnapshotContentDelete - retainPolicy = crdv1.VolumeSnapshotContentRetain - timeNow = time.Now() - timeNowMetav1 = metav1.Now() - False = false - True = true + defaultSize int64 = 1000 + emptySize int64 + deletePolicy = crdv1.VolumeSnapshotContentDelete + retainPolicy = crdv1.VolumeSnapshotContentRetain + timeNow = time.Now() + timeNowMetav1 = metav1.Now() + nonFractionalTime = metav1.NewTime(time.Now().Truncate(time.Second)) + False = false + True = true ) var class1Parameters = map[string]string{ @@ -153,8 +154,8 @@ func TestDeleteSync(t *testing.T) { tests := []controllerTest{ { name: "1-1 - content non-nil DeletionTimestamp with delete policy will delete snapshot", - initialContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1), - expectedContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, false, &timeNowMetav1), + initialContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, true, &nonFractionalTime), + expectedContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, false, &nonFractionalTime), expectedEvents: noevents, errors: noerrors, initialSecrets: []*v1.Secret{secret()}, @@ -177,8 +178,8 @@ func TestDeleteSync(t *testing.T) { }, { name: "1-2 - content non-nil DeletionTimestamp with retain policy will not delete snapshot", - initialContents: newContentArrayWithDeletionTimestamp("content1-2", "snapuid1-2", "snap1-2", "sid1-2", classGold, "", "snap1-2-volumehandle", retainPolicy, nil, nil, true, &timeNowMetav1), - expectedContents: newContentArrayWithDeletionTimestamp("content1-2", "snapuid1-2", "snap1-2", "sid1-2", classGold, "", "snap1-2-volumehandle", retainPolicy, nil, nil, false, &timeNowMetav1), + initialContents: newContentArrayWithDeletionTimestamp("content1-2", "snapuid1-2", "snap1-2", "sid1-2", classGold, "", "snap1-2-volumehandle", retainPolicy, nil, nil, true, &nonFractionalTime), + expectedContents: newContentArrayWithDeletionTimestamp("content1-2", "snapuid1-2", "snap1-2", "sid1-2", classGold, "", "snap1-2-volumehandle", retainPolicy, nil, nil, false, &nonFractionalTime), expectedEvents: noevents, errors: noerrors, expectedCreateCalls: []createCall{ @@ -280,8 +281,8 @@ func TestDeleteSync(t *testing.T) { }, { name: "1-9 - continue deletion with snapshot class that has nonexistent secret, bound finalizer removed", - initialContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "sid1-9", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1), - expectedContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, false, &timeNowMetav1), + initialContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "sid1-9", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime), + expectedContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, false, &nonFractionalTime), expectedEvents: noevents, expectedListCalls: []listCall{{"sid1-9", map[string]string{}, true, time.Now(), 0, nil}}, errors: noerrors, @@ -291,8 +292,8 @@ func TestDeleteSync(t *testing.T) { }, { name: "1-10 - (dynamic)deletion of content with retain policy should not trigger CSI call, not update status, but remove bound finalizer", - initialContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, true, &timeNowMetav1), - expectedContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, false, &timeNowMetav1), + initialContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, true, &nonFractionalTime), + expectedContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, false, &nonFractionalTime), expectedEvents: noevents, expectedListCalls: []listCall{{"sid1-10", map[string]string{}, true, time.Now(), 0, nil}}, errors: noerrors, @@ -301,8 +302,8 @@ func TestDeleteSync(t *testing.T) { }, { name: "1-11 - (dynamic)deletion of content with deletion policy should trigger CSI call, update status, and remove bound finalizer removed.", - initialContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "sid1-11", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1), - expectedContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, nil, false, &timeNowMetav1), + initialContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "sid1-11", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime), + expectedContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, nil, false, &nonFractionalTime), expectedEvents: noevents, errors: noerrors, expectedDeleteCalls: []deleteCall{{"sid1-11", nil, nil}}, @@ -310,8 +311,8 @@ func TestDeleteSync(t *testing.T) { }, { name: "1-12 - (pre-provision)deletion of content with retain policy should not trigger CSI call, not update status, but remove bound finalizer", - initialContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, true, &timeNowMetav1), - expectedContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, false, &timeNowMetav1), + initialContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, true, &nonFractionalTime), + expectedContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, false, &nonFractionalTime), expectedEvents: noevents, expectedListCalls: []listCall{{"sid1-12", map[string]string{}, true, time.Now(), 0, nil}}, errors: noerrors, @@ -320,8 +321,8 @@ func TestDeleteSync(t *testing.T) { }, { name: "1-13 - (pre-provision)deletion of content with deletion policy should trigger CSI call, update status, and remove bound finalizer removed.", - initialContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "sid1-13", emptySecretClass, "sid1-13", "", deletePolicy, nil, &defaultSize, true, &timeNowMetav1), - expectedContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "", emptySecretClass, "sid1-13", "", deletePolicy, nil, nil, false, &timeNowMetav1), + initialContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "sid1-13", emptySecretClass, "sid1-13", "", deletePolicy, nil, &defaultSize, true, &nonFractionalTime), + expectedContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "", emptySecretClass, "sid1-13", "", deletePolicy, nil, nil, false, &nonFractionalTime), expectedEvents: noevents, errors: noerrors, expectedDeleteCalls: []deleteCall{{"sid1-13", nil, nil}}, @@ -329,8 +330,8 @@ func TestDeleteSync(t *testing.T) { }, { name: "1-14 - (pre-provision)deletion of content with deletion policy and no snapshotclass should trigger CSI call, update status, and remove bound finalizer removed.", - initialContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "sid1-14", "", "sid1-14", "", deletePolicy, nil, &defaultSize, true, &timeNowMetav1), - expectedContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "", "", "sid1-14", "", deletePolicy, nil, nil, false, &timeNowMetav1), + initialContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "sid1-14", "", "sid1-14", "", deletePolicy, nil, &defaultSize, true, &nonFractionalTime), + expectedContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "", "", "sid1-14", "", deletePolicy, nil, nil, false, &nonFractionalTime), expectedEvents: noevents, errors: noerrors, expectedDeleteCalls: []deleteCall{{"sid1-14", nil, nil}}, @@ -338,8 +339,8 @@ func TestDeleteSync(t *testing.T) { }, { name: "1-15 - (dynamic)deletion of content with no snapshotclass should succeed", - initialContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "sid1-15", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1), - expectedContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, false, &timeNowMetav1), + initialContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "sid1-15", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime), + expectedContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, false, &nonFractionalTime), errors: noerrors, expectedDeleteCalls: []deleteCall{{"sid1-15", nil, nil}}, test: testSyncContent,