Merge #59995

59995: cli/debug: make `ballast` unable to overwrite existing files r=tbg,stevendanna a=knz

Discussed in customer postmortem.

Release note (backward-incompatible change): The `cockroach debug
ballast` command now refuses to overwrite the target ballast file if
it already exists. This change is intended to prevent mistaken uses of
the `ballast` command by operators. Scripts that integrate `cockroach
debug ballast` can consider adding a `rm` command.

Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>

pkg/cli/debug.go

@@ -279,8 +279,13 @@ func runDebugBallast(cmd *cobra.Command, args []string) error {
 		return nil
 	}
 	ballastSize := targetUsage - used
+
+	// Note: CreateLargeFile fails if the target file already
+	// exists. This is a feature; we have seen users mistakenly applying
+	// the `ballast` command directly to block devices, thereby trashing
+	// their filesystem.
 	if err := sysutil.CreateLargeFile(ballastFile, ballastSize); err != nil {
-		return errors.Wrap(err, "failed to fallocate to ballast file")
+		return errors.Wrap(err, "error allocating ballast file")
 	}
 	return nil
 }

pkg/util/sysutil/BUILD.bazel

@@ -70,6 +70,7 @@ go_test(
     embed = [":sysutil"],
     deps = [
         "@com_github_cockroachdb_errors//:errors",
+        "@com_github_cockroachdb_errors//oserror",
     ] + select({
         "@io_bazel_rules_go//go/platform:aix": [
             "@org_golang_x_sys//unix",

pkg/util/sysutil/large_file_linux.go

@@ -24,13 +24,13 @@ import (
 // given size. On other platforms, it naively writes the specified number of
 // bytes, which can take a long time.
 func CreateLargeFile(path string, bytes int64) error {
-	f, err := os.Create(path)
+	f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0666)
 	if err != nil {
-		return errors.Wrapf(err, "failed to create file %s", path)
+		return err
 	}
 	defer f.Close()
 	if err := unix.Fallocate(int(f.Fd()), 0, 0, bytes); err != nil {
-		return err
+		return errors.Wrap(err, "fallocate")
 	}
-	return f.Sync()
+	return errors.Wrap(f.Sync(), "fsync")
 }

pkg/util/sysutil/large_file_naive.go

@@ -23,9 +23,9 @@ import (
 // platforms, it naively writes the specified number of bytes, which can take a
 // long time when the number of bytes is large.
 func CreateLargeFile(path string, bytes int64) error {
-	f, err := os.Create(path)
+	f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0666)
 	if err != nil {
-		return errors.Wrapf(err, "failed to create file %s", path)
+		return err
 	}
 	defer f.Close()
 	sixtyFourMB := make([]byte, 64<<20)
@@ -35,9 +35,9 @@ func CreateLargeFile(path string, bytes int64) error {
 			z = sixtyFourMB[:bytes]
 		}
 		if _, err := f.Write(z); err != nil {
-			return err
+			return errors.Wrap(err, "write")
 		}
 		bytes -= int64(len(z))
 	}
-	return f.Sync()
+	return errors.Wrap(f.Sync(), "fsync")
 }

pkg/util/sysutil/large_file_test.go

@@ -13,20 +13,25 @@ package sysutil
 import (
 	"io/ioutil"
 	"os"
+	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/cockroachdb/errors"
+	"github.com/cockroachdb/errors/oserror"
 )
 
 func TestLargeFile(t *testing.T) {
-	f, err := ioutil.TempFile("", "input")
+	d, err := ioutil.TempDir("", t.Name())
 	if err != nil {
 		t.Fatal(err)
 	}
-	fname := f.Name()
-	if err := f.Close(); err != nil {
-		t.Fatal(err)
-	}
+	defer func() {
+		if err := os.RemoveAll(d); err != nil {
+			t.Fatal(err)
+		}
+	}()
+	fname := filepath.Join(d, "ballast")
 	const n int64 = 1013
 	if err := CreateLargeFile(fname, n); err != nil {
 		t.Fatal(err)
@@ -38,4 +43,9 @@ func TestLargeFile(t *testing.T) {
 	if s.Size() != n {
 		t.Fatal(errors.Errorf("expected size of file %d, got %d", n, s.Size()))
 	}
+
+	// Check that an existing file cannot be overwritten.
+	if err = CreateLargeFile(fname, n); !(oserror.IsExist(err) || strings.Contains(err.Error(), "exists")) {
+		t.Fatalf("expected 'already exists' error, got (%T) %+v", err, err)
+	}
 }