Skip to content

Commit 0c5b535

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #284 from xing-yang/error_status
Update Error in Snapshot Status
2 parents 9623453 + 5f20906 commit 0c5b535

File tree

3 files changed

+106
-8
lines changed

3 files changed

+106
-8
lines changed

‎pkg/common-controller/framework_test.go‎

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,7 @@ func (r *snapshotReactor) checkSnapshots(expectedSnapshots []*crdv1.VolumeSnapsh
453453
// Don't modify the existing object
454454
c = c.DeepCopy()
455455
c.ResourceVersion = ""
456-
if c.Status.Error != nil {
456+
if c.Status != nil && c.Status.Error != nil {
457457
c.Status.Error.Time = &metav1.Time{}
458458
}
459459
expectedMap[c.Name] = c
@@ -463,7 +463,7 @@ func (r *snapshotReactor) checkSnapshots(expectedSnapshots []*crdv1.VolumeSnapsh
463463
// written by the controller without any locks on it.
464464
c = c.DeepCopy()
465465
c.ResourceVersion = ""
466-
if c.Status.Error != nil {
466+
if c.Status != nil && c.Status.Error != nil {
467467
c.Status.Error.Time = &metav1.Time{}
468468
}
469469
gotMap[c.Name] = c
@@ -872,6 +872,18 @@ func newContentWithUnmatchDriverArray(contentName, boundToSnapshotUID, boundToSn
872872
}
873873
}
874874

875+
func newContentArrayWithError(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle string,
876+
deletionPolicy crdv1.DeletionPolicy, size, creationTime *int64,
877+
withFinalizer bool, snapshotErr *crdv1.VolumeSnapshotError) []*crdv1.VolumeSnapshotContent {
878+
content := newContent(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle, deletionPolicy, size, creationTime, withFinalizer, true)
879+
ready := false
880+
content.Status.ReadyToUse = &ready
881+
content.Status.Error = snapshotErr
882+
return []*crdv1.VolumeSnapshotContent{
883+
content,
884+
}
885+
}
886+
875887
func newSnapshot(
876888
snapshotName, snapshotUID, pvcName, targetContentName, snapshotClassName, boundContentName string,
877889
readyToUse *bool, creationTime *metav1.Time, restoreSize *resource.Quantity,
@@ -1076,6 +1088,30 @@ func testSyncSnapshotError(ctrl *csiSnapshotCommonController, reactor *snapshotR
10761088
return fmt.Errorf("syncSnapshot succeeded when failure was expected")
10771089
}
10781090

1091+
func testUpdateSnapshotErrorStatus(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
1092+
snapshot, err := ctrl.updateSnapshotStatus(test.initialSnapshots[0], test.initialContents[0])
1093+
if err != nil {
1094+
return fmt.Errorf("update snapshot status failed: %v", err)
1095+
}
1096+
var expected, got *crdv1.VolumeSnapshotError
1097+
if test.initialContents[0].Status != nil {
1098+
expected = test.initialContents[0].Status.Error
1099+
}
1100+
if snapshot.Status != nil {
1101+
got = snapshot.Status.Error
1102+
}
1103+
if expected == nil && got != nil {
1104+
return fmt.Errorf("update snapshot status failed: expected nil but got: %v", got)
1105+
}
1106+
if expected != nil && got == nil {
1107+
return fmt.Errorf("update snapshot status failed: expected: %v but got nil", expected)
1108+
}
1109+
if expected != nil && got != nil && !reflect.DeepEqual(expected, got) {
1110+
return fmt.Errorf("update snapshot status failed [A-expected, B-got]: %s", diff.ObjectDiff(expected, got))
1111+
}
1112+
return nil
1113+
}
1114+
10791115
func testSyncContent(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
10801116
return ctrl.syncContent(test.initialContents[0])
10811117
}

‎pkg/common-controller/snapshot_controller.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1031,6 +1031,10 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotStatus(snapshot *crdv1.Vo
10311031
if content.Status != nil && content.Status.ReadyToUse != nil {
10321032
readyToUse = *content.Status.ReadyToUse
10331033
}
1034+
var volumeSnapshotErr *crdv1.VolumeSnapshotError
1035+
if content.Status != nil && content.Status.Error != nil {
1036+
volumeSnapshotErr = content.Status.Error.DeepCopy()
1037+
}
10341038

10351039
klog.V(5).Infof("updateSnapshotStatus: updating VolumeSnapshot [%+v] based on VolumeSnapshotContentStatus [%+v]", snapshot, content.Status)
10361040

@@ -1052,6 +1056,9 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotStatus(snapshot *crdv1.Vo
10521056
if size != nil {
10531057
newStatus.RestoreSize = resource.NewQuantity(*size, resource.BinarySI)
10541058
}
1059+
if volumeSnapshotErr != nil {
1060+
newStatus.Error = volumeSnapshotErr
1061+
}
10551062
updated = true
10561063
} else {
10571064
newStatus = snapshotObj.Status.DeepCopy()
@@ -1074,6 +1081,10 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotStatus(snapshot *crdv1.Vo
10741081
newStatus.RestoreSize = resource.NewQuantity(*size, resource.BinarySI)
10751082
updated = true
10761083
}
1084+
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) {
1085+
newStatus.Error = volumeSnapshotErr
1086+
updated = true
1087+
}
10771088
}
10781089

10791090
if updated {

‎pkg/common-controller/snapshot_update_test.go‎

Lines changed: 57 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,19 +24,13 @@ import (
2424
crdv1 "github.com/kubernetes-csi/external-snapshotter/v2/pkg/apis/volumesnapshot/v1beta1"
2525
"github.com/kubernetes-csi/external-snapshotter/v2/pkg/utils"
2626
v1 "k8s.io/api/core/v1"
27-
storagev1beta1 "k8s.io/api/storage/v1beta1"
2827
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2928
)
3029

3130
var metaTimeNow = &metav1.Time{
3231
Time: time.Now(),
3332
}
3433

35-
var volumeErr = &storagev1beta1.VolumeError{
36-
Time: *metaTimeNow,
37-
Message: "Failed to upload the snapshot",
38-
}
39-
4034
var emptyString = ""
4135

4236
// Test single call to syncSnapshot and syncContent methods.
@@ -46,6 +40,7 @@ var emptyString = ""
4640
// 3. Compare resulting contents and snapshots with expected contents and snapshots.
4741
func TestSync(t *testing.T) {
4842
size := int64(1)
43+
snapshotErr := newVolumeError("Mock content error")
4944
tests := []controllerTest{
5045
{
5146
// snapshot is bound to a non-existing content
@@ -459,6 +454,62 @@ func TestSync(t *testing.T) {
459454
expectSuccess: false,
460455
test: testSyncContentError,
461456
},
457+
{
458+
// Update Error in snapshot status based on content status
459+
name: "6-1 - update snapshot error status",
460+
initialContents: newContentArrayWithError("content6-1", "snapuid6-1", "snap6-1", "sid6-1", validSecretClass, "", "", deletionPolicy, nil, nil, false, snapshotErr),
461+
expectedContents: newContentArrayWithError("content6-1", "snapuid6-1", "snap6-1", "sid6-1", validSecretClass, "", "", deletionPolicy, nil, nil, false, snapshotErr),
462+
initialSnapshots: newSnapshotArray("snap6-1", "snapuid6-1", "claim6-1", "", validSecretClass, "content6-1", &False, nil, nil, nil, false, true, nil),
463+
expectedSnapshots: newSnapshotArray("snap6-1", "snapuid6-1", "claim6-1", "", validSecretClass, "content6-1", &False, nil, nil, snapshotErr, false, true, nil),
464+
initialClaims: newClaimArray("claim6-1", "pvc-uid6-1", "1Gi", "volume6-1", v1.ClaimBound, &classEmpty),
465+
initialVolumes: newVolumeArray("volume6-1", "pv-uid6-1", "pv-handle6-1", "1Gi", "pvc-uid6-1", "claim6-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
466+
initialSecrets: []*v1.Secret{secret()},
467+
errors: noerrors,
468+
expectSuccess: true,
469+
test: testUpdateSnapshotErrorStatus,
470+
},
471+
{
472+
// Clear out Error in snapshot status if no Error in content status
473+
name: "6-2 - clear out snapshot error status",
474+
initialContents: newContentArray("content6-2", "snapuid6-2", "snap6-2", "sid6-2", validSecretClass, "", "", deletionPolicy, nil, nil, false),
475+
expectedContents: newContentArray("content6-2", "snapuid6-2", "snap6-2", "sid6-2", validSecretClass, "", "", deletionPolicy, nil, nil, false),
476+
initialSnapshots: newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", validSecretClass, "content6-2", &False, metaTimeNow, nil, snapshotErr, false, true, nil),
477+
expectedSnapshots: newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", validSecretClass, "content6-2", &True, metaTimeNow, nil, nil, false, true, nil),
478+
initialClaims: newClaimArray("claim6-2", "pvc-uid6-2", "1Gi", "volume6-2", v1.ClaimBound, &classEmpty),
479+
initialVolumes: newVolumeArray("volume6-2", "pv-uid6-2", "pv-handle6-2", "1Gi", "pvc-uid6-2", "claim6-2", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
480+
initialSecrets: []*v1.Secret{secret()},
481+
errors: noerrors,
482+
expectSuccess: true,
483+
test: testUpdateSnapshotErrorStatus,
484+
},
485+
{
486+
// Snapshot status is nil, but gets updated to Error status based on content status
487+
name: "6-3 - nil snapshot status updated with error status from content",
488+
initialContents: newContentArrayWithError("content6-3", "snapuid6-3", "snap6-3", "sid6-3", validSecretClass, "", "", deletionPolicy, nil, nil, false, snapshotErr),
489+
expectedContents: newContentArrayWithError("content6-3", "snapuid6-3", "snap6-3", "sid6-3", validSecretClass, "", "", deletionPolicy, nil, nil, false, snapshotErr),
490+
initialSnapshots: newSnapshotArray("snap6-3", "snapuid6-3", "claim6-3", "", validSecretClass, "", nil, nil, nil, nil, true, true, nil),
491+
expectedSnapshots: newSnapshotArray("snap6-3", "snapuid6-3", "claim6-3", "", validSecretClass, "content6-3", &False, nil, nil, snapshotErr, false, true, nil),
492+
initialClaims: newClaimArray("claim6-3", "pvc-uid6-3", "1Gi", "volume6-3", v1.ClaimBound, &classEmpty),
493+
initialVolumes: newVolumeArray("volume6-3", "pv-uid6-3", "pv-handle6-3", "1Gi", "pvc-uid6-3", "claim6-3", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
494+
initialSecrets: []*v1.Secret{secret()},
495+
errors: noerrors,
496+
expectSuccess: true,
497+
test: testUpdateSnapshotErrorStatus,
498+
},
499+
{
500+
// Snapshot status and content status are both nil, create snapshot status with boundContentName and readyToUse set to false
501+
name: "6-4 - both snapshot status and content status are nil",
502+
initialContents: newContentArrayNoStatus("content6-4", "snapuid6-4", "snap6-4", "sid6-4", validSecretClass, "", "", deletionPolicy, nil, nil, false, false),
503+
expectedContents: newContentArrayNoStatus("content6-4", "snapuid6-4", "snap6-4", "sid6-4", validSecretClass, "", "", deletionPolicy, nil, nil, false, false),
504+
initialSnapshots: newSnapshotArray("snap6-4", "snapuid6-4", "claim6-4", "", validSecretClass, "", nil, nil, nil, nil, true, false, nil),
505+
expectedSnapshots: newSnapshotArray("snap6-4", "snapuid6-4", "claim6-4", "", validSecretClass, "content6-4", &False, nil, nil, nil, false, false, nil),
506+
initialClaims: newClaimArray("claim6-4", "pvc-uid6-4", "1Gi", "volume6-3", v1.ClaimBound, &classEmpty),
507+
initialVolumes: newVolumeArray("volume6-4", "pv-uid6-4", "pv-handle6-4", "1Gi", "pvc-uid6-4", "claim6-4", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
508+
initialSecrets: []*v1.Secret{secret()},
509+
errors: noerrors,
510+
expectSuccess: true,
511+
test: testUpdateSnapshotErrorStatus,
512+
},
462513
}
463514

464515
runSyncTests(t, tests, snapshotClasses)

0 commit comments

Comments
 (0)