Merge #154205

154205: storageconf: support YAML r=RaduBerinde a=RaduBerinde

Add support for marshaling `storageconfig.Store` as yaml and parsing
it back. This is in support of switching from command-line flags to a
configuration file.

Size is now a tri-state that differentiates between unset or set to 0
bytes.

We also clean up some of the related code and move the validation of a
store config into `storageconfig`.

This PR does not cover marshaling/unmarshaling of encryption options;
this will be in a follow-up PR.

Epic: none
Release note: None

Co-authored-by: Radu Berinde <radu@cockroachlabs.com>

Author: craig[bot]

pkg/acceptance/cluster/dockercluster.go

@@ -485,7 +485,9 @@ func (l *DockerCluster) startNode(ctx context.Context, node *testNode, singleNod
 	for _, store := range node.stores {
 		storeSpec := base.StoreSpec{
 			Path: store.dir,
-			Size: storageconfig.Size{Bytes: int64(store.config.MaxRanges) * maxRangeBytes},
+		}
+		if store.config.MaxRanges != 0 {
+			storeSpec.Size = storageconfig.BytesSize(int64(store.config.MaxRanges) * maxRangeBytes)
 		}
 		cmd = append(cmd, fmt.Sprintf("--store=%s", base.StoreSpecCmdLineString(storeSpec)))
 	}

pkg/base/store_spec.go

@@ -20,20 +20,13 @@ import (
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/errors/oserror"
 	"github.com/cockroachdb/pebble"
-	"github.com/cockroachdb/redact"
-	humanize "github.com/dustin/go-humanize"
+	"github.com/dustin/go-humanize"
 	"github.com/spf13/pflag"
 )
 
 // This file implements method receivers for server.Config struct
 // -- 'Stores', which satisfies pflag's value interface
 
-// MinimumStoreSize is the smallest size in bytes that a store can have. This
-// number is based on config's defaultZoneConfig's RangeMaxBytes, which is
-// extremely stable. To avoid adding the dependency on config here, it is just
-// hard coded to 640MiB.
-const MinimumStoreSize = 10 * 64 << 20
-
 // GetAbsoluteFSPath takes a (possibly relative) and returns the absolute path.
 // Returns an error if the path begins with '~' or Abs fails.
 // 'fieldName' is used in error strings.
@@ -49,39 +42,6 @@ func GetAbsoluteFSPath(fieldName string, p string) (string, error) {
 	return ret, nil
 }
 
-func parseStoreProvisionedRate(
-	field redact.SafeString, value string,
-) (storageconfig.ProvisionedRate, error) {
-	split := strings.Split(value, "=")
-	if len(split) != 2 {
-		return storageconfig.ProvisionedRate{}, errors.Errorf("%s field has invalid value %s", field, value)
-	}
-	subField := split[0]
-	subValue := split[1]
-	if subField != "bandwidth" {
-		return storageconfig.ProvisionedRate{}, errors.Errorf("%s field does not have bandwidth sub-field", field)
-	}
-	if len(subValue) == 0 {
-		return storageconfig.ProvisionedRate{}, errors.Errorf("%s field has no value specified for bandwidth", field)
-	}
-	if len(subValue) <= 2 || subValue[len(subValue)-2:] != "/s" {
-		return storageconfig.ProvisionedRate{},
-			errors.Errorf("%s field does not have bandwidth sub-field %s ending in /s",
-				field, subValue)
-	}
-	bandwidthString := subValue[:len(subValue)-2]
-	bandwidth, err := humanizeutil.ParseBytes(bandwidthString)
-	if err != nil {
-		return storageconfig.ProvisionedRate{},
-			errors.Wrapf(err, "could not parse bandwidth in field %s", field)
-	}
-	if bandwidth == 0 {
-		return storageconfig.ProvisionedRate{},
-			errors.Errorf("%s field is trying to set bandwidth to 0", field)
-	}
-	return storageconfig.ProvisionedRate{ProvisionedBandwidth: bandwidth}, nil
-}
-
 // StoreSpec contains the details that can be specified in the cli pertaining
 // to the --store flag.
 type StoreSpec = storageconfig.Store
@@ -96,19 +56,15 @@ func StoreSpecCmdLineString(ss storageconfig.Store) string {
 	if ss.InMemory {
 		fmt.Fprint(&buffer, "type=mem,")
 	}
-	if ss.Size.Bytes > 0 {
-		fmt.Fprintf(&buffer, "size=%s,", humanizeutil.IBytes(ss.Size.Bytes))
+	if ss.Size.IsBytes() {
+		fmt.Fprintf(&buffer, "size=%s,", humanizeutil.IBytes(ss.Size.Bytes()))
+	} else if ss.Size.IsPercent() {
+		fmt.Fprintf(&buffer, "size=%s%%,", humanize.Ftoa(ss.Size.Percent()))
 	}
-	if ss.Size.Percent > 0 {
-		fmt.Fprintf(&buffer, "size=%s%%,", humanize.Ftoa(ss.Size.Percent))
-	}
-	if ss.BallastSize != nil {
-		if ss.BallastSize.Bytes > 0 {
-			fmt.Fprintf(&buffer, "ballast-size=%s,", humanizeutil.IBytes(ss.BallastSize.Bytes))
-		}
-		if ss.BallastSize.Percent > 0 {
-			fmt.Fprintf(&buffer, "ballast-size=%s%%,", humanize.Ftoa(ss.BallastSize.Percent))
-		}
+	if ss.BallastSize.IsBytes() {
+		fmt.Fprintf(&buffer, "ballast-size=%s,", humanizeutil.IBytes(ss.BallastSize.Bytes()))
+	} else if ss.BallastSize.IsPercent() {
+		fmt.Fprintf(&buffer, "ballast-size=%s%%,", humanize.Ftoa(ss.BallastSize.Percent()))
 	}
 	if len(ss.Attributes) > 0 {
 		fmt.Fprint(&buffer, "attrs=")
@@ -196,24 +152,16 @@ func NewStoreSpec(value string) (StoreSpec, error) {
 			ss.Path = value
 		case "size":
 			var err error
-			constraints := storageconfig.SizeSpecConstraints{
-				MinBytes:   MinimumStoreSize,
-				MinPercent: 1,
-				MaxPercent: 100,
-			}
-			ss.Size, err = storageconfig.ParseSizeSpec(value, constraints)
+			ss.Size, err = storageconfig.ParseSizeSpec(value)
 			if err != nil {
 				return StoreSpec{}, err
 			}
 		case "ballast-size":
-			constraints := storageconfig.SizeSpecConstraints{
-				MaxPercent: 50,
-			}
-			ballastSize, err := storageconfig.ParseSizeSpec(value, constraints)
+			ballastSize, err := storageconfig.ParseSizeSpec(value)
 			if err != nil {
 				return StoreSpec{}, errors.Wrap(err, "ballast")
 			}
-			ss.BallastSize = &ballastSize
+			ss.BallastSize = ballastSize
 		case "attrs":
 			// Check to make sure there are no duplicate attributes.
 			attrMap := make(map[string]struct{})
@@ -270,7 +218,7 @@ func NewStoreSpec(value string) (StoreSpec, error) {
 			}
 			ss.PebbleOptions = buf.String()
 		case "provisioned-rate":
-			rateSpec, err := parseStoreProvisionedRate("provisioned-rate", value)
+			rateSpec, err := storageconfig.ParseProvisionedRate(value)
 			if err != nil {
 				return StoreSpec{}, err
 			}
@@ -280,19 +228,8 @@ func NewStoreSpec(value string) (StoreSpec, error) {
 			return StoreSpec{}, fmt.Errorf("%s is not a valid store field", field)
 		}
 	}
-	if ss.InMemory {
-		// Only in memory stores don't need a path and require a size.
-		if ss.Path != "" {
-			return StoreSpec{}, fmt.Errorf("path specified for in memory store")
-		}
-		if ss.Size.Percent == 0 && ss.Size.Bytes == 0 {
-			return StoreSpec{}, fmt.Errorf("size must be specified for an in memory store")
-		}
-		if ss.BallastSize != nil {
-			return StoreSpec{}, fmt.Errorf("ballast-size specified for in memory store")
-		}
-	} else if ss.Path == "" {
-		return StoreSpec{}, fmt.Errorf("no path specified")
+	if err := ss.Validate(); err != nil {
+		return StoreSpec{}, err
 	}
 	return ss, nil
 }

pkg/base/store_spec_test.go

@@ -92,53 +92,53 @@ target_file_size=2097152`
 		{"path=/mnt/hda1,attrs=hdd,attrs=ssd", "attrs field was used twice in store definition", StoreSpec{}},
 
 		// size
-		{"path=/mnt/hda1,size=671088640", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Bytes: 671088640}}},
-		{"path=/mnt/hda1,size=20GB", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Bytes: 20000000000}}},
-		{"size=20GiB,path=/mnt/hda1", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Bytes: 21474836480}}},
-		{"size=0.1TiB,path=/mnt/hda1", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Bytes: 109951162777}}},
-		{"path=/mnt/hda1,size=.1TiB", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Bytes: 109951162777}}},
-		{"path=/mnt/hda1,size=123TB", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Bytes: 123000000000000}}},
-		{"path=/mnt/hda1,size=123TiB", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Bytes: 135239930216448}}},
+		{"path=/mnt/hda1,size=671088640", "", StoreSpec{Path: "/mnt/hda1", Size: storageconfig.BytesSize(671088640)}},
+		{"path=/mnt/hda1,size=20GB", "", StoreSpec{Path: "/mnt/hda1", Size: storageconfig.BytesSize(20000000000)}},
+		{"size=20GiB,path=/mnt/hda1", "", StoreSpec{Path: "/mnt/hda1", Size: storageconfig.BytesSize(21474836480)}},
+		{"size=0.1TiB,path=/mnt/hda1", "", StoreSpec{Path: "/mnt/hda1", Size: storageconfig.BytesSize(109951162778)}},
+		{"path=/mnt/hda1,size=.1TiB", "", StoreSpec{Path: "/mnt/hda1", Size: storageconfig.BytesSize(109951162778)}},
+		{"path=/mnt/hda1,size=123TB", "", StoreSpec{Path: "/mnt/hda1", Size: storageconfig.BytesSize(123000000000000)}},
+		{"path=/mnt/hda1,size=123TiB", "", StoreSpec{Path: "/mnt/hda1", Size: storageconfig.BytesSize(135239930216448)}},
 		// %
-		{"path=/mnt/hda1,size=50.5%", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Percent: 50.5}}},
-		{"path=/mnt/hda1,size=100%", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Percent: 100}}},
-		{"path=/mnt/hda1,size=1%", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Percent: 1}}},
-		{"path=/mnt/hda1,size=0.999999%", "size (0.999999%) must be between 1% and 100%", StoreSpec{}},
-		{"path=/mnt/hda1,size=100.0001%", "size (100.0001%) must be between 1% and 100%", StoreSpec{}},
+		{"path=/mnt/hda1,size=50.5%", "", StoreSpec{Path: "/mnt/hda1", Size: storageconfig.PercentSize(50.5)}},
+		{"path=/mnt/hda1,size=100%", "", StoreSpec{Path: "/mnt/hda1", Size: storageconfig.PercentSize(100)}},
+		{"path=/mnt/hda1,size=1%", "", StoreSpec{Path: "/mnt/hda1", Size: storageconfig.PercentSize(1)}},
+		{"path=/mnt/hda1,size=0.999999%", "store size (0.999999%) must be at least 1%", StoreSpec{}},
+		{"path=/mnt/hda1,size=100.0001%", "size (100.0001%) must be between 0% and 100%", StoreSpec{}},
 		// 0.xxx
-		{"path=/mnt/hda1,size=0.99", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Percent: 99}}},
-		{"path=/mnt/hda1,size=0.5000000", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Percent: 50}}},
-		{"path=/mnt/hda1,size=0.01", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Percent: 1}}},
-		{"path=/mnt/hda1,size=0.009999", "size (0.009999) must be between 1% and 100%", StoreSpec{}},
+		{"path=/mnt/hda1,size=0.99", "", StoreSpec{Path: "/mnt/hda1", Size: storageconfig.PercentSize(99)}},
+		{"path=/mnt/hda1,size=0.5000000", "", StoreSpec{Path: "/mnt/hda1", Size: storageconfig.PercentSize(50)}},
+		{"path=/mnt/hda1,size=0.01", "", StoreSpec{Path: "/mnt/hda1", Size: storageconfig.PercentSize(1)}},
+		{"path=/mnt/hda1,size=0.009999", "store size (0.9999%) must be at least 1%", StoreSpec{}},
 		// .xxx
-		{"path=/mnt/hda1,size=.999", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Percent: 99.9}}},
-		{"path=/mnt/hda1,size=.5000000", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Percent: 50}}},
-		{"path=/mnt/hda1,size=.01", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Percent: 1}}},
-		{"path=/mnt/hda1,size=.009999", "size (.009999) must be between 1% and 100%", StoreSpec{}},
+		{"path=/mnt/hda1,size=.999", "", StoreSpec{Path: "/mnt/hda1", Size: storageconfig.PercentSize(99.9)}},
+		{"path=/mnt/hda1,size=.5000000", "", StoreSpec{Path: "/mnt/hda1", Size: storageconfig.PercentSize(50)}},
+		{"path=/mnt/hda1,size=.01", "", StoreSpec{Path: "/mnt/hda1", Size: storageconfig.PercentSize(1)}},
+		{"path=/mnt/hda1,size=.009999", "store size (0.9999%) must be at least 1%", StoreSpec{}},
 		// errors
-		{"path=/mnt/hda1,size=0", "size (0) must be at least 640 MiB", StoreSpec{}},
-		{"path=/mnt/hda1,size=abc", "could not parse size (abc): strconv.ParseFloat: parsing \"\": invalid syntax", StoreSpec{}},
+		{"path=/mnt/hda1,size=0", "store size (0 B) must be at least 640 MiB", StoreSpec{}},
+		{"path=/mnt/hda1,size=abc", "could not parse size (abc): cannot parse number from \"abc\"", StoreSpec{}},
 		{"path=/mnt/hda1,size=", "no value specified for size", StoreSpec{}},
 		{"size=20GiB,path=/mnt/hda1,size=20GiB", "size field was used twice in store definition", StoreSpec{}},
 		{"size=123TB", "no path specified", StoreSpec{}},
 
 		// ballast size
-		{"path=/mnt/hda1,ballast-size=671088640", "", StoreSpec{Path: "/mnt/hda1", BallastSize: &SizeSpec{Bytes: 671088640}}},
-		{"path=/mnt/hda1,ballast-size=20GB", "", StoreSpec{Path: "/mnt/hda1", BallastSize: &SizeSpec{Bytes: 20000000000}}},
-		{"path=/mnt/hda1,ballast-size=1%", "", StoreSpec{Path: "/mnt/hda1", BallastSize: &SizeSpec{Percent: 1}}},
-		{"path=/mnt/hda1,ballast-size=100.000%", "ballast: size (100.000%) must be between 0% and 50%", StoreSpec{}},
+		{"path=/mnt/hda1,ballast-size=671088640", "", StoreSpec{Path: "/mnt/hda1", BallastSize: storageconfig.BytesSize(671088640)}},
+		{"path=/mnt/hda1,ballast-size=20GB", "", StoreSpec{Path: "/mnt/hda1", BallastSize: storageconfig.BytesSize(20000000000)}},
+		{"path=/mnt/hda1,ballast-size=1%", "", StoreSpec{Path: "/mnt/hda1", BallastSize: storageconfig.PercentSize(1)}},
+		{"path=/mnt/hda1,ballast-size=100.000%", "ballast size (100%) must be at most 50%", StoreSpec{}},
 		{"ballast-size=20GiB,path=/mnt/hda1,ballast-size=20GiB", "ballast-size field was used twice in store definition", StoreSpec{}},
 
 		// type
-		{"type=mem,size=20GiB", "", StoreSpec{Size: SizeSpec{Bytes: 21474836480}, InMemory: true}},
-		{"size=20GiB,type=mem", "", StoreSpec{Size: SizeSpec{Bytes: 21474836480}, InMemory: true}},
-		{"size=20.5GiB,type=mem", "", StoreSpec{Size: SizeSpec{Bytes: 22011707392}, InMemory: true}},
+		{"type=mem,size=20GiB", "", StoreSpec{Size: storageconfig.BytesSize(21474836480), InMemory: true}},
+		{"size=20GiB,type=mem", "", StoreSpec{Size: storageconfig.BytesSize(21474836480), InMemory: true}},
+		{"size=20.5GiB,type=mem", "", StoreSpec{Size: storageconfig.BytesSize(22011707392), InMemory: true}},
 		{"size=20GiB,type=mem,attrs=mem", "", StoreSpec{
-			Size:       SizeSpec{Bytes: 21474836480},
+			Size:       storageconfig.BytesSize(21474836480),
 			InMemory:   true,
 			Attributes: []string{"mem"},
 		}},
-		{"type=mem,size=20", "size (20) must be at least 640 MiB", StoreSpec{}},
+		{"type=mem,size=20", "store size (20 B) must be at least 640 MiB", StoreSpec{}},
 		{"type=mem,size=", "no value specified for size", StoreSpec{}},
 		{"type=mem,attrs=ssd", "size must be specified for an in memory store", StoreSpec{}},
 		{"path=/mnt/hda1,type=mem", "path specified for in memory store", StoreSpec{}},
@@ -148,9 +148,9 @@ target_file_size=2097152`
 		// provisioned rate
 		{"path=/mnt/hda1,provisioned-rate=bandwidth=200MiB/s", "",
 			StoreSpec{Path: "/mnt/hda1", ProvisionedRate: storageconfig.ProvisionedRate{ProvisionedBandwidth: 200 << 20}}},
-		{"path=/mnt/hda1,provisioned-rate=bandwidth=200MiB", "provisioned-rate field does not have bandwidth sub-field 200MiB ending in /s", StoreSpec{}},
-		{"path=/mnt/hda1,provisioned-rate=200MiB/s", "provisioned-rate field has invalid value 200MiB/s", StoreSpec{}},
-		{"path=/mnt/hda1,provisioned-rate=bandwidth=0B/s", "provisioned-rate field is trying to set bandwidth to 0", StoreSpec{}},
+		{"path=/mnt/hda1,provisioned-rate=bandwidth=200MiB", "provisioned-rate has invalid bandwidth value \"200MiB\"", StoreSpec{}},
+		{"path=/mnt/hda1,provisioned-rate=200MiB/s", "provisioned-rate has invalid value \"200MiB/s\"", StoreSpec{}},
+		{"path=/mnt/hda1,provisioned-rate=bandwidth=0B/s", "provisioned-rate has invalid bandwidth value \"0B/s\"", StoreSpec{}},
 
 		// Pebble
 		{"path=/,pebble=[Options] l0_compaction_threshold=2 l0_stop_writes_threshold=10", "", StoreSpec{Path: "/",
@@ -161,11 +161,11 @@ target_file_size=2097152`
 		// all together
 		{"path=/mnt/hda1,attrs=hdd:ssd,size=20GiB", "", StoreSpec{
 			Path:       "/mnt/hda1",
-			Size:       SizeSpec{Bytes: 21474836480},
+			Size:       storageconfig.BytesSize(21474836480),
 			Attributes: []string{"hdd", "ssd"},
 		}},
 		{"type=mem,attrs=hdd:ssd,size=20GiB", "", StoreSpec{
-			Size:       SizeSpec{Bytes: 21474836480},
+			Size:       storageconfig.BytesSize(21474836480),
 			InMemory:   true,
 			Attributes: []string{"hdd", "ssd"},
 		}},
@@ -204,7 +204,7 @@ target_file_size=2097152`
 		storeSpecString := base.StoreSpecCmdLineString(storeSpec)
 		storeSpec2, err := base.NewStoreSpec(storeSpecString)
 		if err != nil {
-			t.Errorf("%d(%s): error parsing String() result: %s", i, testCase.value, err)
+			t.Errorf("%d(%s): error parsing String() result: %s\n%s", i, testCase.value, err, storeSpecString)
 			continue
 		}
 		// Compare strings to deal with floats not matching exactly.
@@ -218,9 +218,6 @@ target_file_size=2097152`
 // StoreSpec aliases base.StoreSpec for convenience.
 type StoreSpec = base.StoreSpec
 
-// Size aliases storageconfig.Size for convenience.
-type SizeSpec = storageconfig.Size
-
 func TestStoreSpecListPreventedStartupMessage(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 

pkg/base/test_server_args.go

@@ -613,9 +613,7 @@ func InternalNonDefaultDecision(
 // with no special attributes.
 var DefaultTestStoreSpec = storageconfig.Store{
 	InMemory: true,
-	Size: storageconfig.Size{
-		Bytes: 512 << 20,
-	},
+	Size:     storageconfig.BytesSize(512 << 20),
 }
 
 // DefaultTestTempStorageConfig is the associated temp storage for

pkg/ccl/storageccl/engineccl/encrypted_fs_test.go

@@ -252,7 +252,7 @@ func TestPebbleEncryption(t *testing.T) {
 			ctx,
 			base.StoreSpec{
 				InMemory:          true,
-				Size:              storageconfig.Size{Bytes: 512 << 20},
+				Size:              storageconfig.BytesSize(512 << 20),
 				EncryptionOptions: encOptions,
 				StickyVFSID:       stickyVFSID,
 			},
@@ -304,7 +304,7 @@ func TestPebbleEncryption(t *testing.T) {
 			ctx,
 			base.StoreSpec{
 				InMemory:          true,
-				Size:              storageconfig.Size{Bytes: 512 << 20},
+				Size:              storageconfig.BytesSize(512 << 20),
 				EncryptionOptions: encOptions,
 				StickyVFSID:       stickyVFSID,
 			},
@@ -395,7 +395,7 @@ func TestPebbleEncryption2(t *testing.T) {
 			ctx,
 			base.StoreSpec{
 				InMemory:          true,
-				Size:              storageconfig.Size{Bytes: 512 << 20},
+				Size:              storageconfig.BytesSize(512 << 20),
 				EncryptionOptions: encOptions,
 				StickyVFSID:       stickyVFSID,
 			},

pkg/cli/context.go

@@ -470,7 +470,7 @@ func setDebugContextDefaults() {
 	debugCtx.sizes = false
 	debugCtx.replicated = false
 	debugCtx.inputFile = ""
-	debugCtx.ballastSize = storageconfig.Size{Bytes: 1000000000}
+	debugCtx.ballastSize = storageconfig.BytesSize(1000000000)
 	debugCtx.maxResults = 0
 	debugCtx.decodeAsTableDesc = ""
 	debugCtx.verbose = false

pkg/cli/debug.go

@@ -397,13 +397,15 @@ func runDebugBallast(cmd *cobra.Command, args []string) error {
 	usedBytes := du.TotalBytes - du.AvailBytes
 
 	var targetUsage uint64
-	p := debugCtx.ballastSize.Percent
-	if math.Abs(p) > 100 {
-		return errors.Errorf("absolute percentage value %f greater than 100", p)
-	}
-	b := debugCtx.ballastSize.Bytes
-	if p != 0 && b != 0 {
-		return errors.New("expected exactly one of percentage or bytes non-zero, found both")
+	var p float64
+	var b int64
+	if debugCtx.ballastSize.IsPercent() {
+		p = debugCtx.ballastSize.Percent()
+		if math.Abs(p) > 100 {
+			return errors.Errorf("absolute percentage value %f greater than 100", p)
+		}
+	} else if debugCtx.ballastSize.IsBytes() {
+		b = debugCtx.ballastSize.Bytes()
 	}
 	switch {
 	case p > 0: