From 5f2090606cbb79b42479208eceb2738392673db2 Mon Sep 17 00:00:00 2001 From: xing-yang Date: Wed, 1 Apr 2020 04:34:10 +0000 Subject: [PATCH] Update Error in Snapshot Status --- pkg/common-controller/framework_test.go | 40 +++++++++++- pkg/common-controller/snapshot_controller.go | 11 ++++ pkg/common-controller/snapshot_update_test.go | 63 +++++++++++++++++-- 3 files changed, 106 insertions(+), 8 deletions(-) diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go index 0eadcd9a..a273dba8 100644 --- a/pkg/common-controller/framework_test.go +++ b/pkg/common-controller/framework_test.go @@ -453,7 +453,7 @@ func (r *snapshotReactor) checkSnapshots(expectedSnapshots []*crdv1.VolumeSnapsh // Don't modify the existing object c = c.DeepCopy() c.ResourceVersion = "" - if c.Status.Error != nil { + if c.Status != nil && c.Status.Error != nil { c.Status.Error.Time = &metav1.Time{} } expectedMap[c.Name] = c @@ -463,7 +463,7 @@ func (r *snapshotReactor) checkSnapshots(expectedSnapshots []*crdv1.VolumeSnapsh // written by the controller without any locks on it. c = c.DeepCopy() c.ResourceVersion = "" - if c.Status.Error != nil { + if c.Status != nil && c.Status.Error != nil { c.Status.Error.Time = &metav1.Time{} } gotMap[c.Name] = c @@ -880,6 +880,18 @@ func newContentWithUnmatchDriverArray(contentName, boundToSnapshotUID, boundToSn } } +func newContentArrayWithError(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle string, + deletionPolicy crdv1.DeletionPolicy, size, creationTime *int64, + withFinalizer bool, snapshotErr *crdv1.VolumeSnapshotError) []*crdv1.VolumeSnapshotContent { + content := newContent(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle, deletionPolicy, size, creationTime, withFinalizer, true) + ready := false + content.Status.ReadyToUse = &ready + content.Status.Error = snapshotErr + return []*crdv1.VolumeSnapshotContent{ + content, + } +} + func newSnapshot( snapshotName, snapshotUID, pvcName, targetContentName, snapshotClassName, boundContentName string, readyToUse *bool, creationTime *metav1.Time, restoreSize *resource.Quantity, @@ -1087,6 +1099,30 @@ func testSyncSnapshotError(ctrl *csiSnapshotCommonController, reactor *snapshotR return fmt.Errorf("syncSnapshot succeeded when failure was expected") } +func testUpdateSnapshotErrorStatus(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error { + snapshot, err := ctrl.updateSnapshotStatus(test.initialSnapshots[0], test.initialContents[0]) + if err != nil { + return fmt.Errorf("update snapshot status failed: %v", err) + } + var expected, got *crdv1.VolumeSnapshotError + if test.initialContents[0].Status != nil { + expected = test.initialContents[0].Status.Error + } + if snapshot.Status != nil { + got = snapshot.Status.Error + } + if expected == nil && got != nil { + return fmt.Errorf("update snapshot status failed: expected nil but got: %v", got) + } + if expected != nil && got == nil { + return fmt.Errorf("update snapshot status failed: expected: %v but got nil", expected) + } + if expected != nil && got != nil && !reflect.DeepEqual(expected, got) { + return fmt.Errorf("update snapshot status failed [A-expected, B-got]: %s", diff.ObjectDiff(expected, got)) + } + return nil +} + func testSyncContent(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error { return ctrl.syncContent(test.initialContents[0]) } diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 467f3240..de3130bd 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -943,6 +943,10 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotStatus(snapshot *crdv1.Vo if content.Status != nil && content.Status.ReadyToUse != nil { readyToUse = *content.Status.ReadyToUse } + var volumeSnapshotErr *crdv1.VolumeSnapshotError + if content.Status != nil && content.Status.Error != nil { + volumeSnapshotErr = content.Status.Error.DeepCopy() + } klog.V(5).Infof("updateSnapshotStatus: updating VolumeSnapshot [%+v] based on VolumeSnapshotContentStatus [%+v]", snapshot, content.Status) @@ -964,6 +968,9 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotStatus(snapshot *crdv1.Vo if size != nil { newStatus.RestoreSize = resource.NewQuantity(*size, resource.BinarySI) } + if volumeSnapshotErr != nil { + newStatus.Error = volumeSnapshotErr + } updated = true } else { newStatus = snapshotObj.Status.DeepCopy() @@ -986,6 +993,10 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotStatus(snapshot *crdv1.Vo newStatus.RestoreSize = resource.NewQuantity(*size, resource.BinarySI) updated = true } + if (newStatus.Error == nil && volumeSnapshotErr != nil) || (newStatus.Error != nil && volumeSnapshotErr != nil && newStatus.Error.Time != nil && volumeSnapshotErr.Time != nil && &newStatus.Error.Time != &volumeSnapshotErr.Time) || (newStatus.Error != nil && volumeSnapshotErr == nil) { + newStatus.Error = volumeSnapshotErr + updated = true + } } if updated { diff --git a/pkg/common-controller/snapshot_update_test.go b/pkg/common-controller/snapshot_update_test.go index 7f71ef06..dc8f2cad 100644 --- a/pkg/common-controller/snapshot_update_test.go +++ b/pkg/common-controller/snapshot_update_test.go @@ -24,7 +24,6 @@ import ( 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" - storagev1beta1 "k8s.io/api/storage/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -32,11 +31,6 @@ var metaTimeNow = &metav1.Time{ Time: time.Now(), } -var volumeErr = &storagev1beta1.VolumeError{ - Time: *metaTimeNow, - Message: "Failed to upload the snapshot", -} - var emptyString = "" // Test single call to syncSnapshot and syncContent methods. @@ -46,6 +40,7 @@ var emptyString = "" // 3. Compare resulting contents and snapshots with expected contents and snapshots. func TestSync(t *testing.T) { size := int64(1) + snapshotErr := newVolumeError("Mock content error") tests := []controllerTest{ { // snapshot is bound to a non-existing content @@ -327,6 +322,62 @@ func TestSync(t *testing.T) { expectSuccess: true, test: testSyncSnapshot, }, + { + // Update Error in snapshot status based on content status + name: "6-1 - update snapshot error status", + initialContents: newContentArrayWithError("content6-1", "snapuid6-1", "snap6-1", "sid6-1", validSecretClass, "", "", deletionPolicy, nil, nil, false, snapshotErr), + expectedContents: newContentArrayWithError("content6-1", "snapuid6-1", "snap6-1", "sid6-1", validSecretClass, "", "", deletionPolicy, nil, nil, false, snapshotErr), + initialSnapshots: newSnapshotArray("snap6-1", "snapuid6-1", "claim6-1", "", validSecretClass, "content6-1", &False, nil, nil, nil, false, true, nil), + expectedSnapshots: newSnapshotArray("snap6-1", "snapuid6-1", "claim6-1", "", validSecretClass, "content6-1", &False, nil, nil, snapshotErr, false, true, nil), + initialClaims: newClaimArray("claim6-1", "pvc-uid6-1", "1Gi", "volume6-1", v1.ClaimBound, &classEmpty), + initialVolumes: newVolumeArray("volume6-1", "pv-uid6-1", "pv-handle6-1", "1Gi", "pvc-uid6-1", "claim6-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty), + initialSecrets: []*v1.Secret{secret()}, + errors: noerrors, + expectSuccess: true, + test: testUpdateSnapshotErrorStatus, + }, + { + // Clear out Error in snapshot status if no Error in content status + name: "6-2 - clear out snapshot error status", + initialContents: newContentArray("content6-2", "snapuid6-2", "snap6-2", "sid6-2", validSecretClass, "", "", deletionPolicy, nil, nil, false), + expectedContents: newContentArray("content6-2", "snapuid6-2", "snap6-2", "sid6-2", validSecretClass, "", "", deletionPolicy, nil, nil, false), + initialSnapshots: newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", validSecretClass, "content6-2", &False, metaTimeNow, nil, snapshotErr, false, true, nil), + expectedSnapshots: newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", validSecretClass, "content6-2", &True, metaTimeNow, nil, nil, false, true, nil), + initialClaims: newClaimArray("claim6-2", "pvc-uid6-2", "1Gi", "volume6-2", v1.ClaimBound, &classEmpty), + initialVolumes: newVolumeArray("volume6-2", "pv-uid6-2", "pv-handle6-2", "1Gi", "pvc-uid6-2", "claim6-2", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty), + initialSecrets: []*v1.Secret{secret()}, + errors: noerrors, + expectSuccess: true, + test: testUpdateSnapshotErrorStatus, + }, + { + // Snapshot status is nil, but gets updated to Error status based on content status + name: "6-3 - nil snapshot status updated with error status from content", + initialContents: newContentArrayWithError("content6-3", "snapuid6-3", "snap6-3", "sid6-3", validSecretClass, "", "", deletionPolicy, nil, nil, false, snapshotErr), + expectedContents: newContentArrayWithError("content6-3", "snapuid6-3", "snap6-3", "sid6-3", validSecretClass, "", "", deletionPolicy, nil, nil, false, snapshotErr), + initialSnapshots: newSnapshotArray("snap6-3", "snapuid6-3", "claim6-3", "", validSecretClass, "", nil, nil, nil, nil, true, true, nil), + expectedSnapshots: newSnapshotArray("snap6-3", "snapuid6-3", "claim6-3", "", validSecretClass, "content6-3", &False, nil, nil, snapshotErr, false, true, nil), + initialClaims: newClaimArray("claim6-3", "pvc-uid6-3", "1Gi", "volume6-3", v1.ClaimBound, &classEmpty), + initialVolumes: newVolumeArray("volume6-3", "pv-uid6-3", "pv-handle6-3", "1Gi", "pvc-uid6-3", "claim6-3", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty), + initialSecrets: []*v1.Secret{secret()}, + errors: noerrors, + expectSuccess: true, + test: testUpdateSnapshotErrorStatus, + }, + { + // Snapshot status and content status are both nil, create snapshot status with boundContentName and readyToUse set to false + name: "6-4 - both snapshot status and content status are nil", + initialContents: newContentArrayNoStatus("content6-4", "snapuid6-4", "snap6-4", "sid6-4", validSecretClass, "", "", deletionPolicy, nil, nil, false, false), + expectedContents: newContentArrayNoStatus("content6-4", "snapuid6-4", "snap6-4", "sid6-4", validSecretClass, "", "", deletionPolicy, nil, nil, false, false), + initialSnapshots: newSnapshotArray("snap6-4", "snapuid6-4", "claim6-4", "", validSecretClass, "", nil, nil, nil, nil, true, false, nil), + expectedSnapshots: newSnapshotArray("snap6-4", "snapuid6-4", "claim6-4", "", validSecretClass, "content6-4", &False, nil, nil, nil, false, false, nil), + initialClaims: newClaimArray("claim6-4", "pvc-uid6-4", "1Gi", "volume6-3", v1.ClaimBound, &classEmpty), + initialVolumes: newVolumeArray("volume6-4", "pv-uid6-4", "pv-handle6-4", "1Gi", "pvc-uid6-4", "claim6-4", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty), + initialSecrets: []*v1.Secret{secret()}, + errors: noerrors, + expectSuccess: true, + test: testUpdateSnapshotErrorStatus, + }, } runSyncTests(t, tests, snapshotClasses)