diff --git a/pkg/uploader/block/snapshot.go b/pkg/uploader/block/snapshot.go index 9abdff3c1..d75e94f3f 100644 --- a/pkg/uploader/block/snapshot.go +++ b/pkg/uploader/block/snapshot.go @@ -169,52 +169,49 @@ func getParentBackupInfo(ctx context.Context, rep udmrepo.BackupRepo, forceFull } var previous *udmrepo.Snapshot + if parentSnapshot != "" { + log.Infof("Loading provided parent snapshot %s", parentSnapshot) - if !forceFull { - if parentSnapshot != "" { - snap, err := rep.GetSnapshot(ctx, udmrepo.ID(parentSnapshot)) - if err != nil { - log.WithError(err).Warn("Failed to load previous snapshot, fallback to full backup") - } else { - previous = &snap - log.Infof("Using provided parent snapshot %s", parentSnapshot) - } - } else { - log.Infof("Searching for parent snapshot") - - snap, err := findPreviousSnapshot(ctx, rep, realSource, snapshotTags, nil, log) - if err != nil { - log.WithError(err).Warn("Failed to search previous snapshot, fallback to full backup") - } else { - previous = &snap - log.Infof("Using previous snapshot %s", snap.ID) - } + snap, err := rep.GetSnapshot(ctx, udmrepo.ID(parentSnapshot)) + if err != nil { + return parentBackupInfo{}, errors.Wrapf(err, "error loading previous snapshot") } + + previous = &snap } else { - log.Info("Forcing full snapshot") - } + log.Infof("Searching for parent snapshot") - parentInfo := parentBackupInfo{} - if previous != nil { - if previous.Tags == nil { - log.Warnf("No tag from parent snapshot %s, fallback to full backup", previous.ID) - } else if previous.Tags[uploader.CBTChangeIDTag] == "" { - log.Warnf("No ChangeID tag from parent snapshot %s, fallback to full backup", previous.ID) - } else if previous.Tags[uploader.CBTVolumeIDTag] == "" { - log.Warnf("No VolumeID tag from parent snapshot %s, fallback to full backup", previous.ID) - } else if previous.Tags[uploader.CBTVolumeIDTag] != volumeID { - log.Warnf("VolumeID %s from parent snapshot %s is not expected as %s, fallback to full backup", previous.Tags[uploader.CBTVolumeIDTag], previous.ID, volumeID) - } else if obj, err := loadObjectFromSnapshot(ctx, rep, previous); err != nil { - log.WithError(err).Warnf("Failed to load object from parent snapshot %s, fallback to full backup", previous.ID) - } else { - parentInfo.parentObject = obj - parentInfo.changeID = previous.Tags[uploader.CBTChangeIDTag] - parentInfo.volumeID = previous.Tags[uploader.CBTVolumeIDTag] - - log.Infof("Using parent snapshot %s, start time %v, end time %v, description %s", previous.ID, previous.StartTime, previous.EndTime, previous.Description) + snap, err := findPreviousSnapshot(ctx, rep, realSource, snapshotTags, nil, log) + if err != nil { + return parentBackupInfo{}, errors.Wrapf(err, "error searching previous snapshot") } + + previous = &snap } + if previous.Tags == nil { + return parentBackupInfo{}, errors.Errorf("no tag from parent snapshot %s", previous.ID) + } + + if previous.Tags[uploader.CBTChangeIDTag] == "" { + return parentBackupInfo{}, errors.Errorf("no ChangeID tag from parent snapshot %s", previous.ID) + } + + if previous.Tags[uploader.CBTVolumeIDTag] == "" { + return parentBackupInfo{}, errors.Errorf("no VolumeID tag from parent snapshot %s", previous.ID) + } + + if previous.Tags[uploader.CBTVolumeIDTag] != volumeID { + return parentBackupInfo{}, errors.Errorf("VolumeID %s from parent snapshot %s is not expected as %s", previous.Tags[uploader.CBTVolumeIDTag], previous.ID, volumeID) + } + + obj, err := loadObjectFromSnapshot(ctx, rep, previous) + if err != nil { + return parentBackupInfo{}, errors.Wrapf(err, "error loading object from parent snapshot %s", previous.ID) + } + + log.Infof("Using parent snapshot %s, start time %v, end time %v, description %s", previous.ID, previous.StartTime, previous.EndTime, previous.Description) + return parentBackupInfo{ parentObject: obj, changeID: previous.Tags[uploader.CBTChangeIDTag], diff --git a/pkg/uploader/block/snapshot_test.go b/pkg/uploader/block/snapshot_test.go index 260f94b82..fb5eaf4f7 100644 --- a/pkg/uploader/block/snapshot_test.go +++ b/pkg/uploader/block/snapshot_test.go @@ -379,11 +379,12 @@ func TestGetParentBackupInfoLogsDiscoveredParentID(t *testing.T) { SubObjects: []udmrepo.ObjectMetadata{{ID: udmrepo.ID("parent-obj")}}, }, nil) - info := getParentBackupInfo( + info, err := getParentBackupInfo( context.Background(), repo, false, "", // no explicit parent -> discovery branch volumeID, realSource, snapshotTags, logger, ) + require.NoError(t, err) require.Equal(t, udmrepo.ID("parent-obj"), info.parentObject) @@ -408,6 +409,7 @@ func TestGetParentBackupInfo(t *testing.T) { } validSnap := udmrepo.Snapshot{ + ID: "snap-valid", RootObject: udmrepo.ObjectMetadata{ID: "root-obj"}, Tags: map[string]string{ uploader.CBTChangeIDTag: "cid-abc", @@ -421,7 +423,10 @@ func TestGetParentBackupInfo(t *testing.T) { name string forceFull bool parentSnapshot string + emptyVolID bool setupMocks func(repo *udmrepomocks.BackupRepo) + expectErr bool + expectedErrStr string expectEmpty bool expectedParent udmrepo.ID expectedCID string @@ -432,6 +437,13 @@ func TestGetParentBackupInfo(t *testing.T) { forceFull: true, expectEmpty: true, }, + { + name: "volumeID not provided", + emptyVolID: true, + expectEmpty: true, + expectErr: true, + expectedErrStr: "volumeID is not provided from the volume snapshot", + }, { name: "GetSnapshot fails — falls back to full", parentSnapshot: "snap-parent", @@ -439,46 +451,69 @@ func TestGetParentBackupInfo(t *testing.T) { repo.On("GetSnapshot", mock.Anything, udmrepo.ID("snap-parent")). Return(udmrepo.Snapshot{}, errors.New("not found")) }, - expectEmpty: true, + expectEmpty: true, + expectErr: true, + expectedErrStr: "error loading previous snapshot", }, { name: "parent snapshot has nil tags — falls back to full", parentSnapshot: "snap-notags", setupMocks: func(repo *udmrepomocks.BackupRepo) { repo.On("GetSnapshot", mock.Anything, udmrepo.ID("snap-notags")). - Return(udmrepo.Snapshot{Tags: nil}, nil) + Return(udmrepo.Snapshot{ID: "snap-notags", Tags: nil}, nil) }, - expectEmpty: true, + expectEmpty: true, + expectErr: true, + expectedErrStr: "no tag from parent snapshot snap-notags", }, { name: "parent snapshot missing ChangeID tag — falls back to full", parentSnapshot: "snap-nocid", setupMocks: func(repo *udmrepomocks.BackupRepo) { repo.On("GetSnapshot", mock.Anything, udmrepo.ID("snap-nocid")). - Return(udmrepo.Snapshot{Tags: map[string]string{uploader.CBTVolumeIDTag: volumeID}}, nil) + Return(udmrepo.Snapshot{ID: "snap-nocid", Tags: map[string]string{uploader.CBTVolumeIDTag: volumeID}}, nil) }, - expectEmpty: true, + expectEmpty: true, + expectErr: true, + expectedErrStr: "no ChangeID tag from parent snapshot snap-nocid", }, { name: "parent snapshot missing VolumeID tag — falls back to full", parentSnapshot: "snap-novid", setupMocks: func(repo *udmrepomocks.BackupRepo) { repo.On("GetSnapshot", mock.Anything, udmrepo.ID("snap-novid")). - Return(udmrepo.Snapshot{Tags: map[string]string{uploader.CBTChangeIDTag: "cid"}}, nil) + Return(udmrepo.Snapshot{ID: "snap-novid", Tags: map[string]string{uploader.CBTChangeIDTag: "cid"}}, nil) }, - expectEmpty: true, + expectEmpty: true, + expectErr: true, + expectedErrStr: "no VolumeID tag from parent snapshot snap-novid", }, { name: "parent snapshot VolumeID mismatch — falls back to full", parentSnapshot: "snap-vidmismatch", setupMocks: func(repo *udmrepomocks.BackupRepo) { repo.On("GetSnapshot", mock.Anything, udmrepo.ID("snap-vidmismatch")). - Return(udmrepo.Snapshot{Tags: map[string]string{ + Return(udmrepo.Snapshot{ID: "snap-vidmismatch", Tags: map[string]string{ uploader.CBTChangeIDTag: "cid", uploader.CBTVolumeIDTag: "different-vol", }}, nil) }, - expectEmpty: true, + expectEmpty: true, + expectErr: true, + expectedErrStr: "VolumeID different-vol from parent snapshot snap-vidmismatch is not expected as vol-123", + }, + { + name: "loadObjectFromSnapshot fails — falls back to full", + parentSnapshot: "snap-valid", + setupMocks: func(repo *udmrepomocks.BackupRepo) { + repo.On("GetSnapshot", mock.Anything, udmrepo.ID("snap-valid")). + Return(validSnap, nil) + repo.On("ReadMetadata", mock.Anything, udmrepo.ID("root-obj")). + Return(nil, errors.New("read error")) + }, + expectEmpty: true, + expectErr: true, + expectedErrStr: "error loading object from parent snapshot snap-valid", }, { name: "valid parent snapshot — returns parent info", @@ -499,7 +534,9 @@ func TestGetParentBackupInfo(t *testing.T) { repo.On("ListSnapshot", mock.Anything, realSource). Return(nil, errors.New("list error")) }, - expectEmpty: true, + expectEmpty: true, + expectErr: true, + expectedErrStr: "error searching previous snapshot", }, { name: "no parentSnapshot — no matching snapshot — falls back to full", @@ -507,7 +544,9 @@ func TestGetParentBackupInfo(t *testing.T) { repo.On("ListSnapshot", mock.Anything, realSource). Return([]udmrepo.Snapshot{{Tags: map[string]string{"other": "tag"}}}, nil) }, - expectEmpty: true, + expectEmpty: true, + expectErr: true, + expectedErrStr: "error searching previous snapshot", }, { name: "no parentSnapshot — matching snapshot found — returns parent info", @@ -532,7 +571,21 @@ func TestGetParentBackupInfo(t *testing.T) { tc.setupMocks(mockRepo) } - info := getParentBackupInfo(ctx, mockRepo, tc.forceFull, tc.parentSnapshot, volumeID, realSource, snapshotTags, testLog()) + volID := volumeID + if tc.emptyVolID { + volID = "" + } + + info, err := getParentBackupInfo(ctx, mockRepo, tc.forceFull, tc.parentSnapshot, volID, realSource, snapshotTags, testLog()) + + if tc.expectErr { + require.Error(t, err) + if tc.expectedErrStr != "" { + assert.Contains(t, err.Error(), tc.expectedErrStr) + } + } else { + require.NoError(t, err) + } if tc.expectEmpty { assert.Empty(t, info.parentObject) @@ -547,6 +600,101 @@ func TestGetParentBackupInfo(t *testing.T) { } } +func TestGetBackupInfo(t *testing.T) { + const volumeID = "vol-123" + + validSnap := udmrepo.Snapshot{ + ID: "snap-valid", + Tags: map[string]string{ + uploader.CBTChangeIDTag: "cid-abc", + uploader.CBTVolumeIDTag: volumeID, + }, + } + + testCases := []struct { + name string + snapshot udmrepo.Snapshot + volumeID string + expectErr bool + expectedErrStr string + expectedCID string + }{ + { + name: "nil tags", + snapshot: udmrepo.Snapshot{ID: "snap-nil-tags"}, + volumeID: volumeID, + expectErr: true, + expectedErrStr: "no tag from snapshot snap-nil-tags", + }, + { + name: "missing ChangeID tag", + snapshot: udmrepo.Snapshot{ + ID: "snap-no-cid", + Tags: map[string]string{uploader.CBTVolumeIDTag: volumeID}, + }, + volumeID: volumeID, + expectErr: true, + expectedErrStr: "no ChangeID tag from snapshot snap-no-cid", + }, + { + name: "missing VolumeID tag", + snapshot: udmrepo.Snapshot{ + ID: "snap-no-vid", + Tags: map[string]string{uploader.CBTChangeIDTag: "cid-abc"}, + }, + volumeID: volumeID, + expectErr: true, + expectedErrStr: "no VolumeID tag from snapshot snap-no-vid", + }, + { + name: "empty volumeID parameter", + snapshot: udmrepo.Snapshot{ + ID: "snap-valid", + Tags: map[string]string{ + uploader.CBTChangeIDTag: "cid-abc", + uploader.CBTVolumeIDTag: volumeID, + }, + }, + volumeID: "", + expectErr: true, + expectedErrStr: "no VolumeID tag from the volume snapshot", + }, + { + name: "volumeID mismatch", + snapshot: udmrepo.Snapshot{ + ID: "snap-vid-mismatch", + Tags: map[string]string{ + uploader.CBTChangeIDTag: "cid-abc", + uploader.CBTVolumeIDTag: "other-vol", + }, + }, + volumeID: volumeID, + expectErr: true, + expectedErrStr: "volumeID other-vol from snapshot snap-vid-mismatch is not expected as vol-123", + }, + { + name: "valid snapshot", + snapshot: validSnap, + volumeID: volumeID, + expectErr: false, + expectedCID: "cid-abc", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + info, err := getBackupInfo(tc.snapshot, tc.volumeID) + if tc.expectErr { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.expectedErrStr) + } else { + require.NoError(t, err) + assert.Equal(t, tc.expectedCID, info.changeID) + } + }) + } +} + func TestFindPreviousSnapshot(t *testing.T) { snapshotTags := map[string]string{ uploader.SnapshotRequesterTag: "test-requester", diff --git a/pkg/uploader/block/uploader_test.go b/pkg/uploader/block/uploader_test.go index f011219e4..b3621ed33 100644 --- a/pkg/uploader/block/uploader_test.go +++ b/pkg/uploader/block/uploader_test.go @@ -295,8 +295,7 @@ func TestBlockUploaderBackup(t *testing.T) { var iterator cbt.Iterator if !tc.nilBitmap { iterMock := cbtmocks.NewIterator(t) - iterMock.On("Error").Return(nil).Maybe() - iterMock.On("Error").Return(nil).Maybe() + iterMock.On("Errors").Return(nil).Maybe() iterator = iterMock backupMode := udmrepo.ObjectDataBackupModeInc @@ -575,7 +574,7 @@ func TestRestoreData(t *testing.T) { reader := bytes.NewReader(data) iterMock := cbtmocks.NewIterator(t) - iterMock.On("Error").Return(nil).Maybe() + iterMock.On("Errors").Return(nil).Maybe() iterMock.On("Count").Return(uint64(1)) iterMock.On("Next").Return(uint64(0), true).Once() iterMock.On("Next").Return(uint64(0), false) @@ -606,7 +605,7 @@ func TestRestoreData(t *testing.T) { reader := &errReader{err: errors.New("read error")} iterMock := cbtmocks.NewIterator(t) - iterMock.On("Error").Return(nil).Maybe() + iterMock.On("Errors").Return(nil).Maybe() iterMock.On("Count").Return(uint64(1)) iterMock.On("Next").Return(uint64(0), true).Once() iterMock.On("Next").Return(uint64(0), false) @@ -627,7 +626,7 @@ func TestBlockUploaderRestore(t *testing.T) { repoWriter.On("ReadMetadata", mock.Anything, udmrepo.ID("root-id")).Return(nil, errors.New("meta not found")) iterMock := cbtmocks.NewIterator(t) - iterMock.On("Error").Return(nil).Maybe() + iterMock.On("Errors").Return(nil).Maybe() _, _, err := blkup.Restore(udmrepo.Snapshot{RootObject: udmrepo.ObjectMetadata{ID: "root-id"}}, destInfo{}, iterMock, nil) require.Error(t, err) assert.Contains(t, err.Error(), "meta not found") @@ -685,7 +684,7 @@ func TestBlockUploaderRestore(t *testing.T) { } iterMock := cbtmocks.NewIterator(t) - iterMock.On("Error").Return(nil).Maybe() + iterMock.On("Errors").Return(nil).Maybe() iterMock.On("Count").Return(uint64(1)) iterMock.On("Next").Return(uint64(0), true).Once() iterMock.On("Next").Return(uint64(0), false) @@ -714,7 +713,7 @@ func TestBlockUploaderRestore(t *testing.T) { } dest := destInfo{size: 4194304, path: "/dev/target"} iterMock := cbtmocks.NewIterator(t) - iterMock.On("Error").Return(nil).Maybe() + iterMock.On("Errors").Return(nil).Maybe() _, _, err := blkup.Restore(snap, dest, iterMock, nil) require.Error(t, err) @@ -739,7 +738,7 @@ func TestBlockUploaderRestore(t *testing.T) { } dest := destInfo{size: 512, path: "/dev/small"} iterMock := cbtmocks.NewIterator(t) - iterMock.On("Error").Return(nil).Maybe() + iterMock.On("Errors").Return(nil).Maybe() _, _, err := blkup.Restore(snap, dest, iterMock, nil) require.Error(t, err) diff --git a/pkg/uploader/cbt/bitmap.go b/pkg/uploader/cbt/bitmap.go index 29da0c190..d861ea6da 100644 --- a/pkg/uploader/cbt/bitmap.go +++ b/pkg/uploader/cbt/bitmap.go @@ -94,6 +94,10 @@ func (c *bitmapImpl) VolumeID() string { } func (c *bitmapImpl) SetError(err error) { + if err == nil { + return + } + c.cbtErrors = append(c.cbtErrors, err) } diff --git a/pkg/uploader/cbt/bitmap_test.go b/pkg/uploader/cbt/bitmap_test.go index 9671ad3d9..8e428f7bd 100644 --- a/pkg/uploader/cbt/bitmap_test.go +++ b/pkg/uploader/cbt/bitmap_test.go @@ -17,6 +17,7 @@ limitations under the License. package cbt import ( + "errors" "testing" "github.com/stretchr/testify/assert" @@ -24,10 +25,18 @@ import ( ) func TestBitmapProperties(t *testing.T) { - b := NewBitmap(1024*1024, 10000*1024*1024, "snap-1", "change-1", "vol-1") + b := NewBitmap(1024*1024, 10000*1024*1024, "snap-1", "vol-1") assert.Equal(t, "snap-1", b.Snapshot()) - assert.Equal(t, "change-1", b.ChangeID()) + assert.Empty(t, b.ChangeID()) assert.Equal(t, "vol-1", b.VolumeID()) + assert.Empty(t, b.Errors()) + + b.SetChangeID("change-1") + assert.Equal(t, "change-1", b.ChangeID()) + + err := errors.New("test error") + b.SetError(err) + assert.Equal(t, []error{err}, b.Errors()) } func TestBitmapSet(t *testing.T) { @@ -138,7 +147,7 @@ func TestBitmapSet(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - b := NewBitmap(tt.blockSize, tt.totalLength, "snap-1", "change-1", "vol-1") + b := NewBitmap(tt.blockSize, tt.totalLength, "snap-1", "vol-1") for _, call := range tt.setCalls { b.Set(call.offset, call.length) @@ -174,7 +183,7 @@ func TestBitmapSetFull(t *testing.T) { // block 0: 0 - 1MB // block 1: 1MB - 2MB // block 2: 2MB - 3MB - b := NewBitmap(mb, 3*mb, "snap-1", "change-1", "vol-1") + b := NewBitmap(mb, 3*mb, "snap-1", "vol-1") b.SetFull() iter := b.Iterator() @@ -199,7 +208,10 @@ func TestBitmapIterator(t *testing.T) { const mb = 1024 * 1024 const gb = 1024 * 1024 * 1024 - b := NewBitmap(mb, 10*gb, "snap-1", "change-1", "vol-1") + b := NewBitmap(mb, 10*gb, "snap-1", "vol-1") + b.SetChangeID("change-1") + err := errors.New("test error") + b.SetError(err) // Set multiple ranges to test contiguous iteration b.Set(mb, 100) // Block 1 @@ -214,6 +226,7 @@ func TestBitmapIterator(t *testing.T) { assert.Equal(t, "change-1", iter.ChangeID()) assert.Equal(t, "vol-1", iter.VolumeID()) assert.Equal(t, uint(mb), iter.BlockSize()) + assert.Equal(t, []error{err}, iter.Errors()) assert.Equal(t, uint64(7), iter.Count()) // 1 + 5 + 1 = 7 blocks expectedOffsets := []uint64{ diff --git a/pkg/uploader/cbt/set_test.go b/pkg/uploader/cbt/set_test.go index 6c1d4a579..e9a6d1a52 100644 --- a/pkg/uploader/cbt/set_test.go +++ b/pkg/uploader/cbt/set_test.go @@ -170,7 +170,8 @@ func TestSetBitmapOrFull(t *testing.T) { svc = svcMock } - bmp := NewBitmap(mb, 3*mb, tt.snapshotID, tt.changeID, "vol-1") + bmp := NewBitmap(mb, 3*mb, tt.snapshotID, "vol-1") + bmp.SetChangeID(tt.changeID) err := SetBitmapOrFull(context.Background(), svc, bmp, tt.incOnly) diff --git a/pkg/uploader/cbt/types/mocks/Bitmap.go b/pkg/uploader/cbt/types/mocks/Bitmap.go index 514e5c28f..3ed29917b 100644 --- a/pkg/uploader/cbt/types/mocks/Bitmap.go +++ b/pkg/uploader/cbt/types/mocks/Bitmap.go @@ -292,23 +292,30 @@ func (_c *Bitmap_VolumeID_Call) RunAndReturn(run func() string) *Bitmap_VolumeID _c.Call.Return(run) return _c } +// SetChangeID provides a mock function for the type Bitmap +func (_mock *Bitmap) SetChangeID(id string) { + _mock.Called(id) +} + // SetError provides a mock function for the type Bitmap func (_mock *Bitmap) SetError(err error) { _mock.Called(err) } -// Error provides a mock function for the type Bitmap -func (_mock *Bitmap) Error() error { +// Errors provides a mock function for the type Bitmap +func (_mock *Bitmap) Errors() []error { ret := _mock.Called() if len(ret) == 0 { - panic("no return value specified for Error") + panic("no return value specified for Errors") } - var r0 error - if returnFunc, ok := ret.Get(0).(func() error); ok { + var r0 []error + if returnFunc, ok := ret.Get(0).(func() []error); ok { r0 = returnFunc() } else { - r0 = ret.Error(0) + if ret.Get(0) != nil { + r0 = ret.Get(0).([]error) + } } return r0 } diff --git a/pkg/uploader/cbt/types/mocks/Iterator.go b/pkg/uploader/cbt/types/mocks/Iterator.go index 00fc163d9..1ddd59f76 100644 --- a/pkg/uploader/cbt/types/mocks/Iterator.go +++ b/pkg/uploader/cbt/types/mocks/Iterator.go @@ -307,18 +307,20 @@ func (_c *Iterator_VolumeID_Call) RunAndReturn(run func() string) *Iterator_Volu _c.Call.Return(run) return _c } -// Error provides a mock function for the type Iterator -func (_mock *Iterator) Error() error { +// Errors provides a mock function for the type Iterator +func (_mock *Iterator) Errors() []error { ret := _mock.Called() if len(ret) == 0 { - panic("no return value specified for Error") + panic("no return value specified for Errors") } - var r0 error - if returnFunc, ok := ret.Get(0).(func() error); ok { + var r0 []error + if returnFunc, ok := ret.Get(0).(func() []error); ok { r0 = returnFunc() } else { - r0 = ret.Error(0) + if ret.Get(0) != nil { + r0 = ret.Get(0).([]error) + } } return r0 }