From df709d39d675df91a2334c041c4bd0087e2f5bbf Mon Sep 17 00:00:00 2001 From: Lyndon-Li Date: Fri, 4 Sep 2026 16:35:39 +0800 Subject: [PATCH] report incremental fallback message Signed-off-by: Lyndon-Li --- pkg/controller/data_download_controller.go | 6 +- pkg/controller/data_upload_controller.go | 6 +- .../pod_volume_backup_controller.go | 6 +- .../pod_volume_restore_controller.go | 6 +- pkg/uploader/cbt/set.go | 7 +- pkg/uploader/cbt/set_test.go | 206 +++++++++++------- pkg/uploader/kopia/snapshot.go | 17 +- 7 files changed, 156 insertions(+), 98 deletions(-) diff --git a/pkg/controller/data_download_controller.go b/pkg/controller/data_download_controller.go index 673b82c02..053a083e7 100644 --- a/pkg/controller/data_download_controller.go +++ b/pkg/controller/data_download_controller.go @@ -617,8 +617,10 @@ func (r *DataDownloadReconciler) OnDataDownloadProgress(ctx context.Context, nam } if progress.Message != "" { - dd.Status.Message += progress.Message - dd.Status.Message += ";" + message := progress.Message + ";" + if !strings.HasSuffix(dd.Status.Message, message) { + dd.Status.Message += message + } } return true diff --git a/pkg/controller/data_upload_controller.go b/pkg/controller/data_upload_controller.go index 8c7795765..dbbf21013 100644 --- a/pkg/controller/data_upload_controller.go +++ b/pkg/controller/data_upload_controller.go @@ -642,8 +642,10 @@ func (r *DataUploadReconciler) OnDataUploadProgress(ctx context.Context, namespa } if progress.Message != "" { - du.Status.Message += progress.Message - du.Status.Message += ";" + message := progress.Message + ";" + if !strings.HasSuffix(du.Status.Message, message) { + du.Status.Message += message + } } return true diff --git a/pkg/controller/pod_volume_backup_controller.go b/pkg/controller/pod_volume_backup_controller.go index 13d0dd851..3e343b073 100644 --- a/pkg/controller/pod_volume_backup_controller.go +++ b/pkg/controller/pod_volume_backup_controller.go @@ -636,8 +636,10 @@ func (r *PodVolumeBackupReconciler) OnDataPathProgress(ctx context.Context, name } if progress.Message != "" { - pvb.Status.Message += progress.Message - pvb.Status.Message += ";" + message := progress.Message + ";" + if !strings.HasSuffix(pvb.Status.Message, message) { + pvb.Status.Message += message + } } return true diff --git a/pkg/controller/pod_volume_restore_controller.go b/pkg/controller/pod_volume_restore_controller.go index 1ca274017..d8ffb0e1a 100644 --- a/pkg/controller/pod_volume_restore_controller.go +++ b/pkg/controller/pod_volume_restore_controller.go @@ -913,8 +913,10 @@ func (r *PodVolumeRestoreReconciler) OnDataPathProgress(ctx context.Context, nam } if progress.Message != "" { - pvr.Status.Message += progress.Message - pvr.Status.Message += ";" + message := progress.Message + ";" + if !strings.HasSuffix(pvr.Status.Message, message) { + pvr.Status.Message += message + } } return true diff --git a/pkg/uploader/cbt/set.go b/pkg/uploader/cbt/set.go index be039d0c7..0e50cb048 100644 --- a/pkg/uploader/cbt/set.go +++ b/pkg/uploader/cbt/set.go @@ -84,7 +84,12 @@ func SetBitmapOrFull(ctx context.Context, service cbtservice.Service, bitmap typ if err != nil { setFull = true - return errors.Wrap(err, "error getting allocated blocks from CBT service, fallback to real full") + + if changedErr != nil { + return errors.Wrap(err, "error getting both changed and allocated blocks from CBT service, fallback to real full") + } else { + return errors.Wrap(err, "error getting allocated blocks from CBT service, fallback to real full") + } } if changedErr != nil { diff --git a/pkg/uploader/cbt/set_test.go b/pkg/uploader/cbt/set_test.go index 8b40c1143..6c1d4a579 100644 --- a/pkg/uploader/cbt/set_test.go +++ b/pkg/uploader/cbt/set_test.go @@ -21,126 +21,148 @@ import ( "errors" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/vmware-tanzu/velero/pkg/cbtservice" cbtservicemocks "github.com/vmware-tanzu/velero/pkg/cbtservice/mocks" - cbtmocks "github.com/vmware-tanzu/velero/pkg/uploader/cbt/types/mocks" ) func TestSetBitmapOrFull(t *testing.T) { + const mb = 1024 * 1024 tests := []struct { name string nilService bool incOnly bool - setupMocks func(*cbtservicemocks.Service, *cbtmocks.Bitmap) + snapshotID string + changeID string + setupMocks func(*cbtservicemocks.Service) expectedErrStr string + expectedCount uint64 + expectedNext []uint64 }{ { - name: "nil service", - nilService: true, - setupMocks: func(svc *cbtservicemocks.Service, bmp *cbtmocks.Bitmap) { - bmp.On("SetFull").Return() - }, + name: "nil service", + nilService: true, + snapshotID: "snap-1", + changeID: "change-1", + setupMocks: func(svc *cbtservicemocks.Service) {}, expectedErrStr: "CBT service is absent, fallback to real full", + expectedCount: 3, + expectedNext: []uint64{0, mb, 2 * mb}, }, { - name: "invalid snapshot", - setupMocks: func(svc *cbtservicemocks.Service, bmp *cbtmocks.Bitmap) { - bmp.On("Snapshot").Return("") - bmp.On("SetFull").Return() - }, + name: "invalid snapshot", + snapshotID: "", + setupMocks: func(svc *cbtservicemocks.Service) {}, expectedErrStr: "invalid snapshot, fallback to real full", + expectedCount: 3, + expectedNext: []uint64{0, mb, 2 * mb}, }, { - name: "invalid changeID", - incOnly: true, - setupMocks: func(svc *cbtservicemocks.Service, bmp *cbtmocks.Bitmap) { - bmp.On("Snapshot").Return("snap-1") - bmp.On("ChangeID").Return("") - bmp.On("SetFull").Return() - }, + name: "invalid changeID", + incOnly: true, + snapshotID: "snap-1", + changeID: "", + setupMocks: func(svc *cbtservicemocks.Service) {}, expectedErrStr: "invalid changeID, fallback to real full", + expectedCount: 3, + expectedNext: []uint64{0, mb, 2 * mb}, }, { - name: "allocated blocks success", - setupMocks: func(svc *cbtservicemocks.Service, bmp *cbtmocks.Bitmap) { - bmp.On("Snapshot").Return("snap-1") - bmp.On("ChangeID").Return("") + name: "allocated blocks success", + snapshotID: "snap-1", + changeID: "", + setupMocks: func(svc *cbtservicemocks.Service) { + svc.On("GetAllocatedBlocks", mock.Anything, "snap-1", mock.Anything).Run(func(args mock.Arguments) { + record := args.Get(2).(func([]cbtservice.Range) error) + record([]cbtservice.Range{ + {Offset: 0, Length: uint64(mb)}, + {Offset: uint64(2 * mb), Length: uint64(mb)}, + }) + }).Return(nil) + }, + expectedCount: 2, + expectedNext: []uint64{0, 2 * mb}, + }, + { + name: "allocated blocks error", + snapshotID: "snap-1", + changeID: "", + setupMocks: func(svc *cbtservicemocks.Service) { + svc.On("GetAllocatedBlocks", mock.Anything, "snap-1", mock.Anything).Return(errors.New("mock alloc error")) + }, + expectedErrStr: "error getting allocated blocks from CBT service, fallback to real full: mock alloc error", + expectedCount: 3, + expectedNext: []uint64{0, mb, 2 * mb}, + }, + { + name: "changed blocks success", + snapshotID: "snap-1", + changeID: "change-1", + setupMocks: func(svc *cbtservicemocks.Service) { + svc.On("GetChangedBlocks", mock.Anything, "snap-1", "change-1", mock.Anything).Run(func(args mock.Arguments) { + record := args.Get(3).(func([]cbtservice.Range) error) + record([]cbtservice.Range{ + {Offset: uint64(mb), Length: uint64(mb)}, + }) + }).Return(nil) + }, + expectedCount: 1, + expectedNext: []uint64{mb}, + }, + { + name: "changed blocks error with incOnly", + incOnly: true, + snapshotID: "snap-1", + changeID: "change-1", + setupMocks: func(svc *cbtservicemocks.Service) { + svc.On("GetChangedBlocks", mock.Anything, "snap-1", "change-1", mock.Anything).Return(errors.New("mock changed error")) + }, + expectedErrStr: "error getting changed blocks from CBT service, fallback to real full: mock changed error", + expectedCount: 3, + expectedNext: []uint64{0, mb, 2 * mb}, + }, + { + name: "both changed blocks error and allocated blocks error", + snapshotID: "snap-1", + changeID: "change-1", + setupMocks: func(svc *cbtservicemocks.Service) { + svc.On("GetChangedBlocks", mock.Anything, "snap-1", "change-1", mock.Anything).Return(errors.New("mock changed error")) + svc.On("GetAllocatedBlocks", mock.Anything, "snap-1", mock.Anything).Return(errors.New("mock alloc error")) + }, + expectedErrStr: "error getting both changed and allocated blocks from CBT service, fallback to real full: mock alloc error", + expectedCount: 3, + expectedNext: []uint64{0, mb, 2 * mb}, + }, + { + name: "changed blocks error fallback to full", + snapshotID: "snap-1", + changeID: "change-1", + setupMocks: func(svc *cbtservicemocks.Service) { + svc.On("GetChangedBlocks", mock.Anything, "snap-1", "change-1", mock.Anything).Return(errors.New("mock changed error")) svc.On("GetAllocatedBlocks", mock.Anything, "snap-1", mock.Anything).Run(func(args mock.Arguments) { record := args.Get(2).(func([]cbtservice.Range) error) record([]cbtservice.Range{ - {Offset: 0, Length: 4096}, - {Offset: 8192, Length: 4096}, + {Offset: 0, Length: uint64(mb)}, + {Offset: uint64(2 * mb), Length: uint64(mb)}, }) }).Return(nil) - - bmp.On("Set", uint64(0), uint64(4096)).Return() - bmp.On("Set", uint64(8192), uint64(4096)).Return() - }, - }, - { - name: "allocated blocks error", - setupMocks: func(svc *cbtservicemocks.Service, bmp *cbtmocks.Bitmap) { - bmp.On("Snapshot").Return("snap-1") - bmp.On("ChangeID").Return("") - - svc.On("GetAllocatedBlocks", mock.Anything, "snap-1", mock.Anything).Return(errors.New("mock alloc error")) - bmp.On("SetFull").Return() - }, - expectedErrStr: "error getting allocated blocks from CBT service, fallback to real full: mock alloc error", - }, - { - name: "changed blocks success", - setupMocks: func(svc *cbtservicemocks.Service, bmp *cbtmocks.Bitmap) { - bmp.On("Snapshot").Return("snap-1") - bmp.On("ChangeID").Return("change-1") - - svc.On("GetChangedBlocks", mock.Anything, "snap-1", "change-1", mock.Anything).Run(func(args mock.Arguments) { - record := args.Get(3).(func([]cbtservice.Range) error) - record([]cbtservice.Range{ - {Offset: 4096, Length: 4096}, - }) - }).Return(nil) - - bmp.On("Set", uint64(4096), uint64(4096)).Return() - }, - }, - { - name: "changed blocks error with incOnly", - incOnly: true, - setupMocks: func(svc *cbtservicemocks.Service, bmp *cbtmocks.Bitmap) { - bmp.On("Snapshot").Return("snap-1") - bmp.On("ChangeID").Return("change-1") - - svc.On("GetChangedBlocks", mock.Anything, "snap-1", "change-1", mock.Anything).Return(errors.New("mock changed error")) - bmp.On("SetFull").Return() - }, - expectedErrStr: "error getting changed blocks from CBT service, fallback to real full: mock changed error", - }, - { - name: "changed blocks error fallback to full", - setupMocks: func(svc *cbtservicemocks.Service, bmp *cbtmocks.Bitmap) { - bmp.On("Snapshot").Return("snap-1") - bmp.On("ChangeID").Return("change-1") - - svc.On("GetChangedBlocks", mock.Anything, "snap-1", "change-1", mock.Anything).Return(errors.New("mock changed error")) - - svc.On("GetAllocatedBlocks", mock.Anything, "snap-1", mock.Anything).Return(nil) }, expectedErrStr: "error getting changed blocks from CBT service, fallback to full: mock changed error", + expectedCount: 2, + expectedNext: []uint64{0, 2 * mb}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { svcMock := new(cbtservicemocks.Service) - bmpMock := new(cbtmocks.Bitmap) if tt.setupMocks != nil { - tt.setupMocks(svcMock, bmpMock) + tt.setupMocks(svcMock) } var svc cbtservice.Service @@ -148,9 +170,9 @@ func TestSetBitmapOrFull(t *testing.T) { svc = svcMock } - bmpMock.On("SetError", mock.Anything).Return() + bmp := NewBitmap(mb, 3*mb, tt.snapshotID, tt.changeID, "vol-1") - err := SetBitmapOrFull(context.Background(), svc, bmpMock, tt.incOnly) + err := SetBitmapOrFull(context.Background(), svc, bmp, tt.incOnly) if tt.expectedErrStr != "" { require.Error(t, err) @@ -162,7 +184,25 @@ func TestSetBitmapOrFull(t *testing.T) { if !tt.nilService { svcMock.AssertExpectations(t) } - bmpMock.AssertExpectations(t) + + iter := bmp.Iterator() + require.NotNil(t, iter) + assert.Equal(t, tt.expectedCount, iter.Count()) + + var actualOffsets []uint64 + for { + offset, hasNext := iter.Next() + if !hasNext { + break + } + actualOffsets = append(actualOffsets, offset) + } + + if len(tt.expectedNext) > 0 { + assert.Equal(t, tt.expectedNext, actualOffsets) + } else { + assert.Empty(t, actualOffsets) + } }) } } diff --git a/pkg/uploader/kopia/snapshot.go b/pkg/uploader/kopia/snapshot.go index 682b0f314..c4c3251c9 100644 --- a/pkg/uploader/kopia/snapshot.go +++ b/pkg/uploader/kopia/snapshot.go @@ -251,9 +251,12 @@ func SnapshotSource( log.Infof("Using provided parent snapshot %s", parentSnapshot) if mani, err := loadSnapshotFunc(ctx, rep, manifest.ID(parentSnapshot)); err != nil { - msg := fmt.Sprintf("Failed to load previous snapshot %v from kopia, fallback to full backup", parentSnapshot) - log.WithError(err).Warn(msg) - updater.UpdateProgress(&uploader.Progress{BytesDone: -1, TotalBytes: -1, Message: msg}) + log.WithError(err).Warnf("Failed to load previous snapshot %v from kopia, fallback to full backup", parentSnapshot) + updater.UpdateProgress(&uploader.Progress{ + BytesDone: -1, + TotalBytes: -1, + Message: fmt.Sprintf("Failed to load previous snapshot %v, fallback to full backup. Err: %v", parentSnapshot, err), + }) } else { previous = append(previous, mani) } @@ -262,9 +265,11 @@ func SnapshotSource( if pre, err := findPreviousSnapshotManifest(ctx, rep, sourceInfo, snapshotTags, nil, log); err != nil { log.WithError(err).Warnf("Failed to find previous kopia snapshot manifests for si %v, fallback to full backup", sourceInfo) - - msg := fmt.Sprint("Failed to find previous kopia snapshot manifests, fallback to full backup") - updater.UpdateProgress(&uploader.Progress{BytesDone: -1, TotalBytes: -1, Message: msg}) + updater.UpdateProgress(&uploader.Progress{ + BytesDone: -1, + TotalBytes: -1, + Message: fmt.Sprintf("Failed to find previous snapshots, fallback to full backup. Err: %v", err), + }) } else { previous = pre }