From 9f02811646171acefdb9c91aac173619fc0de209 Mon Sep 17 00:00:00 2001 From: Annie Pompa Date: Wed, 24 Sep 2025 11:24:50 -0400 Subject: [PATCH] wal: validate recovery directory through a stable identifier We now persist a stable identifier to both the OPTIONS file and a file within the secondary directory (failover_identifier). If recovery finds that the secondary directory does not contain a matching identifier, we abort recovery indicating that the secondary seems incorrect / corrupt. Fixes: #4416 --- open.go | 10 ++ open_test.go | 3 + options.go | 57 ++++++++++ options_test.go | 49 ++++++++- testdata/open_wal_failover | 1 + wal/failover_manager.go | 117 +++++++++++++++++++- wal/standalone_manager.go | 5 + wal/wal.go | 8 ++ wal_failover_identifier_test.go | 188 ++++++++++++++++++++++++++++++++ 9 files changed, 435 insertions(+), 3 deletions(-) create mode 100644 wal_failover_identifier_test.go diff --git a/open.go b/open.go index dbd674c7eb..ffb1a2b66a 100644 --- a/open.go +++ b/open.go @@ -350,6 +350,7 @@ func Open(dirname string, opts *Options) (db *DB, err error) { } if opts.WALFailover != nil { walOpts.Secondary = opts.WALFailover.Secondary + walOpts.Secondary.ID = opts.WALFailover.Secondary.ID // Lock the secondary WAL directory, if distinct from the data directory // and primary WAL directory. if secondaryWalDirName != dirname && secondaryWalDirName != walDirname { @@ -424,10 +425,19 @@ func Open(dirname string, opts *Options) (db *DB, err error) { } } + if opts.WALFailover != nil { + walDir, err := wal.ValidateOrInitWALDir(walOpts.Secondary) + if err != nil { + return nil, err + } + walOpts.Secondary = walDir + opts.WALFailover.Secondary.ID = walDir.ID + } walManager, err := wal.Init(walOpts, retainedWALs) if err != nil { return nil, err } + defer maybeCleanUp(walManager.Close) d.mu.log.manager = walManager diff --git a/open_test.go b/open_test.go index 6a36c5fe14..636a291f6a 100644 --- a/open_test.go +++ b/open_test.go @@ -256,6 +256,9 @@ func TestOpen_WALFailover(t *testing.T) { if o.FS == nil { return "no path" } + // Set a constant identifier for testing to avoid flaky tests + wal.SetGenerateStableIdentifierForTesting("9f69f2c3ffb3c247767290a9b3215fc5") + defer wal.ResetGenerateStableIdentifierForTesting() d, err := Open(dataDir, o) if err != nil { return err.Error() diff --git a/options.go b/options.go index 8bff73e2b0..45ad690174 100644 --- a/options.go +++ b/options.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/crlib/fifo" "github.com/cockroachdb/errors" + "github.com/cockroachdb/errors/oserror" "github.com/cockroachdb/pebble/internal/base" "github.com/cockroachdb/pebble/internal/cache" "github.com/cockroachdb/pebble/internal/humanize" @@ -1863,6 +1864,9 @@ func (o *Options) String() string { fmt.Fprintf(&buf, "\n") fmt.Fprintf(&buf, "[WAL Failover]\n") fmt.Fprintf(&buf, " secondary_dir=%s\n", o.WALFailover.Secondary.Dirname) + if o.WALFailover.Secondary.ID != "" { + fmt.Fprintf(&buf, " secondary_identifier=%s\n", o.WALFailover.Secondary.ID) + } fmt.Fprintf(&buf, " primary_dir_probe_interval=%s\n", o.WALFailover.FailoverOptions.PrimaryDirProbeInterval) fmt.Fprintf(&buf, " healthy_probe_latency_threshold=%s\n", o.WALFailover.FailoverOptions.HealthyProbeLatencyThreshold) fmt.Fprintf(&buf, " healthy_interval=%s\n", o.WALFailover.FailoverOptions.HealthyInterval) @@ -2317,6 +2321,8 @@ func (o *Options) Parse(s string, hooks *ParseHooks) error { switch key { case "secondary_dir": o.WALFailover.Secondary = wal.Dir{Dirname: value, FS: vfs.Default} + case "secondary_identifier": + o.WALFailover.Secondary.ID = value case "primary_dir_probe_interval": o.WALFailover.PrimaryDirProbeInterval, err = time.ParseDuration(value) case "healthy_probe_latency_threshold": @@ -2435,6 +2441,21 @@ func (e ErrMissingWALRecoveryDir) Error() string { return fmt.Sprintf("directory %q may contain relevant WALs but is not in WALRecoveryDirs%s", e.Dir, e.ExtraInfo) } +// ErrSecondaryIdentifierMismatch is an error returned when the secondary directory +// identifier doesn't match the expected identifier, indicating the wrong disk +// may have been mounted at the expected path. +type ErrSecondaryIdentifierMismatch struct { + ExpectedIdentifier string + ActualIdentifier string + SecondaryDir string +} + +// Error implements error. +func (e ErrSecondaryIdentifierMismatch) Error() string { + return fmt.Sprintf("secondary directory %q has identifier %q but expected %q - wrong disk may be mounted", + e.SecondaryDir, e.ActualIdentifier, e.ExpectedIdentifier) +} + // CheckCompatibility verifies the options are compatible with the previous options // serialized by Options.String(). For example, the Comparer and Merger must be // the same, or data will not be able to be properly read from the DB. @@ -2502,6 +2523,12 @@ func (o *Options) checkWALDir(storeDir, walDir, errContext string) error { for _, d := range o.WALRecoveryDirs { // TODO(radu): should we also check that d.FS is the same as walDir's FS? if walPath == resolveStorePath(storeDir, d.Dirname) { + if d.ID != "" { + if err := o.validateWALRecoveryDirIdentifier(d); err != nil { + return err + } + + } return nil } } @@ -2789,3 +2816,33 @@ func resolveStorePath(storeDir, path string) string { } return path } + +// validateWALRecoveryDirIdentifier validates that the identifier in the +// provided wal.Dir matches the expected ID encoded in the OPTIONS file to +// ensure that we're using the correct directory. +func (o *Options) validateWALRecoveryDirIdentifier(d wal.Dir) error { + identifierFile := d.FS.PathJoin(d.Dirname, "failover_identifier") + f, err := d.FS.Open(identifierFile) + if err != nil { + if oserror.IsNotExist(err) { + return nil + } + return err + } + defer f.Close() + + existingIdentifier, err := io.ReadAll(f) + if err != nil { + return err + } + + if err != nil { + return errors.Newf("failed to read secondary identifier from WALRecoveryDir %q: %v", + d.Dirname, err) + } + if strings.TrimSpace(string(existingIdentifier)) != d.ID { + return errors.Newf("WALRecoveryDir %q has identifier %q but expected %q", + d.Dirname, existingIdentifier, d.ID) + } + return nil +} diff --git a/options_test.go b/options_test.go index 8b30d88ae7..2a2fe34a25 100644 --- a/options_test.go +++ b/options_test.go @@ -7,6 +7,7 @@ package pebble import ( "bytes" "fmt" + "io" "math/rand/v2" "runtime" "strings" @@ -259,7 +260,7 @@ func TestOptionsCheckCompatibility(t *testing.T) { // Check that an OPTIONS file that configured an explicit WALDir that will // no longer be used errors if it's not also present in WALRecoveryDirs. - //require.Equal(t, ErrMissingWALRecoveryDir{Dir: "external-wal-dir"}, + // require.Equal(t, ErrMissingWALRecoveryDir{Dir: "external-wal-dir"}, err := DefaultOptions().CheckCompatibility(storeDir, ` [Options] wal_dir=external-wal-dir @@ -339,6 +340,52 @@ func TestOptionsCheckCompatibility(t *testing.T) { `)) } +func TestWALRecoveryDirValidation(t *testing.T) { + storeDir := "/mnt/foo" + mem := vfs.NewMem() + recoveryDir := "/mnt/wrong-disk-dir" + err := mem.MkdirAll(recoveryDir, 0755) + require.NoError(t, err) + + // Create failover_identifier file with different ID. + identifierFile := mem.PathJoin(recoveryDir, "failover_identifier") + wrongID := "11111111111111111111111111111111" + err = writeTestIdentifierToFile(mem, identifierFile, wrongID) + require.NoError(t, err) + + opts := &Options{ + FS: mem, + WALRecoveryDirs: []wal.Dir{ + { + FS: mem, + Dirname: recoveryDir, + ID: "22222222222222222222222222222222", + }, + }, + } + opts.EnsureDefaults() + + err = opts.checkWALDir(storeDir, recoveryDir, "test context") + require.Error(t, err) + require.Contains(t, err.Error(), "has identifier \"11111111111111111111111111111111\" but expected \"22222222222222222222222222222222\"") +} + +// writeTestIdentifierToFile is a helper function to write an identifier to a file +func writeTestIdentifierToFile(fs vfs.FS, filename, identifier string) error { + f, err := fs.Create(filename, "pebble-wal") + if err != nil { + return err + } + defer f.Close() + + _, err = io.WriteString(f, identifier) + if err != nil { + return err + } + + return f.Sync() +} + type testCleaner struct{} func (testCleaner) Clean(fs vfs.FS, fileType base.FileType, path string) error { diff --git a/testdata/open_wal_failover b/testdata/open_wal_failover index 9713782e29..4aad800be1 100644 --- a/testdata/open_wal_failover +++ b/testdata/open_wal_failover @@ -46,6 +46,7 @@ list path=(a,data) grep-between path=(a,data/OPTIONS-000007) start=(\[WAL Failover\]) end=^$ ---- secondary_dir=secondary-wals + secondary_identifier=9f69f2c3ffb3c247767290a9b3215fc5 primary_dir_probe_interval=1s healthy_probe_latency_threshold=25ms healthy_interval=15s diff --git a/wal/failover_manager.go b/wal/failover_manager.go index f0df5288a0..7432d5b04a 100644 --- a/wal/failover_manager.go +++ b/wal/failover_manager.go @@ -6,15 +6,18 @@ package wal import ( "cmp" + crand "crypto/rand" "fmt" "io" - "math/rand/v2" + mathrand "math/rand/v2" "os" "slices" + "strings" "sync" "time" "github.com/cockroachdb/errors" + "github.com/cockroachdb/errors/oserror" "github.com/cockroachdb/pebble/internal/base" "github.com/cockroachdb/pebble/internal/invariants" "github.com/cockroachdb/pebble/vfs" @@ -52,6 +55,71 @@ const probeHistoryLength = 128 // Large value. const failedProbeDuration = 24 * 60 * 60 * time.Second +// For testing, generateStableIdentifierForTesting can be overridden to return +// a constant value when we generate stable identifiers. +var generateStableIdentifierForTesting = "" + +// SetGenerateStableIdentifierForTesting sets a constant identifier for testing. +// This should only be used in tests to avoid flaky behavior. +func SetGenerateStableIdentifierForTesting(identifier string) { + generateStableIdentifierForTesting = identifier +} + +// ResetGenerateStableIdentifierForTesting resets the testing override. +// This should only be used in tests. +func ResetGenerateStableIdentifierForTesting() { + generateStableIdentifierForTesting = "" +} + +// generateStableIdentifier generates a random hex string from 16 bytes. +func generateStableIdentifier() (string, error) { + // For testing, return a constant value if set. + if generateStableIdentifierForTesting != "" { + return generateStableIdentifierForTesting, nil + } + + var uuid [16]byte + if _, err := crand.Read(uuid[:]); err != nil { + return "", err + } + return fmt.Sprintf("%x", uuid), nil +} + +// readSecondaryIdentifier reads the identifier from the secondary directory. +func readSecondaryIdentifier(fs vfs.FS, identifierFile string) (string, error) { + f, err := fs.Open(identifierFile) + if err != nil { + if oserror.IsNotExist(err) { + return "", nil + } + return "", err + } + defer f.Close() + + data, err := io.ReadAll(f) + if err != nil { + return "", err + } + + // Trim whitespace and return the identifier. + return strings.TrimSpace(string(data)), nil +} + +// writeSecondaryIdentifier writes the identifier to the secondary directory. +func writeSecondaryIdentifier(fs vfs.FS, identifierFile string, identifier string) error { + f, err := fs.Create(identifierFile, "pebble-wal") + if err != nil { + return err + } + + if _, err := io.WriteString(f, identifier); err != nil { + f.Close() + return err + } + + return errors.CombineErrors(f.Sync(), f.Close()) +} + // init takes a stopper in order to connect the dirProber's long-running // goroutines with the stopper's wait group, but the dirProber has its own // stop() method that should be invoked to trigger the shutdown. @@ -73,7 +141,7 @@ func (p *dirProber) init( } // Random bytes for writing, to defeat any FS compression optimization. for i := range p.buf { - p.buf[i] = byte(rand.Uint32()) + p.buf[i] = byte(mathrand.Uint32()) } // dirProber has an explicit stop() method instead of listening on // stopper.shouldQuiesce. This structure helps negotiate the shutdown @@ -538,6 +606,46 @@ func (wm *failoverManager) init(o Options, initial Logs) error { return nil } +// ValidateOrInitWALDir manages the secondary directory identifier for +// failover validation. It ensures the correct secondary directory is mounted +// by validating or generating a stable identifier. +func ValidateOrInitWALDir(walDir Dir) (Dir, error) { + identifierFile := walDir.FS.PathJoin(walDir.Dirname, "failover_identifier") + // If we have an identifier from the OPTIONS file, validate it matches what's + // in the directory. + if walDir.ID != "" { + existingIdentifier, err := readSecondaryIdentifier(walDir.FS, identifierFile) + if err != nil { + return Dir{}, errors.Newf("failed to read secondary identifier: %v", err) + } + // Not the same identifier, wrong disk may be mounted. + if existingIdentifier != walDir.ID { + return Dir{}, errors.Newf("secondary directory %q has identifier %q but expected %q - wrong disk may be mounted", + walDir.Dirname, existingIdentifier, walDir.ID) + } + } else { + // No identifier in OPTIONS file, check if one exists in the directory. + existingIdentifier, err := readSecondaryIdentifier(walDir.FS, identifierFile) + if err != nil { + return Dir{}, errors.Newf("failed to read secondary identifier: %v", err) + } + if existingIdentifier == "" { + // Generate a new identifier. + identifier, err := generateStableIdentifier() + if err != nil { + return Dir{}, errors.Newf("failed to generate UUID: %v", err) + } + if err := writeSecondaryIdentifier(walDir.FS, identifierFile, identifier); err != nil { + return Dir{}, errors.Newf("failed to write secondary identifier: %v", err) + } + walDir.ID = identifier + } else { + walDir.ID = existingIdentifier + } + } + return walDir, nil +} + // List implements Manager. func (wm *failoverManager) List() Logs { wm.mu.Lock() @@ -843,6 +951,11 @@ func (wm *failoverManager) logCreator( return logFile, 0, err } +// Opts implements Manager. +func (wm *failoverManager) Opts() Options { + return wm.opts +} + type stopper struct { quiescer chan struct{} // Closed when quiescing wg sync.WaitGroup diff --git a/wal/standalone_manager.go b/wal/standalone_manager.go index bceea0092b..4f489ee48c 100644 --- a/wal/standalone_manager.go +++ b/wal/standalone_manager.go @@ -257,6 +257,11 @@ func (m *StandaloneManager) Close() error { return err } +// Opts implements Manager. +func (m *StandaloneManager) Opts() Options { + return m.o +} + // RecyclerForTesting implements Manager. func (m *StandaloneManager) RecyclerForTesting() *LogRecycler { return &m.recycler diff --git a/wal/wal.go b/wal/wal.go index acdc232fa5..f3f2db3a8c 100644 --- a/wal/wal.go +++ b/wal/wal.go @@ -26,6 +26,12 @@ type Dir struct { Lock *base.DirLock FS vfs.FS Dirname string + // ID is a stable UUID that uniquely identifies the directory. This + // identifier is persisted both in the OPTIONS file in the primary directory + // and in a file (failover_identifier) within the secondary directory to detect + // incorrectness/corruptions (e.g. when the wrong disk has been mounted at + // the expected path during recovery). + ID string } // NumWAL is the number of the virtual WAL. It can map to one or more physical @@ -368,6 +374,8 @@ type Manager interface { // Close the manager. // REQUIRES: Writers and Readers have already been closed. Close() error + // Opts returns the Options used to initialize the Manager. + Opts() Options // RecyclerForTesting exposes the internal LogRecycler. RecyclerForTesting() *LogRecycler diff --git a/wal_failover_identifier_test.go b/wal_failover_identifier_test.go new file mode 100644 index 0000000000..ef1c4541e8 --- /dev/null +++ b/wal_failover_identifier_test.go @@ -0,0 +1,188 @@ +// Copyright 2025 The LevelDB-Go and Pebble Authors. All rights reserved. Use +// of this source code is governed by a BSD-style license that can be found in +// the LICENSE file. + +package pebble + +import ( + "io" + "strings" + "testing" + + "github.com/cockroachdb/pebble/vfs" + "github.com/cockroachdb/pebble/wal" + "github.com/stretchr/testify/require" +) + +// TestWALFailoverIdentifier tests the WAL failover identifier mechanism, which +// ensures that the correct secondary WAL directory is being used during database +// recovery. The identifier is a unique string stored in a "failover_identifier" +// file in the secondary WAL directory that helps prevent accidental use of the +// wrong secondary directory (e.g., when the wrong disk is mounted). +// +// The test covers three scenarios: +// 1. first_time_generation: when opening a database with WAL failover for the +// first time, a new identifier should be generated and written to both the +// secondary directory and the OPTIONS file +// 2. identifier_mismatch_error: when the secondary directory contains a +// different identifier than what's expected in the options, the database +// should fail to open with an error indicating the wrong disk may be mounted +// 3. no_primary_has_secondary: when no identifier is specified in options but +// the secondary directory already contains an identifier, the database should +// adopt the existing identifier and write it to the OPTIONS file +func TestWALFailoverIdentifier(t *testing.T) { + t.Run("first_time_generation", func(t *testing.T) { + mem := vfs.NewMem() + + // First time opening with WAL failover should generate an identifier. + opts := &Options{ + FS: mem, + WALFailover: &WALFailoverOptions{ + Secondary: wal.Dir{Dirname: "secondary", FS: mem}, + }, + } + + db, err := Open("testdb", opts) + require.NoError(t, err, "failed to open database") + defer db.Close() + + // Check that identifier file exists in secondary directory. + identifierFile := mem.PathJoin("secondary", "failover_identifier") + failoverId, err := mem.Open(identifierFile) + require.NoError(t, err) + + // Read the identifier from the file. + defer failoverId.Close() + data := make([]byte, 100) + n, err := failoverId.Read(data) + if err != nil && err != io.EOF { + t.Fatalf("failed to read identifier file: %v", err) + } + + identifier := strings.TrimSpace(string(data[:n])) + require.True(t, identifier != "", "expected identifier to be written to file") + t.Logf("identifier from failover_identifier file: %q", identifier) + + // Check that the identifier is written to the OPTIONS file. + entries, err := mem.List("testdb") + require.NoError(t, err, "failed to list testdb directory") + var optionsFile string + for _, entry := range entries { + if strings.HasPrefix(entry, "OPTIONS-") { + optionsFile = mem.PathJoin("testdb", entry) + break + } + } + require.NotEmpty(t, optionsFile, "OPTIONS file should exist") + + optionsF, err := mem.Open(optionsFile) + require.NoError(t, err, "failed to open OPTIONS file") + defer optionsF.Close() + + optionsData, err := io.ReadAll(optionsF) + require.NoError(t, err, "failed to read OPTIONS file") + + optionsContent := string(optionsData) + // Verify the OPTIONS file contains the [WAL Failover] section with the + // identifier we found in the failover_identifier above. + require.Contains(t, optionsContent, "[WAL Failover]", + "OPTIONS file should contain WAL Failover section") + require.Contains(t, optionsContent, "secondary_identifier="+identifier, + "OPTIONS file should contain the generated identifier") + }) + + t.Run("identifier_mismatch_error", func(t *testing.T) { + mem := vfs.NewMem() + secondaryDir := "secondary" + err := mem.MkdirAll(secondaryDir, 0755) + require.NoError(t, err) + + // Create a secondary directory with a different identifier. + identifierFile := mem.PathJoin("secondary", "failover_identifier") + existingIdentifier := "44444444444444444444444444444444" + err = writeTestIdentifier(mem, identifierFile, existingIdentifier) + require.NoError(t, err) + + // Try to open with a different identifier in options. + opts := &Options{ + FS: mem, + WALFailover: &WALFailoverOptions{ + Secondary: wal.Dir{ + Dirname: "secondary", + FS: mem, + ID: "7f495fb4914ecfc04deeafa41de42b78", + }, + }, + } + + _, err = Open("testdb", opts) + require.Error(t, err, "expected error due to identifier mismatch") + require.Contains(t, err.Error(), "wrong disk may be mounted") + }) + + t.Run("no_primary_has_secondary", func(t *testing.T) { + // Test case: no identifier in primary options, secondary has identifier + // The system should adopt the existing identifier from the secondary directory. + mem := vfs.NewMem() + identifierFile := mem.PathJoin("secondary", "failover_identifier") + err := mem.MkdirAll("secondary", 0755) + require.NoError(t, err) + + // Write an existing identifier to the secondary directory. + existingIdentifier := "44444444444444444444444444444444" + err = writeTestIdentifier(mem, identifierFile, existingIdentifier) + require.NoError(t, err) + + // Open without specifying SecondaryIdentifier in options. + opts := &Options{ + FS: mem, + WALFailover: &WALFailoverOptions{ + Secondary: wal.Dir{Dirname: "secondary", FS: mem}, + }, + } + + db, err := Open("testdb", opts) + require.NoError(t, err, "should succeed and adopt the existing identifier") + defer db.Close() + + // Verify that the OPTIONS file now contains the adopted identifier. + entries, err := mem.List("testdb") + require.NoError(t, err, "failed to list testdb directory") + var optionsFile string + for _, entry := range entries { + if strings.HasPrefix(entry, "OPTIONS-") { + optionsFile = mem.PathJoin("testdb", entry) + break + } + } + require.NotEmpty(t, optionsFile, "OPTIONS file should exist") + + optionsF, err := mem.Open(optionsFile) + require.NoError(t, err, "failed to open OPTIONS file") + defer optionsF.Close() + + optionsData, err := io.ReadAll(optionsF) + require.NoError(t, err, "failed to read OPTIONS file") + + optionsContent := string(optionsData) + require.Contains(t, optionsContent, "[WAL Failover]", + "OPTIONS file should contain WAL Failover section") + require.Contains(t, optionsContent, "secondary_identifier="+existingIdentifier, + "OPTIONS file should contain the adopted identifier") + }) +} + +func writeTestIdentifier(fs vfs.FS, filename, identifier string) error { + f, err := fs.Create(filename, "pebble-wal") + if err != nil { + return err + } + defer f.Close() + + _, err = io.WriteString(f, identifier) + if err != nil { + return err + } + + return f.Sync() +}