storageconfig: move WAL failover flag code to cli

Move the pflag-specific part to cli.

Epic: none
Release note: None

Author: RaduBerinde

pkg/base/BUILD.bazel

@@ -74,7 +74,6 @@ go_test(
         "//pkg/util/leaktest",
         "//pkg/util/log",
         "//pkg/util/uuid",
-        "@com_github_cockroachdb_datadriven//:datadriven",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_davecgh_go_spew//spew",
         "@com_github_stretchr_testify//require",

pkg/base/config_test.go

@@ -6,19 +6,15 @@
 package base_test
 
 import (
-	"bytes"
 	"fmt"
 	"math"
-	"strings"
 	"testing"
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
-	"github.com/cockroachdb/cockroach/pkg/storage/storageconfig"
 	"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/echotest"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
-	"github.com/cockroachdb/datadriven"
 	"github.com/davecgh/go-spew/spew"
 	"github.com/stretchr/testify/require"
 )
@@ -211,20 +207,3 @@ func TestRaftMaxInflightBytes(t *testing.T) {
 		})
 	}
 }
-
-func TestWALFailoverConfigRoundtrip(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-
-	datadriven.RunTest(t, datapathutils.TestDataPath(t, "wal-failover-config"), func(t *testing.T, d *datadriven.TestData) string {
-		var buf bytes.Buffer
-		for _, l := range strings.Split(d.Input, "\n") {
-			var cfg storageconfig.WALFailover
-			if err := cfg.Set(l); err != nil {
-				fmt.Fprintf(&buf, "err: %s\n", err)
-				continue
-			}
-			fmt.Fprintln(&buf, cfg.String())
-		}
-		return buf.String()
-	})
-}

pkg/cli/flags.go

@@ -529,7 +529,7 @@ func init() {
 		cliflagcfg.StringFlag(f, &deprecatedStorageEngine, cliflags.StorageEngine)
 		_ = pf.MarkHidden(cliflags.StorageEngine.Name)
 
-		cliflagcfg.VarFlag(f, &serverCfg.StorageConfig.WALFailover, cliflags.WALFailover)
+		cliflagcfg.VarFlag(f, &walFailoverWrapper{cfg: &serverCfg.StorageConfig.WALFailover}, cliflags.WALFailover)
 		// TODO(storage): Consider combining the uri and cache manual settings.
 		// Alternatively remove the ability to configure shared storage without
 		// passing a bootstrap configuration file.

pkg/cli/flags_test.go

@@ -29,10 +29,12 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/server/status"
 	"github.com/cockroachdb/cockroach/pkg/storage/storageconfig"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/datadriven"
 	"github.com/pmezard/go-difflib/difflib"
 	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/assert"
@@ -1863,3 +1865,20 @@ func TestParseEncryptionSpec(t *testing.T) {
 		}
 	}
 }
+
+func TestWALFailoverWrapperRoundtrip(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	datadriven.RunTest(t, datapathutils.TestDataPath(t, "wal-failover-config"), func(t *testing.T, d *datadriven.TestData) string {
+		var buf bytes.Buffer
+		for _, l := range strings.Split(d.Input, "\n") {
+			cfg := walFailoverWrapper{cfg: &storageconfig.WALFailover{}}
+			if err := cfg.Set(l); err != nil {
+				fmt.Fprintf(&buf, "err: %s\n", err)
+				continue
+			}
+			fmt.Fprintln(&buf, cfg.String())
+		}
+		return buf.String()
+	})
+}

pkg/cli/flags_util.go

@@ -706,3 +706,28 @@ func getAbsoluteFSPath(fieldName string, p string) (string, error) {
 	}
 	return ret, nil
 }
+
+// walFailoverWrapper implements pflag.Value for the wal failover flag.
+type walFailoverWrapper struct {
+	cfg *storageconfig.WALFailover
+}
+
+var _ pflag.Value = (*walFailoverWrapper)(nil)
+
+// Type implements the pflag.Value interface.
+func (c *walFailoverWrapper) Type() string { return "string" }
+
+// String implements the pflag.Value interface.
+func (c *walFailoverWrapper) String() string {
+	return c.cfg.String()
+}
+
+// Set implements the pflag.Value interface.
+func (c *walFailoverWrapper) Set(s string) error {
+	cfg, err := storageconfig.ParseWALFailover(s)
+	if err != nil {
+		return err
+	}
+	*c.cfg = cfg
+	return nil
+}

pkg/base/testdata/wal-failover-config renamed to pkg/cli/testdata/wal-failover-config

@@ -101,7 +101,9 @@ func TestWALFailover(t *testing.T) {
 
 				var cfg storageconfig.WALFailover
 				if flagStr != "" {
-					if err := cfg.Set(flagStr); err != nil {
+					var err error
+					cfg, err = storageconfig.ParseWALFailover(flagStr)
+					if err != nil {
 						return fmt.Sprintf("error parsing flag: %q", err)
 					}
 				}

pkg/storage/open_test.go

@@ -43,6 +43,9 @@ type ExternalPath struct {
 	Encryption *EncryptionOptions
 }
 
+// IsSet returns whether the path was provided.
+func (e ExternalPath) IsSet() bool { return e.Path != "" }
+
 // WALFailover is the configuration for write-ahead log (WAL) failover, used
 // to temporarily write WALs to a separate location when disk
 // stalls are encountered.
@@ -62,10 +65,13 @@ type WALFailover struct {
 	PrevPath ExternalPath
 }
 
-// Type implements the pflag.Value interface.
-func (c *WALFailover) Type() string { return "string" }
-
 // String implements fmt.Stringer.
+//
+// Representation:
+// - DefaultMode: ""
+// - Disabled: "disabled[,prev_path=<prev_path>]"
+// - AmongStores: "among-stores"
+// - ToExplicitPath: "path=<path>[,prev_path=<prev_path>]"
 func (c *WALFailover) String() string {
 	return redact.StringWithoutMarkers(c)
 }
@@ -95,15 +101,18 @@ func (c *WALFailover) SafeFormat(p redact.SafePrinter, _ rune) {
 	}
 }
 
-// Set implements the pflag.Value interface.
-func (c *WALFailover) Set(s string) error {
+// ParseWALFailover parses a string in the format produced by String().
+//
+// Used to parse the --wal-failover command-line flag.
+func ParseWALFailover(s string) (WALFailover, error) {
+	var c WALFailover
 	switch {
 	case strings.HasPrefix(s, "disabled"):
 		c.Mode = WALFailoverDisabled
 		var ok bool
 		c.Path.Path, c.PrevPath.Path, ok = parseWALFailoverPathFields(strings.TrimPrefix(s, "disabled"))
 		if !ok || c.Path.IsSet() {
-			return errors.Newf("invalid disabled --wal-failover setting: %s "+
+			return WALFailover{}, errors.Newf("invalid disabled --wal-failover setting: %s "+
 				"expect disabled[,prev_path=<prev_path>]", s)
 		}
 	case s == "among-stores":
@@ -113,14 +122,14 @@ func (c *WALFailover) Set(s string) error {
 		var ok bool
 		c.Path.Path, c.PrevPath.Path, ok = parseWALFailoverPathFields(s)
 		if !ok || !c.Path.IsSet() {
-			return errors.Newf("invalid path --wal-failover setting: %s "+
+			return WALFailover{}, errors.Newf("invalid path --wal-failover setting: %s "+
 				"expect path=<path>[,prev_path=<prev_path>]", s)
 		}
 	default:
-		return errors.Newf("invalid --wal-failover setting: %s "+
+		return WALFailover{}, errors.Newf("invalid --wal-failover setting: %s "+
 			"(possible values: disabled, among-stores, path=<path>)", s)
 	}
-	return nil
+	return c, nil
 }
 
 func parseWALFailoverPathFields(s string) (path, prevPath string, ok bool) {
@@ -144,6 +153,3 @@ func parseWALFailoverPathFields(s string) (path, prevPath string, ok bool) {
 	prevPath = strings.TrimPrefix(s, ",prev_path=")
 	return path, prevPath, true
 }
-
-// IsSet returns whether or not the path was provided.
-func (e ExternalPath) IsSet() bool { return e.Path != "" }