Merge pull request #254 from ggriffiths/common-ctrl-test-refactor

Refactor common controller tests to use withXYZ functions and add tests

Author: k8s-ci-robot

pkg/common-controller/framework_test.go

@@ -164,16 +164,17 @@ 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 {
 	content.ObjectMeta.Finalizers = append(content.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer)
-	metav1.SetMetaDataAnnotation(&content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted, "yes")
 	return content
 }
 
@@ -824,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 {
@@ -863,7 +876,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,18 +917,18 @@ func newSnapshot(
 			VolumeSnapshotContentName: &targetContentName,
 		}
 	}
-	if withFinalizer {
-		return withSnapshotFinalizer(&snapshot)
+	if withAllFinalizers {
+		return withSnapshotFinalizers([]*crdv1.VolumeSnapshot{&snapshot}, utils.VolumeSnapshotAsSourceFinalizer, utils.VolumeSnapshotBoundFinalizer)[0]
 	}
 	return &snapshot
 }
 
 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),
 	}
 }
 
@@ -1071,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])
 }

pkg/common-controller/snapshot_delete_test.go

@@ -17,14 +17,13 @@ limitations under the License.
 package common_controller
 
 import (
-	//"errors"
+	"errors"
 	"testing"
 
 	crdv1 "github.com/kubernetes-csi/external-snapshotter/v2/pkg/apis/volumesnapshot/v1beta1"
 	"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{
@@ -175,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,42 +288,32 @@ 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.VolumeSnapshotBoundFinalizer,
+			),
 			initialClaims:  newClaimArray("claim3-1", "pvc-uid3-1", "1Gi", "volume3-1", v1.ClaimBound, &classEmpty),
 			expectedEvents: noevents,
 			initialSecrets: []*v1.Secret{secret()},
 			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)
 }

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)