prune: Stricter error checks by MichaelEischer · Pull Request #2674 · restic/restic

MichaelEischer · 2020-03-31T17:43:38Z

What is the purpose of this change? What does it change?

This PR tightens the sanity checks made by prune in the following three regards:

Prune now exactly checks that every single used blob can be found and no longer just approximately checks this condition. This is mostly an overcautious cleanup, but might help to prevent data loss if the backend temporarily returns broken packs.
Prune aborts repacking when it finds a different blob in a pack than expected. This usually indicates that the backup has been tampered with or hardware problems on the computer running restic.
Prune and rebuild-index fail now if an old index file cannot be removed. For rebuild-index this is problematic as that command is usually used to repair a broken index and shouldn't leave possibly invalid index files. For prune this behavior could lead to data loss, as prune would continue and remove old packs which might still be referenced by one of the old index files that were not deleted. A later backup run would the not re-upload any blobs that are listed in these index, but which might actually be missing from the repository. This could result in a snapshot that's missing data.

Most of the code in this PR implements tests and adds support for the integration tests to wrap a repository to simplify testing repository damages.

Was the change discussed in an issue or in the forum before?

This PRs attempts to solve possible problems in the prune implementation in the hope of possibly fixing some of the reported repository damages. The changes themselves were not discussed before.

Checklist

I have read the Contribution Guidelines
I have enabled maintainer edits for this PR
I have added tests for all changes in this PR
~~[ ] I have added documentation for the changes (in the manual)~~
There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
I have run gofmt on the code in all commits
All commit messages are formatted in the same style as the other commits in the repo
I'm done, this Pull Request is ready for review

cmd/restic/cmd_prune.go

rawtaz · 2020-07-17T22:57:43Z

@aawsome and @greatroar could you guys review this to make sure it looks sane and shouldn't carry any unexpected results?

aawsome

Most of the changes are due to using the restic.Repository interface in all places and added checks.

IMO this PR doesn't fully solve the index issue and #2839 should be considered instead - but merging this PR will improve the current situation.

Also this PR needs a rebase and modifications after #2773 has been merged (e.g. new SaveBlob syntax)

cmd/restic/cmd_prune.go

cmd/restic/cmd_rebuild_index.go

cmd/restic/integration_test.go

internal/repository/repack.go

cmd/restic/integration_test.go

aawsome

LGTM

The previous check only approximately verified whether all required blobs were found. However, after forgetting a few snapshots the repository contains lots of unused blobs whose number can be sufficient to make up for missing packs. When coupled with a malfunctioning backend that temporarily returns broken data this could cause restic to regard the corresponding packs as invalid and thereby delete data that's still in use. This change lets restic play it safe and refuse to delete anything if data is missing.

The old behavior was problematic in the context of rebuild-index as it could leave old, possibly invalid index files behind without returning a fatal error. Prune calls rebuildIndex before removing any data from the repository. For this use case failing to delete an old index MUST be treated as a fatal error. Otherwise the index could still contain an old index file that refers to blobs/packs that were later on deleted by prune. Later backup runs will assume that the affected blobs already exist in the repository which results in a backup which misses data.

If a blob in a pack file can be decrypted successfully but contains data that results in a different hash than stated in the header pack, then abort repacking. As both the pack header and the blob are cryptographically verified this either means than a malicious entity tampered with the backup or indicates hardware problems on the client. prune should fail with an error in both cases.

The slicing operator `slice[low:high]` default to 0 for the lower bound and len(slice) for the upper bound when either or both are not specified. Fix the code to use `cap(slice)` to check for the slice capacity.

fd0

Awesome PR, thanks for taking the time to split this into small chunks!

MichaelEischer force-pushed the prune-strict-checks branch from 54ca880 to b45fb53 Compare March 31, 2020 18:51

rawtaz reviewed Mar 31, 2020

View reviewed changes

cmd/restic/cmd_prune.go Outdated Show resolved Hide resolved

MichaelEischer force-pushed the prune-strict-checks branch from 4a36770 to b7c4a99 Compare March 31, 2020 21:32

rawtaz added the state: need review label Apr 1, 2020

MichaelEischer mentioned this pull request Apr 5, 2020

prune: Better handling of eventually consistent backend #2685

Closed

7 tasks

MichaelEischer mentioned this pull request Apr 21, 2020

Prune issues: add new commands 'cleanup-index', 'cleanup-packs' and 'repack-index' #2513

Closed

7 tasks

aawsome reviewed Jul 18, 2020

View reviewed changes

cmd/restic/cmd_prune.go Show resolved Hide resolved

cmd/restic/cmd_rebuild_index.go Outdated Show resolved Hide resolved

cmd/restic/integration_test.go Outdated Show resolved Hide resolved

internal/repository/repack.go Show resolved Hide resolved

MichaelEischer mentioned this pull request Jul 18, 2020

Reimplementation of prune #2718

Merged

11 tasks

aawsome mentioned this pull request Jul 19, 2020

Add prune integration tests for many edge cases #2844

Merged

7 tasks

greatroar reviewed Jul 20, 2020

View reviewed changes

cmd/restic/integration_test.go Outdated Show resolved Hide resolved

greatroar reviewed Jul 20, 2020

View reviewed changes

cmd/restic/integration_test.go Outdated Show resolved Hide resolved

greatroar reviewed Jul 20, 2020

View reviewed changes

cmd/restic/integration_test.go Outdated Show resolved Hide resolved

MichaelEischer force-pushed the prune-strict-checks branch 3 times, most recently from 66a4bee to 4c534e8 Compare July 25, 2020 17:54

aawsome approved these changes Jul 26, 2020

View reviewed changes

MichaelEischer force-pushed the prune-strict-checks branch 2 times, most recently from fe01136 to 60248d4 Compare August 2, 2020 08:36

MichaelEischer force-pushed the prune-strict-checks branch from 60248d4 to 327b8b1 Compare August 15, 2020 15:31

MichaelEischer added 10 commits August 16, 2020 11:34

prune: Reduce memory allocations while repacking

367449d

The slicing operator `slice[low:high]` default to 0 for the lower bound and len(slice) for the upper bound when either or both are not specified. Fix the code to use `cap(slice)` to check for the slice capacity.

prune: Add test that repack aborts on wrong blob

f4b9544

Add support for integration tests to wrap the backend

8f81164

Test that rebuild-index errors when old index cannot be removed

575ed9a

prune: Test for abort on damaged repositories

1c84ace

Add changelog

d8b80e9

prune: Include ID of all missing blobs in error message

08d24ff

MichaelEischer force-pushed the prune-strict-checks branch from 327b8b1 to 08d24ff Compare August 16, 2020 17:30

fd0 approved these changes Aug 23, 2020

View reviewed changes

fd0 merged commit b1b3f1e into restic:master Aug 23, 2020

MichaelEischer deleted the prune-strict-checks branch August 23, 2020 20:03

MichaelEischer mentioned this pull request Sep 10, 2020

Fatal: number of used blobs is larger than number of available blobs #2663

Closed

MichaelEischer mentioned this pull request Oct 1, 2020

Treat superseded indexes correctly #2839

Closed

8 tasks

MichaelEischer mentioned this pull request Jan 9, 2021

Tree xxxxxx not found in repository and abort #1798

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prune: Stricter error checks#2674

prune: Stricter error checks#2674
fd0 merged 10 commits intorestic:masterfrom
MichaelEischer:prune-strict-checks

MichaelEischer commented Mar 31, 2020 •

edited

Loading

Uh oh!

Uh oh!

rawtaz commented Jul 17, 2020

Uh oh!

aawsome left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aawsome left a comment

Uh oh!

fd0 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

MichaelEischer commented Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of this change? What does it change?

Was the change discussed in an issue or in the forum before?

Checklist

Uh oh!

Uh oh!

rawtaz commented Jul 17, 2020

Uh oh!

aawsome left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aawsome left a comment

Choose a reason for hiding this comment

Uh oh!

fd0 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MichaelEischer commented Mar 31, 2020 •

edited

Loading

aawsome left a comment •

edited

Loading