From 7811e59803fa9888acc4abc616b3ce58b1bfb120 Mon Sep 17 00:00:00 2001 From: Grant Griffiths Date: Wed, 12 Feb 2020 15:44:54 -0800 Subject: [PATCH 1/2] Refactor delete test to use withSnapshotFinalizers function Signed-off-by: Grant Griffiths --- pkg/common-controller/framework_test.go | 22 ++++++------ pkg/common-controller/snapshot_delete_test.go | 34 ++----------------- 2 files changed, 15 insertions(+), 41 deletions(-) diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go index 1a40219e0..14313f367 100644 --- a/pkg/common-controller/framework_test.go +++ b/pkg/common-controller/framework_test.go @@ -164,11 +164,13 @@ type reactorError struct { error error } -func withSnapshotFinalizer(snapshot *crdv1.VolumeSnapshot) *crdv1.VolumeSnapshot { - snapshot.ObjectMeta.Finalizers = append(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer) - snapshot.ObjectMeta.Finalizers = append(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) - snapshot.ObjectMeta.Finalizers = append(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) - return snapshot +func withSnapshotFinalizers(snapshots []*crdv1.VolumeSnapshot, finalizers ...string) []*crdv1.VolumeSnapshot { + for i := range snapshots { + for _, f := range finalizers { + snapshots[i].ObjectMeta.Finalizers = append(snapshots[i].ObjectMeta.Finalizers, f) + } + } + return snapshots } func withContentFinalizer(content *crdv1.VolumeSnapshotContent) *crdv1.VolumeSnapshotContent { @@ -863,7 +865,7 @@ func newContentWithUnmatchDriverArray(contentName, boundToSnapshotUID, boundToSn func newSnapshot( snapshotName, snapshotUID, pvcName, targetContentName, snapshotClassName, boundContentName string, readyToUse *bool, creationTime *metav1.Time, restoreSize *resource.Quantity, - err *crdv1.VolumeSnapshotError, nilStatus bool, withFinalizer bool, deletionTimestamp *metav1.Time) *crdv1.VolumeSnapshot { + err *crdv1.VolumeSnapshotError, nilStatus bool, withAllFinalizers bool, deletionTimestamp *metav1.Time) *crdv1.VolumeSnapshot { snapshot := crdv1.VolumeSnapshot{ ObjectMeta: metav1.ObjectMeta{ Name: snapshotName, @@ -904,8 +906,8 @@ func newSnapshot( VolumeSnapshotContentName: &targetContentName, } } - if withFinalizer { - return withSnapshotFinalizer(&snapshot) + if withAllFinalizers { + return withSnapshotFinalizers([]*crdv1.VolumeSnapshot{&snapshot}, utils.VolumeSnapshotContentFinalizer, utils.VolumeSnapshotAsSourceFinalizer, utils.VolumeSnapshotBoundFinalizer)[0] } return &snapshot } @@ -913,9 +915,9 @@ func newSnapshot( func newSnapshotArray( snapshotName, snapshotUID, pvcName, targetContentName, snapshotClassName, boundContentName string, readyToUse *bool, creationTime *metav1.Time, restoreSize *resource.Quantity, - err *crdv1.VolumeSnapshotError, nilStatus bool, withFinalizer bool, deletionTimestamp *metav1.Time) []*crdv1.VolumeSnapshot { + err *crdv1.VolumeSnapshotError, nilStatus bool, withAllFinalizers bool, deletionTimestamp *metav1.Time) []*crdv1.VolumeSnapshot { return []*crdv1.VolumeSnapshot{ - newSnapshot(snapshotName, snapshotUID, pvcName, targetContentName, snapshotClassName, boundContentName, readyToUse, creationTime, restoreSize, err, nilStatus, withFinalizer, deletionTimestamp), + newSnapshot(snapshotName, snapshotUID, pvcName, targetContentName, snapshotClassName, boundContentName, readyToUse, creationTime, restoreSize, err, nilStatus, withAllFinalizers, deletionTimestamp), } } diff --git a/pkg/common-controller/snapshot_delete_test.go b/pkg/common-controller/snapshot_delete_test.go index 0fd3c8f87..250c34a2e 100644 --- a/pkg/common-controller/snapshot_delete_test.go +++ b/pkg/common-controller/snapshot_delete_test.go @@ -24,7 +24,6 @@ import ( "github.com/kubernetes-csi/external-snapshotter/v2/pkg/utils" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" ) var class1Parameters = map[string]string{ @@ -304,36 +303,9 @@ func TestDeleteSync(t *testing.T) { initialContents: newContentArray("content3-1", "", "snap3-1", "sid3-1", validSecretClass, "", "", deletePolicy, nil, nil, true), expectedContents: nocontents, initialSnapshots: newSnapshotArray("snap3-1", "snapuid3-1", "claim3-1", "", validSecretClass, "content3-1", &False, nil, nil, nil, false, true, &timeNowMetav1), - expectedSnapshots: []*crdv1.VolumeSnapshot{ - &crdv1.VolumeSnapshot{ - ObjectMeta: metav1.ObjectMeta{ - Name: "snap3-1", - Namespace: testNamespace, - UID: types.UID("snapuid3-1"), - ResourceVersion: "1", - Finalizers: []string{ - "snapshot.storage.kubernetes.io/volumesnapshotcontent-bound-protection", - "snapshot.storage.kubernetes.io/volumesnapshot-bound-protection", - }, - SelfLink: "/apis/snapshot.storage.k8s.io/v1beta1/namespaces/" + testNamespace + "/volumesnapshots/" + "snap3-1", - DeletionTimestamp: &timeNowMetav1, - }, - Spec: crdv1.VolumeSnapshotSpec{ - VolumeSnapshotClassName: &validSecretClass, - Source: crdv1.VolumeSnapshotSource{ - PersistentVolumeClaimName: &claim31, - }, - }, - - Status: &crdv1.VolumeSnapshotStatus{ - CreationTime: nil, - ReadyToUse: &False, - Error: nil, - RestoreSize: nil, - BoundVolumeSnapshotContentName: &content31, - }, - }, - }, + expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap3-1", "snapuid3-1", "claim3-1", "", validSecretClass, "content3-1", &False, nil, nil, nil, false, false, &timeNowMetav1), + utils.VolumeSnapshotContentFinalizer, utils.VolumeSnapshotBoundFinalizer, + ), initialClaims: newClaimArray("claim3-1", "pvc-uid3-1", "1Gi", "volume3-1", v1.ClaimBound, &classEmpty), expectedEvents: noevents, initialSecrets: []*v1.Secret{secret()}, From 05efba2a3073cb7549d454aca866f63cb0995ca7 Mon Sep 17 00:00:00 2001 From: Grant Griffiths Date: Wed, 12 Feb 2020 18:47:43 -0800 Subject: [PATCH 2/2] Add more snapshot and content sync tests Signed-off-by: Grant Griffiths --- pkg/common-controller/framework_test.go | 23 +++++++- pkg/common-controller/snapshot_delete_test.go | 38 +++++++------ pkg/common-controller/snapshot_update_test.go | 56 ++++++++++++++++++- 3 files changed, 96 insertions(+), 21 deletions(-) diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go index 14313f367..9acc3eea6 100644 --- a/pkg/common-controller/framework_test.go +++ b/pkg/common-controller/framework_test.go @@ -175,7 +175,6 @@ func withSnapshotFinalizers(snapshots []*crdv1.VolumeSnapshot, finalizers ...str func withContentFinalizer(content *crdv1.VolumeSnapshotContent) *crdv1.VolumeSnapshotContent { content.ObjectMeta.Finalizers = append(content.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer) - metav1.SetMetaDataAnnotation(&content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted, "yes") return content } @@ -826,6 +825,18 @@ func newContent(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHa return &content } +func withContentAnnotations(contents []*crdv1.VolumeSnapshotContent, annotations map[string]string) []*crdv1.VolumeSnapshotContent { + for i := range contents { + if contents[i].ObjectMeta.Annotations == nil { + contents[i].ObjectMeta.Annotations = make(map[string]string) + } + for k, v := range annotations { + contents[i].ObjectMeta.Annotations[k] = v + } + } + return contents +} + func newContentArray(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle string, deletionPolicy crdv1.DeletionPolicy, size, creationTime *int64, withFinalizer bool) []*crdv1.VolumeSnapshotContent { @@ -907,7 +918,7 @@ func newSnapshot( } } if withAllFinalizers { - return withSnapshotFinalizers([]*crdv1.VolumeSnapshot{&snapshot}, utils.VolumeSnapshotContentFinalizer, utils.VolumeSnapshotAsSourceFinalizer, utils.VolumeSnapshotBoundFinalizer)[0] + return withSnapshotFinalizers([]*crdv1.VolumeSnapshot{&snapshot}, utils.VolumeSnapshotAsSourceFinalizer, utils.VolumeSnapshotBoundFinalizer)[0] } return &snapshot } @@ -1073,6 +1084,14 @@ func testSyncContent(ctrl *csiSnapshotCommonController, reactor *snapshotReactor return ctrl.syncContent(test.initialContents[0]) } +func testSyncContentError(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error { + err := ctrl.syncContent(test.initialContents[0]) + if err != nil { + return nil + } + return fmt.Errorf("syncContent succeeded when failure was expected") +} + func testAddPVCFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error { return ctrl.ensurePVCFinalizer(test.initialSnapshots[0]) } diff --git a/pkg/common-controller/snapshot_delete_test.go b/pkg/common-controller/snapshot_delete_test.go index 250c34a2e..782dff151 100644 --- a/pkg/common-controller/snapshot_delete_test.go +++ b/pkg/common-controller/snapshot_delete_test.go @@ -17,7 +17,7 @@ limitations under the License. package common_controller import ( - //"errors" + "errors" "testing" crdv1 "github.com/kubernetes-csi/external-snapshotter/v2/pkg/apis/volumesnapshot/v1beta1" @@ -174,22 +174,7 @@ func TestDeleteSync(t *testing.T) { initialSecrets: []*v1.Secret{secret()}, //expectedDeleteCalls: []deleteCall{{"sid1-3", map[string]string{"foo": "bar"}, nil}}, test: testSyncContent, - }, /*{ - name: "1-6 - api server delete content returns error", - initialContents: newContentArray("content1-6", "sid1-6", "snap1-6", "sid1-6", validSecretClass, "", "", deletionPolicy, nil, nil, true), - expectedContents: newContentArray("content1-6", "sid1-6", "snap1-6", "sid1-6", validSecretClass, "", "", deletionPolicy, nil, nil, true), - initialSnapshots: nosnapshots, - expectedSnapshots: nosnapshots, - initialSecrets: []*v1.Secret{secret()}, - //expectedDeleteCalls: []deleteCall{{"sid1-6", map[string]string{"foo": "bar"}, nil}}, - expectedEvents: []string{"Warning SnapshotContentObjectDeleteError"}, - errors: []reactorError{ - // Inject error to the first client.VolumesnapshotV1beta1().VolumeSnapshotContents().Delete call. - // All other calls will succeed. - {"delete", "volumesnapshotcontents", errors.New("mock delete error")}, - }, - test: testSyncContent, - },*/ + }, { // delete success - snapshot that the content was pointing to was deleted, and another // with the same name created. @@ -304,7 +289,7 @@ func TestDeleteSync(t *testing.T) { expectedContents: nocontents, initialSnapshots: newSnapshotArray("snap3-1", "snapuid3-1", "claim3-1", "", validSecretClass, "content3-1", &False, nil, nil, nil, false, true, &timeNowMetav1), expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap3-1", "snapuid3-1", "claim3-1", "", validSecretClass, "content3-1", &False, nil, nil, nil, false, false, &timeNowMetav1), - utils.VolumeSnapshotContentFinalizer, utils.VolumeSnapshotBoundFinalizer, + utils.VolumeSnapshotBoundFinalizer, ), initialClaims: newClaimArray("claim3-1", "pvc-uid3-1", "1Gi", "volume3-1", v1.ClaimBound, &classEmpty), expectedEvents: noevents, @@ -312,6 +297,23 @@ func TestDeleteSync(t *testing.T) { errors: noerrors, test: testSyncSnapshot, }, + { + name: "3-2 - content will not be deleted if deletion API call fails", + initialContents: newContentArray("content3-2", "", "snap3-2", "sid3-2", validSecretClass, "", "", deletePolicy, nil, nil, true), + expectedContents: newContentArray("content3-2", "", "snap3-2", "sid3-2", validSecretClass, "", "", deletePolicy, nil, nil, true), + initialSnapshots: newSnapshotArray("snap3-2", "snapuid3-2", "claim3-2", "", validSecretClass, "content3-2", &False, nil, nil, nil, false, true, &timeNowMetav1), + expectedSnapshots: newSnapshotArray("snap3-2", "snapuid3-2", "claim3-2", "", validSecretClass, "content3-2", &False, nil, nil, nil, false, true, &timeNowMetav1), + initialClaims: newClaimArray("claim3-2", "pvc-uid3-2", "1Gi", "volume3-2", v1.ClaimBound, &classEmpty), + expectedEvents: []string{"Warning SnapshotContentObjectDeleteError"}, + initialSecrets: []*v1.Secret{secret()}, + errors: []reactorError{ + // Inject error to the first client.VolumesnapshotV1beta1().VolumeSnapshotContents().Delete call. + // All other calls will succeed. + {"delete", "volumesnapshotcontents", errors.New("mock delete error")}, + }, + expectSuccess: false, + test: testSyncSnapshotError, + }, } runSyncTests(t, tests, snapshotClasses) } diff --git a/pkg/common-controller/snapshot_update_test.go b/pkg/common-controller/snapshot_update_test.go index cbb1b5c16..9c261ce9a 100644 --- a/pkg/common-controller/snapshot_update_test.go +++ b/pkg/common-controller/snapshot_update_test.go @@ -17,10 +17,11 @@ limitations under the License. package common_controller import ( - //"errors" + "errors" "testing" "time" + "github.com/kubernetes-csi/external-snapshotter/v2/pkg/utils" v1 "k8s.io/api/core/v1" storagev1beta1 "k8s.io/api/storage/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -282,6 +283,59 @@ func TestSync(t *testing.T) { errors: noerrors, test: testSyncSnapshot, }, + { + name: "5-1 - content missing finalizer is updated to have finalizer", + initialContents: newContentArray("content5-1", "snapuid5-1", "snap5-1", "sid5-1", validSecretClass, "", "", deletionPolicy, nil, nil, false), + expectedContents: newContentArray("content5-1", "snapuid5-1", "snap5-1", "sid5-1", validSecretClass, "", "", deletionPolicy, nil, nil, true), + initialClaims: newClaimArray("claim5-1", "pvc-uid5-1", "1Gi", "volume5-1", v1.ClaimBound, &classEmpty), + initialVolumes: newVolumeArray("volume5-1", "pv-uid5-1", "pv-handle5-1", "1Gi", "pvc-uid5-1", "claim5-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty), + initialSecrets: []*v1.Secret{secret()}, + errors: noerrors, + test: testSyncContent, + }, + { + name: "5-2 - content missing finalizer update attempt fails because of failed API call", + initialContents: newContentArray("content5-2", "snapuid5-2", "snap5-2", "sid5-2", validSecretClass, "", "", deletionPolicy, nil, nil, false), + expectedContents: newContentArray("content5-2", "snapuid5-2", "snap5-2", "sid5-2", validSecretClass, "", "", deletionPolicy, nil, nil, false), + initialClaims: newClaimArray("claim5-2", "pvc-uid5-2", "1Gi", "volume5-2", v1.ClaimBound, &classEmpty), + initialVolumes: newVolumeArray("volume5-2", "pv-uid5-2", "pv-handle5-2", "1Gi", "pvc-uid5-2", "claim5-2", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty), + initialSecrets: []*v1.Secret{secret()}, + errors: []reactorError{ + // Inject error to the forth client.VolumesnapshotV1beta1().VolumeSnapshots().Update call. + {"update", "volumesnapshotcontents", errors.New("mock update error")}, + }, + expectSuccess: false, + test: testSyncContentError, + }, + { + name: "5-3 - snapshot deletion candidate marked for deletion", + initialSnapshots: newSnapshotArray("snap5-3", "snapuid5-3", "claim5-3", "", validSecretClass, "content5-3", &False, nil, nil, nil, false, true, &timeNowMetav1), + expectedSnapshots: newSnapshotArray("snap5-3", "snapuid5-3", "claim5-3", "", validSecretClass, "content5-3", &False, nil, nil, nil, false, true, &timeNowMetav1), + initialContents: newContentArray("content5-3", "snapuid5-3", "snap5-3", "sid5-3", validSecretClass, "", "", deletionPolicy, nil, nil, true), + expectedContents: withContentAnnotations(newContentArray("content5-3", "snapuid5-3", "snap5-3", "sid5-3", validSecretClass, "", "", deletionPolicy, nil, nil, true), map[string]string{utils.AnnVolumeSnapshotBeingDeleted: "yes"}), + initialClaims: newClaimArray("claim5-3", "pvc-uid5-3", "1Gi", "volume5-3", v1.ClaimBound, &classEmpty), + initialVolumes: newVolumeArray("volume5-3", "pv-uid5-3", "pv-handle5-3", "1Gi", "pvc-uid5-3", "claim5-3", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty), + initialSecrets: []*v1.Secret{secret()}, + expectSuccess: true, + test: testSyncContent, + }, + { + name: "5-4 - snapshot deletion candidate fail to mark for deletion due to failed API call", + initialSnapshots: newSnapshotArray("snap5-4", "snapuid5-4", "claim5-4", "", validSecretClass, "content5-4", &False, nil, nil, nil, false, true, &timeNowMetav1), + expectedSnapshots: newSnapshotArray("snap5-4", "snapuid5-4", "claim5-4", "", validSecretClass, "content5-4", &False, nil, nil, nil, false, true, &timeNowMetav1), + initialContents: newContentArray("content5-4", "snapuid5-4", "snap5-4", "sid5-4", validSecretClass, "", "", deletionPolicy, nil, nil, true), + // result of the test framework - annotation is still set in memory, but update call fails. + expectedContents: withContentAnnotations(newContentArray("content5-4", "snapuid5-4", "snap5-4", "sid5-4", validSecretClass, "", "", deletionPolicy, nil, nil, true), map[string]string{utils.AnnVolumeSnapshotBeingDeleted: "yes"}), + initialClaims: newClaimArray("claim5-4", "pvc-uid5-4", "1Gi", "volume5-4", v1.ClaimBound, &classEmpty), + initialVolumes: newVolumeArray("volume5-4", "pv-uid5-4", "pv-handle5-4", "1Gi", "pvc-uid5-4", "claim5-4", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty), + initialSecrets: []*v1.Secret{secret()}, + errors: []reactorError{ + // Inject error to the forth client.VolumesnapshotV1beta1().VolumeSnapshots().Update call. + {"update", "volumesnapshotcontents", errors.New("mock update error")}, + }, + expectSuccess: false, + test: testSyncContentError, + }, } runSyncTests(t, tests, snapshotClasses)