From 23b8b2f88896f1abaf71108b581931b1c7388603 Mon Sep 17 00:00:00 2001 From: Jason Paulos Date: Tue, 16 Apr 2024 11:53:23 -0400 Subject: [PATCH 1/8] Initial helper function --- util/io.go | 24 +++++++++++++++++++++ util/io_test.go | 57 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+) diff --git a/util/io.go b/util/io.go index f8290aec40..74078e0847 100644 --- a/util/io.go +++ b/util/io.go @@ -17,13 +17,37 @@ package util import ( + "errors" "fmt" "io" "os" "path/filepath" "strings" + "syscall" ) +func MoveFile(src, dst string) error { + err := os.Rename(src, dst) + var errno syscall.Errno + if err != nil && errors.As(err, &errno) && errno == syscall.EXDEV { + // os.Rename() failed because src and dst are on different filesystems + // Must manually copy and delete the original file + return manuallyMoveFile(src, dst) + } + return err +} + +func manuallyMoveFile(src, dst string) error { + tmpDst := dst + ".tmp" + if _, err := CopyFile(src, tmpDst); err != nil { + return err + } + if err := os.Rename(tmpDst, dst); err != nil { + return err + } + return os.Remove(src) +} + // 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..a23e35fae6 100644 --- a/util/io_test.go +++ b/util/io_test.go @@ -18,16 +18,19 @@ package util import ( "os" + "os/exec" "path" "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") err := os.MkdirAll(testPath, os.ModePerm) @@ -39,3 +42,57 @@ func TestIsEmpty(t *testing.T) { assert.NoError(t, err) assert.False(t, IsEmpty(testPath)) } + +func testMoveFile(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() + + src := path.Join(os.TempDir(), "src.txt") + dst := path.Join(os.TempDir(), "dst.txt") + testMoveFile(t, src, dst) +} + +func TestMoveFileAcrossFilesystems(t *testing.T) { + partitiontest.PartitionTest(t) + + t.Skip("This is a manual test that must be run on a linux system") + + os.Mkdir("/mnt/tmpfs", os.ModePerm) + defer os.Remove("/mnt/tmpfs") + + err := exec.Command("mount", "-t", "tmpfs", "-o", "size=1K", "tmpfs", "/mnt/tmpfs").Run() + require.NoError(t, err) + defer exec.Command("umount", "/mnt/tmpfs").Run() + + src := path.Join(os.TempDir(), "src.txt") + dst := path.Join("/mnt/tmpfs", "dst.txt") + + testMoveFile(t, src, dst) +} From 4c677650985eb4924fdacb7dcc85df3a1a71805f Mon Sep 17 00:00:00 2001 From: Jason Paulos Date: Tue, 16 Apr 2024 12:12:40 -0400 Subject: [PATCH 2/8] Adopt in cyclicWriter --- logging/cyclicWriter.go | 3 ++- logging/cyclicWriter_test.go | 39 ++++++++++++++++++++++++++++++++---- util/io.go | 2 ++ util/io_test.go | 12 ++++++----- 4 files changed, 46 insertions(+), 10 deletions(-) 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..df8daf0eb2 100644 --- a/logging/cyclicWriter_test.go +++ b/logging/cyclicWriter_test.go @@ -18,16 +18,17 @@ package logging import ( "os" + "os/exec" + "path" "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 +61,33 @@ 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 := path.Join(tmpDir, "live.test") + archiveFileName := path.Join(tmpDir, "archive.test") + + testCyclicWrite(t, liveFileName, archiveFileName) +} + +func TestCyclicWriteAcrossFilesystems(t *testing.T) { + partitiontest.PartitionTest(t) + + t.Skip("This is a manual test that must be run on a linux system") + + os.Mkdir("/mnt/tmpfs", os.ModePerm) + defer os.Remove("/mnt/tmpfs") + + err := exec.Command("mount", "-t", "tmpfs", "-o", "size=2K", "tmpfs", "/mnt/tmpfs").Run() + require.NoError(t, err) + defer exec.Command("umount", "/mnt/tmpfs").Run() + + liveFileName := path.Join(t.TempDir(), "live.test") + archiveFileName := "/mnt/tmpfs/archive.test" + + testCyclicWrite(t, liveFileName, archiveFileName) +} diff --git a/util/io.go b/util/io.go index 74078e0847..ab35272f9a 100644 --- a/util/io.go +++ b/util/io.go @@ -26,6 +26,8 @@ import ( "syscall" ) +// 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) var errno syscall.Errno diff --git a/util/io_test.go b/util/io_test.go index a23e35fae6..6020e278df 100644 --- a/util/io_test.go +++ b/util/io_test.go @@ -32,7 +32,7 @@ func TestIsEmpty(t *testing.T) { partitiontest.PartitionTest(t) t.Parallel() - testPath := path.Join(os.TempDir(), "this", "is", "a", "long", "path") + testPath := path.Join(t.TempDir(), "this", "is", "a", "long", "path") err := os.MkdirAll(testPath, os.ModePerm) assert.NoError(t, err) defer os.RemoveAll(testPath) @@ -74,8 +74,10 @@ func TestMoveFile(t *testing.T) { partitiontest.PartitionTest(t) t.Parallel() - src := path.Join(os.TempDir(), "src.txt") - dst := path.Join(os.TempDir(), "dst.txt") + tmpDir := t.TempDir() + + src := path.Join(tmpDir, "src.txt") + dst := path.Join(tmpDir, "dst.txt") testMoveFile(t, src, dst) } @@ -91,8 +93,8 @@ func TestMoveFileAcrossFilesystems(t *testing.T) { require.NoError(t, err) defer exec.Command("umount", "/mnt/tmpfs").Run() - src := path.Join(os.TempDir(), "src.txt") - dst := path.Join("/mnt/tmpfs", "dst.txt") + src := path.Join(t.TempDir(), "src.txt") + dst := "/mnt/tmpfs/dst.txt" testMoveFile(t, src, dst) } From d21751413132565c8f8c98cb91f5afe8a7eaee38 Mon Sep 17 00:00:00 2001 From: Jason Paulos Date: Tue, 16 Apr 2024 13:15:50 -0400 Subject: [PATCH 3/8] Attempt to copy and delete after any rename failure --- logging/cyclicWriter_test.go | 8 +++--- util/io.go | 54 +++++++++++++++++++++++++++++------- util/io_test.go | 18 ++++++------ 3 files changed, 57 insertions(+), 23 deletions(-) diff --git a/logging/cyclicWriter_test.go b/logging/cyclicWriter_test.go index df8daf0eb2..848ad1748e 100644 --- a/logging/cyclicWriter_test.go +++ b/logging/cyclicWriter_test.go @@ -19,7 +19,7 @@ package logging import ( "os" "os/exec" - "path" + "path/filepath" "testing" "github.com/algorand/go-algorand/test/partitiontest" @@ -68,8 +68,8 @@ func TestCyclicWrite(t *testing.T) { tmpDir := t.TempDir() - liveFileName := path.Join(tmpDir, "live.test") - archiveFileName := path.Join(tmpDir, "archive.test") + liveFileName := filepath.Join(tmpDir, "live.test") + archiveFileName := filepath.Join(tmpDir, "archive.test") testCyclicWrite(t, liveFileName, archiveFileName) } @@ -86,7 +86,7 @@ func TestCyclicWriteAcrossFilesystems(t *testing.T) { require.NoError(t, err) defer exec.Command("umount", "/mnt/tmpfs").Run() - liveFileName := path.Join(t.TempDir(), "live.test") + liveFileName := filepath.Join(t.TempDir(), "live.test") archiveFileName := "/mnt/tmpfs/archive.test" testCyclicWrite(t, liveFileName, archiveFileName) diff --git a/util/io.go b/util/io.go index ab35272f9a..3e87f9f7ac 100644 --- a/util/io.go +++ b/util/io.go @@ -17,37 +17,71 @@ package util import ( - "errors" "fmt" "io" "os" "path/filepath" "strings" - "syscall" ) // 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) - var errno syscall.Errno - if err != nil && errors.As(err, &errno) && errno == syscall.EXDEV { - // os.Rename() failed because src and dst are on different filesystems - // Must manually copy and delete the original file - return manuallyMoveFile(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 manuallyMoveFile(src, dst string) error { - tmpDst := dst + ".tmp" +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 && dstInfo.Mode().IsDir() { + return fmt.Errorf("cannot move source file '%s' to destination '%s': destination is a directory", 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 } - return os.Remove(src) + 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 diff --git a/util/io_test.go b/util/io_test.go index 6020e278df..addc9da832 100644 --- a/util/io_test.go +++ b/util/io_test.go @@ -19,7 +19,7 @@ package util import ( "os" "os/exec" - "path" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -32,18 +32,18 @@ func TestIsEmpty(t *testing.T) { partitiontest.PartitionTest(t) t.Parallel() - testPath := path.Join(t.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 testMoveFile(t *testing.T, src, dst string) { +func testMoveFileSimple(t *testing.T, src, dst string) { t.Helper() require.NoFileExists(t, src) @@ -76,9 +76,9 @@ func TestMoveFile(t *testing.T) { tmpDir := t.TempDir() - src := path.Join(tmpDir, "src.txt") - dst := path.Join(tmpDir, "dst.txt") - testMoveFile(t, src, dst) + src := filepath.Join(tmpDir, "src.txt") + dst := filepath.Join(tmpDir, "dst.txt") + testMoveFileSimple(t, src, dst) } func TestMoveFileAcrossFilesystems(t *testing.T) { @@ -93,8 +93,8 @@ func TestMoveFileAcrossFilesystems(t *testing.T) { require.NoError(t, err) defer exec.Command("umount", "/mnt/tmpfs").Run() - src := path.Join(t.TempDir(), "src.txt") + src := filepath.Join(t.TempDir(), "src.txt") dst := "/mnt/tmpfs/dst.txt" - testMoveFile(t, src, dst) + testMoveFileSimple(t, src, dst) } From 299869729d268c73a92a41022b5d6a3653936bd5 Mon Sep 17 00:00:00 2001 From: Jason Paulos Date: Tue, 16 Apr 2024 13:31:12 -0400 Subject: [PATCH 4/8] More testing --- util/io_test.go | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/util/io_test.go b/util/io_test.go index addc9da832..11acb3da38 100644 --- a/util/io_test.go +++ b/util/io_test.go @@ -17,6 +17,7 @@ package util import ( + "fmt" "os" "os/exec" "path/filepath" @@ -98,3 +99,66 @@ func TestMoveFileAcrossFilesystems(t *testing.T) { 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 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)) +} From b58f70a2e5306e7d78389ecd9cdcb7e185bf9c84 Mon Sep 17 00:00:00 2001 From: Jason Paulos Date: Tue, 16 Apr 2024 13:34:11 -0400 Subject: [PATCH 5/8] Apply to config db moving as well --- config/localTemplate.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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) From 6c1025376c88732c176cfb7a5cc1f54911940e62 Mon Sep 17 00:00:00 2001 From: Jason Paulos Date: Wed, 17 Apr 2024 10:40:02 -0400 Subject: [PATCH 6/8] error if source and destination are same file --- util/io.go | 9 +++++++-- util/io_test.go | 22 ++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/util/io.go b/util/io.go index 3e87f9f7ac..43e47d3f49 100644 --- a/util/io.go +++ b/util/io.go @@ -50,8 +50,13 @@ func moveFileByCopying(src, dst string) error { 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 && dstInfo.Mode().IsDir() { - return fmt.Errorf("cannot move source file '%s' to destination '%s': destination is a directory", src, dst) + 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) diff --git a/util/io_test.go b/util/io_test.go index 11acb3da38..d1c1f707f3 100644 --- a/util/io_test.go +++ b/util/io_test.go @@ -144,6 +144,28 @@ func TestMoveFileSourceIsASymlink(t *testing.T) { 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() From cb549850bad961c41e8e4053267e8f69dafe1cbd Mon Sep 17 00:00:00 2001 From: ohill <145173879+ohill@users.noreply.github.com> Date: Wed, 17 Apr 2024 11:38:03 -0400 Subject: [PATCH 7/8] Add CIRCLECI checks for mount/umount tests --- logging/cyclicWriter_test.go | 18 +++++++++++++++--- util/io_test.go | 18 +++++++++++++++--- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/logging/cyclicWriter_test.go b/logging/cyclicWriter_test.go index 848ad1748e..d1c9fb0dc0 100644 --- a/logging/cyclicWriter_test.go +++ b/logging/cyclicWriter_test.go @@ -77,14 +77,26 @@ func TestCyclicWrite(t *testing.T) { func TestCyclicWriteAcrossFilesystems(t *testing.T) { partitiontest.PartitionTest(t) - t.Skip("This is a manual test that must be run on a linux system") + // Skip unless CIRCLECI or TEST_MOUNT_TMPFS is set + if os.Getenv("CIRCLECI") == "" && os.Getenv("TEST_MOUNT_TMPFS") == "" { + t.Skip("This is a manual test that must be run on a linux system") + } os.Mkdir("/mnt/tmpfs", os.ModePerm) defer os.Remove("/mnt/tmpfs") - err := exec.Command("mount", "-t", "tmpfs", "-o", "size=2K", "tmpfs", "/mnt/tmpfs").Run() + mountCmd := []string{"mount", "-t", "tmpfs", "-o", "size=2K", "tmpfs", "/mnt/tmpfs"} + if os.Getenv("CIRCLECI") != "" { // Use sudo on CircleCI + mountCmd = append([]string{"sudo"}, mountCmd...) + } + err := exec.Command(mountCmd[0], mountCmd[1:]...).Run() require.NoError(t, err) - defer exec.Command("umount", "/mnt/tmpfs").Run() + + umountCmd := []string{"umount", "/mnt/tmpfs"} + if os.Getenv("CIRCLECI") != "" { + umountCmd = append([]string{"sudo"}, umountCmd...) + } + defer exec.Command(umountCmd[0], umountCmd[1:]...).Run() liveFileName := filepath.Join(t.TempDir(), "live.test") archiveFileName := "/mnt/tmpfs/archive.test" diff --git a/util/io_test.go b/util/io_test.go index d1c1f707f3..14357450b8 100644 --- a/util/io_test.go +++ b/util/io_test.go @@ -85,14 +85,26 @@ func TestMoveFile(t *testing.T) { func TestMoveFileAcrossFilesystems(t *testing.T) { partitiontest.PartitionTest(t) - t.Skip("This is a manual test that must be run on a linux system") + // Skip unless CIRCLECI or TEST_MOUNT_TMPFS is set + if os.Getenv("CIRCLECI") == "" && os.Getenv("TEST_MOUNT_TMPFS") == "" { + t.Skip("This is a manual test that must be run on a linux system") + } os.Mkdir("/mnt/tmpfs", os.ModePerm) defer os.Remove("/mnt/tmpfs") - err := exec.Command("mount", "-t", "tmpfs", "-o", "size=1K", "tmpfs", "/mnt/tmpfs").Run() + mountCmd := []string{"mount", "-t", "tmpfs", "-o", "size=1K", "tmpfs", "/mnt/tmpfs"} + if os.Getenv("CIRCLECI") != "" { // Use sudo on CircleCI + mountCmd = append([]string{"sudo"}, mountCmd...) + } + err := exec.Command(mountCmd[0], mountCmd[1:]...).Run() require.NoError(t, err) - defer exec.Command("umount", "/mnt/tmpfs").Run() + + umountCmd := []string{"umount", "/mnt/tmpfs"} + if os.Getenv("CIRCLECI") != "" { + umountCmd = append([]string{"sudo"}, umountCmd...) + } + defer exec.Command(umountCmd[0], umountCmd[1:]...).Run() src := filepath.Join(t.TempDir(), "src.txt") dst := "/mnt/tmpfs/dst.txt" From eab29c043cbceac58b92ca55410f88670a0e2958 Mon Sep 17 00:00:00 2001 From: Jason Paulos Date: Wed, 17 Apr 2024 12:20:20 -0400 Subject: [PATCH 8/8] Fix mounting tests --- logging/cyclicWriter_test.go | 39 +++++++++++++++++++----------------- util/io_test.go | 39 +++++++++++++++++++----------------- 2 files changed, 42 insertions(+), 36 deletions(-) diff --git a/logging/cyclicWriter_test.go b/logging/cyclicWriter_test.go index d1c9fb0dc0..1149ae2098 100644 --- a/logging/cyclicWriter_test.go +++ b/logging/cyclicWriter_test.go @@ -20,6 +20,8 @@ import ( "os" "os/exec" "path/filepath" + "runtime" + "strings" "testing" "github.com/algorand/go-algorand/test/partitiontest" @@ -74,32 +76,33 @@ func TestCyclicWrite(t *testing.T) { 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) - // Skip unless CIRCLECI or TEST_MOUNT_TMPFS is set - if os.Getenv("CIRCLECI") == "" && os.Getenv("TEST_MOUNT_TMPFS") == "" { - t.Skip("This is a manual test that must be run on a linux system") - } + isLinux := strings.HasPrefix(runtime.GOOS, "linux") - os.Mkdir("/mnt/tmpfs", os.ModePerm) - defer os.Remove("/mnt/tmpfs") - - mountCmd := []string{"mount", "-t", "tmpfs", "-o", "size=2K", "tmpfs", "/mnt/tmpfs"} - if os.Getenv("CIRCLECI") != "" { // Use sudo on CircleCI - mountCmd = append([]string{"sudo"}, mountCmd...) + // 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") } - err := exec.Command(mountCmd[0], mountCmd[1:]...).Run() - require.NoError(t, err) - umountCmd := []string{"umount", "/mnt/tmpfs"} - if os.Getenv("CIRCLECI") != "" { - umountCmd = append([]string{"sudo"}, umountCmd...) - } - defer exec.Command(umountCmd[0], umountCmd[1:]...).Run() + 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 := "/mnt/tmpfs/archive.test" + archiveFileName := filepath.Join(mountDir, "archive.test") testCyclicWrite(t, liveFileName, archiveFileName) } diff --git a/util/io_test.go b/util/io_test.go index 14357450b8..3c587286f7 100644 --- a/util/io_test.go +++ b/util/io_test.go @@ -21,6 +21,8 @@ import ( "os" "os/exec" "path/filepath" + "runtime" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -82,32 +84,33 @@ func TestMoveFile(t *testing.T) { 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) - // Skip unless CIRCLECI or TEST_MOUNT_TMPFS is set - if os.Getenv("CIRCLECI") == "" && os.Getenv("TEST_MOUNT_TMPFS") == "" { - t.Skip("This is a manual test that must be run on a linux system") - } + isLinux := strings.HasPrefix(runtime.GOOS, "linux") - os.Mkdir("/mnt/tmpfs", os.ModePerm) - defer os.Remove("/mnt/tmpfs") - - mountCmd := []string{"mount", "-t", "tmpfs", "-o", "size=1K", "tmpfs", "/mnt/tmpfs"} - if os.Getenv("CIRCLECI") != "" { // Use sudo on CircleCI - mountCmd = append([]string{"sudo"}, mountCmd...) + // 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") } - err := exec.Command(mountCmd[0], mountCmd[1:]...).Run() - require.NoError(t, err) - umountCmd := []string{"umount", "/mnt/tmpfs"} - if os.Getenv("CIRCLECI") != "" { - umountCmd = append([]string{"sudo"}, umountCmd...) - } - defer exec.Command(umountCmd[0], umountCmd[1:]...).Run() + 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 := "/mnt/tmpfs/dst.txt" + dst := filepath.Join(mountDir, "dst.txt") testMoveFileSimple(t, src, dst) }