From a46b86fa29cefb2b26e1ff9cf974411b75ae7a56 Mon Sep 17 00:00:00 2001 From: 0xLeo258 Date: Thu, 11 Sep 2025 14:45:39 +0800 Subject: [PATCH 1/3] fix9247: Protect VolumeSnapshot field Signed-off-by: 0xLeo258 --- pkg/backup/item_backupper.go | 2 ++ pkg/backup/request.go | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/backup/item_backupper.go b/pkg/backup/item_backupper.go index b0e5fadfa..e16e4cd23 100644 --- a/pkg/backup/item_backupper.go +++ b/pkg/backup/item_backupper.go @@ -673,6 +673,8 @@ func (ib *itemBackupper) takePVSnapshot(obj runtime.Unstructured, log logrus.Fie snapshot.Status.Phase = volume.SnapshotPhaseCompleted snapshot.Status.ProviderSnapshotID = snapshotID } + ib.backupRequest.requestLock.Lock() + defer ib.backupRequest.requestLock.Unlock() ib.backupRequest.VolumeSnapshots = append(ib.backupRequest.VolumeSnapshots, snapshot) // nil errors are automatically removed diff --git a/pkg/backup/request.go b/pkg/backup/request.go index 3ec05ee04..b81bde74d 100644 --- a/pkg/backup/request.go +++ b/pkg/backup/request.go @@ -17,6 +17,8 @@ limitations under the License. package backup import ( + "sync" + "github.com/vmware-tanzu/velero/internal/hook" "github.com/vmware-tanzu/velero/internal/resourcepolicies" "github.com/vmware-tanzu/velero/internal/volume" @@ -36,7 +38,7 @@ type itemKey struct { // materialized (e.g. backup/snapshot locations, includes/excludes, etc.) type Request struct { *velerov1api.Backup - + requestLock sync.Mutex StorageLocation *velerov1api.BackupStorageLocation SnapshotLocations []*velerov1api.VolumeSnapshotLocation NamespaceIncludesExcludes *collections.IncludesExcludes From d82af8d8b5967cc3918ba71b1614a3cdc3164f1b Mon Sep 17 00:00:00 2001 From: 0xLeo258 Date: Thu, 11 Sep 2025 14:45:39 +0800 Subject: [PATCH 2/3] add changelog Signed-off-by: 0xLeo258 --- changelogs/unreleased/9248-0xLeo258 | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/9248-0xLeo258 diff --git a/changelogs/unreleased/9248-0xLeo258 b/changelogs/unreleased/9248-0xLeo258 new file mode 100644 index 000000000..8332dcade --- /dev/null +++ b/changelogs/unreleased/9248-0xLeo258 @@ -0,0 +1 @@ +Protect VolumeSnapshot field from race condition during multi-thread backup From d1f7f152b71d801f691787e75e258b8f6fa1a3a6 Mon Sep 17 00:00:00 2001 From: 0xLeo258 Date: Tue, 16 Sep 2025 13:25:54 +0800 Subject: [PATCH 3/3] Add built-in mutex for SynchronizedVSList && Update unit tests Signed-off-by: 0xLeo258 --- .../{9248-0xLeo258 => 9292-0xLeo258} | 0 pkg/backup/backup_test.go | 4 ++-- pkg/backup/item_backupper.go | 4 +--- pkg/backup/request.go | 22 ++++++++++++++++--- pkg/controller/backup_controller.go | 6 ++--- 5 files changed, 25 insertions(+), 11 deletions(-) rename changelogs/unreleased/{9248-0xLeo258 => 9292-0xLeo258} (100%) diff --git a/changelogs/unreleased/9248-0xLeo258 b/changelogs/unreleased/9292-0xLeo258 similarity index 100% rename from changelogs/unreleased/9248-0xLeo258 rename to changelogs/unreleased/9292-0xLeo258 diff --git a/pkg/backup/backup_test.go b/pkg/backup/backup_test.go index 1cf80fe0f..eb6c2d9d4 100644 --- a/pkg/backup/backup_test.go +++ b/pkg/backup/backup_test.go @@ -3269,7 +3269,7 @@ func TestBackupWithSnapshots(t *testing.T) { err := h.backupper.Backup(h.log, tc.req, backupFile, nil, nil, tc.snapshotterGetter) require.NoError(t, err) - assert.Equal(t, tc.want, tc.req.VolumeSnapshots) + assert.Equal(t, tc.want, tc.req.VolumeSnapshots.Get()) }) } } @@ -4213,7 +4213,7 @@ func TestBackupWithPodVolume(t *testing.T) { assert.Equal(t, tc.want, req.PodVolumeBackups) // this assumes that we don't have any test cases where some PVs should be snapshotted using a VolumeSnapshotter - assert.Nil(t, req.VolumeSnapshots) + assert.Nil(t, req.VolumeSnapshots.Get()) }) } } diff --git a/pkg/backup/item_backupper.go b/pkg/backup/item_backupper.go index e16e4cd23..3622ac7e1 100644 --- a/pkg/backup/item_backupper.go +++ b/pkg/backup/item_backupper.go @@ -673,9 +673,7 @@ func (ib *itemBackupper) takePVSnapshot(obj runtime.Unstructured, log logrus.Fie snapshot.Status.Phase = volume.SnapshotPhaseCompleted snapshot.Status.ProviderSnapshotID = snapshotID } - ib.backupRequest.requestLock.Lock() - defer ib.backupRequest.requestLock.Unlock() - ib.backupRequest.VolumeSnapshots = append(ib.backupRequest.VolumeSnapshots, snapshot) + ib.backupRequest.VolumeSnapshots.Add(snapshot) // nil errors are automatically removed return kubeerrs.NewAggregate(errs) diff --git a/pkg/backup/request.go b/pkg/backup/request.go index b81bde74d..c3dae48a6 100644 --- a/pkg/backup/request.go +++ b/pkg/backup/request.go @@ -34,11 +34,27 @@ type itemKey struct { name string } +type SynchronizedVSList struct { + sync.Mutex + VolumeSnapshotList []*volume.Snapshot +} + +func (s *SynchronizedVSList) Add(vs *volume.Snapshot) { + s.Lock() + defer s.Unlock() + s.VolumeSnapshotList = append(s.VolumeSnapshotList, vs) +} + +func (s *SynchronizedVSList) Get() []*volume.Snapshot { + s.Lock() + defer s.Unlock() + return s.VolumeSnapshotList +} + // Request is a request for a backup, with all references to other objects // materialized (e.g. backup/snapshot locations, includes/excludes, etc.) type Request struct { *velerov1api.Backup - requestLock sync.Mutex StorageLocation *velerov1api.BackupStorageLocation SnapshotLocations []*velerov1api.VolumeSnapshotLocation NamespaceIncludesExcludes *collections.IncludesExcludes @@ -46,7 +62,7 @@ type Request struct { ResourceHooks []hook.ResourceHook ResolvedActions []framework.BackupItemResolvedActionV2 ResolvedItemBlockActions []framework.ItemBlockResolvedAction - VolumeSnapshots []*volume.Snapshot + VolumeSnapshots SynchronizedVSList PodVolumeBackups []*velerov1api.PodVolumeBackup BackedUpItems *backedUpItemsMap itemOperationsList *[]*itemoperation.BackupOperation @@ -82,7 +98,7 @@ func (r *Request) FillVolumesInformation() { } r.VolumesInformation.SkippedPVs = skippedPVMap - r.VolumesInformation.NativeSnapshots = r.VolumeSnapshots + r.VolumesInformation.NativeSnapshots = r.VolumeSnapshots.Get() r.VolumesInformation.PodVolumeBackups = r.PodVolumeBackups r.VolumesInformation.BackupOperations = *r.GetItemOperationsList() r.VolumesInformation.BackupName = r.Backup.Name diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 532a5f332..c9728ea82 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -734,8 +734,8 @@ func (b *backupReconciler) runBackup(backup *pkgbackup.Request) error { // native snapshots phase will either be failed or completed right away // https://github.com/vmware-tanzu/velero/blob/de3ea52f0cc478e99efa7b9524c7f353514261a4/pkg/backup/item_backupper.go#L632-L639 - backup.Status.VolumeSnapshotsAttempted = len(backup.VolumeSnapshots) - for _, snap := range backup.VolumeSnapshots { + backup.Status.VolumeSnapshotsAttempted = len(backup.VolumeSnapshots.Get()) + for _, snap := range backup.VolumeSnapshots.Get() { if snap.Status.Phase == volume.SnapshotPhaseCompleted { backup.Status.VolumeSnapshotsCompleted++ } @@ -882,7 +882,7 @@ func persistBackup(backup *pkgbackup.Request, } // Velero-native volume snapshots (as opposed to CSI ones) - nativeVolumeSnapshots, errs := encode.ToJSONGzip(backup.VolumeSnapshots, "native volumesnapshots list") + nativeVolumeSnapshots, errs := encode.ToJSONGzip(backup.VolumeSnapshots.Get(), "native volumesnapshots list") if errs != nil { persistErrs = append(persistErrs, errs...) }