From 4df2e33568426da1fe5991d4bb14949146a9a8d1 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 18 Oct 2024 22:26:18 +0200 Subject: [PATCH 1/7] archiver: properly create node for vss backups Previously, NodeFromFileInfo used the original file path to create the node, which also meant that extended metadata was read from there instead of within the vss snapshot. This change is a temporary solution for restic 0.17.2 and will be replaced with a clean fix in restic 0.18.0. --- internal/archiver/archiver.go | 3 ++- internal/fs/fs_local.go | 6 ++++++ internal/fs/fs_local_vss.go | 6 ++++++ internal/fs/fs_reader.go | 6 ++++++ internal/fs/interface.go | 1 + 5 files changed, 21 insertions(+), 1 deletion(-) diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go index 839320816..03b3b9986 100644 --- a/internal/archiver/archiver.go +++ b/internal/archiver/archiver.go @@ -248,7 +248,8 @@ func (arch *Archiver) trackItem(item string, previous, current *restic.Node, s I // nodeFromFileInfo returns the restic node from an os.FileInfo. func (arch *Archiver) nodeFromFileInfo(snPath, filename string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) { - node, err := restic.NodeFromFileInfo(filename, fi, ignoreXattrListError) + mappedFilename := arch.FS.MapFilename(filename) + node, err := restic.NodeFromFileInfo(mappedFilename, fi, ignoreXattrListError) if !arch.WithAtime { node.AccessTime = node.ModTime } diff --git a/internal/fs/fs_local.go b/internal/fs/fs_local.go index 48c40dc90..06dbae9a0 100644 --- a/internal/fs/fs_local.go +++ b/internal/fs/fs_local.go @@ -18,6 +18,12 @@ func (fs Local) VolumeName(path string) string { return filepath.VolumeName(path) } +// MapFilename is a temporary hack to prepare a filename for usage with +// NodeFromFileInfo. This is only relevant for LocalVss. +func (fs Local) MapFilename(filename string) string { + return filename +} + // Open opens a file for reading. func (fs Local) Open(name string) (File, error) { f, err := os.Open(fixpath(name)) diff --git a/internal/fs/fs_local_vss.go b/internal/fs/fs_local_vss.go index 718dfc46d..db6c95155 100644 --- a/internal/fs/fs_local_vss.go +++ b/internal/fs/fs_local_vss.go @@ -145,6 +145,12 @@ func (fs *LocalVss) Lstat(name string) (os.FileInfo, error) { return os.Lstat(fs.snapshotPath(name)) } +// MapFilename is a temporary hack to prepare a filename for usage with +// NodeFromFileInfo. This is only relevant for LocalVss. +func (fs *LocalVss) MapFilename(filename string) string { + return fs.snapshotPath(filename) +} + // isMountPointIncluded is true if given mountpoint included by user. func (fs *LocalVss) isMountPointIncluded(mountPoint string) bool { if fs.excludeVolumes == nil { diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index 47af74245..a39b4dad2 100644 --- a/internal/fs/fs_reader.go +++ b/internal/fs/fs_reader.go @@ -39,6 +39,12 @@ func (fs *Reader) VolumeName(_ string) string { return "" } +// MapFilename is a temporary hack to prepare a filename for usage with +// NodeFromFileInfo. This is only relevant for LocalVss. +func (fs *Reader) MapFilename(filename string) string { + return filename +} + // Open opens a file for reading. func (fs *Reader) Open(name string) (f File, err error) { switch name { diff --git a/internal/fs/interface.go b/internal/fs/interface.go index b26c56944..0fd84715d 100644 --- a/internal/fs/interface.go +++ b/internal/fs/interface.go @@ -11,6 +11,7 @@ type FS interface { OpenFile(name string, flag int, perm os.FileMode) (File, error) Stat(name string) (os.FileInfo, error) Lstat(name string) (os.FileInfo, error) + MapFilename(filename string) string Join(elem ...string) string Separator() string From 0c711f5605e42c12db5812bb47d51e1d7b57974e Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 18 Oct 2024 21:41:02 +0200 Subject: [PATCH 2/7] archiver: use correct filepath in fileSaver for vss When using the VSS FS, then `f.Name()` contained the filename in the snapshot. This caused a double mapping when calling NodeFromFileInfo. --- internal/archiver/file_saver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/archiver/file_saver.go b/internal/archiver/file_saver.go index d10334301..70666506d 100644 --- a/internal/archiver/file_saver.go +++ b/internal/archiver/file_saver.go @@ -156,7 +156,7 @@ func (s *FileSaver) saveFile(ctx context.Context, chnker *chunker.Chunker, snPat debug.Log("%v", snPath) - node, err := s.NodeFromFileInfo(snPath, f.Name(), fi, false) + node, err := s.NodeFromFileInfo(snPath, target, fi, false) if err != nil { _ = f.Close() completeError(err) From a7b13bd603eece2f8509dfb4fcfb74b9af398832 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 18 Oct 2024 21:43:23 +0200 Subject: [PATCH 3/7] fs: remove file.Name() from interface The only user was archiver.fileSaver. --- internal/fs/fs_reader.go | 6 +----- internal/fs/interface.go | 1 - 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index a39b4dad2..57864c87b 100644 --- a/internal/fs/fs_reader.go +++ b/internal/fs/fs_reader.go @@ -229,7 +229,7 @@ func (r *readerFile) Close() error { var _ File = &readerFile{} // fakeFile implements all File methods, but only returns errors for anything -// except Stat() and Name(). +// except Stat() type fakeFile struct { name string os.FileInfo @@ -266,10 +266,6 @@ func (f fakeFile) Stat() (os.FileInfo, error) { return f.FileInfo, nil } -func (f fakeFile) Name() string { - return f.name -} - // fakeDir implements Readdirnames and Readdir, everything else is delegated to fakeFile. type fakeDir struct { entries []os.FileInfo diff --git a/internal/fs/interface.go b/internal/fs/interface.go index 0fd84715d..147773e2d 100644 --- a/internal/fs/interface.go +++ b/internal/fs/interface.go @@ -34,5 +34,4 @@ type File interface { Readdir(int) ([]os.FileInfo, error) Seek(int64, int) (int64, error) Stat() (os.FileInfo, error) - Name() string } From 1f5791222a3ab3833b541e93f069170146340141 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 18 Oct 2024 20:05:32 +0200 Subject: [PATCH 4/7] backup: test that vss backups work if underlying data was removed --- cmd/restic/cmd_backup.go | 5 +++ cmd/restic/cmd_backup_integration_test.go | 47 +++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/cmd/restic/cmd_backup.go b/cmd/restic/cmd_backup.go index 562108a33..c7c0bcc50 100644 --- a/cmd/restic/cmd_backup.go +++ b/cmd/restic/cmd_backup.go @@ -95,6 +95,7 @@ type BackupOptions struct { } var backupOptions BackupOptions +var backupFSTestHook func(fs fs.FS) fs.FS // ErrInvalidSourceData is used to report an incomplete backup var ErrInvalidSourceData = errors.New("at least one source file could not be read") @@ -598,6 +599,10 @@ func runBackup(ctx context.Context, opts BackupOptions, gopts GlobalOptions, ter targets = []string{filename} } + if backupFSTestHook != nil { + targetFS = backupFSTestHook(targetFS) + } + wg, wgCtx := errgroup.WithContext(ctx) cancelCtx, cancel := context.WithCancel(wgCtx) defer cancel() diff --git a/cmd/restic/cmd_backup_integration_test.go b/cmd/restic/cmd_backup_integration_test.go index 5e00b84b0..cc6a2ca22 100644 --- a/cmd/restic/cmd_backup_integration_test.go +++ b/cmd/restic/cmd_backup_integration_test.go @@ -111,6 +111,53 @@ func TestBackupWithRelativePath(t *testing.T) { rtest.Assert(t, latestSn.Parent != nil && latestSn.Parent.Equal(firstSnapshotID), "second snapshot selected unexpected parent %v instead of %v", latestSn.Parent, firstSnapshotID) } +type vssDeleteOriginalFS struct { + fs.FS + testdata string + hasRemoved bool +} + +func (f *vssDeleteOriginalFS) Lstat(name string) (os.FileInfo, error) { + if !f.hasRemoved { + // call Lstat to trigger snapshot creation + _, _ = f.FS.Lstat(name) + // nuke testdata + if err := os.RemoveAll(f.testdata); err != nil { + return nil, err + } + f.hasRemoved = true + } + return f.FS.Lstat(name) +} + +func TestBackupVSS(t *testing.T) { + if runtime.GOOS != "windows" || fs.HasSufficientPrivilegesForVSS() != nil { + t.Skip("vss fs test can only be run on windows with admin privileges") + } + + env, cleanup := withTestEnvironment(t) + defer cleanup() + + testSetupBackupData(t, env) + opts := BackupOptions{UseFsSnapshot: true} + + var testFS *vssDeleteOriginalFS + backupFSTestHook = func(fs fs.FS) fs.FS { + testFS = &vssDeleteOriginalFS{ + FS: fs, + testdata: env.testdata, + } + return testFS + } + defer func() { + backupFSTestHook = nil + }() + + testRunBackup(t, filepath.Dir(env.testdata), []string{"testdata"}, opts, env.gopts) + testListSnapshots(t, env.gopts, 1) + rtest.Equals(t, true, testFS.hasRemoved, "testdata was not removed") +} + func TestBackupParentSelection(t *testing.T) { env, cleanup := withTestEnvironment(t) defer cleanup() From 841f8bfef025dff55443664ea744a740bf4a14df Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 18 Oct 2024 21:18:22 +0200 Subject: [PATCH 5/7] redirect test log output to t.Log() --- cmd/restic/integration_helpers_test.go | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/cmd/restic/integration_helpers_test.go b/cmd/restic/integration_helpers_test.go index 978deab3d..8ae3bb78a 100644 --- a/cmd/restic/integration_helpers_test.go +++ b/cmd/restic/integration_helpers_test.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "sync" "testing" @@ -168,6 +169,16 @@ type testEnvironment struct { gopts GlobalOptions } +type logOutputter struct { + t testing.TB +} + +func (l *logOutputter) Write(p []byte) (n int, err error) { + l.t.Helper() + l.t.Log(strings.TrimSuffix(string(p), "\n")) + return len(p), nil +} + // withTestEnvironment creates a test environment and returns a cleanup // function which removes it. func withTestEnvironment(t testing.TB) (env *testEnvironment, cleanup func()) { @@ -200,8 +211,11 @@ func withTestEnvironment(t testing.TB) (env *testEnvironment, cleanup func()) { Quiet: true, CacheDir: env.cache, password: rtest.TestPassword, - stdout: os.Stdout, - stderr: os.Stderr, + // stdout and stderr are written to by Warnf etc. That is the written data + // usually consists of one or multiple lines and therefore can be handled well + // by t.Log. + stdout: &logOutputter{t}, + stderr: &logOutputter{t}, extended: make(options.Options), // replace this hook with "nil" if listing a filetype more than once is necessary From 46dce1f4faad5f5801b32a0864fc6a3c140f8b60 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 18 Oct 2024 22:08:10 +0200 Subject: [PATCH 6/7] backup: work around file deletion error in test --- cmd/restic/cmd_backup_integration_test.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/cmd/restic/cmd_backup_integration_test.go b/cmd/restic/cmd_backup_integration_test.go index cc6a2ca22..5926fdd54 100644 --- a/cmd/restic/cmd_backup_integration_test.go +++ b/cmd/restic/cmd_backup_integration_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "runtime" "testing" + "time" "github.com/restic/restic/internal/fs" "github.com/restic/restic/internal/restic" @@ -122,7 +123,17 @@ func (f *vssDeleteOriginalFS) Lstat(name string) (os.FileInfo, error) { // call Lstat to trigger snapshot creation _, _ = f.FS.Lstat(name) // nuke testdata - if err := os.RemoveAll(f.testdata); err != nil { + var err error + for i := 0; i < 3; i++ { + // The CI sometimes runs into "The process cannot access the file because it is being used by another process" errors + // thus try a few times to remove the data + err = os.RemoveAll(f.testdata) + if err == nil { + break + } + time.Sleep(10 * time.Millisecond) + } + if err != nil { return nil, err } f.hasRemoved = true From ee9a5cdf70fdc787ae764abec74efadf59fc318d Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 18 Oct 2024 22:36:03 +0200 Subject: [PATCH 7/7] add vss metadata changelog --- changelog/unreleased/issue-5063 | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 changelog/unreleased/issue-5063 diff --git a/changelog/unreleased/issue-5063 b/changelog/unreleased/issue-5063 new file mode 100644 index 000000000..65aa379e4 --- /dev/null +++ b/changelog/unreleased/issue-5063 @@ -0,0 +1,11 @@ +Bugfix: Correctly `backup` extended metadata when using VSS on Windows + +On Windows, when creating a backup using the `--use-fs-snapshot` option, +then the extended metadata was not read from the filesystem snapshot. This +could result in errors when files have been removed in the meantime. + +This issue has been resolved. + +https://github.com/restic/restic/issues/5063 +https://github.com/restic/restic/pull/5097 +https://github.com/restic/restic/pull/5099