Merge #148338

148338: storageconfig: cleanup and move StoreSpec r=RaduBerinde a=RaduBerinde

#### storageconfig: clean up SizeSpec

This change renames SizeSpec to Size (everything in `storageconfig` is
a "spec"); cleans up the interface of Size; and separates the
pflag-specific code to a wrapper inside the `cli` package.

Epic: none
Release note: None

#### storageconfig: move provisioned rate

Move `base.ProvisionedRateSpec` to `storageconfig.ProvisionedRate`.

Epic: none
Release note: None

#### storageconfig: move StoreSpec

Epic: none
Release note: None

Co-authored-by: Radu Berinde <[email protected]>

Author: craig[bot]

pkg/acceptance/cluster/dockercluster.go

@@ -485,9 +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.SizeSpec{Capacity: int64(store.config.MaxRanges) * maxRangeBytes},
+			Size: storageconfig.Size{Bytes: int64(store.config.MaxRanges) * maxRangeBytes},
 		}
-		cmd = append(cmd, fmt.Sprintf("--store=%s", storeSpec))
+		cmd = append(cmd, fmt.Sprintf("--store=%s", base.StoreSpecCmdLineString(storeSpec)))
 	}
 	// Append --join flag for all nodes.
 	if !singleNode {

pkg/base/BUILD.bazel

@@ -63,7 +63,6 @@ go_test(
     data = glob(["testdata/**"]),
     embed = [":base"],
     deps = [
-        "//pkg/roachpb",
         "//pkg/security/securityassets",
         "//pkg/security/securitytest",
         "//pkg/storage/storageconfig",

pkg/base/store_spec.go

@@ -15,7 +15,6 @@ import (
 	"unicode"
 
 	"github.com/cockroachdb/cockroach/pkg/cli/cliflags"
-	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/storage/storageconfig"
 	"github.com/cockroachdb/cockroach/pkg/util/humanizeutil"
 	"github.com/cockroachdb/errors"
@@ -50,72 +49,45 @@ func GetAbsoluteFSPath(fieldName string, p string) (string, error) {
 	return ret, nil
 }
 
-// ProvisionedRateSpec is an optional part of the StoreSpec.
-type ProvisionedRateSpec struct {
-	// ProvisionedBandwidth is the bandwidth provisioned for this store in bytes/s.
-	ProvisionedBandwidth int64
-}
-
-func newStoreProvisionedRateSpec(
+func parseStoreProvisionedRate(
 	field redact.SafeString, value string,
-) (ProvisionedRateSpec, error) {
+) (storageconfig.ProvisionedRate, error) {
 	split := strings.Split(value, "=")
 	if len(split) != 2 {
-		return ProvisionedRateSpec{}, errors.Errorf("%s field has invalid value %s", field, value)
+		return storageconfig.ProvisionedRate{}, errors.Errorf("%s field has invalid value %s", field, value)
 	}
 	subField := split[0]
 	subValue := split[1]
 	if subField != "bandwidth" {
-		return ProvisionedRateSpec{}, errors.Errorf("%s field does not have bandwidth sub-field", field)
+		return storageconfig.ProvisionedRate{}, errors.Errorf("%s field does not have bandwidth sub-field", field)
 	}
 	if len(subValue) == 0 {
-		return ProvisionedRateSpec{}, errors.Errorf("%s field has no value specified for bandwidth", field)
+		return storageconfig.ProvisionedRate{}, errors.Errorf("%s field has no value specified for bandwidth", field)
 	}
 	if len(subValue) <= 2 || subValue[len(subValue)-2:] != "/s" {
-		return ProvisionedRateSpec{},
+		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 ProvisionedRateSpec{},
+		return storageconfig.ProvisionedRate{},
 			errors.Wrapf(err, "could not parse bandwidth in field %s", field)
 	}
 	if bandwidth == 0 {
-		return ProvisionedRateSpec{},
+		return storageconfig.ProvisionedRate{},
 			errors.Errorf("%s field is trying to set bandwidth to 0", field)
 	}
-	return ProvisionedRateSpec{ProvisionedBandwidth: bandwidth}, nil
+	return storageconfig.ProvisionedRate{ProvisionedBandwidth: bandwidth}, nil
 }
 
 // StoreSpec contains the details that can be specified in the cli pertaining
 // to the --store flag.
-type StoreSpec struct {
-	Path        string
-	Size        storageconfig.SizeSpec
-	BallastSize *storageconfig.SizeSpec
-	InMemory    bool
-	Attributes  roachpb.Attributes
-	// StickyVFSID is a unique identifier associated with a given store which
-	// will preserve the in-memory virtual file system (VFS) even after the
-	// storage engine has been closed. This only applies to in-memory storage
-	// engine.
-	StickyVFSID string
-	// PebbleOptions contains Pebble-specific options in the same format as a
-	// Pebble OPTIONS file. For example:
-	// [Options]
-	// delete_range_flush_delay=2s
-	// flush_split_bytes=4096
-	PebbleOptions string
-	// EncryptionOptions is set if encryption is enabled.
-	EncryptionOptions *storageconfig.EncryptionOptions
-	// ProvisionedRateSpec is optional.
-	ProvisionedRateSpec ProvisionedRateSpec
-}
+type StoreSpec = storageconfig.Store
 
-// String returns a fully parsable version of the store spec.
-func (ss StoreSpec) String() string {
+// StoreSpecCmdLineString returns a fully parsable version of the store spec.
+func StoreSpecCmdLineString(ss storageconfig.Store) string {
 	// TODO(jackson): Implement redact.SafeFormatter
 	var buffer bytes.Buffer
 	if len(ss.Path) != 0 {
@@ -124,23 +96,23 @@ func (ss StoreSpec) String() string {
 	if ss.InMemory {
 		fmt.Fprint(&buffer, "type=mem,")
 	}
-	if ss.Size.Capacity > 0 {
-		fmt.Fprintf(&buffer, "size=%s,", humanizeutil.IBytes(ss.Size.Capacity))
+	if ss.Size.Bytes > 0 {
+		fmt.Fprintf(&buffer, "size=%s,", humanizeutil.IBytes(ss.Size.Bytes))
 	}
 	if ss.Size.Percent > 0 {
 		fmt.Fprintf(&buffer, "size=%s%%,", humanize.Ftoa(ss.Size.Percent))
 	}
 	if ss.BallastSize != nil {
-		if ss.BallastSize.Capacity > 0 {
-			fmt.Fprintf(&buffer, "ballast-size=%s,", humanizeutil.IBytes(ss.BallastSize.Capacity))
+		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 len(ss.Attributes.Attrs) > 0 {
+	if len(ss.Attributes) > 0 {
 		fmt.Fprint(&buffer, "attrs=")
-		for i, attr := range ss.Attributes.Attrs {
+		for i, attr := range ss.Attributes {
 			if i != 0 {
 				fmt.Fprint(&buffer, ":")
 			}
@@ -154,9 +126,9 @@ func (ss StoreSpec) String() string {
 		fmt.Fprint(&buffer, optsStr)
 		fmt.Fprint(&buffer, ",")
 	}
-	if ss.ProvisionedRateSpec.ProvisionedBandwidth > 0 {
+	if ss.ProvisionedRate.ProvisionedBandwidth > 0 {
 		fmt.Fprintf(&buffer, "provisioned-rate=bandwidth=%s/s,",
-			humanizeutil.IBytes(ss.ProvisionedRateSpec.ProvisionedBandwidth))
+			humanizeutil.IBytes(ss.ProvisionedRate.ProvisionedBandwidth))
 	}
 	// Trim the extra comma from the end if it exists.
 	if l := buffer.Len(); l > 0 {
@@ -165,11 +137,6 @@ func (ss StoreSpec) String() string {
 	return buffer.String()
 }
 
-// IsEncrypted returns whether the StoreSpec has encryption enabled.
-func (ss StoreSpec) IsEncrypted() bool {
-	return ss.EncryptionOptions != nil
-}
-
 // NewStoreSpec parses the string passed into a --store flag and returns a
 // StoreSpec if it is correctly parsed.
 // There are five possible fields that can be passed in, comma separated:
@@ -229,30 +196,22 @@ func NewStoreSpec(value string) (StoreSpec, error) {
 			ss.Path = value
 		case "size":
 			var err error
-			var minBytesAllowed int64 = MinimumStoreSize
-			var minPercent float64 = 1
-			var maxPercent float64 = 100
-			ss.Size, err = storageconfig.NewSizeSpec(
-				"store",
-				value,
-				&storageconfig.IntInterval{Min: &minBytesAllowed},
-				&storageconfig.FloatInterval{Min: &minPercent, Max: &maxPercent},
-			)
+			constraints := storageconfig.SizeSpecConstraints{
+				MinBytes:   MinimumStoreSize,
+				MinPercent: 1,
+				MaxPercent: 100,
+			}
+			ss.Size, err = storageconfig.ParseSizeSpec(value, constraints)
 			if err != nil {
 				return StoreSpec{}, err
 			}
 		case "ballast-size":
-			var minBytesAllowed int64
-			var minPercent float64 = 0
-			var maxPercent float64 = 50
-			ballastSize, err := storageconfig.NewSizeSpec(
-				"ballast",
-				value,
-				&storageconfig.IntInterval{Min: &minBytesAllowed},
-				&storageconfig.FloatInterval{Min: &minPercent, Max: &maxPercent},
-			)
+			constraints := storageconfig.SizeSpecConstraints{
+				MaxPercent: 50,
+			}
+			ballastSize, err := storageconfig.ParseSizeSpec(value, constraints)
 			if err != nil {
-				return StoreSpec{}, err
+				return StoreSpec{}, errors.Wrap(err, "ballast")
 			}
 			ss.BallastSize = &ballastSize
 		case "attrs":
@@ -265,9 +224,9 @@ func NewStoreSpec(value string) (StoreSpec, error) {
 				attrMap[attribute] = struct{}{}
 			}
 			for attribute := range attrMap {
-				ss.Attributes.Attrs = append(ss.Attributes.Attrs, attribute)
+				ss.Attributes = append(ss.Attributes, attribute)
 			}
-			sort.Strings(ss.Attributes.Attrs)
+			sort.Strings(ss.Attributes)
 		case "type":
 			if value == "mem" {
 				ss.InMemory = true
@@ -311,11 +270,11 @@ func NewStoreSpec(value string) (StoreSpec, error) {
 			}
 			ss.PebbleOptions = buf.String()
 		case "provisioned-rate":
-			rateSpec, err := newStoreProvisionedRateSpec("provisioned-rate", value)
+			rateSpec, err := parseStoreProvisionedRate("provisioned-rate", value)
 			if err != nil {
 				return StoreSpec{}, err
 			}
-			ss.ProvisionedRateSpec = rateSpec
+			ss.ProvisionedRate = rateSpec
 
 		default:
 			return StoreSpec{}, fmt.Errorf("%s is not a valid store field", field)
@@ -326,7 +285,7 @@ func NewStoreSpec(value string) (StoreSpec, error) {
 		if ss.Path != "" {
 			return StoreSpec{}, fmt.Errorf("path specified for in memory store")
 		}
-		if ss.Size.Percent == 0 && ss.Size.Capacity == 0 {
+		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 {
@@ -352,7 +311,7 @@ var _ pflag.Value = &StoreSpecList{}
 func (ssl StoreSpecList) String() string {
 	var buffer bytes.Buffer
 	for _, ss := range ssl.Specs {
-		fmt.Fprintf(&buffer, "--%s=%s ", cliflags.Store.Name, ss)
+		fmt.Fprintf(&buffer, "--%s=%s ", cliflags.Store.Name, StoreSpecCmdLineString(ss))
 	}
 	// Trim the extra space from the end if it exists.
 	if l := buffer.Len(); l > 0 {
@@ -397,7 +356,10 @@ func (ssl StoreSpecList) PriorCriticalAlertError() (err error) {
 		err = errors.WithDetailf(err, "%v", newErr)
 	}
 	for _, ss := range ssl.Specs {
-		path := ss.PreventedStartupFile()
+		if ss.InMemory {
+			continue
+		}
+		path := PreventedStartupFile(filepath.Join(ss.Path, AuxiliaryDir))
 		if path == "" {
 			continue
 		}
@@ -413,16 +375,6 @@ func (ssl StoreSpecList) PriorCriticalAlertError() (err error) {
 	return err
 }
 
-// PreventedStartupFile returns the path to a file which, if it exists, should
-// prevent the server from starting up. Returns an empty string for in-memory
-// engines.
-func (ss StoreSpec) PreventedStartupFile() string {
-	if ss.InMemory {
-		return ""
-	}
-	return PreventedStartupFile(filepath.Join(ss.Path, AuxiliaryDir))
-}
-
 // Type returns the underlying type in string form. This is part of pflag's
 // value interface.
 func (ssl *StoreSpecList) Type() string {