Update Error in Snapshot Status

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])
 }

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 {

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)