fix: remove unused VSErrorFirstObservedTimeAnnotation and update error handling in VolumeSnapshot progress

Signed-off-by: Priyansh Choudhary <im1706@gmail.com>

Author: priyansh17

pkg/apis/velero/v1/labels_annotations.go

@@ -178,9 +178,4 @@ const (
 
 	// Annotation prefix on Backup to override VGS class per CSI driver
 	VolumeGroupSnapshotClassAnnotationBackupPrefix = "velero.io/csi-volumegroupsnapshot-class_"
-
-	// VSErrorFirstObservedTimeAnnotation is the annotation key used to record the time
-	// when a VolumeSnapshot error was first observed during an async backup operation.
-	// This is used by Progress() to detect errors that persist beyond CSISnapshotTimeout.
-	VSErrorFirstObservedTimeAnnotation = "velero.io/vs-error-first-observed-time"
 )

pkg/backup/actions/csi/volumesnapshot_action.go

@@ -295,37 +295,13 @@ func (p *volumeSnapshotBackupItemAction) Progress(
 			errorMessage = *vs.Status.Error.Message
 		}
 
-		now := time.Now()
-		firstObservedTime := now
-
-		if existingTime, ok := vs.Annotations[velerov1api.VSErrorFirstObservedTimeAnnotation]; ok {
-			if t, err := time.Parse(time.RFC3339, existingTime); err == nil {
-				firstObservedTime = t
-			} else {
-				p.log.Warnf("VolumeSnapshot %s/%s has an unparsable %s annotation value %q, resetting timer: %v",
-					vs.Namespace, vs.Name, velerov1api.VSErrorFirstObservedTimeAnnotation, existingTime, err)
-			}
-		} else {
-			// First time we observe this error — stamp the annotation onto the VS.
-			originVS := vs.DeepCopy()
-			kubeutil.AddAnnotations(&vs.ObjectMeta, map[string]string{
-				velerov1api.VSErrorFirstObservedTimeAnnotation: now.Format(time.RFC3339),
-			})
-			if patchErr := p.crClient.Patch(
-				context.Background(), vs, crclient.MergeFrom(originVS),
-			); patchErr != nil {
-				p.log.Warnf("Failed to patch VolumeSnapshot %s/%s with error first observed time: %v",
-					vs.Namespace, vs.Name, patchErr)
-			}
-		}
-
 		timeout := backup.Spec.CSISnapshotTimeout.Duration
-		if timeout > 0 && now.Sub(firstObservedTime) >= timeout {
+		if timeout > 0 && time.Since(progress.Started) >= timeout {
 			p.log.Errorf(
 				"VolumeSnapshot %s/%s has a persistent error beyond CSISnapshotTimeout (%s): %s",
 				vs.Namespace, vs.Name, timeout, errorMessage)
 			progress.Completed = true
-			progress.Updated = now
+			progress.Updated = time.Now()
 			progress.Err = fmt.Sprintf("VolumeSnapshot %s/%s has a persistent error: %s",
 				vs.Namespace, vs.Name, errorMessage)
 			return progress, nil
@@ -362,12 +338,24 @@ func (p *volumeSnapshotBackupItemAction) Progress(
 			progress.Completed = true
 			progress.Updated = now
 		} else if vsc.Status.Error != nil {
-			progress.Completed = true
-			progress.Updated = now
+			errorMessage := ""
 			if vsc.Status.Error.Message != nil {
-				progress.Err = *vsc.Status.Error.Message
+				errorMessage = *vsc.Status.Error.Message
+			}
+
+			timeout := backup.Spec.CSISnapshotTimeout.Duration
+			if timeout > 0 && time.Since(progress.Started) >= timeout {
+				p.log.Errorf(
+					"VolumeSnapshotContent %s has a persistent error beyond CSISnapshotTimeout (%s): %s",
+					vsc.Name, timeout, errorMessage)
+				progress.Completed = true
+				progress.Updated = now
+				progress.Err = fmt.Sprintf("VolumeSnapshotContent %s has a persistent error: %s",
+					vsc.Name, errorMessage)
+			} else {
+				p.log.Warnf("VolumeSnapshotContent %s has an error within the CSISnapshotTimeout window: %s. Snapshot controller will retry later.",
+					vsc.Name, errorMessage)
 			}
-			p.log.Warnf("VolumeSnapshotContent meets an error %s.", progress.Err)
 		}
 	}
 

pkg/backup/actions/csi/volumesnapshot_action_test.go

@@ -230,13 +230,9 @@ func TestVSProgress(t *testing.T) {
 			expectedErr: false,
 		},
 		{
-			name:        "VS status has error, annotation within CSISnapshotTimeout",
-			operationID: "ns/name/2024-04-11T18:49:00+08:00",
-			vs: builder.ForVolumeSnapshot("ns", "name").
-				ObjectMeta(builder.WithAnnotations(
-					velerov1api.VSErrorFirstObservedTimeAnnotation, time.Now().Format(time.RFC3339),
-				)).
-				Status().
+			name:        "VS status has error, within CSISnapshotTimeout (recent start time)",
+			operationID: "ns/name/" + time.Now().Format(time.RFC3339),
+			vs: builder.ForVolumeSnapshot("ns", "name").Status().
 				StatusError(snapshotv1api.VolumeSnapshotError{
 					Message: &errorStr,
 				}).Result(),
@@ -246,11 +242,7 @@ func TestVSProgress(t *testing.T) {
 		{
 			name:        "VS status has persistent error beyond CSISnapshotTimeout",
 			operationID: "ns/name/2024-04-11T18:49:00+08:00",
-			vs: builder.ForVolumeSnapshot("ns", "name").
-				ObjectMeta(builder.WithAnnotations(
-					velerov1api.VSErrorFirstObservedTimeAnnotation, "2024-04-11T18:49:00+08:00",
-				)).
-				Status().
+			vs: builder.ForVolumeSnapshot("ns", "name").Status().
 				StatusError(snapshotv1api.VolumeSnapshotError{
 					Message: &errorStr,
 				}).Result(),
@@ -292,7 +284,21 @@ func TestVSProgress(t *testing.T) {
 			expectedProgress: &velero.OperationProgress{Completed: true},
 		},
 		{
-			name:        "VSC status has error",
+			name:        "VSC status has error within CSISnapshotTimeout",
+			operationID: "ns/name/" + time.Now().Format(time.RFC3339),
+			vs: builder.ForVolumeSnapshot("ns", "name").Status().
+				ReadyToUse(true).BoundVolumeSnapshotContentName("vsc").Result(),
+			vsc: builder.ForVolumeSnapshotContent("vsc").
+				Status(&snapshotv1api.VolumeSnapshotContentStatus{
+					Error: &snapshotv1api.VolumeSnapshotError{
+						Message: &errorStr,
+					},
+				}).Result(),
+			backup:      builder.ForBackup("velero", "backup").CSISnapshotTimeout(10 * time.Minute).Result(),
+			expectedErr: false,
+		},
+		{
+			name:        "VSC status has persistent error beyond CSISnapshotTimeout",
 			operationID: "ns/name/2024-04-11T18:49:00+08:00",
 			vs: builder.ForVolumeSnapshot("ns", "name").Status().
 				ReadyToUse(true).BoundVolumeSnapshotContentName("vsc").Result(),
@@ -302,11 +308,11 @@ func TestVSProgress(t *testing.T) {
 						Message: &errorStr,
 					},
 				}).Result(),
-			backup:      builder.ForBackup("velero", "backup").Result(),
+			backup:      builder.ForBackup("velero", "backup").CSISnapshotTimeout(10 * time.Minute).Result(),
 			expectedErr: false,
 			expectedProgress: &velero.OperationProgress{
 				Completed: true,
-				Err:       "error",
+				Err:       fmt.Sprintf("VolumeSnapshotContent vsc has a persistent error: %s", errorStr),
 			},
 		},
 	}