Merge pull request #14280 from influxdata/er-rename

feat(fs): API for replacing os calls

chronograf/etc/build.py

@@ -11,6 +11,7 @@
 import logging
 import argparse
 import json
+import fs
 
 ################
 #### Chronograf Variables

chronograf/filestore/apps.go

@@ -9,6 +9,7 @@ import (
 	"path"
 
 	"github.com/influxdata/influxdb/chronograf"
+	"github.com/influxdata/influxdb/pkg/fs"
 )
 
 // AppExt is the the file extension searched for in the directory for layout files
@@ -58,7 +59,7 @@ func loadFile(name string) (chronograf.Layout, error) {
 }
 
 func createLayout(file string, layout chronograf.Layout) error {
-	h, err := os.Create(file)
+	h, err := fs.CreateFile(file)
 	if err != nil {
 		return err
 	}

chronograf/filestore/dashboards.go

@@ -10,6 +10,7 @@ import (
 	"strconv"
 
 	"github.com/influxdata/influxdb/chronograf"
+	"github.com/influxdata/influxdb/pkg/fs"
 )
 
 // DashExt is the the file extension searched for in the directory for dashboard files
@@ -56,7 +57,7 @@ func load(name string, resource interface{}) error {
 }
 
 func create(file string, resource interface{}) error {
-	h, err := os.Create(file)
+	h, err := fs.CreateFile(file)
 	if err != nil {
 		return err
 	}

cmd/influx_inspect/buildtsi/buildtsi.go

@@ -17,6 +17,7 @@ import (
 
 	"github.com/influxdata/influxdb/logger"
 	"github.com/influxdata/influxdb/models"
+	"github.com/influxdata/influxdb/pkg/fs"
 	"github.com/influxdata/influxdb/storage"
 	"github.com/influxdata/influxdb/storage/wal"
 	"github.com/influxdata/influxdb/toml"
@@ -327,7 +328,7 @@ func IndexShard(sfile *tsdb.SeriesFile, indexPath, dataDir, walDir string, maxLo
 
 	// Rename TSI to standard path.
 	log.Info("Moving tsi to permanent location")
-	return os.Rename(tmpPath, indexPath)
+	return fs.RenameFile(tmpPath, indexPath)
 }
 
 func IndexTSMFile(index *tsi1.Index, path string, batchSize int, log *zap.Logger, verboseLogging bool) error {

cmd/influxd/internal/profile/profile.go

@@ -5,6 +5,8 @@ import (
 	"os"
 	"runtime"
 	"runtime/pprof"
+
+	"github.com/influxdata/influxdb/pkg/fs"
 )
 
 type Config struct {
@@ -32,7 +34,7 @@ func (c *Config) Start() func() {
 	}
 
 	if c.CPU != "" {
-		f, err := os.Create(c.CPU)
+		f, err := fs.CreateFile(c.CPU)
 		if err != nil {
 			log.Fatalf("cpuprofile: %v", err)
 		}
@@ -41,7 +43,7 @@ func (c *Config) Start() func() {
 	}
 
 	if c.Memory != "" {
-		f, err := os.Create(c.Memory)
+		f, err := fs.CreateFile(c.Memory)
 		if err != nil {
 			log.Fatalf("memprofile: %v", err)
 		}

pkg/fs/fs_test.go

@@ -0,0 +1,159 @@
+package fs_test
+
+import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/influxdata/influxdb/pkg/fs"
+)
+
+func TestRenameFileWithReplacement(t *testing.T) {
+	// 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 := fs.RenameFileWithReplacement(oldpath, newpath); err != nil {
+			t.Fatalf("ReplaceFileIfExists returned an error: %s", err)
+		}
+
+		if err := fs.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 := fs.RenameFileWithReplacement(oldpath, newpath); err != nil {
+			t.Fatalf("ReplaceFileIfExists returned an error: %s", err)
+		}
+
+		if err := fs.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)
+		}
+	})
+}
+
+func TestCreateFileWithReplacement(t *testing.T) {
+	path := MustCreateTempFile(t, "sample data")
+	defer MustRemoveAll(path)
+
+	// should return an error if we CreateFile to the same path
+	_, err := fs.CreateFile(path)
+	if err == nil {
+		t.Fatalf("CreateFile did not return an error")
+	}
+
+	// contents of the file should be intact
+	contents := MustReadAllFile(path)
+	if got, exp := contents, "sample data"; got != exp {
+		t.Fatalf("got contents %q, expected %q", got, exp)
+	}
+
+	// running CreateFileWithReplacement on path should not return an error
+	if _, err = fs.CreateFileWithReplacement(path); err != nil {
+		t.Fatalf("CreateFileWithReplacement returned err: %v", err)
+	}
+
+	// the file at path should now be empty
+	contents = MustReadAllFile(path)
+	if contents != "" {
+		t.Fatalf("expected file to be empty but got: %v", contents)
+	}
+
+}
+
+// CreateTempFileOrFail creates a temporary file returning the path to the file.
+func MustCreateTempFile(t testing.TB, data string) string {
+	t.Helper()
+
+	f, err := ioutil.TempFile("", "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 fd.Close()
+
+	data, err := ioutil.ReadAll(fd)
+	if err != nil {
+		panic(err)
+	}
+	return string(data)
+}

pkg/fs/fs_unix.go

@@ -0,0 +1,83 @@
+// +build !windows
+
+package fs
+
+import (
+	"fmt"
+	"os"
+	"syscall"
+)
+
+// A FileExistsError is returned when an operation cannot be completed due to a
+// file already existing.
+type FileExistsError struct {
+	path string
+}
+
+func newFileExistsError(path string) FileExistsError {
+	return FileExistsError{path: path}
+}
+
+func (e FileExistsError) Error() string {
+	return fmt.Sprintf("operation not allowed, file %q exists", e.path)
+}
+
+// SyncDir flushes any file renames to the filesystem.
+func SyncDir(dirName string) error {
+	// fsync the dir to flush the rename
+	dir, err := os.OpenFile(dirName, os.O_RDONLY, os.ModeDir)
+	if err != nil {
+		return err
+	}
+	defer dir.Close()
+
+	// While we're on unix, we may be running in a Docker container that is
+	// pointed at a Windows volume over samba. That doesn't support fsyncs
+	// on directories. This shows itself as an EINVAL, so we ignore that
+	// error.
+	err = dir.Sync()
+	if pe, ok := err.(*os.PathError); ok && pe.Err == syscall.EINVAL {
+		err = nil
+	} else if err != nil {
+		return err
+	}
+
+	return dir.Close()
+}
+
+// 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 {
+	return os.Rename(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 {
+	if _, err := os.Stat(newpath); err == nil {
+		return newFileExistsError(newpath)
+	}
+
+	return os.Rename(oldpath, newpath)
+}
+
+// CreateFileWithReplacement will create a new file at any path, removing the
+// contents of the old file
+func CreateFileWithReplacement(newpath string) (*os.File, error) {
+	return os.Create(newpath)
+}
+
+// CreateFile creates a new file at newpath, returning an error if newpath already
+// exists
+func CreateFile(newpath string) (*os.File, error) {
+	if _, err := os.Stat(newpath); err == nil {
+		return nil, newFileExistsError(newpath)
+	}
+
+	return os.Create(newpath)
+}