From 2ec0f3303a8b588c816ff72d485d14130f871af9 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 6 Nov 2021 00:32:46 +0100 Subject: [PATCH 1/8] backup/diff/dump/restore/stats: List snapshots before index During a backup the index is written before the corresponding snapshots. To ensure that a concurrent/later restic run can read a snapshot's data, restic thus must first load the snapshots and only afterwards the index. Otherwise it is not possible to ensure that the loaded index is recent enough to cover all of the snapshot's data. --- cmd/restic/cmd_backup.go | 16 ++++++++-------- cmd/restic/cmd_diff.go | 8 ++++---- cmd/restic/cmd_dump.go | 10 +++++----- cmd/restic/cmd_prune.go | 1 + cmd/restic/cmd_restore.go | 10 +++++----- cmd/restic/cmd_stats.go | 8 ++++---- 6 files changed, 27 insertions(+), 26 deletions(-) diff --git a/cmd/restic/cmd_backup.go b/cmd/restic/cmd_backup.go index 873fce163..a899fb013 100644 --- a/cmd/restic/cmd_backup.go +++ b/cmd/restic/cmd_backup.go @@ -571,14 +571,6 @@ func runBackup(opts BackupOptions, gopts GlobalOptions, term *termstatus.Termina return err } - if !gopts.JSON { - progressPrinter.V("load index files") - } - err = repo.LoadIndex(gopts.ctx) - if err != nil { - return err - } - var parentSnapshotID *restic.ID if !opts.Stdin { parentSnapshotID, err = findParentSnapshot(gopts.ctx, repo, opts, targets, timeStamp) @@ -595,6 +587,14 @@ func runBackup(opts BackupOptions, gopts GlobalOptions, term *termstatus.Termina } } + if !gopts.JSON { + progressPrinter.V("load index files") + } + err = repo.LoadIndex(gopts.ctx) + if err != nil { + return err + } + selectByNameFilter := func(item string) bool { for _, reject := range rejectByNameFuncs { if reject(item) { diff --git a/cmd/restic/cmd_diff.go b/cmd/restic/cmd_diff.go index f83c87132..60a50a21f 100644 --- a/cmd/restic/cmd_diff.go +++ b/cmd/restic/cmd_diff.go @@ -334,10 +334,6 @@ func runDiff(opts DiffOptions, gopts GlobalOptions, args []string) error { return err } - if err = repo.LoadIndex(ctx); err != nil { - return err - } - if !gopts.NoLock { lock, err := lockRepo(ctx, repo) defer unlockRepo(lock) @@ -360,6 +356,10 @@ func runDiff(opts DiffOptions, gopts GlobalOptions, args []string) error { Verbosef("comparing snapshot %v to %v:\n\n", sn1.ID().Str(), sn2.ID().Str()) } + if err = repo.LoadIndex(ctx); err != nil { + return err + } + if sn1.Tree == nil { return errors.Errorf("snapshot %v has nil tree", sn1.ID().Str()) } diff --git a/cmd/restic/cmd_dump.go b/cmd/restic/cmd_dump.go index 4449af5ce..cadc05476 100644 --- a/cmd/restic/cmd_dump.go +++ b/cmd/restic/cmd_dump.go @@ -144,11 +144,6 @@ func runDump(opts DumpOptions, gopts GlobalOptions, args []string) error { } } - err = repo.LoadIndex(ctx) - if err != nil { - return err - } - var id restic.ID if snapshotIDString == "latest" { @@ -168,6 +163,11 @@ func runDump(opts DumpOptions, gopts GlobalOptions, args []string) error { Exitf(2, "loading snapshot %q failed: %v", snapshotIDString, err) } + err = repo.LoadIndex(ctx) + if err != nil { + return err + } + tree, err := repo.LoadTree(ctx, *sn.Tree) if err != nil { Exitf(2, "loading tree for snapshot %q failed: %v", snapshotIDString, err) diff --git a/cmd/restic/cmd_prune.go b/cmd/restic/cmd_prune.go index 76d7d7ea9..4bbd6d2fd 100644 --- a/cmd/restic/cmd_prune.go +++ b/cmd/restic/cmd_prune.go @@ -150,6 +150,7 @@ func runPruneWithRepo(opts PruneOptions, gopts GlobalOptions, repo *repository.R } Verbosef("loading indexes...\n") + // loading the index before the snapshots is ok, as we use an exclusive lock here err := repo.LoadIndex(gopts.ctx) if err != nil { return err diff --git a/cmd/restic/cmd_restore.go b/cmd/restic/cmd_restore.go index 4d5818593..d2313e5c9 100644 --- a/cmd/restic/cmd_restore.go +++ b/cmd/restic/cmd_restore.go @@ -110,11 +110,6 @@ func runRestore(opts RestoreOptions, gopts GlobalOptions, args []string) error { } } - err = repo.LoadIndex(ctx) - if err != nil { - return err - } - var id restic.ID if snapshotIDString == "latest" { @@ -129,6 +124,11 @@ func runRestore(opts RestoreOptions, gopts GlobalOptions, args []string) error { } } + err = repo.LoadIndex(ctx) + if err != nil { + return err + } + res, err := restorer.NewRestorer(ctx, repo, id) if err != nil { Exitf(2, "creating restorer failed: %v\n", err) diff --git a/cmd/restic/cmd_stats.go b/cmd/restic/cmd_stats.go index deb649e26..32161bd03 100644 --- a/cmd/restic/cmd_stats.go +++ b/cmd/restic/cmd_stats.go @@ -86,10 +86,6 @@ func runStats(gopts GlobalOptions, args []string) error { return err } - if err = repo.LoadIndex(ctx); err != nil { - return err - } - if !gopts.NoLock { lock, err := lockRepo(ctx, repo) defer unlockRepo(lock) @@ -98,6 +94,10 @@ func runStats(gopts GlobalOptions, args []string) error { } } + if err = repo.LoadIndex(ctx); err != nil { + return err + } + if !gopts.JSON { Printf("scanning...\n") } From 3d29083e60d011f609017e21eab07d6df43168d6 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 6 Nov 2021 01:14:24 +0100 Subject: [PATCH 2/8] copy/find/ls/recover/stats: Memorize snapshot listing before index These commands filter the snapshots according to some criteria which essentially requires loading the index before filtering the snapshots. Thus create a copy of the snapshots list beforehand and use it later on. --- cmd/restic/cmd_backup.go | 4 +-- cmd/restic/cmd_cat.go | 2 +- cmd/restic/cmd_copy.go | 15 +++++++-- cmd/restic/cmd_debug.go | 2 +- cmd/restic/cmd_diff.go | 2 +- cmd/restic/cmd_dump.go | 4 +-- cmd/restic/cmd_find.go | 8 ++++- cmd/restic/cmd_forget.go | 2 +- cmd/restic/cmd_ls.go | 8 ++++- cmd/restic/cmd_prune.go | 2 +- cmd/restic/cmd_recover.go | 8 ++++- cmd/restic/cmd_restore.go | 4 +-- cmd/restic/cmd_snapshots.go | 2 +- cmd/restic/cmd_stats.go | 8 ++++- cmd/restic/cmd_tag.go | 2 +- cmd/restic/find.go | 11 +++---- internal/backend/utils.go | 38 ++++++++++++++++++++++ internal/backend/utils_test.go | 46 +++++++++++++++++++++++++++ internal/checker/checker.go | 2 +- internal/checker/checker_test.go | 2 +- internal/fuse/snapshots_dir.go | 2 +- internal/restic/repository.go | 5 +++ internal/restic/snapshot.go | 16 +++++++--- internal/restic/snapshot_find.go | 14 ++++---- internal/restic/snapshot_find_test.go | 4 +-- internal/restic/testing_test.go | 2 +- 26 files changed, 173 insertions(+), 42 deletions(-) diff --git a/cmd/restic/cmd_backup.go b/cmd/restic/cmd_backup.go index a899fb013..7ce4f52de 100644 --- a/cmd/restic/cmd_backup.go +++ b/cmd/restic/cmd_backup.go @@ -475,7 +475,7 @@ func collectTargets(opts BackupOptions, args []string) (targets []string, err er func findParentSnapshot(ctx context.Context, repo restic.Repository, opts BackupOptions, targets []string, timeStampLimit time.Time) (parentID *restic.ID, err error) { // Force using a parent if !opts.Force && opts.Parent != "" { - id, err := restic.FindSnapshot(ctx, repo, opts.Parent) + id, err := restic.FindSnapshot(ctx, repo.Backend(), opts.Parent) if err != nil { return nil, errors.Fatalf("invalid id %q: %v", opts.Parent, err) } @@ -485,7 +485,7 @@ func findParentSnapshot(ctx context.Context, repo restic.Repository, opts Backup // Find last snapshot to set it as parent, if not already set if !opts.Force && parentID == nil { - id, err := restic.FindLatestSnapshot(ctx, repo, targets, []restic.TagList{}, []string{opts.Host}, &timeStampLimit) + id, err := restic.FindLatestSnapshot(ctx, repo.Backend(), repo, targets, []restic.TagList{}, []string{opts.Host}, &timeStampLimit) if err == nil { parentID = &id } else if err != restic.ErrNoSnapshotFound { diff --git a/cmd/restic/cmd_cat.go b/cmd/restic/cmd_cat.go index ec0535db0..86d538e70 100644 --- a/cmd/restic/cmd_cat.go +++ b/cmd/restic/cmd_cat.go @@ -62,7 +62,7 @@ func runCat(gopts GlobalOptions, args []string) error { } // find snapshot id with prefix - id, err = restic.FindSnapshot(gopts.ctx, repo, args[1]) + id, err = restic.FindSnapshot(gopts.ctx, repo.Backend(), args[1]) if err != nil { return errors.Fatalf("could not find snapshot: %v\n", err) } diff --git a/cmd/restic/cmd_copy.go b/cmd/restic/cmd_copy.go index 048210ba8..03b34ecca 100644 --- a/cmd/restic/cmd_copy.go +++ b/cmd/restic/cmd_copy.go @@ -4,6 +4,7 @@ import ( "context" "fmt" + "github.com/restic/restic/internal/backend" "github.com/restic/restic/internal/debug" "github.com/restic/restic/internal/repository" "github.com/restic/restic/internal/restic" @@ -88,6 +89,16 @@ func runCopy(opts CopyOptions, gopts GlobalOptions, args []string) error { return err } + srcSnapshotLister, err := backend.MemorizeList(ctx, srcRepo.Backend(), restic.SnapshotFile) + if err != nil { + return err + } + + dstSnapshotLister, err := backend.MemorizeList(ctx, dstRepo.Backend(), restic.SnapshotFile) + if err != nil { + return err + } + debug.Log("Loading source index") if err := srcRepo.LoadIndex(ctx); err != nil { return err @@ -99,7 +110,7 @@ func runCopy(opts CopyOptions, gopts GlobalOptions, args []string) error { } dstSnapshotByOriginal := make(map[restic.ID][]*restic.Snapshot) - for sn := range FindFilteredSnapshots(ctx, dstRepo, opts.Hosts, opts.Tags, opts.Paths, nil) { + for sn := range FindFilteredSnapshots(ctx, dstSnapshotLister, dstRepo, opts.Hosts, opts.Tags, opts.Paths, nil) { if sn.Original != nil && !sn.Original.IsNull() { dstSnapshotByOriginal[*sn.Original] = append(dstSnapshotByOriginal[*sn.Original], sn) } @@ -110,7 +121,7 @@ func runCopy(opts CopyOptions, gopts GlobalOptions, args []string) error { // remember already processed trees across all snapshots visitedTrees := restic.NewIDSet() - for sn := range FindFilteredSnapshots(ctx, srcRepo, opts.Hosts, opts.Tags, opts.Paths, args) { + for sn := range FindFilteredSnapshots(ctx, srcSnapshotLister, srcRepo, opts.Hosts, opts.Tags, opts.Paths, args) { Verbosef("\nsnapshot %s of %v at %s)\n", sn.ID().Str(), sn.Paths, sn.Time) // check whether the destination has a snapshot with the same persistent ID which has similar snapshot fields diff --git a/cmd/restic/cmd_debug.go b/cmd/restic/cmd_debug.go index a6e83f98f..7947f789f 100644 --- a/cmd/restic/cmd_debug.go +++ b/cmd/restic/cmd_debug.go @@ -73,7 +73,7 @@ func prettyPrintJSON(wr io.Writer, item interface{}) error { } func debugPrintSnapshots(ctx context.Context, repo *repository.Repository, wr io.Writer) error { - return restic.ForAllSnapshots(ctx, repo, nil, func(id restic.ID, snapshot *restic.Snapshot, err error) error { + return restic.ForAllSnapshots(ctx, repo.Backend(), repo, nil, func(id restic.ID, snapshot *restic.Snapshot, err error) error { if err != nil { return err } diff --git a/cmd/restic/cmd_diff.go b/cmd/restic/cmd_diff.go index 60a50a21f..ae76e4071 100644 --- a/cmd/restic/cmd_diff.go +++ b/cmd/restic/cmd_diff.go @@ -54,7 +54,7 @@ func init() { } func loadSnapshot(ctx context.Context, repo *repository.Repository, desc string) (*restic.Snapshot, error) { - id, err := restic.FindSnapshot(ctx, repo, desc) + id, err := restic.FindSnapshot(ctx, repo.Backend(), desc) if err != nil { return nil, errors.Fatal(err.Error()) } diff --git a/cmd/restic/cmd_dump.go b/cmd/restic/cmd_dump.go index cadc05476..b4ce299cd 100644 --- a/cmd/restic/cmd_dump.go +++ b/cmd/restic/cmd_dump.go @@ -147,12 +147,12 @@ func runDump(opts DumpOptions, gopts GlobalOptions, args []string) error { var id restic.ID if snapshotIDString == "latest" { - id, err = restic.FindLatestSnapshot(ctx, repo, opts.Paths, opts.Tags, opts.Hosts, nil) + id, err = restic.FindLatestSnapshot(ctx, repo.Backend(), repo, opts.Paths, opts.Tags, opts.Hosts, nil) if err != nil { Exitf(1, "latest snapshot for criteria not found: %v Paths:%v Hosts:%v", err, opts.Paths, opts.Hosts) } } else { - id, err = restic.FindSnapshot(ctx, repo, snapshotIDString) + id, err = restic.FindSnapshot(ctx, repo.Backend(), snapshotIDString) if err != nil { Exitf(1, "invalid id %q: %v", snapshotIDString, err) } diff --git a/cmd/restic/cmd_find.go b/cmd/restic/cmd_find.go index 033915ab0..7171314e2 100644 --- a/cmd/restic/cmd_find.go +++ b/cmd/restic/cmd_find.go @@ -9,6 +9,7 @@ import ( "github.com/spf13/cobra" + "github.com/restic/restic/internal/backend" "github.com/restic/restic/internal/debug" "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/filter" @@ -584,6 +585,11 @@ func runFind(opts FindOptions, gopts GlobalOptions, args []string) error { } } + snapshotLister, err := backend.MemorizeList(gopts.ctx, repo.Backend(), restic.SnapshotFile) + if err != nil { + return err + } + if err = repo.LoadIndex(gopts.ctx); err != nil { return err } @@ -618,7 +624,7 @@ func runFind(opts FindOptions, gopts GlobalOptions, args []string) error { } } - for sn := range FindFilteredSnapshots(ctx, repo, opts.Hosts, opts.Tags, opts.Paths, opts.Snapshots) { + for sn := range FindFilteredSnapshots(ctx, snapshotLister, repo, opts.Hosts, opts.Tags, opts.Paths, opts.Snapshots) { if f.blobIDs != nil || f.treeIDs != nil { if err = f.findIDs(ctx, sn); err != nil && err.Error() != "OK" { return err diff --git a/cmd/restic/cmd_forget.go b/cmd/restic/cmd_forget.go index c814472f7..1feb39994 100644 --- a/cmd/restic/cmd_forget.go +++ b/cmd/restic/cmd_forget.go @@ -128,7 +128,7 @@ func runForget(opts ForgetOptions, gopts GlobalOptions, args []string) error { var snapshots restic.Snapshots removeSnIDs := restic.NewIDSet() - for sn := range FindFilteredSnapshots(ctx, repo, opts.Hosts, opts.Tags, opts.Paths, args) { + for sn := range FindFilteredSnapshots(ctx, repo.Backend(), repo, opts.Hosts, opts.Tags, opts.Paths, args) { snapshots = append(snapshots, sn) } diff --git a/cmd/restic/cmd_ls.go b/cmd/restic/cmd_ls.go index 71a44949e..ec6695714 100644 --- a/cmd/restic/cmd_ls.go +++ b/cmd/restic/cmd_ls.go @@ -9,6 +9,7 @@ import ( "github.com/spf13/cobra" + "github.com/restic/restic/internal/backend" "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/fs" "github.com/restic/restic/internal/restic" @@ -169,6 +170,11 @@ func runLs(opts LsOptions, gopts GlobalOptions, args []string) error { return err } + snapshotLister, err := backend.MemorizeList(gopts.ctx, repo.Backend(), restic.SnapshotFile) + if err != nil { + return err + } + if err = repo.LoadIndex(gopts.ctx); err != nil { return err } @@ -211,7 +217,7 @@ func runLs(opts LsOptions, gopts GlobalOptions, args []string) error { } } - for sn := range FindFilteredSnapshots(ctx, repo, opts.Hosts, opts.Tags, opts.Paths, args[:1]) { + for sn := range FindFilteredSnapshots(ctx, snapshotLister, repo, opts.Hosts, opts.Tags, opts.Paths, args[:1]) { printSnapshot(sn) err := walker.Walk(ctx, repo, *sn.Tree, nil, func(_ restic.ID, nodepath string, node *restic.Node, err error) (bool, error) { diff --git a/cmd/restic/cmd_prune.go b/cmd/restic/cmd_prune.go index 4bbd6d2fd..195b2554d 100644 --- a/cmd/restic/cmd_prune.go +++ b/cmd/restic/cmd_prune.go @@ -555,7 +555,7 @@ func getUsedBlobs(gopts GlobalOptions, repo restic.Repository, ignoreSnapshots r var snapshotTrees restic.IDs Verbosef("loading all snapshots...\n") - err = restic.ForAllSnapshots(gopts.ctx, repo, ignoreSnapshots, + err = restic.ForAllSnapshots(gopts.ctx, repo.Backend(), repo, ignoreSnapshots, func(id restic.ID, sn *restic.Snapshot, err error) error { debug.Log("add snapshot %v (tree %v, error %v)", id, *sn.Tree, err) if err != nil { diff --git a/cmd/restic/cmd_recover.go b/cmd/restic/cmd_recover.go index 860c45a57..93810ce0f 100644 --- a/cmd/restic/cmd_recover.go +++ b/cmd/restic/cmd_recover.go @@ -5,6 +5,7 @@ import ( "os" "time" + "github.com/restic/restic/internal/backend" "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/restic" "github.com/spf13/cobra" @@ -50,6 +51,11 @@ func runRecover(gopts GlobalOptions) error { return err } + snapshotLister, err := backend.MemorizeList(gopts.ctx, repo.Backend(), restic.SnapshotFile) + if err != nil { + return err + } + Verbosef("load index files\n") if err = repo.LoadIndex(gopts.ctx); err != nil { return err @@ -84,7 +90,7 @@ func runRecover(gopts GlobalOptions) error { bar.Done() Verbosef("load snapshots\n") - err = restic.ForAllSnapshots(gopts.ctx, repo, nil, func(id restic.ID, sn *restic.Snapshot, err error) error { + err = restic.ForAllSnapshots(gopts.ctx, snapshotLister, repo, nil, func(id restic.ID, sn *restic.Snapshot, err error) error { trees[*sn.Tree] = true return nil }) diff --git a/cmd/restic/cmd_restore.go b/cmd/restic/cmd_restore.go index d2313e5c9..3c7baeb0f 100644 --- a/cmd/restic/cmd_restore.go +++ b/cmd/restic/cmd_restore.go @@ -113,12 +113,12 @@ func runRestore(opts RestoreOptions, gopts GlobalOptions, args []string) error { var id restic.ID if snapshotIDString == "latest" { - id, err = restic.FindLatestSnapshot(ctx, repo, opts.Paths, opts.Tags, opts.Hosts, nil) + id, err = restic.FindLatestSnapshot(ctx, repo.Backend(), repo, opts.Paths, opts.Tags, opts.Hosts, nil) if err != nil { Exitf(1, "latest snapshot for criteria not found: %v Paths:%v Hosts:%v", err, opts.Paths, opts.Hosts) } } else { - id, err = restic.FindSnapshot(ctx, repo, snapshotIDString) + id, err = restic.FindSnapshot(ctx, repo.Backend(), snapshotIDString) if err != nil { Exitf(1, "invalid id %q: %v", snapshotIDString, err) } diff --git a/cmd/restic/cmd_snapshots.go b/cmd/restic/cmd_snapshots.go index 70db09417..d305878da 100644 --- a/cmd/restic/cmd_snapshots.go +++ b/cmd/restic/cmd_snapshots.go @@ -79,7 +79,7 @@ func runSnapshots(opts SnapshotOptions, gopts GlobalOptions, args []string) erro defer cancel() var snapshots restic.Snapshots - for sn := range FindFilteredSnapshots(ctx, repo, opts.Hosts, opts.Tags, opts.Paths, args) { + for sn := range FindFilteredSnapshots(ctx, repo.Backend(), repo, opts.Hosts, opts.Tags, opts.Paths, args) { snapshots = append(snapshots, sn) } snapshotGroups, grouped, err := restic.GroupSnapshots(snapshots, opts.GroupBy) diff --git a/cmd/restic/cmd_stats.go b/cmd/restic/cmd_stats.go index 32161bd03..59dc75852 100644 --- a/cmd/restic/cmd_stats.go +++ b/cmd/restic/cmd_stats.go @@ -6,6 +6,7 @@ import ( "fmt" "path/filepath" + "github.com/restic/restic/internal/backend" "github.com/restic/restic/internal/restic" "github.com/restic/restic/internal/walker" @@ -94,6 +95,11 @@ func runStats(gopts GlobalOptions, args []string) error { } } + snapshotLister, err := backend.MemorizeList(gopts.ctx, repo.Backend(), restic.SnapshotFile) + if err != nil { + return err + } + if err = repo.LoadIndex(ctx); err != nil { return err } @@ -111,7 +117,7 @@ func runStats(gopts GlobalOptions, args []string) error { snapshotsCount: 0, } - for sn := range FindFilteredSnapshots(ctx, repo, statsOptions.Hosts, statsOptions.Tags, statsOptions.Paths, args) { + for sn := range FindFilteredSnapshots(ctx, snapshotLister, repo, statsOptions.Hosts, statsOptions.Tags, statsOptions.Paths, args) { err = statsWalkSnapshot(ctx, sn, repo, stats) if err != nil { return fmt.Errorf("error walking snapshot: %v", err) diff --git a/cmd/restic/cmd_tag.go b/cmd/restic/cmd_tag.go index 13842c077..e6688b2fc 100644 --- a/cmd/restic/cmd_tag.go +++ b/cmd/restic/cmd_tag.go @@ -129,7 +129,7 @@ func runTag(opts TagOptions, gopts GlobalOptions, args []string) error { changeCnt := 0 ctx, cancel := context.WithCancel(gopts.ctx) defer cancel() - for sn := range FindFilteredSnapshots(ctx, repo, opts.Hosts, opts.Tags, opts.Paths, args) { + for sn := range FindFilteredSnapshots(ctx, repo.Backend(), repo, opts.Hosts, opts.Tags, opts.Paths, args) { changed, err := changeTags(ctx, repo, sn, opts.SetTags.Flatten(), opts.AddTags.Flatten(), opts.RemoveTags.Flatten()) if err != nil { Warnf("unable to modify the tags for snapshot ID %q, ignoring: %v\n", sn.ID(), err) diff --git a/cmd/restic/find.go b/cmd/restic/find.go index 792f1f6a2..a319d23de 100644 --- a/cmd/restic/find.go +++ b/cmd/restic/find.go @@ -3,12 +3,11 @@ package main import ( "context" - "github.com/restic/restic/internal/repository" "github.com/restic/restic/internal/restic" ) // FindFilteredSnapshots yields Snapshots, either given explicitly by `snapshotIDs` or filtered from the list of all snapshots. -func FindFilteredSnapshots(ctx context.Context, repo *repository.Repository, hosts []string, tags []restic.TagList, paths []string, snapshotIDs []string) <-chan *restic.Snapshot { +func FindFilteredSnapshots(ctx context.Context, be restic.Lister, loader restic.LoadJSONUnpackeder, hosts []string, tags []restic.TagList, paths []string, snapshotIDs []string) <-chan *restic.Snapshot { out := make(chan *restic.Snapshot) go func() { defer close(out) @@ -23,13 +22,13 @@ func FindFilteredSnapshots(ctx context.Context, repo *repository.Repository, hos for _, s := range snapshotIDs { if s == "latest" { usedFilter = true - id, err = restic.FindLatestSnapshot(ctx, repo, paths, tags, hosts, nil) + id, err = restic.FindLatestSnapshot(ctx, be, loader, paths, tags, hosts, nil) if err != nil { Warnf("Ignoring %q, no snapshot matched given filter (Paths:%v Tags:%v Hosts:%v)\n", s, paths, tags, hosts) continue } } else { - id, err = restic.FindSnapshot(ctx, repo, s) + id, err = restic.FindSnapshot(ctx, be, s) if err != nil { Warnf("Ignoring %q: %v\n", s, err) continue @@ -44,7 +43,7 @@ func FindFilteredSnapshots(ctx context.Context, repo *repository.Repository, hos } for _, id := range ids.Uniq() { - sn, err := restic.LoadSnapshot(ctx, repo, id) + sn, err := restic.LoadSnapshot(ctx, loader, id) if err != nil { Warnf("Ignoring %q, could not load snapshot: %v\n", id, err) continue @@ -58,7 +57,7 @@ func FindFilteredSnapshots(ctx context.Context, repo *repository.Repository, hos return } - snapshots, err := restic.FindFilteredSnapshots(ctx, repo, hosts, tags, paths) + snapshots, err := restic.FindFilteredSnapshots(ctx, be, loader, hosts, tags, paths) if err != nil { Warnf("could not load snapshots: %v\n", err) return diff --git a/internal/backend/utils.go b/internal/backend/utils.go index 39c68b4ce..0dd081232 100644 --- a/internal/backend/utils.go +++ b/internal/backend/utils.go @@ -3,6 +3,7 @@ package backend import ( "bytes" "context" + "fmt" "io" "github.com/restic/restic/internal/restic" @@ -57,3 +58,40 @@ func DefaultLoad(ctx context.Context, h restic.Handle, length int, offset int64, } return rd.Close() } + +type memorizedLister struct { + fileInfos []restic.FileInfo + tpe restic.FileType +} + +func (m *memorizedLister) List(ctx context.Context, t restic.FileType, fn func(restic.FileInfo) error) error { + if t != m.tpe { + return fmt.Errorf("filetype mismatch, expected %s got %s", m.tpe, t) + } + for _, fi := range m.fileInfos { + if ctx.Err() != nil { + break + } + err := fn(fi) + if err != nil { + return err + } + } + return ctx.Err() +} + +func MemorizeList(ctx context.Context, be restic.Lister, t restic.FileType) (restic.Lister, error) { + var fileInfos []restic.FileInfo + err := be.List(ctx, t, func(fi restic.FileInfo) error { + fileInfos = append(fileInfos, fi) + return nil + }) + if err != nil { + return nil, err + } + + return &memorizedLister{ + fileInfos: fileInfos, + tpe: t, + }, nil +} diff --git a/internal/backend/utils_test.go b/internal/backend/utils_test.go index 1030537bc..0841a57e4 100644 --- a/internal/backend/utils_test.go +++ b/internal/backend/utils_test.go @@ -3,6 +3,7 @@ package backend_test import ( "bytes" "context" + "fmt" "io" "math/rand" "testing" @@ -10,6 +11,7 @@ import ( "github.com/restic/restic/internal/backend" "github.com/restic/restic/internal/backend/mem" "github.com/restic/restic/internal/errors" + "github.com/restic/restic/internal/mock" "github.com/restic/restic/internal/restic" rtest "github.com/restic/restic/internal/test" ) @@ -157,3 +159,47 @@ func TestDefaultLoad(t *testing.T) { rtest.Equals(t, true, rd.closed) rtest.Equals(t, "consumer error", err.Error()) } + +func TestMemoizeList(t *testing.T) { + // setup backend to serve as data source for memoized list + be := mock.NewBackend() + files := []restic.FileInfo{ + {Size: 42, Name: restic.NewRandomID().String()}, + {Size: 45, Name: restic.NewRandomID().String()}, + } + be.ListFn = func(ctx context.Context, t restic.FileType, fn func(restic.FileInfo) error) error { + for _, fi := range files { + if err := fn(fi); err != nil { + return err + } + } + return nil + } + + mem, err := backend.MemorizeList(context.TODO(), be, restic.SnapshotFile) + rtest.OK(t, err) + + err = mem.List(context.TODO(), restic.IndexFile, func(fi restic.FileInfo) error { + t.Fatal("file type mismatch") + return nil // the memoized lister must return an error by itself + }) + rtest.Assert(t, err != nil, "missing error on file typ mismatch") + + var memFiles []restic.FileInfo + err = mem.List(context.TODO(), restic.SnapshotFile, func(fi restic.FileInfo) error { + memFiles = append(memFiles, fi) + return nil + }) + rtest.OK(t, err) + rtest.Equals(t, files, memFiles) +} + +func TestMemoizeListError(t *testing.T) { + // setup backend to serve as data source for memoized list + be := mock.NewBackend() + be.ListFn = func(ctx context.Context, t restic.FileType, fn func(restic.FileInfo) error) error { + return fmt.Errorf("list error") + } + _, err := backend.MemorizeList(context.TODO(), be, restic.SnapshotFile) + rtest.Assert(t, err != nil, "missing error on list error") +} diff --git a/internal/checker/checker.go b/internal/checker/checker.go index ebf38b632..93e58cedc 100644 --- a/internal/checker/checker.go +++ b/internal/checker/checker.go @@ -279,7 +279,7 @@ func (c *Checker) checkTreeWorker(ctx context.Context, trees <-chan restic.TreeI } func loadSnapshotTreeIDs(ctx context.Context, repo restic.Repository) (ids restic.IDs, errs []error) { - err := restic.ForAllSnapshots(ctx, repo, nil, func(id restic.ID, sn *restic.Snapshot, err error) error { + err := restic.ForAllSnapshots(ctx, repo.Backend(), repo, nil, func(id restic.ID, sn *restic.Snapshot, err error) error { if err != nil { errs = append(errs, err) return nil diff --git a/internal/checker/checker_test.go b/internal/checker/checker_test.go index 6d17a4593..948d5a480 100644 --- a/internal/checker/checker_test.go +++ b/internal/checker/checker_test.go @@ -587,7 +587,7 @@ func benchmarkSnapshotScaling(t *testing.B, newSnapshots int) { chkr, repo, cleanup := loadBenchRepository(t) defer cleanup() - snID, err := restic.FindSnapshot(context.TODO(), repo, "51d249d2") + snID, err := restic.FindSnapshot(context.TODO(), repo.Backend(), "51d249d2") if err != nil { t.Fatal(err) } diff --git a/internal/fuse/snapshots_dir.go b/internal/fuse/snapshots_dir.go index 40854082e..88638d50c 100644 --- a/internal/fuse/snapshots_dir.go +++ b/internal/fuse/snapshots_dir.go @@ -223,7 +223,7 @@ func updateSnapshots(ctx context.Context, root *Root) error { return nil } - snapshots, err := restic.FindFilteredSnapshots(ctx, root.repo, root.cfg.Hosts, root.cfg.Tags, root.cfg.Paths) + snapshots, err := restic.FindFilteredSnapshots(ctx, root.repo.Backend(), root.repo, root.cfg.Hosts, root.cfg.Tags, root.cfg.Paths) if err != nil { return err } diff --git a/internal/restic/repository.go b/internal/restic/repository.go index 35f2f997b..61654fa1d 100644 --- a/internal/restic/repository.go +++ b/internal/restic/repository.go @@ -60,6 +60,11 @@ type Lister interface { List(context.Context, FileType, func(FileInfo) error) error } +// LoadJSONUnpackeder allows loading a JSON file not stored in a pack file +type LoadJSONUnpackeder interface { + LoadJSONUnpacked(ctx context.Context, t FileType, id ID, dest interface{}) error +} + type PackBlobs struct { PackID ID Blobs []Blob diff --git a/internal/restic/snapshot.go b/internal/restic/snapshot.go index ac5f2cf44..a74893778 100644 --- a/internal/restic/snapshot.go +++ b/internal/restic/snapshot.go @@ -59,9 +59,9 @@ func NewSnapshot(paths []string, tags []string, hostname string, time time.Time) } // LoadSnapshot loads the snapshot with the id and returns it. -func LoadSnapshot(ctx context.Context, repo Repository, id ID) (*Snapshot, error) { +func LoadSnapshot(ctx context.Context, loader LoadJSONUnpackeder, id ID) (*Snapshot, error) { sn := &Snapshot{id: &id} - err := repo.LoadJSONUnpacked(ctx, SnapshotFile, id, sn) + err := loader.LoadJSONUnpacked(ctx, SnapshotFile, id, sn) if err != nil { return nil, err } @@ -76,7 +76,7 @@ const loadSnapshotParallelism = 5 // If the called function returns an error, this function is cancelled and // also returns this error. // If a snapshot ID is in excludeIDs, it will be ignored. -func ForAllSnapshots(ctx context.Context, repo Repository, excludeIDs IDSet, fn func(ID, *Snapshot, error) error) error { +func ForAllSnapshots(ctx context.Context, be Lister, loader LoadJSONUnpackeder, excludeIDs IDSet, fn func(ID, *Snapshot, error) error) error { var m sync.Mutex // track spawned goroutines using wg, create a new context which is @@ -88,7 +88,13 @@ func ForAllSnapshots(ctx context.Context, repo Repository, excludeIDs IDSet, fn // send list of snapshot files through ch, which is closed afterwards wg.Go(func() error { defer close(ch) - return repo.List(ctx, SnapshotFile, func(id ID, size int64) error { + return be.List(ctx, SnapshotFile, func(fi FileInfo) error { + id, err := ParseID(fi.Name) + if err != nil { + debug.Log("unable to parse %v as an ID", fi.Name) + return nil + } + if excludeIDs.Has(id) { return nil } @@ -107,7 +113,7 @@ func ForAllSnapshots(ctx context.Context, repo Repository, excludeIDs IDSet, fn worker := func() error { for id := range ch { debug.Log("load snapshot %v", id) - sn, err := LoadSnapshot(ctx, repo, id) + sn, err := LoadSnapshot(ctx, loader, id) m.Lock() err = fn(id, sn, err) diff --git a/internal/restic/snapshot_find.go b/internal/restic/snapshot_find.go index 1f309f0bd..74275b552 100644 --- a/internal/restic/snapshot_find.go +++ b/internal/restic/snapshot_find.go @@ -14,7 +14,9 @@ import ( var ErrNoSnapshotFound = errors.New("no snapshot found") // FindLatestSnapshot finds latest snapshot with optional target/directory, tags, hostname, and timestamp filters. -func FindLatestSnapshot(ctx context.Context, repo Repository, targets []string, tagLists []TagList, hostnames []string, timeStampLimit *time.Time) (ID, error) { +func FindLatestSnapshot(ctx context.Context, be Lister, loader LoadJSONUnpackeder, targets []string, + tagLists []TagList, hostnames []string, timeStampLimit *time.Time) (ID, error) { + var err error absTargets := make([]string, 0, len(targets)) for _, target := range targets { @@ -33,7 +35,7 @@ func FindLatestSnapshot(ctx context.Context, repo Repository, targets []string, found bool ) - err = ForAllSnapshots(ctx, repo, nil, func(id ID, snapshot *Snapshot, err error) error { + err = ForAllSnapshots(ctx, be, loader, nil, func(id ID, snapshot *Snapshot, err error) error { if err != nil { return errors.Errorf("Error loading snapshot %v: %v", id.Str(), err) } @@ -77,10 +79,10 @@ func FindLatestSnapshot(ctx context.Context, repo Repository, targets []string, // FindSnapshot takes a string and tries to find a snapshot whose ID matches // the string as closely as possible. -func FindSnapshot(ctx context.Context, repo Repository, s string) (ID, error) { +func FindSnapshot(ctx context.Context, be Lister, s string) (ID, error) { // find snapshot id with prefix - name, err := Find(ctx, repo.Backend(), SnapshotFile, s) + name, err := Find(ctx, be, SnapshotFile, s) if err != nil { return ID{}, err } @@ -90,10 +92,10 @@ func FindSnapshot(ctx context.Context, repo Repository, s string) (ID, error) { // FindFilteredSnapshots yields Snapshots filtered from the list of all // snapshots. -func FindFilteredSnapshots(ctx context.Context, repo Repository, hosts []string, tags []TagList, paths []string) (Snapshots, error) { +func FindFilteredSnapshots(ctx context.Context, be Lister, loader LoadJSONUnpackeder, hosts []string, tags []TagList, paths []string) (Snapshots, error) { results := make(Snapshots, 0, 20) - err := ForAllSnapshots(ctx, repo, nil, func(id ID, sn *Snapshot, err error) error { + err := ForAllSnapshots(ctx, be, loader, nil, func(id ID, sn *Snapshot, err error) error { if err != nil { fmt.Fprintf(os.Stderr, "could not load snapshot %v: %v\n", id.Str(), err) return nil diff --git a/internal/restic/snapshot_find_test.go b/internal/restic/snapshot_find_test.go index d4fedcc9d..534eb456d 100644 --- a/internal/restic/snapshot_find_test.go +++ b/internal/restic/snapshot_find_test.go @@ -16,7 +16,7 @@ func TestFindLatestSnapshot(t *testing.T) { restic.TestCreateSnapshot(t, repo, parseTimeUTC("2017-07-07 07:07:07"), 1, 0) latestSnapshot := restic.TestCreateSnapshot(t, repo, parseTimeUTC("2019-09-09 09:09:09"), 1, 0) - id, err := restic.FindLatestSnapshot(context.TODO(), repo, []string{}, []restic.TagList{}, []string{"foo"}, nil) + id, err := restic.FindLatestSnapshot(context.TODO(), repo.Backend(), repo, []string{}, []restic.TagList{}, []string{"foo"}, nil) if err != nil { t.Fatalf("FindLatestSnapshot returned error: %v", err) } @@ -36,7 +36,7 @@ func TestFindLatestSnapshotWithMaxTimestamp(t *testing.T) { maxTimestamp := parseTimeUTC("2018-08-08 08:08:08") - id, err := restic.FindLatestSnapshot(context.TODO(), repo, []string{}, []restic.TagList{}, []string{"foo"}, &maxTimestamp) + id, err := restic.FindLatestSnapshot(context.TODO(), repo.Backend(), repo, []string{}, []restic.TagList{}, []string{"foo"}, &maxTimestamp) if err != nil { t.Fatalf("FindLatestSnapshot returned error: %v", err) } diff --git a/internal/restic/testing_test.go b/internal/restic/testing_test.go index 5607dc5fa..7ee7461a5 100644 --- a/internal/restic/testing_test.go +++ b/internal/restic/testing_test.go @@ -20,7 +20,7 @@ const ( // LoadAllSnapshots returns a list of all snapshots in the repo. // If a snapshot ID is in excludeIDs, it will not be included in the result. func loadAllSnapshots(ctx context.Context, repo restic.Repository, excludeIDs restic.IDSet) (snapshots restic.Snapshots, err error) { - err = restic.ForAllSnapshots(ctx, repo, excludeIDs, func(id restic.ID, sn *restic.Snapshot, err error) error { + err = restic.ForAllSnapshots(ctx, repo.Backend(), repo, excludeIDs, func(id restic.ID, sn *restic.Snapshot, err error) error { if err != nil { return err } From 9e121592307ecb0162d0a121f4d03ada6c4fb4e6 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 6 Nov 2021 01:23:12 +0100 Subject: [PATCH 3/8] Fix O(n) backend list calls in FindFilteredSnapshots When resolving snapshotIDs in FindFilteredSnapshots either FindLatestSnapshot or FindSnapshot is called. Both operations issue a list operation to the backend. When for example passing a long list of snapshot ids to `forget` this could lead to a large number of list operations. --- cmd/restic/find.go | 9 ++++++++- internal/backend/utils.go | 4 ++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/cmd/restic/find.go b/cmd/restic/find.go index a319d23de..f93f37278 100644 --- a/cmd/restic/find.go +++ b/cmd/restic/find.go @@ -3,6 +3,7 @@ package main import ( "context" + "github.com/restic/restic/internal/backend" "github.com/restic/restic/internal/restic" ) @@ -12,10 +13,16 @@ func FindFilteredSnapshots(ctx context.Context, be restic.Lister, loader restic. go func() { defer close(out) if len(snapshotIDs) != 0 { + // memorize snapshots list to prevent repeated backend listings + be, err := backend.MemorizeList(ctx, be, restic.SnapshotFile) + if err != nil { + Warnf("could not load snapshots: %v\n", err) + return + } + var ( id restic.ID usedFilter bool - err error ) ids := make(restic.IDs, 0, len(snapshotIDs)) // Process all snapshot IDs given as arguments. diff --git a/internal/backend/utils.go b/internal/backend/utils.go index 0dd081232..be1c2a9e0 100644 --- a/internal/backend/utils.go +++ b/internal/backend/utils.go @@ -81,6 +81,10 @@ func (m *memorizedLister) List(ctx context.Context, t restic.FileType, fn func(r } func MemorizeList(ctx context.Context, be restic.Lister, t restic.FileType) (restic.Lister, error) { + if _, ok := be.(*memorizedLister); ok { + return be, nil + } + var fileInfos []restic.FileInfo err := be.List(ctx, t, func(fi restic.FileInfo) error { fileInfos = append(fileInfos, fi) From 4636c203976dc42140d5629413251b3373a201e9 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 7 Nov 2021 20:26:33 +0100 Subject: [PATCH 4/8] test that TestFindListOnce calls List only once --- cmd/restic/integration_test.go | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/cmd/restic/integration_test.go b/cmd/restic/integration_test.go index f926306eb..4b935d2c8 100644 --- a/cmd/restic/integration_test.go +++ b/cmd/restic/integration_test.go @@ -2136,3 +2136,37 @@ func TestBackendLoadWriteTo(t *testing.T) { rtest.Assert(t, len(firstSnapshot) == 1, "expected one snapshot, got %v", firstSnapshot) } + +func TestFindListOnce(t *testing.T) { + env, cleanup := withTestEnvironment(t) + defer cleanup() + + env.gopts.backendTestHook = func(r restic.Backend) (restic.Backend, error) { + return newListOnceBackend(r), nil + } + + testSetupBackupData(t, env) + opts := BackupOptions{} + + testRunBackup(t, "", []string{filepath.Join(env.testdata, "0", "0", "9")}, opts, env.gopts) + testRunBackup(t, "", []string{filepath.Join(env.testdata, "0", "0", "9", "2")}, opts, env.gopts) + secondSnapshot := testRunList(t, "snapshots", env.gopts) + testRunBackup(t, "", []string{filepath.Join(env.testdata, "0", "0", "9", "3")}, opts, env.gopts) + thirdSnapshot := restic.NewIDSet(testRunList(t, "snapshots", env.gopts)...) + + repo, err := OpenRepository(env.gopts) + rtest.OK(t, err) + + snapshotIDs := restic.NewIDSet() + // specify the two oldest snapshots explicitly and use "latest" to reference the newest one + for sn := range FindFilteredSnapshots(context.TODO(), repo.Backend(), repo, nil, nil, nil, []string{ + secondSnapshot[0].String(), + secondSnapshot[1].String()[:8], + "latest", + }) { + snapshotIDs.Insert(*sn.ID()) + } + + // the snapshots can only be listed once, if both lists match then the there has been only a single List() call + rtest.Equals(t, thirdSnapshot, snapshotIDs) +} From 5af828e3e6849de0913d3f9bae2f2f4fc1c3aa03 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 7 Nov 2021 21:08:07 +0100 Subject: [PATCH 5/8] add changelogs --- changelog/unreleased/issue-3428 | 14 ++++++++++++++ changelog/unreleased/issue-3432 | 14 ++++++++++++++ 2 files changed, 28 insertions(+) create mode 100644 changelog/unreleased/issue-3428 create mode 100644 changelog/unreleased/issue-3432 diff --git a/changelog/unreleased/issue-3428 b/changelog/unreleased/issue-3428 new file mode 100644 index 000000000..6d2b055c8 --- /dev/null +++ b/changelog/unreleased/issue-3428 @@ -0,0 +1,14 @@ +Bugfix: List snapshots in backend at most once to resolve snapshot ids + +Many commands support specifying a list of snapshot ids which are then used to +determine the snapshot accessed by the command. To resolve snapshot ids or +"latest" and check that these exist, restic listed all snapshots stored in the +repository. Depending on the backend this can be a slow and/or expensive +operation. + +Restic now lists the snapshots only once and remembers the result to resolve +all further snapshot ids. + +https://github.com/restic/restic/issues/3428 +https://github.com/restic/restic/pull/3570 +https://github.com/restic/restic/pull/3395 diff --git a/changelog/unreleased/issue-3432 b/changelog/unreleased/issue-3432 new file mode 100644 index 000000000..b874e7da6 --- /dev/null +++ b/changelog/unreleased/issue-3432 @@ -0,0 +1,14 @@ +Bugfix: Fix rare 'not found in repository' error for copy command + +In rare cases copy (and other commands) could report that LoadTree(...) +returned a `id [...] not found in repository` error. This could be caused by a +backup or copy command running concurrently. The error is only temporary, +running the failed restic command a second time as a workaround solves the +error. + +This issue has been fixed by correcting the order in which restic reads data +from the repository. It is now guaranteed that restic only loads snapshots for +which all necessary data is already available. + +https://github.com/restic/restic/issues/3432 +https://github.com/restic/restic/pull/3570 From 47243176fa460cae6209bc446b580d4333992a4b Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 7 Nov 2021 21:26:05 +0100 Subject: [PATCH 6/8] diff: list snapshots only once --- cmd/restic/cmd_diff.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/cmd/restic/cmd_diff.go b/cmd/restic/cmd_diff.go index ae76e4071..8099bf296 100644 --- a/cmd/restic/cmd_diff.go +++ b/cmd/restic/cmd_diff.go @@ -7,9 +7,9 @@ import ( "reflect" "sort" + "github.com/restic/restic/internal/backend" "github.com/restic/restic/internal/debug" "github.com/restic/restic/internal/errors" - "github.com/restic/restic/internal/repository" "github.com/restic/restic/internal/restic" "github.com/spf13/cobra" ) @@ -53,8 +53,8 @@ func init() { f.BoolVar(&diffOptions.ShowMetadata, "metadata", false, "print changes in metadata") } -func loadSnapshot(ctx context.Context, repo *repository.Repository, desc string) (*restic.Snapshot, error) { - id, err := restic.FindSnapshot(ctx, repo.Backend(), desc) +func loadSnapshot(ctx context.Context, be restic.Lister, repo restic.Repository, desc string) (*restic.Snapshot, error) { + id, err := restic.FindSnapshot(ctx, be, desc) if err != nil { return nil, errors.Fatal(err.Error()) } @@ -342,12 +342,17 @@ func runDiff(opts DiffOptions, gopts GlobalOptions, args []string) error { } } - sn1, err := loadSnapshot(ctx, repo, args[0]) + // cache snapshots listing + be, err := backend.MemorizeList(ctx, repo.Backend(), restic.SnapshotFile) + if err != nil { + return err + } + sn1, err := loadSnapshot(ctx, be, repo, args[0]) if err != nil { return err } - sn2, err := loadSnapshot(ctx, repo, args[1]) + sn2, err := loadSnapshot(ctx, be, repo, args[1]) if err != nil { return err } From 7b9ae91e04f7d715a016df397f69c6d4444131f8 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 7 Nov 2021 22:33:44 +0100 Subject: [PATCH 7/8] copy: Load snapshots before indexes --- cmd/restic/cmd_check.go | 4 ++++ internal/checker/checker.go | 14 +++++++++++--- internal/checker/checker_test.go | 4 ++++ internal/checker/testing.go | 5 +++++ internal/repository/master_index_test.go | 5 +++++ 5 files changed, 29 insertions(+), 3 deletions(-) diff --git a/cmd/restic/cmd_check.go b/cmd/restic/cmd_check.go index fa71bdba8..e7edff39e 100644 --- a/cmd/restic/cmd_check.go +++ b/cmd/restic/cmd_check.go @@ -211,6 +211,10 @@ func runCheck(opts CheckOptions, gopts GlobalOptions, args []string) error { } chkr := checker.New(repo, opts.CheckUnused) + err = chkr.LoadSnapshots(gopts.ctx) + if err != nil { + return err + } Verbosef("load indexes\n") hints, errs := chkr.LoadIndex(gopts.ctx) diff --git a/internal/checker/checker.go b/internal/checker/checker.go index 93e58cedc..8e49209f5 100644 --- a/internal/checker/checker.go +++ b/internal/checker/checker.go @@ -11,6 +11,7 @@ import ( "sync" "github.com/minio/sha256-simd" + "github.com/restic/restic/internal/backend" "github.com/restic/restic/internal/debug" "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/hashing" @@ -35,6 +36,7 @@ type Checker struct { trackUnused bool masterIndex *repository.MasterIndex + snapshots restic.Lister repo restic.Repository } @@ -75,6 +77,12 @@ func (err ErrOldIndexFormat) Error() string { return fmt.Sprintf("index %v has old format", err.ID.Str()) } +func (c *Checker) LoadSnapshots(ctx context.Context) error { + var err error + c.snapshots, err = backend.MemorizeList(ctx, c.repo.Backend(), restic.SnapshotFile) + return err +} + // LoadIndex loads all index files. func (c *Checker) LoadIndex(ctx context.Context) (hints []error, errs []error) { debug.Log("Start") @@ -278,8 +286,8 @@ func (c *Checker) checkTreeWorker(ctx context.Context, trees <-chan restic.TreeI } } -func loadSnapshotTreeIDs(ctx context.Context, repo restic.Repository) (ids restic.IDs, errs []error) { - err := restic.ForAllSnapshots(ctx, repo.Backend(), repo, nil, func(id restic.ID, sn *restic.Snapshot, err error) error { +func loadSnapshotTreeIDs(ctx context.Context, lister restic.Lister, repo restic.Repository) (ids restic.IDs, errs []error) { + err := restic.ForAllSnapshots(ctx, lister, repo, nil, func(id restic.ID, sn *restic.Snapshot, err error) error { if err != nil { errs = append(errs, err) return nil @@ -300,7 +308,7 @@ func loadSnapshotTreeIDs(ctx context.Context, repo restic.Repository) (ids resti // subtrees are available in the index. errChan is closed after all trees have // been traversed. func (c *Checker) Structure(ctx context.Context, p *progress.Counter, errChan chan<- error) { - trees, errs := loadSnapshotTreeIDs(ctx, c.repo) + trees, errs := loadSnapshotTreeIDs(ctx, c.snapshots, c.repo) p.SetMax(uint64(len(trees))) debug.Log("need to check %d trees from snapshots, %d errs returned", len(trees), len(errs)) diff --git a/internal/checker/checker_test.go b/internal/checker/checker_test.go index 948d5a480..1330211eb 100644 --- a/internal/checker/checker_test.go +++ b/internal/checker/checker_test.go @@ -44,6 +44,10 @@ func checkPacks(chkr *checker.Checker) []error { } func checkStruct(chkr *checker.Checker) []error { + err := chkr.LoadSnapshots(context.TODO()) + if err != nil { + return []error{err} + } return collectErrors(context.TODO(), func(ctx context.Context, errChan chan<- error) { chkr.Structure(ctx, nil, errChan) }) diff --git a/internal/checker/testing.go b/internal/checker/testing.go index d672911b1..0668406d8 100644 --- a/internal/checker/testing.go +++ b/internal/checker/testing.go @@ -20,6 +20,11 @@ func TestCheckRepo(t testing.TB, repo restic.Repository) { t.Fatalf("errors loading index: %v", hints) } + err := chkr.LoadSnapshots(context.TODO()) + if err != nil { + t.Error(err) + } + // packs errChan := make(chan error) go chkr.Packs(context.TODO(), errChan) diff --git a/internal/repository/master_index_test.go b/internal/repository/master_index_test.go index f1fe9af7e..2470dadfc 100644 --- a/internal/repository/master_index_test.go +++ b/internal/repository/master_index_test.go @@ -369,6 +369,11 @@ func TestIndexSave(t *testing.T) { } checker := checker.New(repo, false) + err = checker.LoadSnapshots(context.TODO()) + if err != nil { + t.Error(err) + } + hints, errs := checker.LoadIndex(context.TODO()) for _, h := range hints { t.Logf("hint: %v\n", h) From ebab35581caed46e6dfddef646ba4dd354cddddb Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 7 Nov 2021 22:39:38 +0100 Subject: [PATCH 8/8] Check in integration test that snapshots are listed before the index As an exception prune is still allowed to load the index before snapshots, as it uses exclusive locks. In case of problems with locking it is also better to load snapshots created after loading the index, as this will lead to a prune sanity check failure instead of a broken snapshot. --- cmd/restic/integration_fuse_test.go | 4 ++++ cmd/restic/integration_helpers_test.go | 3 +++ cmd/restic/integration_test.go | 25 +++++++++++++++++++++++++ 3 files changed, 32 insertions(+) diff --git a/cmd/restic/integration_fuse_test.go b/cmd/restic/integration_fuse_test.go index 1337be88e..156a8abae 100644 --- a/cmd/restic/integration_fuse_test.go +++ b/cmd/restic/integration_fuse_test.go @@ -154,6 +154,8 @@ func TestMount(t *testing.T) { } env, cleanup := withTestEnvironment(t) + // must list snapshots more than once + env.gopts.backendTestHook = nil defer cleanup() testRunInit(t, env.gopts) @@ -197,6 +199,8 @@ func TestMountSameTimestamps(t *testing.T) { } env, cleanup := withTestEnvironment(t) + // must list snapshots more than once + env.gopts.backendTestHook = nil defer cleanup() rtest.SetupTarTestFixture(t, env.base, filepath.Join("testdata", "repo-same-timestamps.tar.gz")) diff --git a/cmd/restic/integration_helpers_test.go b/cmd/restic/integration_helpers_test.go index df9893350..e87baddca 100644 --- a/cmd/restic/integration_helpers_test.go +++ b/cmd/restic/integration_helpers_test.go @@ -198,6 +198,9 @@ func withTestEnvironment(t testing.TB) (env *testEnvironment, cleanup func()) { stdout: os.Stdout, stderr: os.Stderr, extended: make(options.Options), + + // replace this hook with "nil" if listing a filetype more than once is necessary + backendTestHook: func(r restic.Backend) (restic.Backend, error) { return newOrderedListOnceBackend(r), nil }, } // always overwrite global options diff --git a/cmd/restic/integration_test.go b/cmd/restic/integration_test.go index 4b935d2c8..49121bb1d 100644 --- a/cmd/restic/integration_test.go +++ b/cmd/restic/integration_test.go @@ -275,6 +275,11 @@ func testRunForgetJSON(t testing.TB, gopts GlobalOptions, args ...string) { } func testRunPrune(t testing.TB, gopts GlobalOptions, opts PruneOptions) { + oldHook := gopts.backendTestHook + gopts.backendTestHook = func(r restic.Backend) (restic.Backend, error) { return newListOnceBackend(r), nil } + defer func() { + gopts.backendTestHook = oldHook + }() rtest.OK(t, runPrune(opts, gopts)) } @@ -1065,6 +1070,8 @@ func TestKeyAddRemove(t *testing.T) { } env, cleanup := withTestEnvironment(t) + // must list keys more than once + env.gopts.backendTestHook = nil defer cleanup() testRunInit(t, env.gopts) @@ -1659,6 +1666,11 @@ func TestPruneWithDamagedRepository(t *testing.T) { rtest.Assert(t, len(snapshotIDs) == 1, "expected one snapshot, got %v", snapshotIDs) + oldHook := env.gopts.backendTestHook + env.gopts.backendTestHook = func(r restic.Backend) (restic.Backend, error) { return newListOnceBackend(r), nil } + defer func() { + env.gopts.backendTestHook = oldHook + }() // prune should fail rtest.Assert(t, runPrune(pruneDefaultOptions, env.gopts) == errorPacksMissing, "prune should have reported index not complete error") @@ -1752,12 +1764,22 @@ func testEdgeCaseRepo(t *testing.T, tarfile string, optionsCheck CheckOptions, o type listOnceBackend struct { restic.Backend listedFileType map[restic.FileType]bool + strictOrder bool } func newListOnceBackend(be restic.Backend) *listOnceBackend { return &listOnceBackend{ Backend: be, listedFileType: make(map[restic.FileType]bool), + strictOrder: false, + } +} + +func newOrderedListOnceBackend(be restic.Backend) *listOnceBackend { + return &listOnceBackend{ + Backend: be, + listedFileType: make(map[restic.FileType]bool), + strictOrder: true, } } @@ -1765,6 +1787,9 @@ func (be *listOnceBackend) List(ctx context.Context, t restic.FileType, fn func( if t != restic.LockFile && be.listedFileType[t] { return errors.Errorf("tried listing type %v the second time", t) } + if be.strictOrder && t == restic.SnapshotFile && be.listedFileType[restic.IndexFile] { + return errors.Errorf("tried listing type snapshots after index") + } be.listedFileType[t] = true return be.Backend.List(ctx, t, fn) }