Merge pull request #10138 from Jay2006sawant/fix/block-uploader-and-batchforget-errors

fix: return errors correctly in block restore validation and BatchForget

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")}
+		return append(errs, errors.Wrap(err, "error to flush repo"))
 	}
 
 	log.Debug("Forget snapshot complete")

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 {

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)

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")
+	})
 }