From a61a073aeafbb330b8e1ef59d80367e5e6881b6e Mon Sep 17 00:00:00 2001 From: Xun Jiang Date: Thu, 17 Jul 2025 15:43:03 +0800 Subject: [PATCH] Avoid checking the VS and VSC status in the backup finalizing phase. Signed-off-by: Xun Jiang --- changelogs/unreleased/9092-blackpiglet | 1 + .../actions/csi/volumesnapshot_action.go | 24 +++++++--------- .../actions/csi/volumesnapshot_action_test.go | 28 +++++++++++++++++++ pkg/util/csi/volume_snapshot.go | 19 +++++++------ pkg/util/csi/volume_snapshot_test.go | 4 +-- 5 files changed, 52 insertions(+), 24 deletions(-) create mode 100644 changelogs/unreleased/9092-blackpiglet diff --git a/changelogs/unreleased/9092-blackpiglet b/changelogs/unreleased/9092-blackpiglet new file mode 100644 index 000000000..6aa795ec1 --- /dev/null +++ b/changelogs/unreleased/9092-blackpiglet @@ -0,0 +1 @@ +Avoid checking the VS and VSC status in the backup finalizing phase. diff --git a/pkg/backup/actions/csi/volumesnapshot_action.go b/pkg/backup/actions/csi/volumesnapshot_action.go index 14a3d0b5e..4f012f8cc 100644 --- a/pkg/backup/actions/csi/volumesnapshot_action.go +++ b/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 { diff --git a/pkg/backup/actions/csi/volumesnapshot_action_test.go b/pkg/backup/actions/csi/volumesnapshot_action_test.go index 381daa2ee..1db5cd475 100644 --- a/pkg/backup/actions/csi/volumesnapshot_action_test.go +++ b/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 { diff --git a/pkg/util/csi/volume_snapshot.go b/pkg/util/csi/volume_snapshot.go index f9be418a5..ef70efad0 100644 --- a/pkg/util/csi/volume_snapshot.go +++ b/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) } } diff --git a/pkg/util/csi/volume_snapshot_test.go b/pkg/util/csi/volume_snapshot_test.go index d73e0f910..72a8e4765 100644 --- a/pkg/util/csi/volume_snapshot_test.go +++ b/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(