Skip to content

PR closed - wrong email: restic check - enable --read-data-* with snapshot filter#5213

Closed
wplapper wants to merge 10 commits into
restic:masterfrom
wplapper:check_snapshots
Closed

PR closed - wrong email: restic check - enable --read-data-* with snapshot filter#5213
wplapper wants to merge 10 commits into
restic:masterfrom
wplapper:check_snapshots

Conversation

@wplapper

@wplapper wplapper commented Jan 22, 2025

Copy link
Copy Markdown
Contributor

Enable restic check based on snapshots. The standard snapshot filtering routine "FindFilteredSnapshots" is used for snapshots selection. However if no snapshots are specified, the subcommand behaves as before.

What does this PR change? What problem does it solve?

The PR solves issue 3326.

Add standard snapshot filtering to subcommand.
Add code to cmd/restic/cmd_check.go to detect snapshot filtering
Add code to internal/checker/checker.go to collect data blobs based on tree visitor. The functions CountPacks() and GetPacks() have been modified, so that snapshot filtering creates a subset of packfiles on which all size calculations will be based.

When snapshot filtering is activated, the complete tree for this snapshot is read and the data blobs - and their packfiles - are extracted.

Was the change previously discussed in an issue or on the forum?

#3326

Closes #3326

Checklist

  • I have added tests for all code changes.
  • I have added documentation for relevant changes (in the manual).
  • There's a new file in changelog/unreleased/ that describes the changes for our users (see template).
  • I'm done! This pull request is ready for review.

@wplapper wplapper marked this pull request as draft February 2, 2025 10:25
Comment thread cmd/restic/cmd_check.go Outdated
Comment thread cmd/restic/cmd_check.go Outdated
Comment thread internal/checker/checker.go Outdated
Comment thread internal/checker/checker.go Outdated
Comment thread internal/checker/checker.go Outdated
Comment thread internal/checker/checker.go Outdated
Comment thread internal/checker/checker.go Outdated
Comment thread internal/checker/checker.go Outdated
@wplapper wplapper marked this pull request as ready for review February 4, 2025 21:21
@wplapper

wplapper commented Feb 4, 2025

Copy link
Copy Markdown
Contributor Author

Sorry, my git history is messed up again. I don't know what I am doing wrong nor do I know of how to rectify it.

@wplapper

wplapper commented Feb 5, 2025

Copy link
Copy Markdown
Contributor Author

.

@MichaelEischer

Copy link
Copy Markdown
Member

Fixing the branch was actually a bit of challenge this time. I had to drop all the manual merge commits of your other PRs. To sync your local state with the updated content of check_snapshots either clone the repository again or use the following commands. (Assumes that the git remote origin points to your fork of restic).

git fetch origin
git checkout check_snapshots
# Warning: this will delete all local changes. If necessary create a backup copy first!!!
git reset --hard origin/check_snapshots

The basic steps for rebasing your branch should be:

  • sync master branch of fork with that of restic. The easiest way is probably to use the Sync fork button on https://github.com/wplapper/restic/ .
  • then use the following commands:
git checkout master
git pull
# if `git pull` results in the creation of a merge commit or an error then reset your local branch as follows
# warning this deletes any local changes compared to the master branch in your fork
# `git reset --hard origin/master`
git checkout check_snapshots
# `-i` shows what will be done
git rebase -i master
  • If a problem occurs then git status is usually very helpful. If the explanation there isn't sufficient for you, you could feed it to ChatGPT, which should be capable of guiding you through basic git issues.

@MichaelEischer MichaelEischer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes are moving in the right direction. I've left a additional comments how to clean up the implementation further.

Comment thread internal/checker/checker.go Outdated
Comment thread internal/checker/checker.go Outdated
Comment thread internal/checker/checker.go Outdated
Comment thread internal/checker/checker.go Outdated
Comment thread internal/checker/checker.go Outdated
@wplapper

wplapper commented Feb 16, 2025

Copy link
Copy Markdown
Contributor Author

I carefully rebased this PR (check_snapshots), following https://www.squash.io/how-to-rebase-local-branch-onto-remote-master-in-git/ . I hope my rebasing is acceptable.

Add code to cmd/restic/cmd_check.go to detect snapshots
Resolved conflict for cmd/restic/cmd_check.go - runCheck
integrated newCheckCommand(...)
…rt 2

Added code for selecting multiple snapshots.
Added message how many packfiles and their cumulative size were selected.
In internal/checker/checker.go replaced the datablob / packfile selection from using walker.Walk
to restic.StreamTrees - parallelizing the packfile selection.

resolved conflict in cmd_check: allow check for snapshot filter
reworked the code using snapshotFilter.FindAll to find all snapshots and
restic.FindUsedBlobs to retrieve all used blobs.
range repo.LookupBlob (as before) to convert the blobs to their containing packfiles
and c.repo.List(ctx, restic.PackFile, ...) to retrieve the sizes of those packfiles.

Additional documentation and tests are still outstanding.
fixed lint errors
add documentation for checking via snapshot filter.

cmd_check: change 1st line of issue-3326 to recognized format.

check with snapshot filter: changed issue description
…rt 4

Hand down filtered tree IDs to CheckWithSnapshots which builds 'usedBlobs'.
repo.LookupBlob is used to derive packfiles from 'usedBlobs' and constructs
'snapPacks' and overwrites c.packs.
…add checker_test

cmd_check, checker: added error return
checker_test: testing CheckWithSnapshots

checker_test: fixed lint error for empty tree list
based on the change in runCheck return values the following functions have been afftected
cmd/restic/cmd_check_integration_test.go
cmd/restic/cmd_migrate.go
cmd/restic/cmd_prune_integration_test.go
cmd/restic/integration_test.go

check - corrected rebase error by not removing obsolete code.

the check for the filter if len(args) > 0 { ... } is obsolete and has been replaced by
if len(args) > 0 || !opts.SnapshotFilter.Empty() { ... }
@wplapper wplapper changed the title issue 3326: restic check - enable --read-data-* for specified snapshots restic check - enable --read-data-* with snapshot filter Feb 19, 2025
wplapper added 2 commits March 7, 2025 17:57
cmd_check.go - minor changes: changes error meesage to New instead of Fatal
added a short description in the help text.

cmd_check_integration_test.go: added two tests which should both fail:
invalid input
checker.go: added error message if 'selectedTrees' is empty
checker_test.go: modified test checking using test.OK() and test.Assert() to
make the checking more compact.
@wplapper

Copy link
Copy Markdown
Contributor Author

I have decided to revert this PR to closed status to ease the pressure on the
maintainers of restic. Once the number of PRs has dropped considerably, I might
moved this PR back to the queue to be reviewed. In case you consider this PR
important, please contact me via the usual channels. Thank you!

@wplapper wplapper closed this May 30, 2025
@kinazarov1999

Copy link
Copy Markdown

I have decided to revert this PR to closed status to ease the pressure on the maintainers of restic. Once the number of PRs has dropped considerably, I might moved this PR back to the queue to be reviewed. In case you consider this PR important, please contact me via the usual channels. Thank you!

Looking forward to this feature! I’d be happy to see this PR go through.

@wplapper wplapper reopened this Aug 7, 2025
wplapper added a commit to wplapper/restic that referenced this pull request Aug 7, 2025
This is the continuation of PR restic#5213. I decided to use a git provided
e-mail address instead of my private e-mail address. All changes up-to
2025-03-07 have been incorporated. Details to follow here:

changelog/unreleased/issue-3326:
Introduction of snapshot based filter for command `restic check`

cmd/restic/cmd_check.go:
Integration of snapshot based filtering. Some messages had to be changed
so that the text does not refer to 'all packfiles' but to 'selected packfiles'.

cmd/restic/cmd_check_integration_test.go:
A new test for filtering via snapshots has been added.

doc/077_troubleshooting.rst:
A short paragraph in the manual has been added about snapshot based filtering.

internal/checker/checker.go:
A new func CheckWithSnapshots() has been added which collects the needed
packfiles via restic.FindUsedBlobs().

internal/checker/checker_test.go:
A new test TestCheckRepoSnapshot() has been added to check out the new functionality.
@wplapper

wplapper commented Aug 7, 2025

Copy link
Copy Markdown
Contributor Author

I decided to use a github provided e-mal address and I did not see a way of changed my private e-mail address in already existing commits. Therefore I close this PR. It is continued in #5469.

@wplapper wplapper closed this Aug 7, 2025
@wplapper wplapper changed the title restic check - enable --read-data-* with snapshot filter PR closed - wrong email: restic check - enable --read-data-* with snapshot filter Aug 7, 2025
@wplapper wplapper deleted the check_snapshots branch August 7, 2025 07:40
@wplapper wplapper restored the check_snapshots branch August 7, 2025 07:42
@wplapper wplapper deleted the check_snapshots branch November 29, 2025 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Check and verify a specific snapshot

3 participants