Merge pull request velero-io#9092 from blackpiglet/8116_fix

Avoid checking the VS and VSC status in the backup finalizing phase.

Author: blackpiglet

changelogs/unreleased/9092-blackpiglet

@@ -0,0 +1 @@
+Avoid checking the VS and VSC status in the backup finalizing phase.

pkg/backup/actions/csi/volumesnapshot_action.go

@@ -95,6 +95,16 @@ func (p *volumeSnapshotBackupItemAction) Execute(
 		)
 	}
 
+	if backup.Status.Phase == velerov1api.BackupPhaseFinalizing ||
+		backup.Status.Phase == velerov1api.BackupPhaseFinalizingPartiallyFailed {
+		p.log.
+			WithField("Backup", fmt.Sprintf("%s/%s", backup.Namespace, backup.Name)).
+			WithField("BackupPhase", backup.Status.Phase).Debugf("Cleaning VolumeSnapshots.")
+
+		csi.DeleteReadyVolumeSnapshot(*vs, p.crClient, p.log)
+		return item, nil, "", nil, nil
+	}
+
 	p.log.Infof("Getting VolumesnapshotContent for Volumesnapshot %s/%s",
 		vs.Namespace, vs.Name)
 
@@ -109,20 +119,6 @@ func (p *volumeSnapshotBackupItemAction) Execute(
 		return nil, nil, "", nil, errors.WithStack(err)
 	}
 
-	if backup.Status.Phase == velerov1api.BackupPhaseFinalizing ||
-		backup.Status.Phase == velerov1api.BackupPhaseFinalizingPartiallyFailed {
-		p.log.
-			WithField("Backup", fmt.Sprintf("%s/%s", backup.Namespace, backup.Name)).
-			WithField("BackupPhase", backup.Status.Phase).Debugf("Cleaning VolumeSnapshots.")
-
-		if vsc == nil {
-			vsc = &snapshotv1api.VolumeSnapshotContent{}
-		}
-
-		csi.DeleteReadyVolumeSnapshot(*vs, *vsc, p.crClient, p.log)
-		return item, nil, "", nil, nil
-	}
-
 	annotations := make(map[string]string)
 
 	if vsc != nil {

pkg/backup/actions/csi/volumesnapshot_action_test.go

@@ -118,6 +118,34 @@ func TestVSExecute(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "Backup in finalizing phase - skip VSC lookup",
+			backup: builder.ForBackup("velero", "backup").
+				Phase(velerov1api.BackupPhaseFinalizing).Result(),
+			vs: builder.ForVolumeSnapshot("velero", "vs").
+				ObjectMeta(builder.WithLabels(
+					velerov1api.BackupNameLabel, "backup")).
+				Status().
+				BoundVolumeSnapshotContentName("vsc").Result(),
+			vsc:                     nil, // VSC won't be created/fetched
+			expectedErr:             "",
+			expectedAdditionalItems: nil,
+			expectedItemToUpdate:    nil,
+		},
+		{
+			name: "Backup in finalizing partially failed phase - skip VSC lookup",
+			backup: builder.ForBackup("velero", "backup").
+				Phase(velerov1api.BackupPhaseFinalizingPartiallyFailed).Result(),
+			vs: builder.ForVolumeSnapshot("velero", "vs").
+				ObjectMeta(builder.WithLabels(
+					velerov1api.BackupNameLabel, "backup")).
+				Status().
+				BoundVolumeSnapshotContentName("vsc").Result(),
+			vsc:                     nil, // VSC won't be created/fetched
+			expectedErr:             "",
+			expectedAdditionalItems: nil,
+			expectedItemToUpdate:    nil,
+		},
 	}
 
 	for _, tc := range tests {

pkg/util/csi/volume_snapshot.go

@@ -489,16 +489,16 @@ func SetVolumeSnapshotContentDeletionPolicy(
 	vscName string,
 	crClient crclient.Client,
 	policy snapshotv1api.DeletionPolicy,
-) error {
+) (*snapshotv1api.VolumeSnapshotContent, error) {
 	vsc := new(snapshotv1api.VolumeSnapshotContent)
 	if err := crClient.Get(context.TODO(), crclient.ObjectKey{Name: vscName}, vsc); err != nil {
-		return err
+		return nil, err
 	}
 
 	originVSC := vsc.DeepCopy()
 	vsc.Spec.DeletionPolicy = policy
 
-	return crClient.Patch(context.TODO(), vsc, crclient.MergeFrom(originVSC))
+	return vsc, crClient.Patch(context.TODO(), vsc, crclient.MergeFrom(originVSC))
 }
 
 // CleanupVolumeSnapshot deletes the VolumeSnapshot and the associated VolumeSnapshotContent.  It will make sure the
@@ -523,7 +523,7 @@ func CleanupVolumeSnapshot(
 	if vs.Status != nil && vs.Status.BoundVolumeSnapshotContentName != nil {
 		// we patch the DeletionPolicy of the VolumeSnapshotContent to set it to Delete.
 		// This ensures that the volume snapshot in the storage provider is also deleted.
-		err := SetVolumeSnapshotContentDeletionPolicy(
+		_, err := SetVolumeSnapshotContentDeletionPolicy(
 			*vs.Status.BoundVolumeSnapshotContentName,
 			crClient,
 			snapshotv1api.VolumeSnapshotContentDelete,
@@ -544,7 +544,6 @@ func CleanupVolumeSnapshot(
 
 func DeleteReadyVolumeSnapshot(
 	vs snapshotv1api.VolumeSnapshot,
-	vsc snapshotv1api.VolumeSnapshotContent,
 	client crclient.Client,
 	logger logrus.FieldLogger,
 ) {
@@ -557,11 +556,15 @@ func DeleteReadyVolumeSnapshot(
 		return
 	}
 
+	var vsc *snapshotv1api.VolumeSnapshotContent
+
 	if vs.Status != nil && vs.Status.BoundVolumeSnapshotContentName != nil {
+		var err error
+
 		// Patch the DeletionPolicy of the VolumeSnapshotContent to set it to Retain.
 		// This ensures that the volume snapshot in the storage provider is kept.
-		if err := SetVolumeSnapshotContentDeletionPolicy(
-			vsc.Name,
+		if vsc, err = SetVolumeSnapshotContentDeletionPolicy(
+			*vs.Status.BoundVolumeSnapshotContentName,
 			client,
 			snapshotv1api.VolumeSnapshotContentRetain,
 		); err != nil {
@@ -570,7 +573,7 @@ func DeleteReadyVolumeSnapshot(
 			return
 		}
 
-		if err := client.Delete(context.TODO(), &vsc); err != nil {
+		if err := client.Delete(context.TODO(), vsc); err != nil {
 			logger.WithError(err).Warnf("Failed to delete the VolumeSnapshotContent %s", vsc.Name)
 		}
 	}

pkg/util/csi/volume_snapshot_test.go

@@ -1439,7 +1439,7 @@ func TestSetVolumeSnapshotContentDeletionPolicy(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			fakeClient := velerotest.NewFakeControllerRuntimeClient(t, tc.objs...)
-			err := SetVolumeSnapshotContentDeletionPolicy(tc.inputVSCName, fakeClient, tc.policy)
+			_, err := SetVolumeSnapshotContentDeletionPolicy(tc.inputVSCName, fakeClient, tc.policy)
 			if tc.expectError {
 				assert.Error(t, err)
 			} else {
@@ -1496,7 +1496,7 @@ func TestDeleteVolumeSnapshots(t *testing.T) {
 			)
 			logger := logging.DefaultLogger(logrus.DebugLevel, logging.FormatText)
 
-			DeleteReadyVolumeSnapshot(tc.vs, tc.vsc, client, logger)
+			DeleteReadyVolumeSnapshot(tc.vs, client, logger)
 
 			vsList := new(snapshotv1api.VolumeSnapshotList)
 			err := client.List(