From 8ec4b224968693eba686cffb305781a802051f34 Mon Sep 17 00:00:00 2001 From: Jay2006sawant Date: Mon, 3 Aug 2026 09:44:00 +0530 Subject: [PATCH 1/3] fix: return errors correctly in block restore validation and BatchForget Signed-off-by: Jay2006sawant --- .../fix-error-handling-Jay2006sawant | 9 ++++ pkg/repository/provider/unified_repo.go | 2 +- pkg/repository/provider/unified_repo_test.go | 35 ++++++++++++++ pkg/uploader/block/uploader.go | 6 +-- pkg/uploader/block/uploader_test.go | 48 +++++++++++++++++++ 5 files changed, 96 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/fix-error-handling-Jay2006sawant diff --git a/changelogs/unreleased/fix-error-handling-Jay2006sawant b/changelogs/unreleased/fix-error-handling-Jay2006sawant new file mode 100644 index 000000000..82a05f6c3 --- /dev/null +++ b/changelogs/unreleased/fix-error-handling-Jay2006sawant @@ -0,0 +1,9 @@ +fix: return errors correctly in block restore validation and BatchForget + +Block uploader Restore used errors.Wrapf with a stale nil err after +successful getSourceSize, causing size validation failures to return +(0, nil). flushZeroBlocks had the same pattern on short writes. + +BatchForget dropped delete errors when flush also failed. + +Signed-off-by: Jay2006sawant diff --git a/pkg/repository/provider/unified_repo.go b/pkg/repository/provider/unified_repo.go index bfe1a2bd9..664750c77 100644 --- a/pkg/repository/provider/unified_repo.go +++ b/pkg/repository/provider/unified_repo.go @@ -384,7 +384,7 @@ func (urp *unifiedRepoProvider) BatchForget(ctx context.Context, snapshotIDs []s err = bkRepo.Flush(ctx) if err != nil { - return []error{errors.Wrap(err, "error to flush repo")} + errs = append(errs, errors.Wrap(err, "error to flush repo")) } log.Debug("Forget snapshot complete") diff --git a/pkg/repository/provider/unified_repo_test.go b/pkg/repository/provider/unified_repo_test.go index e0e0a8b8f..2cd9bf576 100644 --- a/pkg/repository/provider/unified_repo_test.go +++ b/pkg/repository/provider/unified_repo_test.go @@ -1062,6 +1062,41 @@ func TestBatchForget(t *testing.T) { }, expectedErr: []string{"error to flush repo: fake-error-4"}, }, + { + name: "delete and flush fail", + getter: new(credmock.SecretStore), + credStoreReturn: "fake-password", + funcTable: localFuncTable{ + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, map[string]string, velerocredentials.CredentialGetter) (map[string]string, error) { + return map[string]string{}, nil + }, + getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { + return map[string]string{}, nil + }, + }, + repoService: new(reposervicenmocks.BackupRepoService), + backupRepo: new(reposervicenmocks.BackupRepo), + retFuncOpen: []any{ + func(context.Context, udmrepo.RepoOptions) udmrepo.BackupRepo { + return backupRepo + }, + + func(context.Context, udmrepo.RepoOptions) error { + return nil + }, + }, + retFuncDelete: func(context.Context, udmrepo.ID) error { + return errors.New("fake-delete-error") + }, + retFuncFlush: func(context.Context) error { + return errors.New("fake-flush-error") + }, + snapshots: []string{"snapshot-1"}, + expectedErr: []string{ + "error to delete manifest snapshot-1: fake-delete-error", + "error to flush repo: fake-flush-error", + }, + }, } for _, tc := range testCases { diff --git a/pkg/uploader/block/uploader.go b/pkg/uploader/block/uploader.go index 1d74bd462..824c0ae9f 100644 --- a/pkg/uploader/block/uploader.go +++ b/pkg/uploader/block/uploader.go @@ -169,11 +169,11 @@ func (blkup *blockUploader) Restore(snapshot udmrepo.Snapshot, dest destInfo, bi } if sourceSize > meta.SubObjects[0].Size { - return 0, errors.Wrapf(err, "unexpected size (%v vs. %v) for bdev object %s", meta.SubObjects[0].Size, sourceSize, meta.SubObjects[0].Name) + return 0, errors.Errorf("unexpected size (%v vs. %v) for bdev object %s", meta.SubObjects[0].Size, sourceSize, meta.SubObjects[0].Name) } if sourceSize > dest.size { - return 0, errors.Wrapf(err, "dest dev(%s) size is too small (%v vs. %v)", dest.path, dest.size, sourceSize) + return 0, errors.Errorf("dest dev(%s) size is too small (%v vs. %v)", dest.path, dest.size, sourceSize) } reader, err := blkup.repoWriter.OpenObject(blkup.ctx, meta.SubObjects[0].ID) @@ -616,7 +616,7 @@ func flushZeroBlocks(dest *os.File, start int64, length int64, zeroBlock []byte, } if writeSize != n { - return errors.Wrapf(err, "short write zero buffer at %v, length %v", start+written, writeSize) + return errors.Errorf("short write zero buffer at %v, length %v", start+written, writeSize) } written += int64(writeSize) diff --git a/pkg/uploader/block/uploader_test.go b/pkg/uploader/block/uploader_test.go index 1765eb045..3b8930476 100644 --- a/pkg/uploader/block/uploader_test.go +++ b/pkg/uploader/block/uploader_test.go @@ -689,4 +689,52 @@ func TestBlockUploaderRestore(t *testing.T) { require.NoError(t, err) assert.Equal(t, int64(1048576), written) }) + + t.Run("source size tag larger than object size", func(t *testing.T) { + ctx := context.Background() + repoWriter := udmrepomocks.NewBackupRepo(t) + blkup := NewUploader(ctx, repoWriter, nil, logrus.New()) + + meta := &udmrepo.Metadata{ + SubObjects: []udmrepo.ObjectMetadata{ + {ID: "data-id", Name: "bdev", Size: 1048576}, + }, + } + repoWriter.On("ReadMetadata", mock.Anything, udmrepo.ID("root-id")).Return(meta, nil) + + snap := udmrepo.Snapshot{ + RootObject: udmrepo.ObjectMetadata{ID: "root-id"}, + Tags: map[string]string{bdevSourceSizeTag: "2097152"}, + } + dest := destInfo{size: 4194304, path: "/dev/target"} + iterMock := cbtmocks.NewIterator(t) + + _, err := blkup.Restore(snap, dest, iterMock, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "unexpected size (1048576 vs. 2097152) for bdev object bdev") + }) + + t.Run("destination smaller than source size", func(t *testing.T) { + ctx := context.Background() + repoWriter := udmrepomocks.NewBackupRepo(t) + blkup := NewUploader(ctx, repoWriter, nil, logrus.New()) + + meta := &udmrepo.Metadata{ + SubObjects: []udmrepo.ObjectMetadata{ + {ID: "data-id", Name: "bdev", Size: 1048576}, + }, + } + repoWriter.On("ReadMetadata", mock.Anything, udmrepo.ID("root-id")).Return(meta, nil) + + snap := udmrepo.Snapshot{ + RootObject: udmrepo.ObjectMetadata{ID: "root-id"}, + Tags: map[string]string{bdevSourceSizeTag: "1048576"}, + } + dest := destInfo{size: 512, path: "/dev/small"} + iterMock := cbtmocks.NewIterator(t) + + _, err := blkup.Restore(snap, dest, iterMock, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "dest dev(/dev/small) size is too small") + }) } From 46f5adb7a37f9096835c65b27b1814dc67422f7b Mon Sep 17 00:00:00 2001 From: Jay2006sawant Date: Mon, 3 Aug 2026 11:26:00 +0530 Subject: [PATCH 2/3] fix(provider): return immediately when BatchForget flush fails Signed-off-by: Jay2006sawant --- pkg/repository/provider/unified_repo.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/repository/provider/unified_repo.go b/pkg/repository/provider/unified_repo.go index 664750c77..b30e4618b 100644 --- a/pkg/repository/provider/unified_repo.go +++ b/pkg/repository/provider/unified_repo.go @@ -384,7 +384,7 @@ func (urp *unifiedRepoProvider) BatchForget(ctx context.Context, snapshotIDs []s err = bkRepo.Flush(ctx) if err != nil { - errs = append(errs, errors.Wrap(err, "error to flush repo")) + return append(errs, errors.Wrap(err, "error to flush repo")) } log.Debug("Forget snapshot complete") From b2dea8d169f55413bc5553d13b677271fa55d27b Mon Sep 17 00:00:00 2001 From: Jay2006sawant Date: Tue, 4 Aug 2026 14:49:08 +0530 Subject: [PATCH 3/3] chore: add one-line changelog for PR 10138 Signed-off-by: Jay2006sawant --- changelogs/unreleased/10138-Jay2006sawant | 1 + changelogs/unreleased/fix-error-handling-Jay2006sawant | 9 --------- 2 files changed, 1 insertion(+), 9 deletions(-) create mode 100644 changelogs/unreleased/10138-Jay2006sawant delete mode 100644 changelogs/unreleased/fix-error-handling-Jay2006sawant diff --git a/changelogs/unreleased/10138-Jay2006sawant b/changelogs/unreleased/10138-Jay2006sawant new file mode 100644 index 000000000..cc5339217 --- /dev/null +++ b/changelogs/unreleased/10138-Jay2006sawant @@ -0,0 +1 @@ +Fix block uploader restore validation and BatchForget error handling diff --git a/changelogs/unreleased/fix-error-handling-Jay2006sawant b/changelogs/unreleased/fix-error-handling-Jay2006sawant deleted file mode 100644 index 82a05f6c3..000000000 --- a/changelogs/unreleased/fix-error-handling-Jay2006sawant +++ /dev/null @@ -1,9 +0,0 @@ -fix: return errors correctly in block restore validation and BatchForget - -Block uploader Restore used errors.Wrapf with a stale nil err after -successful getSourceSize, causing size validation failures to return -(0, nil). flushZeroBlocks had the same pattern on short writes. - -BatchForget dropped delete errors when flush also failed. - -Signed-off-by: Jay2006sawant