Merge #156476

156476: cli: delete temporary directories before determining ENOSPC r=RaduBerinde a=jbowens

Before determining whether there's sufficient disk space available to start the
node, remove the temporary directories. If SQL-spilling created a substantial
volume of data on the filesystem, it's possible cleaning up this ephemeral data
will allow the node to start.

Release note: none
Fixes: #85147.
Epic: none

Co-authored-by: Jackson Owens <[email protected]>

Author: craig[bot]

pkg/cli/BUILD.bazel

@@ -457,6 +457,7 @@ go_test(
         "//pkg/ts/tspb",
         "//pkg/util",
         "//pkg/util/envutil",
+        "//pkg/util/humanizeutil",
         "//pkg/util/ioctx",
         "//pkg/util/leaktest",
         "//pkg/util/log",
@@ -470,6 +471,7 @@ go_test(
         "//pkg/util/tracing",
         "//pkg/util/uuid",
         "//pkg/workload/examples",
+        "@com_github_cockroachdb_crlib//crstrings",
         "@com_github_cockroachdb_datadriven//:datadriven",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_cockroachdb_errors//oserror",

pkg/cli/interactive_tests/test_temp_dir.tcl

@@ -66,7 +66,7 @@ interrupt_and_wait
 # Verify the temp directory is removed.
 glob_not_exists "$tempdir/$tempprefix*"
 # Verify temp directory path is removed from record file.
-if {[file size "$storedir/$recordfile"] > 0} {
+if {[file exists "$storedir/$recordfile"] && [file size "$storedir/$recordfile"] > 0} {
   report "RECORD FILE NOT EMPTY"
   exit 1
 }
@@ -120,7 +120,7 @@ send "$argv start-single-node --insecure --store=$storedir --background\r"
 eexpect ":/# "
 # Try to start up a second cockroach instance with the same store path.
 send "$argv start-single-node --insecure --store=$storedir\r"
-eexpect "ERROR: could not cleanup temporary directories from record file: could not lock temporary directory $cwd/$storedir/$tempprefix*"
+eexpect "ERROR: could not lock temporary directory $cwd/$storedir/$tempprefix*"
 # Verify the temp directory still exists.
 glob_exists "$storedir/$tempprefix*"
 send "pkill -9 cockroach\r"

pkg/cli/start.go

@@ -193,9 +193,9 @@ func initTempStorageConfig(
 	// target, if any. If we can't find one, we use the first StoreSpec in the
 	// list.
 	//
-	// While we look, we also clean up any abandoned temporary directories. We
-	// don't know which store spec was used previously—and it may change if
-	// encryption gets enabled after the fact—so we check each store.
+	// Note that we already cleaned up any abandoned temporary directories from
+	// the previous process earlier in the startup sequence (see
+	// reclaimDiskSpace).
 	specIdxDisk := -1
 	specIdxEncrypted := -1
 	for i, spec := range stores.Specs {
@@ -211,11 +211,6 @@ func initTempStorageConfig(
 		if specIdxDisk == -1 {
 			specIdxDisk = i
 		}
-		recordPath := filepath.Join(spec.Path, server.TempDirsRecordFilename)
-		if err := fs.CleanupTempDirs(recordPath); err != nil {
-			return base.TempStorageConfig{}, errors.Wrap(err,
-				"could not cleanup temporary directories from record file")
-		}
 	}
 
 	// Use first store by default. This might be an in-memory store.
@@ -311,7 +306,7 @@ func initTempStorageConfig(
 		// Remove temporary directory on shutdown.
 		stopper.AddCloser(stop.CloserFn(func() {
 			unlockDirFn()
-			if err := fs.CleanupTempDirs(recordPath); err != nil {
+			if err := fs.CleanupTempDirs(ctx, vfs.Default, recordPath); err != nil {
 				log.Dev.Errorf(ctx, "could not remove temporary store directory: %v", err.Error())
 			}
 		}))
@@ -435,13 +430,18 @@ func runStartInternal(
 		signal.Notify(signalCh, exitAbruptlySignal)
 	}
 
+	// Set up a cancellable context for the entire start command.
+	// The context will be canceled at the end.
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
 	// Check for stores with full disks and exit with an informative exit
 	// code. This needs to happen early during start, before we perform any
 	// writes to the filesystem including log rotation. We need to guarantee
 	// that the process continues to exit with the Disk Full exit code. A
 	// flapping exit code can affect alerting, including the alerting
 	// performed within CockroachCloud.
-	if err := exitIfDiskFull(vfs.Default, serverCfg.Stores.Specs); err != nil {
+	if err := exitIfDiskFull(ctx, vfs.Default, serverCfg.Stores.Specs); err != nil {
 		return err
 	}
 
@@ -456,11 +456,6 @@ func runStartInternal(
 	// making progress.
 	log.SetMakeProcessUnavailableFunc(closeAllSockets)
 
-	// Set up a cancellable context for the entire start command.
-	// The context will be canceled at the end.
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
 	// The context annotation ensures that server identifiers show up
 	// in the logging metadata as soon as they are known.
 	ambientCtx := serverCfg.AmbientCtx
@@ -1398,7 +1393,14 @@ func maybeWarnMemorySizes(ctx context.Context) {
 	}
 }
 
-func exitIfDiskFull(fs vfs.FS, specs []base.StoreSpec) error {
+func exitIfDiskFull(ctx context.Context, fs vfs.FS, specs []base.StoreSpec) error {
+	// First try to reclaim disk space by cleaning up obsolete files. It's
+	// possible this will free up enough space to allow us to start when we
+	// otherwise would not be able to.
+	if err := reclaimDiskSpace(ctx, fs, specs); err != nil {
+		return err
+	}
+
 	var cause error
 	var ballastPaths []string
 	var ballastMissing bool
@@ -1438,6 +1440,22 @@ disk space exhaustion may result in node loss.`, ballastPathsStr)
 	return err
 }
 
+// reclaimDiskSpace attempts to reclaim disk space by cleaning up obsolete files
+// leftover from the previous process. For example, SQL temporary directories
+// become obsolete when a process exits and can be reclaimed.
+func reclaimDiskSpace(ctx context.Context, rootFS vfs.FS, specs []base.StoreSpec) error {
+	var err error
+	// Reclaim any temporary directories.
+	for _, spec := range specs {
+		if spec.InMemory {
+			continue
+		}
+		recordPath := filepath.Join(spec.Path, server.TempDirsRecordFilename)
+		err = errors.CombineErrors(err, fs.CleanupTempDirs(ctx, rootFS, recordPath))
+	}
+	return err
+}
+
 // setupAndInitializeLoggingAndProfiling does what it says on the label.
 // Prior to this however it determines suitable defaults for the
 // logging output directory and the verbosity level of stderr logging.

pkg/cli/start_test.go

@@ -6,19 +6,25 @@
 package cli
 
 import (
+	"bytes"
 	"errors"
+	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/cli/clierror"
 	"github.com/cockroachdb/cockroach/pkg/cli/exit"
 	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/util/humanizeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/netutil/addr"
+	"github.com/cockroachdb/crlib/crstrings"
+	"github.com/cockroachdb/datadriven"
 	"github.com/cockroachdb/pebble/vfs"
 	"github.com/stretchr/testify/require"
 )
@@ -215,9 +221,8 @@ func TestExitIfDiskFull(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	err := exitIfDiskFull(mockDiskSpaceFS{FS: vfs.NewMem()}, []base.StoreSpec{
-		{},
-	})
+	err := exitIfDiskFull(t.Context(), &mockDiskSpaceFS{FS: vfs.NewMem()},
+		[]base.StoreSpec{{}})
 	require.Error(t, err)
 	var cliErr *clierror.Error
 	require.True(t, errors.As(err, &cliErr))
@@ -226,12 +231,88 @@ func TestExitIfDiskFull(t *testing.T) {
 
 type mockDiskSpaceFS struct {
 	vfs.FS
+	diskUsages map[string]vfs.DiskUsage
 }
 
-func (fs mockDiskSpaceFS) GetDiskUsage(path string) (vfs.DiskUsage, error) {
+func (fs *mockDiskSpaceFS) Unwrap() vfs.FS { return fs.FS }
+
+func (fs *mockDiskSpaceFS) GetDiskUsage(path string) (vfs.DiskUsage, error) {
+	if fs.diskUsages != nil {
+		for usagePath, usage := range fs.diskUsages {
+			if strings.HasPrefix(path, usagePath) {
+				return usage, nil
+			}
+		}
+	}
 	return vfs.DiskUsage{
 		AvailBytes: 10 << 20,
 		TotalBytes: 100 << 30,
 		UsedBytes:  100<<30 - 10<<20,
 	}, nil
 }
+
+func TestExitIfDiskFullDatadriven(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	var buf bytes.Buffer
+	var fs vfs.FS = vfs.NewMem()
+	require.NoError(t, fs.MkdirAll("/mnt", 0755))
+	mockDiskUsage := &mockDiskSpaceFS{FS: fs, diskUsages: make(map[string]vfs.DiskUsage)}
+	fs = vfs.WithLogging(mockDiskUsage, func(format string, args ...interface{}) {
+		fmt.Fprint(&buf, "# ")
+		fmt.Fprintf(&buf, format, args...)
+		fmt.Fprintln(&buf)
+	})
+	datadriven.RunTestAny(t, "testdata/exit_if_disk_full", func(t testing.TB, td *datadriven.TestData) string {
+		buf.Reset()
+		switch td.Cmd {
+		case "run":
+			var specs []base.StoreSpec
+			clear(mockDiskUsage.diskUsages)
+			for _, line := range crstrings.Lines(td.Input) {
+				switch {
+				case strings.HasPrefix(line, "disk-usage:"):
+					// disk-usage: <path> <avail> <total>
+					line = strings.TrimSpace(strings.TrimPrefix(line, "disk-usage:"))
+					fields := strings.Fields(line)
+					if len(fields) != 3 {
+						return fmt.Sprintf("invalid disk-usage line: %q", line)
+					}
+					path := fields[0]
+					avail, err := humanizeutil.ParseBytes(fields[1])
+					if err != nil {
+						return fmt.Sprintf("invalid disk-usage line: %q", line)
+					}
+					total, err := humanizeutil.ParseBytes(fields[2])
+					if err != nil {
+						return fmt.Sprintf("invalid disk-usage line: %q", line)
+					}
+					mockDiskUsage.diskUsages[path] = vfs.DiskUsage{
+						AvailBytes: uint64(avail),
+						TotalBytes: uint64(total),
+						UsedBytes:  uint64(total - avail),
+					}
+				case strings.HasPrefix(line, "store:"):
+					line = strings.TrimSpace(strings.TrimPrefix(line, "store:"))
+					spec, err := base.NewStoreSpec(line)
+					if err != nil {
+						return fmt.Sprintf("failed to parse store spec: %v", err)
+					}
+					specs = append(specs, spec)
+				default:
+					return fmt.Sprintf("unknown line: %q", line)
+				}
+			}
+			err := exitIfDiskFull(t.Context(), fs, specs)
+			if err == nil {
+				fmt.Fprint(&buf, "<no-error>")
+			} else {
+				fmt.Fprintf(&buf, "<error:%s>", err)
+			}
+			return buf.String()
+		default:
+			return fmt.Sprintf("unknown command: %s", td.Cmd)
+		}
+	})
+}

pkg/cli/testdata/exit_if_disk_full

@@ -0,0 +1,11 @@
+run
+disk-usage: /mnt/foo 1KiB 300GiB
+disk-usage: /mnt/bar 1KiB 300GiB
+store: path=/mnt/foo
+store: path=/mnt/bar
+----
+# open: /mnt/foo/temp-dirs-record.txt
+# open: /mnt/bar/temp-dirs-record.txt
+# get-disk-usage: /mnt/foo
+# get-disk-usage: /mnt/bar
+<error:store /mnt/foo: out of disk space>

pkg/storage/ballast.go

@@ -123,6 +123,14 @@ func maybeEstablishBallast(
 		currentSizeBytes = fi.Size()
 	}
 
+	// MemFS doesn't support resizing, so just return false. This exists
+	// primarily to ease testing. If starting a real cockroach binary with a
+	// store with a MemFS, we shouldn't even get this far because the StoreSpec
+	// is marked as InMemory.
+	if _, isMem := vfs.Root(fs).(*vfs.MemFS); isMem {
+		return false, nil
+	}
+
 	switch {
 	case currentSizeBytes > ballastSizeBytes:
 		// If the current ballast is too big, shrink it regardless of current

pkg/storage/fs/temp_dir.go

@@ -25,8 +25,8 @@ type lockStruct struct {
 }
 
 // lockFile sets a lock on the specified file, using flock.
-func lockFile(filename string) (lockStruct, error) {
-	closer, err := vfs.Default.Lock(filename)
+func lockFile(fs vfs.FS, filename string) (lockStruct, error) {
+	closer, err := fs.Lock(filename)
 	if err != nil {
 		return lockStruct{}, err
 	}
@@ -64,7 +64,7 @@ func CreateTempDir(parentDir, prefix string) (_ string, unlockDirFn func(), _ er
 	}
 
 	// Create a lock file.
-	flock, err := lockFile(filepath.Join(absPath, lockFilename))
+	flock, err := lockFile(vfs.Default, filepath.Join(absPath, lockFilename))
 	if err != nil {
 		return "", nil, errors.Wrapf(err, "could not create lock on new temporary directory")
 	}
@@ -98,19 +98,23 @@ func RecordTempDir(recordPath, tempPath string) error {
 // up abandoned temporary directories.
 // It should also be invoked when a newly created temporary directory is no
 // longer needed and needs to be removed from the record file.
-func CleanupTempDirs(recordPath string) error {
+func CleanupTempDirs(ctx context.Context, fs vfs.FS, recordPath string) error {
 	// Reading the entire file into memory shouldn't be a problem since
 	// it is extremely rare for this record file to contain more than a few
 	// entries.
-	f, err := os.OpenFile(recordPath, os.O_RDWR, 0644)
+	f, err := fs.Open(recordPath)
 	// There is no existing record file and thus nothing to clean up.
 	if oserror.IsNotExist(err) {
 		return nil
 	}
 	if err != nil {
 		return err
 	}
-	defer f.Close()
+	defer func() {
+		if f != nil {
+			f.Close()
+		}
+	}()
 
 	scanner := bufio.NewScanner(f)
 	// Iterate through each temporary directory path and remove the
@@ -122,14 +126,14 @@ func CleanupTempDirs(recordPath string) error {
 		}
 
 		// Check if the temporary directory exists; if it does not, skip over it.
-		if _, err := os.Stat(path); oserror.IsNotExist(err) {
-			log.Dev.Warningf(context.Background(), "could not locate previous temporary directory %s, might require manual cleanup, or might have already been cleaned up.", path)
+		if _, err := fs.Stat(path); oserror.IsNotExist(err) {
+			log.Dev.Warningf(ctx, "could not locate previous temporary directory %s, might require manual cleanup, or might have already been cleaned up.", path)
 			continue
 		}
 
 		// Check if another Cockroach instance is using this temporary
 		// directory i.e. has a lock on the temp dir lock file.
-		flock, err := lockFile(filepath.Join(path, lockFilename))
+		flock, err := lockFile(fs, fs.PathJoin(path, lockFilename))
 		if err != nil {
 			return errors.Wrapf(err, "could not lock temporary directory %s, may still be in use", path)
 		}
@@ -143,15 +147,29 @@ func CleanupTempDirs(recordPath string) error {
 		// process is dead because we were able to acquire the lock in the first
 		// place.
 		if err := unlockFile(flock); err != nil {
-			log.Dev.Errorf(context.TODO(), "could not unlock file lock when removing temporary directory: %s", err.Error())
+			log.Dev.Errorf(ctx, "could not unlock file lock when removing temporary directory: %s", err.Error())
 		}
 
 		// If path/directory does not exist, error is nil.
-		if err := os.RemoveAll(path); err != nil {
+		if err := fs.RemoveAll(path); err != nil {
 			return err
 		}
 	}
+	// If the scanner ecnoutners an error, we may not have been able to remove
+	// all the temporary directories.
+	if err := scanner.Err(); err != nil {
+		err = errors.CombineErrors(err, f.Close())
+		f = nil
+		return err
+	}
 
-	// Clear out the record file now that we're done.
-	return f.Truncate(0)
+	// Close and remove the record file now that we're done. We need to close
+	// the file first to accommodate Windows.
+	err = f.Close()
+	f = nil
+	rmErr := fs.Remove(recordPath)
+	if rmErr != nil && !oserror.IsNotExist(rmErr) {
+		err = errors.CombineErrors(err, rmErr)
+	}
+	return err
 }