From 514f024b7dbf6f36748f3325dd9c1ebd40984c8f Mon Sep 17 00:00:00 2001 From: davidby-influx <72418212+davidby-influx@users.noreply.github.com> Date: Thu, 13 Oct 2022 11:56:35 -0700 Subject: [PATCH] fix: use copy when a rename spans volumes (#23785) (#23792) When a file rename fails with EXDEV (cross device or volume error), copy the file and delete the original instead Differs from master branch by overwriting existing files instead of erring. closes https://github.com/influxdata/influxdb/issues/22997 (cherry picked from commit 0913276ff0f33d1639a14cb49c971772232b5af3) * fix: add tests for file rename across volumes (#23787) Also move shared code from file_unix.go (cherry picked from commit bc8d9ea9f33b964ada77a9057a603255f13eeb59) closes https://github.com/influxdata/influxdb/issues/23791 --- pkg/file/file.go | 44 ++++++++++++ pkg/file/file_test.go | 142 +++++++++++++++++++++++++++++++++++++++ pkg/file/file_unix.go | 25 ++++++- pkg/file/file_windows.go | 16 +++++ 4 files changed, 225 insertions(+), 2 deletions(-) create mode 100644 pkg/file/file.go create mode 100644 pkg/file/file_test.go diff --git a/pkg/file/file.go b/pkg/file/file.go new file mode 100644 index 00000000000..b19032a2191 --- /dev/null +++ b/pkg/file/file.go @@ -0,0 +1,44 @@ +package file + +import ( + "fmt" + "io" + "os" + + "github.com/influxdata/influxdb/pkg/errors" +) + +func copyFile(src, dst string) (err error) { + in, err := os.Open(src) + if err != nil { + return err + } + + out, err := os.Create(dst) + if err != nil { + return err + } + + defer errors.Capture(&err, out.Close)() + + defer errors.Capture(&err, in.Close)() + + if _, err = io.Copy(out, in); err != nil { + return err + } + + return out.Sync() +} + +// MoveFileWithReplacement copies the file contents at `src` to `dst`. +// and deletes `src` on success. +// +// If the file at `dst` already exists, it will be truncated and its contents +// overwritten. +func MoveFileWithReplacement(src, dst string) error { + if err := copyFile(src, dst); err != nil { + return fmt.Errorf("copy: %w", err) + } + + return os.Remove(src) +} diff --git a/pkg/file/file_test.go b/pkg/file/file_test.go new file mode 100644 index 00000000000..43b1cf01013 --- /dev/null +++ b/pkg/file/file_test.go @@ -0,0 +1,142 @@ +package file_test + +import ( + "io" + "os" + "path/filepath" + "testing" + + "github.com/influxdata/influxdb/pkg/file" +) + +func TestRenameFileWithReplacement(t *testing.T) { + testFileMoveOrRename(t, "rename", file.RenameFileWithReplacement) +} + +func TestMoveFileWithReplacement(t *testing.T) { + testFileMoveOrRename(t, "move", file.MoveFileWithReplacement) +} + +func testFileMoveOrRename(t *testing.T, name string, testFunc func(src string, dst string) error) { + // sample data for loading into files + sampleData1 := "this is some data" + sampleData2 := "we got some more data" + + t.Run("exists", func(t *testing.T) { + oldPath := MustCreateTempFile(t, sampleData1) + newPath := MustCreateTempFile(t, sampleData2) + defer MustRemoveAll(oldPath) + defer MustRemoveAll(newPath) + + oldContents := MustReadAllFile(oldPath) + newContents := MustReadAllFile(newPath) + + if got, exp := oldContents, sampleData1; got != exp { + t.Fatalf("got contents %q, expected %q", got, exp) + } else if got, exp := newContents, sampleData2; got != exp { + t.Fatalf("got contents %q, expected %q", got, exp) + } + + if err := testFunc(oldPath, newPath); err != nil { + t.Fatalf("%s returned an error: %s", name, err) + } + + if err := file.SyncDir(filepath.Dir(oldPath)); err != nil { + panic(err) + } + + // Contents of newpath will now be equivalent to oldpath' contents. + newContents = MustReadAllFile(newPath) + if newContents != oldContents { + t.Fatalf("contents for files differ: %q versus %q", newContents, oldContents) + } + + // oldpath will be removed. + if MustFileExists(oldPath) { + t.Fatalf("file %q still exists, but it shouldn't", oldPath) + } + }) + + t.Run("not exists", func(t *testing.T) { + oldpath := MustCreateTempFile(t, sampleData1) + defer MustRemoveAll(oldpath) + + oldContents := MustReadAllFile(oldpath) + if got, exp := oldContents, sampleData1; got != exp { + t.Fatalf("got contents %q, expected %q", got, exp) + } + + root := filepath.Dir(oldpath) + newpath := filepath.Join(root, "foo") + + if err := testFunc(oldpath, newpath); err != nil { + t.Fatalf("%s returned an error: %s", name, err) + } + + if err := file.SyncDir(filepath.Dir(oldpath)); err != nil { + panic(err) + } + + // Contents of newpath will now be equivalent to oldpath's contents. + newContents := MustReadAllFile(newpath) + if newContents != oldContents { + t.Fatalf("contents for files differ: %q versus %q", newContents, oldContents) + } + + // oldpath will be removed. + if MustFileExists(oldpath) { + t.Fatalf("file %q still exists, but it shouldn't", oldpath) + } + }) +} + +// CreateTempFileOrFail creates a temporary file returning the path to the file. +func MustCreateTempFile(t testing.TB, data string) string { + t.Helper() + + f, err := os.CreateTemp("", "fs-test") + if err != nil { + t.Fatalf("failed to create temp file: %v", err) + } else if _, err := f.WriteString(data); err != nil { + t.Fatal(err) + } else if err := f.Close(); err != nil { + t.Fatal(err) + } + return f.Name() +} + +func MustRemoveAll(path string) { + if err := os.RemoveAll(path); err != nil { + panic(err) + } +} + +// MustFileExists determines if a file exists, panicking if any error +// (other than one associated with the file not existing) is returned. +func MustFileExists(path string) bool { + _, err := os.Stat(path) + if err == nil { + return true + } else if os.IsNotExist(err) { + return false + } + panic(err) +} + +// MustReadAllFile reads the contents of path, panicking if there is an error. +func MustReadAllFile(path string) string { + fd, err := os.Open(path) + if err != nil { + panic(err) + } + defer func() { + if err = fd.Close(); err != nil { + panic(err) + } + }() + data, err := io.ReadAll(fd) + if err != nil { + panic(err) + } + return string(data) +} diff --git a/pkg/file/file_unix.go b/pkg/file/file_unix.go index 66609888e58..8735625e8b4 100644 --- a/pkg/file/file_unix.go +++ b/pkg/file/file_unix.go @@ -3,6 +3,7 @@ package file import ( + "errors" "os" "syscall" ) @@ -29,7 +30,27 @@ func SyncDir(dirName string) error { return dir.Close() } -// RenameFile will rename the source to target using os function. +// RenameFileWithReplacement will replace any existing file at newpath with the contents +// of oldpath. It works also if it the rename spans over several file systems. +// +// If no file already exists at newpath, newpath will be created using the contents +// of oldpath. If this function returns successfully, the contents of newpath will +// be identical to oldpath, and oldpath will be removed. +func RenameFileWithReplacement(oldpath, newpath string) error { + if err := os.Rename(oldpath, newpath); !errors.Is(err, syscall.EXDEV) { + // note: also includes err == nil + return err + } + + // move over filesystem boundaries, we have to copy. + // (if there was another error, it will likely fail a second time) + return MoveFileWithReplacement(oldpath, newpath) + +} + +// RenameFile renames oldpath to newpath, returning an error if newpath already +// exists. If this function returns successfully, the contents of newpath will +// be identical to oldpath, and oldpath will be removed. func RenameFile(oldpath, newpath string) error { - return os.Rename(oldpath, newpath) + return RenameFileWithReplacement(oldpath, newpath) } diff --git a/pkg/file/file_windows.go b/pkg/file/file_windows.go index 97f31b062f1..fc79fa538d9 100644 --- a/pkg/file/file_windows.go +++ b/pkg/file/file_windows.go @@ -16,3 +16,19 @@ func RenameFile(oldpath, newpath string) error { return os.Rename(oldpath, newpath) } + +// RenameFileWithReplacement will replace any existing file at newpath with the contents +// of oldpath. +// +// If no file already exists at newpath, newpath will be created using the contents +// of oldpath. If this function returns successfully, the contents of newpath will +// be identical to oldpath, and oldpath will be removed. +func RenameFileWithReplacement(oldpath, newpath string) error { + if _, err := os.Stat(newpath); err == nil { + if err = os.Remove(newpath); nil != err { + return err + } + } + + return os.Rename(oldpath, newpath) +}