diff --git a/config/localTemplate.go b/config/localTemplate.go index ce4294de01..67218f8ade 100644 --- a/config/localTemplate.go +++ b/config/localTemplate.go @@ -24,6 +24,7 @@ import ( "time" "github.com/algorand/go-algorand/protocol" + "github.com/algorand/go-algorand/util" "github.com/algorand/go-algorand/util/codecs" ) @@ -893,7 +894,7 @@ func moveDirIfExists(logger logger, srcdir, dstdir string, files ...string) erro // then, check if any files exist in srcdir, and move them to dstdir for _, file := range files { if _, err := os.Stat(filepath.Join(srcdir, file)); err == nil { - if err := os.Rename(filepath.Join(srcdir, file), filepath.Join(dstdir, file)); err != nil { + if err := util.MoveFile(filepath.Join(srcdir, file), filepath.Join(dstdir, file)); err != nil { return fmt.Errorf("failed to move file %s from %s to %s: %v", file, srcdir, dstdir, err) } logger.Infof("Moved DB file %s from ColdDataDir %s to HotDataDir %s", file, srcdir, dstdir) diff --git a/logging/cyclicWriter.go b/logging/cyclicWriter.go index d15782c93b..579fb4f881 100644 --- a/logging/cyclicWriter.go +++ b/logging/cyclicWriter.go @@ -26,6 +26,7 @@ import ( "text/template" "time" + "github.com/algorand/go-algorand/util" "github.com/algorand/go-deadlock" ) @@ -173,7 +174,7 @@ func (cyclic *CyclicFileWriter) Write(p []byte) (n int, err error) { shouldBz2 = true archivePath = archivePath[:len(archivePath)-4] } - if err = os.Rename(cyclic.liveLog, archivePath); err != nil { + if err = util.MoveFile(cyclic.liveLog, archivePath); err != nil { panic(fmt.Sprintf("CyclicFileWriter: cannot archive full log %v", err)) } if shouldGz { diff --git a/logging/cyclicWriter_test.go b/logging/cyclicWriter_test.go index 1d5de4bb1a..1149ae2098 100644 --- a/logging/cyclicWriter_test.go +++ b/logging/cyclicWriter_test.go @@ -18,16 +18,19 @@ package logging import ( "os" + "os/exec" + "path/filepath" + "runtime" + "strings" "testing" "github.com/algorand/go-algorand/test/partitiontest" "github.com/stretchr/testify/require" ) -func TestCyclicWrite(t *testing.T) { - partitiontest.PartitionTest(t) - liveFileName := "live.test" - archiveFileName := "archive.test" +func testCyclicWrite(t *testing.T, liveFileName, archiveFileName string) { + t.Helper() + defer os.Remove(liveFileName) defer os.Remove(archiveFileName) @@ -60,3 +63,46 @@ func TestCyclicWrite(t *testing.T) { require.Equal(t, byte('A'), oldData[i]) } } + +func TestCyclicWrite(t *testing.T) { + partitiontest.PartitionTest(t) + t.Parallel() + + tmpDir := t.TempDir() + + liveFileName := filepath.Join(tmpDir, "live.test") + archiveFileName := filepath.Join(tmpDir, "archive.test") + + testCyclicWrite(t, liveFileName, archiveFileName) +} + +func execCommand(t *testing.T, cmdAndArsg ...string) { + t.Helper() + + cmd := exec.Command(cmdAndArsg[0], cmdAndArsg[1:]...) + var errOutput strings.Builder + cmd.Stderr = &errOutput + err := cmd.Run() + require.NoError(t, err, errOutput.String()) +} + +func TestCyclicWriteAcrossFilesystems(t *testing.T) { + partitiontest.PartitionTest(t) + + isLinux := strings.HasPrefix(runtime.GOOS, "linux") + + // Skip unless CIRCLECI or TEST_MOUNT_TMPFS is set, and we are on a linux system + if !isLinux || (os.Getenv("CIRCLECI") == "" && os.Getenv("TEST_MOUNT_TMPFS") == "") { + t.Skip("This test must be run on a linux system with administrator privileges") + } + + mountDir := t.TempDir() + execCommand(t, "sudo", "mount", "-t", "tmpfs", "-o", "size=2K", "tmpfs", mountDir) + + defer execCommand(t, "sudo", "umount", mountDir) + + liveFileName := filepath.Join(t.TempDir(), "live.test") + archiveFileName := filepath.Join(mountDir, "archive.test") + + testCyclicWrite(t, liveFileName, archiveFileName) +} diff --git a/util/io.go b/util/io.go index f8290aec40..43e47d3f49 100644 --- a/util/io.go +++ b/util/io.go @@ -24,6 +24,71 @@ import ( "strings" ) +// MoveFile moves a file from src to dst. The advantages of using this over +// os.Rename() is that it can move files across different filesystems. +func MoveFile(src, dst string) error { + err := os.Rename(src, dst) + if err != nil { + // os.Rename() may have failed because src and dst are on different + // filesystems. Let's try to move the file by copying and deleting the + // source file. + return moveFileByCopying(src, dst) + } + return err +} + +func moveFileByCopying(src, dst string) error { + // Lstat is specifically used to detect if src is a symlink. We could + // support moving symlinks by deleting src and creating a new symlink at + // dst, but we don't currently expect to encounter that case, so it has not + // been implemented. + srcInfo, srcErr := os.Lstat(src) + if srcErr != nil { + return srcErr + } + if !srcInfo.Mode().IsRegular() { + return fmt.Errorf("cannot move source file '%s': it is not a regular file (%v)", src, srcInfo.Mode()) + } + + if dstInfo, dstErr := os.Lstat(dst); dstErr == nil { + if dstInfo.Mode().IsDir() { + return fmt.Errorf("cannot move source file '%s' to destination '%s': destination is a directory", src, dst) + } + if os.SameFile(dstInfo, srcInfo) { + return fmt.Errorf("cannot move source file '%s' to destination '%s': source and destination are the same file", src, dst) + } + } + + dstDir := filepath.Dir(dst) + dstBase := filepath.Base(dst) + + tmpDstFile, errTmp := os.CreateTemp(dstDir, dstBase+".tmp-") + if errTmp != nil { + return errTmp + } + tmpDst := tmpDstFile.Name() + if errClose := tmpDstFile.Close(); errClose != nil { + return errClose + } + + if _, err := CopyFile(src, tmpDst); err != nil { + // If the copy fails, try to clean up the temporary file + _ = os.Remove(tmpDst) + return err + } + if err := os.Rename(tmpDst, dst); err != nil { + // If the rename fails, try to clean up the temporary file + _ = os.Remove(tmpDst) + return err + } + if err := os.Remove(src); err != nil { + // Don't try to clean up the destination file here. Duplicate data is + // better than lost/incomplete data. + return fmt.Errorf("failed to remove source file '%s' after moving it to '%s': %w", src, dst, err) + } + return nil +} + // CopyFile uses io.Copy() to copy a file to another location // This was copied from https://opensource.com/article/18/6/copying-files-go func CopyFile(src, dst string) (int64, error) { diff --git a/util/io_test.go b/util/io_test.go index 6f9f6dcfdd..3c587286f7 100644 --- a/util/io_test.go +++ b/util/io_test.go @@ -17,25 +17,185 @@ package util import ( + "fmt" "os" - "path" + "os/exec" + "path/filepath" + "runtime" + "strings" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/algorand/go-algorand/test/partitiontest" ) func TestIsEmpty(t *testing.T) { partitiontest.PartitionTest(t) + t.Parallel() - testPath := path.Join(os.TempDir(), "this", "is", "a", "long", "path") + testPath := filepath.Join(t.TempDir(), "this", "is", "a", "long", "path") err := os.MkdirAll(testPath, os.ModePerm) assert.NoError(t, err) defer os.RemoveAll(testPath) assert.True(t, IsEmpty(testPath)) - _, err = os.Create(path.Join(testPath, "file.txt")) + _, err = os.Create(filepath.Join(testPath, "file.txt")) assert.NoError(t, err) assert.False(t, IsEmpty(testPath)) } + +func testMoveFileSimple(t *testing.T, src, dst string) { + t.Helper() + + require.NoFileExists(t, src) + require.NoFileExists(t, dst) + + defer os.Remove(src) + defer os.Remove(dst) + + f, err := os.Create(src) + require.NoError(t, err) + + _, err = f.WriteString("test file contents") + require.NoError(t, err) + require.NoError(t, f.Close()) + + err = MoveFile(src, dst) + require.NoError(t, err) + + require.FileExists(t, dst) + require.NoFileExists(t, src) + + dstContents, err := os.ReadFile(dst) + require.NoError(t, err) + assert.Equal(t, "test file contents", string(dstContents)) +} + +func TestMoveFile(t *testing.T) { + partitiontest.PartitionTest(t) + t.Parallel() + + tmpDir := t.TempDir() + + src := filepath.Join(tmpDir, "src.txt") + dst := filepath.Join(tmpDir, "dst.txt") + testMoveFileSimple(t, src, dst) +} + +func execCommand(t *testing.T, cmdAndArsg ...string) { + t.Helper() + + cmd := exec.Command(cmdAndArsg[0], cmdAndArsg[1:]...) + var errOutput strings.Builder + cmd.Stderr = &errOutput + err := cmd.Run() + require.NoError(t, err, errOutput.String()) +} + +func TestMoveFileAcrossFilesystems(t *testing.T) { + partitiontest.PartitionTest(t) + + isLinux := strings.HasPrefix(runtime.GOOS, "linux") + + // Skip unless CIRCLECI or TEST_MOUNT_TMPFS is set, and we are on a linux system + if !isLinux || (os.Getenv("CIRCLECI") == "" && os.Getenv("TEST_MOUNT_TMPFS") == "") { + t.Skip("This test must be run on a linux system with administrator privileges") + } + + mountDir := t.TempDir() + execCommand(t, "sudo", "mount", "-t", "tmpfs", "-o", "size=1K", "tmpfs", mountDir) + + defer execCommand(t, "sudo", "umount", mountDir) + + src := filepath.Join(t.TempDir(), "src.txt") + dst := filepath.Join(mountDir, "dst.txt") + + testMoveFileSimple(t, src, dst) +} + +func TestMoveFileSourceDoesNotExist(t *testing.T) { + partitiontest.PartitionTest(t) + t.Parallel() + + tmpDir := t.TempDir() + + src := filepath.Join(tmpDir, "src.txt") + dst := filepath.Join(tmpDir, "dst.txt") + + err := MoveFile(src, dst) + var pathError *os.PathError + require.ErrorAs(t, err, &pathError) + require.Equal(t, "lstat", pathError.Op) + require.Equal(t, src, pathError.Path) +} + +func TestMoveFileSourceIsASymlink(t *testing.T) { + partitiontest.PartitionTest(t) + t.Parallel() + + tmpDir := t.TempDir() + + root := filepath.Join(tmpDir, "root.txt") + src := filepath.Join(tmpDir, "src.txt") + dst := filepath.Join(tmpDir, "dst.txt") + + _, err := os.Create(root) + require.NoError(t, err) + + err = os.Symlink(root, src) + require.NoError(t, err) + + // os.Rename should work in this case + err = MoveFile(src, dst) + require.NoError(t, err) + + // Undo the move + require.NoError(t, MoveFile(dst, src)) + + // But our moveFileByCopying should fail, since we haven't implemented this case + err = moveFileByCopying(src, dst) + require.ErrorContains(t, err, fmt.Sprintf("cannot move source file '%s': it is not a regular file", src)) +} + +func TestMoveFileSourceAndDestinationAreSame(t *testing.T) { + partitiontest.PartitionTest(t) + t.Parallel() + + tmpDir := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(tmpDir, "folder"), os.ModePerm)) + + src := filepath.Join(tmpDir, "src.txt") + dst := src[:len(src)-len("src.txt")] + "folder/../src.txt" + + // dst refers to the same file as src, but with a different path + require.NotEqual(t, src, dst) + require.Equal(t, src, filepath.Clean(dst)) + + _, err := os.Create(src) + require.NoError(t, err) + + // os.Rename can handle this case, but our moveFileByCopying should fail + err = moveFileByCopying(src, dst) + require.ErrorContains(t, err, fmt.Sprintf("cannot move source file '%s' to destination '%s': source and destination are the same file", src, dst)) +} + +func TestMoveFileDestinationIsADirectory(t *testing.T) { + partitiontest.PartitionTest(t) + t.Parallel() + + tmpDir := t.TempDir() + + src := filepath.Join(tmpDir, "src.txt") + dst := filepath.Join(tmpDir, "dst.txt") + + _, err := os.Create(src) + require.NoError(t, err) + + err = os.Mkdir(dst, os.ModePerm) + require.NoError(t, err) + + err = MoveFile(src, dst) + require.ErrorContains(t, err, fmt.Sprintf("cannot move source file '%s' to destination '%s': destination is a directory", src, dst)) +}