Skip to content

Commit f6e03c8

Browse files
RaduBerindeRaduBerinde
committed
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
1 parent 4c2edf3 commit f6e03c8

21 files changed

+217
-225
lines changed

‎pkg/acceptance/cluster/dockercluster.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -485,7 +485,7 @@ func (l *DockerCluster) startNode(ctx context.Context, node *testNode, singleNod
485485
for _, store := range node.stores {
486486
storeSpec := base.StoreSpec{
487487
Path: store.dir,
488-
Size: storageconfig.SizeSpec{Capacity: int64(store.config.MaxRanges) * maxRangeBytes},
488+
Size: storageconfig.Size{Bytes: int64(store.config.MaxRanges) * maxRangeBytes},
489489
}
490490
cmd = append(cmd, fmt.Sprintf("--store=%s", storeSpec))
491491
}

‎pkg/base/store_spec.go‎

Lines changed: 18 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,8 @@ func newStoreProvisionedRateSpec(
9393
// to the --store flag.
9494
type StoreSpec struct {
9595
Path string
96-
Size storageconfig.SizeSpec
97-
BallastSize *storageconfig.SizeSpec
96+
Size storageconfig.Size
97+
BallastSize *storageconfig.Size
9898
InMemory bool
9999
Attributes roachpb.Attributes
100100
// StickyVFSID is a unique identifier associated with a given store which
@@ -124,15 +124,15 @@ func (ss StoreSpec) String() string {
124124
if ss.InMemory {
125125
fmt.Fprint(&buffer, "type=mem,")
126126
}
127-
if ss.Size.Capacity > 0 {
128-
fmt.Fprintf(&buffer, "size=%s,", humanizeutil.IBytes(ss.Size.Capacity))
127+
if ss.Size.Bytes > 0 {
128+
fmt.Fprintf(&buffer, "size=%s,", humanizeutil.IBytes(ss.Size.Bytes))
129129
}
130130
if ss.Size.Percent > 0 {
131131
fmt.Fprintf(&buffer, "size=%s%%,", humanize.Ftoa(ss.Size.Percent))
132132
}
133133
if ss.BallastSize != nil {
134-
if ss.BallastSize.Capacity > 0 {
135-
fmt.Fprintf(&buffer, "ballast-size=%s,", humanizeutil.IBytes(ss.BallastSize.Capacity))
134+
if ss.BallastSize.Bytes > 0 {
135+
fmt.Fprintf(&buffer, "ballast-size=%s,", humanizeutil.IBytes(ss.BallastSize.Bytes))
136136
}
137137
if ss.BallastSize.Percent > 0 {
138138
fmt.Fprintf(&buffer, "ballast-size=%s%%,", humanize.Ftoa(ss.BallastSize.Percent))
@@ -229,30 +229,22 @@ func NewStoreSpec(value string) (StoreSpec, error) {
229229
ss.Path = value
230230
case "size":
231231
var err error
232-
var minBytesAllowed int64 = MinimumStoreSize
233-
var minPercent float64 = 1
234-
var maxPercent float64 = 100
235-
ss.Size, err = storageconfig.NewSizeSpec(
236-
"store",
237-
value,
238-
&storageconfig.IntInterval{Min: &minBytesAllowed},
239-
&storageconfig.FloatInterval{Min: &minPercent, Max: &maxPercent},
240-
)
232+
constraints := storageconfig.SizeSpecConstraints{
233+
MinBytes: MinimumStoreSize,
234+
MinPercent: 1,
235+
MaxPercent: 100,
236+
}
237+
ss.Size, err = storageconfig.ParseSizeSpec(value, constraints)
241238
if err != nil {
242239
return StoreSpec{}, err
243240
}
244241
case "ballast-size":
245-
var minBytesAllowed int64
246-
var minPercent float64 = 0
247-
var maxPercent float64 = 50
248-
ballastSize, err := storageconfig.NewSizeSpec(
249-
"ballast",
250-
value,
251-
&storageconfig.IntInterval{Min: &minBytesAllowed},
252-
&storageconfig.FloatInterval{Min: &minPercent, Max: &maxPercent},
253-
)
242+
constraints := storageconfig.SizeSpecConstraints{
243+
MaxPercent: 50,
244+
}
245+
ballastSize, err := storageconfig.ParseSizeSpec(value, constraints)
254246
if err != nil {
255-
return StoreSpec{}, err
247+
return StoreSpec{}, errors.Wrap(err, "ballast")
256248
}
257249
ss.BallastSize = &ballastSize
258250
case "attrs":
@@ -326,7 +318,7 @@ func NewStoreSpec(value string) (StoreSpec, error) {
326318
if ss.Path != "" {
327319
return StoreSpec{}, fmt.Errorf("path specified for in memory store")
328320
}
329-
if ss.Size.Percent == 0 && ss.Size.Capacity == 0 {
321+
if ss.Size.Percent == 0 && ss.Size.Bytes == 0 {
330322
return StoreSpec{}, fmt.Errorf("size must be specified for an in memory store")
331323
}
332324
if ss.BallastSize != nil {

‎pkg/base/store_spec_test.go‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -93,13 +93,13 @@ target_file_size=2097152`
9393
{"path=/mnt/hda1,attrs=hdd,attrs=ssd", "attrs field was used twice in store definition", StoreSpec{}},
9494

9595
// size
96-
{"path=/mnt/hda1,size=671088640", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Capacity: 671088640}}},
97-
{"path=/mnt/hda1,size=20GB", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Capacity: 20000000000}}},
98-
{"size=20GiB,path=/mnt/hda1", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Capacity: 21474836480}}},
99-
{"size=0.1TiB,path=/mnt/hda1", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Capacity: 109951162777}}},
100-
{"path=/mnt/hda1,size=.1TiB", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Capacity: 109951162777}}},
101-
{"path=/mnt/hda1,size=123TB", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Capacity: 123000000000000}}},
102-
{"path=/mnt/hda1,size=123TiB", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Capacity: 135239930216448}}},
96+
{"path=/mnt/hda1,size=671088640", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Bytes: 671088640}}},
97+
{"path=/mnt/hda1,size=20GB", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Bytes: 20000000000}}},
98+
{"size=20GiB,path=/mnt/hda1", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Bytes: 21474836480}}},
99+
{"size=0.1TiB,path=/mnt/hda1", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Bytes: 109951162777}}},
100+
{"path=/mnt/hda1,size=.1TiB", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Bytes: 109951162777}}},
101+
{"path=/mnt/hda1,size=123TB", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Bytes: 123000000000000}}},
102+
{"path=/mnt/hda1,size=123TiB", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Bytes: 135239930216448}}},
103103
// %
104104
{"path=/mnt/hda1,size=50.5%", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Percent: 50.5}}},
105105
{"path=/mnt/hda1,size=100%", "", StoreSpec{Path: "/mnt/hda1", Size: SizeSpec{Percent: 100}}},
@@ -124,18 +124,18 @@ target_file_size=2097152`
124124
{"size=123TB", "no path specified", StoreSpec{}},
125125

126126
// ballast size
127-
{"path=/mnt/hda1,ballast-size=671088640", "", StoreSpec{Path: "/mnt/hda1", BallastSize: &SizeSpec{Capacity: 671088640}}},
128-
{"path=/mnt/hda1,ballast-size=20GB", "", StoreSpec{Path: "/mnt/hda1", BallastSize: &SizeSpec{Capacity: 20000000000}}},
127+
{"path=/mnt/hda1,ballast-size=671088640", "", StoreSpec{Path: "/mnt/hda1", BallastSize: &SizeSpec{Bytes: 671088640}}},
128+
{"path=/mnt/hda1,ballast-size=20GB", "", StoreSpec{Path: "/mnt/hda1", BallastSize: &SizeSpec{Bytes: 20000000000}}},
129129
{"path=/mnt/hda1,ballast-size=1%", "", StoreSpec{Path: "/mnt/hda1", BallastSize: &SizeSpec{Percent: 1}}},
130130
{"path=/mnt/hda1,ballast-size=100.000%", "ballast size (100.000%) must be between 0.000000% and 50.000000%", StoreSpec{}},
131131
{"ballast-size=20GiB,path=/mnt/hda1,ballast-size=20GiB", "ballast-size field was used twice in store definition", StoreSpec{}},
132132

133133
// type
134-
{"type=mem,size=20GiB", "", StoreSpec{Size: SizeSpec{Capacity: 21474836480}, InMemory: true}},
135-
{"size=20GiB,type=mem", "", StoreSpec{Size: SizeSpec{Capacity: 21474836480}, InMemory: true}},
136-
{"size=20.5GiB,type=mem", "", StoreSpec{Size: SizeSpec{Capacity: 22011707392}, InMemory: true}},
134+
{"type=mem,size=20GiB", "", StoreSpec{Size: SizeSpec{Bytes: 21474836480}, InMemory: true}},
135+
{"size=20GiB,type=mem", "", StoreSpec{Size: SizeSpec{Bytes: 21474836480}, InMemory: true}},
136+
{"size=20.5GiB,type=mem", "", StoreSpec{Size: SizeSpec{Bytes: 22011707392}, InMemory: true}},
137137
{"size=20GiB,type=mem,attrs=mem", "", StoreSpec{
138-
Size: SizeSpec{Capacity: 21474836480},
138+
Size: SizeSpec{Bytes: 21474836480},
139139
InMemory: true,
140140
Attributes: roachpb.Attributes{Attrs: []string{"mem"}},
141141
}},
@@ -162,11 +162,11 @@ target_file_size=2097152`
162162
// all together
163163
{"path=/mnt/hda1,attrs=hdd:ssd,size=20GiB", "", StoreSpec{
164164
Path: "/mnt/hda1",
165-
Size: SizeSpec{Capacity: 21474836480},
165+
Size: SizeSpec{Bytes: 21474836480},
166166
Attributes: roachpb.Attributes{Attrs: []string{"hdd", "ssd"}},
167167
}},
168168
{"type=mem,attrs=hdd:ssd,size=20GiB", "", StoreSpec{
169-
Size: SizeSpec{Capacity: 21474836480},
169+
Size: SizeSpec{Bytes: 21474836480},
170170
InMemory: true,
171171
Attributes: roachpb.Attributes{Attrs: []string{"hdd", "ssd"}},
172172
}},
@@ -219,8 +219,8 @@ target_file_size=2097152`
219219
// StoreSpec aliases base.StoreSpec for convenience.
220220
type StoreSpec = base.StoreSpec
221221

222-
// SizeSpec aliases storageconfig.SizeSpec for convenience.
223-
type SizeSpec = storageconfig.SizeSpec
222+
// Size aliases storageconfig.Size for convenience.
223+
type SizeSpec = storageconfig.Size
224224

225225
func TestStoreSpecListPreventedStartupMessage(t *testing.T) {
226226
defer leaktest.AfterTest(t)()

‎pkg/base/test_server_args.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -596,8 +596,8 @@ var (
596596
// with no special attributes.
597597
DefaultTestStoreSpec = StoreSpec{
598598
InMemory: true,
599-
Size: storageconfig.SizeSpec{
600-
Capacity: 512 << 20,
599+
Size: storageconfig.Size{
600+
Bytes: 512 << 20,
601601
},
602602
}
603603
)

‎pkg/ccl/storageccl/engineccl/encrypted_fs_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ func TestPebbleEncryption(t *testing.T) {
252252
base.StoreSpec{
253253
InMemory: true,
254254
Attributes: roachpb.Attributes{},
255-
Size: storageconfig.SizeSpec{Capacity: 512 << 20},
255+
Size: storageconfig.Size{Bytes: 512 << 20},
256256
EncryptionOptions: encOptions,
257257
StickyVFSID: stickyVFSID,
258258
},
@@ -301,7 +301,7 @@ func TestPebbleEncryption(t *testing.T) {
301301
base.StoreSpec{
302302
InMemory: true,
303303
Attributes: roachpb.Attributes{},
304-
Size: storageconfig.SizeSpec{Capacity: 512 << 20},
304+
Size: storageconfig.Size{Bytes: 512 << 20},
305305
EncryptionOptions: encOptions,
306306
StickyVFSID: stickyVFSID,
307307
},
@@ -389,7 +389,7 @@ func TestPebbleEncryption2(t *testing.T) {
389389
base.StoreSpec{
390390
InMemory: true,
391391
Attributes: roachpb.Attributes{},
392-
Size: storageconfig.SizeSpec{Capacity: 512 << 20},
392+
Size: storageconfig.Size{Bytes: 512 << 20},
393393
EncryptionOptions: encOptions,
394394
StickyVFSID: stickyVFSID,
395395
},

‎pkg/cli/context.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,7 @@ var debugCtx struct {
453453
sizes bool
454454
replicated bool
455455
inputFile string
456-
ballastSize storageconfig.SizeSpec
456+
ballastSize storageconfig.Size
457457
maxResults int
458458
decodeAsTableDesc string
459459
verbose bool
@@ -470,7 +470,7 @@ func setDebugContextDefaults() {
470470
debugCtx.sizes = false
471471
debugCtx.replicated = false
472472
debugCtx.inputFile = ""
473-
debugCtx.ballastSize = storageconfig.SizeSpec{Capacity: 1000000000}
473+
debugCtx.ballastSize = storageconfig.Size{Bytes: 1000000000}
474474
debugCtx.maxResults = 0
475475
debugCtx.decodeAsTableDesc = ""
476476
debugCtx.verbose = false

‎pkg/cli/debug.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -401,7 +401,7 @@ func runDebugBallast(cmd *cobra.Command, args []string) error {
401401
if math.Abs(p) > 100 {
402402
return errors.Errorf("absolute percentage value %f greater than 100", p)
403403
}
404-
b := debugCtx.ballastSize.Capacity
404+
b := debugCtx.ballastSize.Bytes
405405
if p != 0 && b != 0 {
406406
return errors.New("expected exactly one of percentage or bytes non-zero, found both")
407407
}

‎pkg/cli/flags.go‎

Lines changed: 43 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,11 @@ import (
2929
"github.com/cockroachdb/cockroach/pkg/server/telemetry"
3030
"github.com/cockroachdb/cockroach/pkg/storage/storageconfig"
3131
"github.com/cockroachdb/cockroach/pkg/util/envutil"
32+
"github.com/cockroachdb/cockroach/pkg/util/humanizeutil"
3233
"github.com/cockroachdb/cockroach/pkg/util/log/logflags"
3334
"github.com/cockroachdb/cockroach/pkg/util/netutil/addr"
3435
"github.com/cockroachdb/errors"
36+
"github.com/dustin/go-humanize"
3537
"github.com/fsnotify/fsnotify"
3638
"github.com/spf13/cobra"
3739
"github.com/spf13/pflag"
@@ -530,7 +532,7 @@ func init() {
530532
// Alternatively remove the ability to configure shared storage without
531533
// passing a bootstrap configuration file.
532534
cliflagcfg.StringFlag(f, &serverCfg.StorageConfig.SharedStorage.URI, cliflags.SharedStorage)
533-
cliflagcfg.VarFlag(f, &serverCfg.StorageConfig.SharedStorage.Cache, cliflags.SecondaryCache)
535+
cliflagcfg.VarFlag(f, newSizeFlagVal(&serverCfg.StorageConfig.SharedStorage.Cache), cliflags.SecondaryCache)
534536
cliflagcfg.VarFlag(f, &serverCfg.MaxOffset, cliflags.MaxOffset)
535537
cliflagcfg.BoolFlag(f, &serverCfg.DisableMaxOffsetCheck, cliflags.DisableMaxOffsetCheck)
536538
cliflagcfg.StringFlag(f, &serverCfg.ClockDevicePath, cliflags.ClockDevice)
@@ -975,7 +977,7 @@ func init() {
975977
}
976978
{
977979
f := debugBallastCmd.Flags()
978-
cliflagcfg.VarFlag(f, &debugCtx.ballastSize, cliflags.Size)
980+
cliflagcfg.VarFlag(f, newSizeFlagVal(&debugCtx.ballastSize), cliflags.Size)
979981
}
980982
{
981983
// TODO(ayang): clean up so dir isn't passed to both pebble and --store
@@ -1479,7 +1481,7 @@ func mtStartSQLFlagsInit(cmd *cobra.Command) error {
14791481
if spec.BallastSize == nil {
14801482
// Only override if there was no ballast size specified to start
14811483
// with.
1482-
zero := storageconfig.SizeSpec{Capacity: 0, Percent: 0}
1484+
zero := storageconfig.Size{Bytes: 0, Percent: 0}
14831485
spec.BallastSize = &zero
14841486
}
14851487
}
@@ -1497,6 +1499,44 @@ func populateStoreSpecsEncryption() error {
14971499
)
14981500
}
14991501

1502+
// sizeFlagVal is a pflag.Value wrapper for storageconfig.Size. It can be
1503+
// set to an absolute bytes amount or a percentage.
1504+
type sizeFlagVal struct {
1505+
spec *storageconfig.Size
1506+
}
1507+
1508+
var _ pflag.Value = &sizeFlagVal{}
1509+
1510+
func newSizeFlagVal(spec *storageconfig.Size) *sizeFlagVal {
1511+
return &sizeFlagVal{spec: spec}
1512+
}
1513+
1514+
// String returns a string representation of the Size. It is part of the
1515+
// pflag.Value interface.
1516+
func (sv *sizeFlagVal) String() string {
1517+
if sv.spec.Percent != 0 {
1518+
return string(humanize.Ftoa(sv.spec.Percent)) + "%"
1519+
}
1520+
return string(humanizeutil.IBytes(sv.spec.Bytes))
1521+
}
1522+
1523+
// Type returns the underlying type in string form. It is part of the
1524+
// pflag.Value interface.
1525+
func (sv *sizeFlagVal) Type() string {
1526+
return "<bytes>|<percent%>"
1527+
}
1528+
1529+
// Set adds a new value to the StoreSpecValue. It is part of the pflag.Value
1530+
// interface.
1531+
func (sv *sizeFlagVal) Set(value string) error {
1532+
spec, err := storageconfig.ParseSizeSpec(value, storageconfig.SizeSpecConstraints{})
1533+
if err != nil {
1534+
return err
1535+
}
1536+
*sv.spec = spec
1537+
return nil
1538+
}
1539+
15001540
// RegisterFlags exists so that other packages can register flags using the
15011541
// Register<Type>FlagDepth functions and end up in a call frame in the cli
15021542
// package rather than the cliccl package to defeat the duplicate envvar

‎pkg/cli/flags_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1486,7 +1486,7 @@ func TestSQLPodStorageDefaults(t *testing.T) {
14861486
require.NoError(t, err)
14871487
assert.Equal(t, td.storePath, serverCfg.Stores.Specs[0].Path)
14881488
for _, s := range serverCfg.Stores.Specs {
1489-
assert.Zero(t, s.BallastSize.Capacity)
1489+
assert.Zero(t, s.BallastSize.Bytes)
14901490
assert.Zero(t, s.BallastSize.Percent)
14911491
}
14921492
} else {

‎pkg/cli/start_test.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -146,20 +146,20 @@ func TestStartArgChecking(t *testing.T) {
146146
{[]string{`--store=path=blah,path=blih`}, `field was used twice`},
147147
{[]string{`--store=path=~/blah`}, `path cannot start with '~': ~/blah`},
148148
{[]string{`--store=path=./~/blah`}, ``},
149-
{[]string{`--store=size=blih`}, `could not parse store size`},
150-
{[]string{`--store=size=0.005`}, `store size \(0.005\) must be between 1.000000% and 100.000000%`},
151-
{[]string{`--store=size=100.5`}, `store size \(100.5\) must be between 1.000000% and 100.000000%`},
152-
{[]string{`--store=size=0.5%`}, `store size \(0.5%\) must be between 1.000000% and 100.000000%`},
153-
{[]string{`--store=size=0.5%`}, `store size \(0.5%\) must be between 1.000000% and 100.000000%`},
154-
{[]string{`--store=size=500.0%`}, `store size \(500.0%\) must be between 1.000000% and 100.000000%`},
149+
{[]string{`--store=size=blih`}, `could not parse size`},
150+
{[]string{`--store=size=0.005`}, `size \(0.005\) must be between 1% and 100%`},
151+
{[]string{`--store=size=100.5`}, `size \(100.5\) must be between 1% and 100%`},
152+
{[]string{`--store=size=0.5%`}, `size \(0.5%\) must be between 1% and 100%`},
153+
{[]string{`--store=size=0.5%`}, `size \(0.5%\) must be between 1% and 100%`},
154+
{[]string{`--store=size=500.0%`}, `size \(500.0%\) must be between 1% and 100%`},
155155
{[]string{`--store=size=.5,path=.`}, ``},
156156
{[]string{`--store=size=50.%,path=.`}, ``},
157157
{[]string{`--store=size=50%,path=.`}, ``},
158158
{[]string{`--store=size=50.5%,path=.`}, ``},
159-
{[]string{`--store=size=-1231MB`}, `store size \(-1231MB\) must be larger than`},
160-
{[]string{`--store=size=1231B`}, `store size \(1231B\) must be larger than`},
159+
{[]string{`--store=size=-1231MB`}, `size \(-1231MB\) must be at least`},
160+
{[]string{`--store=size=1231B`}, `size \(1231B\) must be at least`},
161161
{[]string{`--store=size=1231BLA`}, `unhandled size name: bla`},
162-
{[]string{`--store=ballast-size=60.0`}, `ballast size \(60.0\) must be between 0.000000% and 50.000000%`},
162+
{[]string{`--store=ballast-size=60.0`}, `ballast: size \(60.0\) must be between 0% and 50%`},
163163
{[]string{`--store=ballast-size=1231BLA`}, `unhandled size name: bla`},
164164
{[]string{`--store=ballast-size=0.5%,path=.`}, ``},
165165
{[]string{`--store=ballast-size=.5,path=.`}, ``},

0 commit comments

Comments
 (0)