Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor common controller tests to use withXYZ functions and add tests #254

Merged
merged 2 commits into from
Feb 19, 2020
Merged

Refactor common controller tests to use withXYZ functions and add tests #254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7811e59
Refactor delete test to use withSnapshotFinalizers function
ggriffiths Feb 12, 2020
05efba2
Add more snapshot and content sync tests
ggriffiths Feb 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 32 additions & 11 deletions pkg/common-controller/framework_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see that utils.VolumeSnapshotContentFinalizer is added to VolumeSnapshot API object here. This is wrong. We only need utils.VolumeSnapshotContentFinalizer for VolumeSnapshotContent, not for VolumeSnapshot.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, removed the contentFinalizer.

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
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the test framework in pv/pvc controller will allow you to create a basic newSnapshot() without passing in the withAllFinalizers flag, and then we can have another function withAllFinalizers() which takes a created snapshot object as input and apply finalizers on top of it. So that means we shouldn't need "withAllFinalizers bool" as an input parameter for newSnapshot().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left "withFinalizers" as "withAllFinalizers" for the purpose of reducing the refactor required. Some tests require all finalizers while others require no finalizers, so pretty much every test would have the "withSnapFinalizers" wrapper.

I mainly introduced "func withSnapshotFinalizers()" to fine-tune which finalizers are added. i.e. for cases where only one finalizer is wanted.

snapshot := crdv1.VolumeSnapshot{
ObjectMeta: metav1.ObjectMeta{
Name: snapshotName,
Expand Down Expand Up @@ -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),
}
}

Expand Down Expand Up @@ -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])
}
Expand Down
70 changes: 22 additions & 48 deletions pkg/common-controller/snapshot_delete_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
}
56 changes: 55 additions & 1 deletion pkg/common-controller/snapshot_update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,11 @@ limitations under the License.
package common_controller

import (
//"errors"
"errors"
ggriffiths marked this conversation as resolved.
Show resolved Hide resolved
"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"
Expand Down Expand Up @@ -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)
Expand Down