feat: improved config and env override handling (#27383)

* feat: improved config and env override handling

- The following config parameters now support size suffixes:
  - query-initial-memory-bytes
  - query-memory-bytes
  - query-max-memory-bytes
- The following parameters are now range checked properly to prevent
  compaction issues and log corruption with values that overflow
  internal size limits:
  - aggressive-points-per-block
  - compact-throughput
  - compact-throughput-burst
  - max-index-log-file-size
- Fix potential loss of accuracy when displaying default parameter
  values when displaying help
- Fix potential panics for default parameter values when displaying help

Author: gwossum

cmd/influxd/launcher/cmd.go

@@ -17,6 +17,7 @@ import (
 	"github.com/influxdata/influxdb/v2/pprof"
 	"github.com/influxdata/influxdb/v2/sqlite"
 	"github.com/influxdata/influxdb/v2/storage"
+	"github.com/influxdata/influxdb/v2/toml"
 	"github.com/influxdata/influxdb/v2/v1/coordinator"
 	"github.com/influxdata/influxdb/v2/vault"
 	"github.com/spf13/cobra"
@@ -182,9 +183,9 @@ type InfluxdOpts struct {
 
 	// Query options.
 	ConcurrencyQuota                int32
-	InitialMemoryBytesQuotaPerQuery int64
-	MemoryBytesQuotaPerQuery        int64
-	MaxMemoryBytes                  int64
+	InitialMemoryBytesQuotaPerQuery toml.SSize
+	MemoryBytesQuotaPerQuery        toml.SSize
+	MaxMemoryBytes                  toml.SSize
 	QueueSize                       int32
 	CoordinatorConfig               coordinator.Config
 
@@ -473,6 +474,9 @@ func (o *InfluxdOpts) BindCliOpts() []cli.Opt {
 			Default: o.ConcurrencyQuota,
 			Desc:    "the number of queries that are allowed to execute concurrently. Set to 0 to allow an unlimited number of concurrent queries",
 		},
+		// Default on the next three Opts is documentary: for a pflag.Value
+		// DestP, pflag.Var already uses destP's NewOpts-set value as the help
+		// default, so omitting Default would not change behavior.
 		{
 			DestP:   &o.InitialMemoryBytesQuotaPerQuery,
 			Flag:    "query-initial-memory-bytes",

cmd/influxd/launcher/launcher.go

@@ -434,9 +434,9 @@ func (m *Launcher) run(ctx context.Context, opts *InfluxdOpts) (err error) {
 
 	m.queryController, err = control.New(control.Config{
 		ConcurrencyQuota:                opts.ConcurrencyQuota,
-		InitialMemoryBytesQuotaPerQuery: opts.InitialMemoryBytesQuotaPerQuery,
-		MemoryBytesQuotaPerQuery:        opts.MemoryBytesQuotaPerQuery,
-		MaxMemoryBytes:                  opts.MaxMemoryBytes,
+		InitialMemoryBytesQuotaPerQuery: int64(opts.InitialMemoryBytesQuotaPerQuery),
+		MemoryBytesQuotaPerQuery:        int64(opts.MemoryBytesQuotaPerQuery),
+		MaxMemoryBytes:                  int64(opts.MaxMemoryBytes),
 		QueueSize:                       opts.QueueSize,
 		ExecutorDependencies:            dependencyList,
 		FluxLogEnabled:                  opts.FluxLogEnabled,

kit/cli/viper.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"os"
 	"path"
+	"reflect"
 	"strings"
 	"time"
 
@@ -296,7 +297,26 @@ func BindOptions(v *viper.Viper, cmd *cobra.Command, opts []Opt) error {
 				flagset.Var(destP, o.Flag, o.Desc)
 			}
 			if o.Default != nil {
-				_ = destP.Set(o.Default.(string))
+				// Prefer a direct value copy when Default's concrete type
+				// matches destP's pointee type. This bypasses the Stringer →
+				// Set round-trip, which is lossy for types whose String() goes
+				// through humanize.IBytes (toml.Size / toml.SSize emit a
+				// rounded form like "24 MiB" for non-power-of-2 byte counts,
+				// which then parses back to a different value). Falling
+				// through to cast.ToStringE keeps Defaults like a plain string
+				// or untyped int working for callers whose Default differs in
+				// type from DestP — e.g. Default: "on" for a customFlag.
+				dv := reflect.ValueOf(o.Default)
+				pv := reflect.ValueOf(destP)
+				if dv.IsValid() && pv.Kind() == reflect.Ptr && dv.Type() == pv.Elem().Type() {
+					pv.Elem().Set(dv)
+				} else {
+					s, err := cast.ToStringE(o.Default)
+					if err != nil {
+						return fmt.Errorf("flag %q: cannot resolve Default of type %T: %w", o.Flag, o.Default, err)
+					}
+					_ = destP.Set(s)
+				}
 			}
 			if err := v.BindPFlag(o.Flag, flagset.Lookup(o.Flag)); err != nil {
 				return fmt.Errorf("failed to bind flag %q: %w", o.Flag, err)

kit/cli/viper_test.go

@@ -315,6 +315,87 @@ func yamlConfigWriter(shortExt bool) configWriter {
 	}
 }
 
+// Test_PFlagValueTypedDefault confirms the pflag.Value binding path
+// resolves Defaults of various concrete types into the destP value: a
+// string Default goes through the cast.ToStringE → Set path, a typed
+// Default whose type matches destP takes the direct-copy fast path, and
+// a Default whose type neither matches destP nor casts to a string is
+// rejected at bind time with an error naming the flag and offending type.
+func Test_PFlagValueTypedDefault(t *testing.T) {
+	tests := []struct {
+		name             string
+		dflt             interface{}
+		want             customFlag
+		wantBindErrParts []string // non-empty means NewCommand is expected to fail
+	}{
+		{name: "string default still works", dflt: "on", want: customFlag(true)},
+		{name: "typed Stringer default", dflt: customFlag(true), want: customFlag(true)},
+		{
+			name:             "non-stringable default is rejected",
+			dflt:             struct{}{},
+			wantBindErrParts: []string{`flag "fancy-bool"`, "cannot resolve Default", "struct {}"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var got customFlag
+			cmd, err := NewCommand(viper.New(), &Program{
+				Name: "test",
+				Run:  func() error { return nil },
+				Opts: []Opt{
+					{DestP: &got, Flag: "fancy-bool", Default: tt.dflt},
+				},
+			})
+			if len(tt.wantBindErrParts) > 0 {
+				require.Error(t, err)
+				for _, p := range tt.wantBindErrParts {
+					require.Contains(t, err.Error(), p)
+				}
+				return
+			}
+			require.NoError(t, err)
+			cmd.SetArgs([]string{})
+			require.NoError(t, cmd.Execute())
+			require.Equal(t, tt.want, got)
+		})
+	}
+}
+
+// lossyValue is a pflag.Value whose String() form is deliberately not
+// reversible by Set(): String returns a constant token, and Set ignores its
+// input and assigns a sentinel value. This makes any Stringer→Set round-trip
+// observable: if the kit ever regresses to round-tripping typed Defaults
+// through cast.ToStringE, dest will land on the sentinel instead of the
+// caller's value.
+type lossyValue uint64
+
+func (v lossyValue) String() string    { return "lossy" }
+func (v *lossyValue) Set(string) error { *v = 999; return nil }
+func (v *lossyValue) Type() string     { return "lossy" }
+
+// Test_PFlagValueTypedDefault_BypassesLossyStringer pins down that when
+// Default's concrete type matches destP's pointee type, the kit copies the
+// value directly rather than going through Stringer→Set. Without that
+// bypass, the Default would be silently corrupted for any pflag.Value type
+// whose String() is lossy (the motivating real-world case is toml.Size and
+// toml.SSize, whose humanize.IBytes formatting drifts non-power-of-2 byte
+// counts across a marshal/unmarshal cycle).
+func Test_PFlagValueTypedDefault_BypassesLossyStringer(t *testing.T) {
+	var got lossyValue
+	cmd, err := NewCommand(viper.New(), &Program{
+		Name: "test",
+		Run:  func() error { return nil },
+		Opts: []Opt{
+			{DestP: &got, Flag: "lossy", Default: lossyValue(42)},
+		},
+	})
+	require.NoError(t, err)
+	cmd.SetArgs([]string{})
+	require.NoError(t, cmd.Execute())
+	require.Equal(t, lossyValue(42), got, "typed Default should be copied directly, not round-tripped through Stringer/Set")
+}
+
 func Test_RequiredFlag(t *testing.T) {
 	var testVar string
 	program := &Program{

tsdb/config.go

@@ -245,5 +245,22 @@ func (c *Config) Validate() error {
 		return fmt.Errorf("unrecognized index %s", c.Index)
 	}
 
+	// These sizes are uint64 on disk but are passed to rate limiters (int) and
+	// the tsi1 log file writer (int64); an oversized config value would
+	// otherwise silently wrap to a negative number and disable rate limiting
+	// or corrupt log-file size accounting.
+	if _, err := c.CompactThroughput.ToInt(); err != nil {
+		return fmt.Errorf("compact-throughput: %w", err)
+	}
+	if _, err := c.CompactThroughputBurst.ToInt(); err != nil {
+		return fmt.Errorf("compact-throughput-burst: %w", err)
+	}
+	if _, err := c.AggressivePointsPerBlock.ToInt(); err != nil {
+		return fmt.Errorf("aggressive-points-per-block: %w", err)
+	}
+	if _, err := c.MaxIndexLogFileSize.ToInt64(); err != nil {
+		return fmt.Errorf("max-index-log-file-size: %w", err)
+	}
+
 	return nil
 }

tsdb/config_test.go

@@ -1,12 +1,14 @@
 package tsdb_test
 
 import (
+	"math"
 	"testing"
 	"time"
 
 	"github.com/BurntSushi/toml"
 	"github.com/stretchr/testify/require"
 
+	itoml "github.com/influxdata/influxdb/v2/toml"
 	"github.com/influxdata/influxdb/v2/tsdb"
 )
 
@@ -64,6 +66,63 @@ func TestConfig_Validate_Error(t *testing.T) {
 	}
 }
 
+// TestConfig_Validate_SizeOverflow exercises the overflow guards added to
+// Validate() for the toml.Size fields that downstream code casts to int or
+// int64. Without these guards, an oversized config value would silently wrap
+// (e.g., disabling rate limiting or corrupting log-file accounting).
+//
+// The intKind assertion pins down which conversion helper each field is
+// wired to: ToInt produces "exceeds maximum int value" while ToInt64
+// produces "exceeds maximum int64 value". Cross-wiring the two would still
+// trip the validator on most inputs but would fail to catch values in the
+// (math.MaxInt, math.MaxInt64] range on 32-bit platforms.
+func TestConfig_Validate_SizeOverflow(t *testing.T) {
+	tests := []struct {
+		name        string
+		mutate      func(*tsdb.Config)
+		fieldPrefix string
+		intKind     string
+	}{
+		{
+			name:        "compact-throughput",
+			mutate:      func(c *tsdb.Config) { c.CompactThroughput = math.MaxUint64 },
+			fieldPrefix: "compact-throughput:",
+			intKind:     "maximum int value",
+		},
+		{
+			name:        "compact-throughput-burst",
+			mutate:      func(c *tsdb.Config) { c.CompactThroughputBurst = math.MaxUint64 },
+			fieldPrefix: "compact-throughput-burst:",
+			intKind:     "maximum int value",
+		},
+		{
+			name:        "aggressive-points-per-block",
+			mutate:      func(c *tsdb.Config) { c.AggressivePointsPerBlock = math.MaxUint64 },
+			fieldPrefix: "aggressive-points-per-block:",
+			intKind:     "maximum int value",
+		},
+		{
+			name:        "max-index-log-file-size",
+			mutate:      func(c *tsdb.Config) { c.MaxIndexLogFileSize = math.MaxUint64 },
+			fieldPrefix: "max-index-log-file-size:",
+			intKind:     "maximum int64 value",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c := tsdb.NewConfig()
+			c.Dir = "/var/lib/influxdb/data"
+			c.WALDir = "/var/lib/influxdb/wal"
+			tt.mutate(&c)
+			err := c.Validate()
+			require.Error(t, err)
+			require.Contains(t, err.Error(), tt.fieldPrefix)
+			require.Contains(t, err.Error(), tt.intKind)
+			require.ErrorIs(t, err, itoml.ErrSizeOutOfRange)
+		})
+	}
+}
+
 func TestConfig_ByteSizes(t *testing.T) {
 	// Parse configuration.
 	c := tsdb.NewConfig()