From fc506f8538ddbd2519b46f65144eba71523a53f5 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 21 Aug 2022 11:11:00 +0200 Subject: [PATCH 1/5] b2: Repeat deleting until all file versions are removed When hard deleting the latest file version on B2, this uncovers earlier versions. If an upload required retries, multiple version might exist for a file. Thus to reliably delete a file, we have to remove all versions of it. --- internal/backend/b2/b2.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/internal/backend/b2/b2.go b/internal/backend/b2/b2.go index 150d396d5..26e23627a 100644 --- a/internal/backend/b2/b2.go +++ b/internal/backend/b2/b2.go @@ -274,14 +274,22 @@ func (be *b2Backend) Remove(ctx context.Context, h restic.Handle) error { be.sem.GetToken() defer be.sem.ReleaseToken() - obj := be.bucket.Object(be.Filename(h)) - err := obj.Delete(ctx) - // consider a file as removed if b2 informs us that it does not exist - if b2.IsNotExist(err) { - return nil + // the retry backend will also repeat the remove method up to 10 times + for i := 0; i < 3; i++ { + obj := be.bucket.Object(be.Filename(h)) + err := obj.Delete(ctx) + if err == nil { + // keep deleting until we are sure that no leftover file versions exist + continue + } + // consider a file as removed if b2 informs us that it does not exist + if b2.IsNotExist(err) { + return nil + } + return errors.Wrap(err, "Delete") } - return errors.Wrap(err, "Delete") + return errors.New("failed to delete all file versions") } type semLocker struct { From de0162ea76e7201ead3bd8ddbccd8193ee4b8804 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 21 Aug 2022 11:14:53 +0200 Subject: [PATCH 2/5] backend/retry: Overwrite failed uploads instead of deleting them For backends which are able to atomically replace files, we just can overwrite the old copy, if it is necessary to retry an upload. This has the benefit of issuing one operation less and might be beneficial if a backend storage, due to bugs or similar, could mix up the order of the upload and delete calls. --- internal/backend/backend_retry.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/internal/backend/backend_retry.go b/internal/backend/backend_retry.go index 5d7a58b64..fe0d34799 100644 --- a/internal/backend/backend_retry.go +++ b/internal/backend/backend_retry.go @@ -68,10 +68,16 @@ func (be *RetryBackend) Save(ctx context.Context, h restic.Handle, rd restic.Rew return nil } - debug.Log("Save(%v) failed with error, removing file: %v", h, err) - rerr := be.Backend.Remove(ctx, h) - if rerr != nil { - debug.Log("Remove(%v) returned error: %v", h, err) + if be.Backend.HasAtomicReplace() { + debug.Log("Save(%v) failed with error: %v", h, err) + // there is no need to remove files from backends which can atomically replace files + // in fact if something goes wrong at the backend side the delete operation might delete the wrong instance of the file + } else { + debug.Log("Save(%v) failed with error, removing file: %v", h, err) + rerr := be.Backend.Remove(ctx, h) + if rerr != nil { + debug.Log("Remove(%v) returned error: %v", h, err) + } } // return original error From 623556bab6e2edc18961724d1146159d615254b8 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 21 Aug 2022 11:20:03 +0200 Subject: [PATCH 3/5] b2: Increase list size to maximum Just request as many files as possible in one call to reduce the number of network roundtrips. --- internal/backend/b2/b2.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/backend/b2/b2.go b/internal/backend/b2/b2.go index 26e23627a..19328706d 100644 --- a/internal/backend/b2/b2.go +++ b/internal/backend/b2/b2.go @@ -27,7 +27,8 @@ type b2Backend struct { sem sema.Semaphore } -const defaultListMaxItems = 1000 +// Billing happens in 1000 item granlarity, but we are more interested in reducing the number of network round trips +const defaultListMaxItems = 10 * 1000 // ensure statically that *b2Backend implements restic.Backend. var _ restic.Backend = &b2Backend{} From 694dfa026a6c71423f1e4a3ddd9ff578cd8853b2 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 21 Aug 2022 11:27:51 +0200 Subject: [PATCH 4/5] add changelog for reliable B2 deletes --- changelog/unreleased/issue-3161 | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 changelog/unreleased/issue-3161 diff --git a/changelog/unreleased/issue-3161 b/changelog/unreleased/issue-3161 new file mode 100644 index 000000000..ec18b6639 --- /dev/null +++ b/changelog/unreleased/issue-3161 @@ -0,0 +1,13 @@ +Bugfix: Reliably delete files on Backblaze B2 + +Restic used to only delete the latest version of files stored in B2. In most +cases this works well as there is only a single version. However, due to +retries while uploading it is possible for multiple file versions to be stored +at B2. This can lead to various problems if files still exist that should have +been deleted. + +We have changed the implementation to delete all of them. This doubles the +number of Class B transactions necessary to delete files. + +https://github.com/restic/restic/issues/3161 +https://github.com/restic/restic/pull/3885 From 4c90d91d4d680732a61003d7481e60c03e0422d1 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 26 Aug 2022 21:20:52 +0200 Subject: [PATCH 5/5] backend: Test that failed uploads are not removed for backends with atomic replace --- internal/backend/backend_retry_test.go | 30 ++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/internal/backend/backend_retry_test.go b/internal/backend/backend_retry_test.go index e8f4d7315..cd58b9d77 100644 --- a/internal/backend/backend_retry_test.go +++ b/internal/backend/backend_retry_test.go @@ -50,6 +50,36 @@ func TestBackendSaveRetry(t *testing.T) { } } +func TestBackendSaveRetryAtomic(t *testing.T) { + errcount := 0 + calledRemove := false + be := &mock.Backend{ + SaveFn: func(ctx context.Context, h restic.Handle, rd restic.RewindReader) error { + if errcount == 0 { + errcount++ + return errors.New("injected error") + } + return nil + }, + RemoveFn: func(ctx context.Context, h restic.Handle) error { + calledRemove = true + return nil + }, + HasAtomicReplaceFn: func() bool { return true }, + } + + retryBackend := NewRetryBackend(be, 10, nil) + + data := test.Random(23, 5*1024*1024+11241) + err := retryBackend.Save(context.TODO(), restic.Handle{}, restic.NewByteReader(data, be.Hasher())) + if err != nil { + t.Fatal(err) + } + if calledRemove { + t.Fatal("remove must not be called") + } +} + func TestBackendListRetry(t *testing.T) { const ( ID1 = "id1"