From ef58c97d45cf6e2aca4230a6b7cecd8ec3705837 Mon Sep 17 00:00:00 2001 From: Andy Asp <90626759+andyasp@users.noreply.github.com> Date: Mon, 28 Mar 2022 03:42:45 -0400 Subject: [PATCH] Add test for stale field category overrides (#1210) * Add test for stale field category overrides * Place flags in main into a taggable struct * Separate configFile and configExpandEnv from mainFlags * Revert to original parseConfigFileParameter style --- cmd/mimir/main.go | 82 ++++++++++++++++------------- cmd/mimir/main_test.go | 39 +++++++++++--- cmd/mimir/usage.go | 27 +++++----- pkg/util/fieldcategory/overrides.go | 10 ++-- 4 files changed, 98 insertions(+), 60 deletions(-) diff --git a/cmd/mimir/main.go b/cmd/mimir/main.go index 191a706b8a7..4777ee2c73b 100644 --- a/cmd/mimir/main.go +++ b/cmd/mimir/main.go @@ -46,31 +46,46 @@ func init() { const ( configFileOption = "config.file" - configExpandENV = "config.expand-env" + configExpandEnv = "config.expand-env" ) var testMode = false +type mainFlags struct { + ballastBytes int `category:"advanced"` + mutexProfileFraction int `category:"advanced"` + blockProfileRate int `category:"advanced"` + printVersion bool + printModules bool + printHelp bool + printHelpAll bool +} + +func (mf *mainFlags) registerFlags(fs *flag.FlagSet) { + fs.IntVar(&mf.ballastBytes, "mem-ballast-size-bytes", 0, "Size of memory ballast to allocate.") + fs.IntVar(&mf.mutexProfileFraction, "debug.mutex-profile-fraction", 0, "Fraction of mutex contention events that are reported in the mutex profile. On average 1/rate events are reported. 0 to disable.") + fs.IntVar(&mf.blockProfileRate, "debug.block-profile-rate", 0, "Fraction of goroutine blocking events that are reported in the blocking profile. 1 to include every blocking event in the profile, 0 to disable.") + fs.BoolVar(&mf.printVersion, "version", false, "Print application version and exit.") + fs.BoolVar(&mf.printModules, "modules", false, "List available values that can be used as target.") + fs.BoolVar(&mf.printHelp, "help", false, "Print basic help.") + fs.BoolVar(&mf.printHelp, "h", false, "Print basic help.") + fs.BoolVar(&mf.printHelpAll, "help-all", false, "Print help, also including advanced and experimental parameters.") +} + func main() { var ( - cfg mimir.Config - ballastBytes int - mutexProfileFraction int - blockProfileRate int - printVersion bool - printModules bool - printHelp bool - printHelpAll bool + cfg mimir.Config + mainFlags mainFlags ) - configFile, expandENV := parseConfigFileParameter(os.Args[1:]) + configFile, expandEnv := parseConfigFileParameter(os.Args[1:]) // This sets default values from flags to the config. // It needs to be called before parsing the config file! flagext.RegisterFlagsWithLogger(util_log.Logger, &cfg) if configFile != "" { - if err := LoadConfig(configFile, expandENV, &cfg); err != nil { + if err := LoadConfig(configFile, expandEnv, &cfg); err != nil { fmt.Fprintf(os.Stderr, "error loading config from %s: %v\n", configFile, err) if testMode { return @@ -79,18 +94,11 @@ func main() { } } - // Ignore -config.file and -config.expand-env here, since it was already parsed, but it's still present on command line. + // Ignore -config.file and -config.expand-env here, since they are parsed separately, but are still present on the command line. flagext.IgnoredFlag(flag.CommandLine, configFileOption, "Configuration file to load.") - _ = flag.CommandLine.Bool(configExpandENV, false, "Expands ${var} or $var in config according to the values of the environment variables.") + _ = flag.CommandLine.Bool(configExpandEnv, false, "Expands ${var} or $var in config according to the values of the environment variables.") - flag.IntVar(&ballastBytes, "mem-ballast-size-bytes", 0, "Size of memory ballast to allocate.") - flag.IntVar(&mutexProfileFraction, "debug.mutex-profile-fraction", 0, "Fraction of mutex contention events that are reported in the mutex profile. On average 1/rate events are reported. 0 to disable.") - flag.IntVar(&blockProfileRate, "debug.block-profile-rate", 0, "Fraction of goroutine blocking events that are reported in the blocking profile. 1 to include every blocking event in the profile, 0 to disable.") - flag.BoolVar(&printVersion, "version", false, "Print application version and exit.") - flag.BoolVar(&printModules, "modules", false, "List available values that can be used as target.") - flag.BoolVar(&printHelp, "help", false, "Print basic help.") - flag.BoolVar(&printHelp, "h", false, "Print basic help.") - flag.BoolVar(&printHelpAll, "help-all", false, "Print help, also including advanced and experimental parameters.") + mainFlags.registerFlags(flag.CommandLine) flag.CommandLine.Usage = func() { /* don't do anything by default, we will print usage ourselves, but only when requested. */ } flag.CommandLine.Init(flag.CommandLine.Name(), flag.ContinueOnError) @@ -102,10 +110,10 @@ func main() { } } - if printHelp || printHelpAll { + if mainFlags.printHelp || mainFlags.printHelpAll { // Print available parameters to stdout, so that users can grep/less them easily. flag.CommandLine.SetOutput(os.Stdout) - if err := usage(&cfg, printHelpAll); err != nil { + if err := usage(&mainFlags, &cfg); err != nil { fmt.Fprintf(os.Stderr, "error printing usage: %s\n", err) os.Exit(1) } @@ -116,7 +124,7 @@ func main() { return } - if printVersion { + if mainFlags.printVersion { fmt.Fprintln(os.Stdout, version.Print("Mimir")) return } @@ -132,22 +140,22 @@ func main() { // Continue on if -modules flag is given. Code handling the // -modules flag will not start mimir. - if testMode && !printModules { + if testMode && !mainFlags.printModules { DumpYaml(&cfg) return } - if mutexProfileFraction > 0 { - runtime.SetMutexProfileFraction(mutexProfileFraction) + if mainFlags.mutexProfileFraction > 0 { + runtime.SetMutexProfileFraction(mainFlags.mutexProfileFraction) } - if blockProfileRate > 0 { - runtime.SetBlockProfileRate(blockProfileRate) + if mainFlags.blockProfileRate > 0 { + runtime.SetBlockProfileRate(mainFlags.blockProfileRate) } util_log.InitLogger(&cfg.Server) // Allocate a block of memory to alter GC behaviour. See https://github.com/golang/go/issues/23044 - ballast := make([]byte, ballastBytes) + ballast := make([]byte, mainFlags.ballastBytes) // In testing mode skip JAEGER setup to avoid panic due to // "duplicate metrics collector registration attempted" @@ -171,7 +179,7 @@ func main() { t, err := mimir.New(cfg) util_log.CheckFatal("initializing application", err) - if printModules { + if mainFlags.printModules { allDeps := t.ModuleManager.DependenciesForModule(mimir.All) for _, m := range t.ModuleManager.UserVisibleModuleNames() { @@ -206,7 +214,7 @@ func parseConfigFileParameter(args []string) (configFile string, expandEnv bool) // usage not used in these functions. fs.StringVar(&configFile, configFileOption, "", "") - fs.BoolVar(&expandEnv, configExpandENV, false, "") + fs.BoolVar(&expandEnv, configExpandEnv, false, "") // Try to find -config.file and -config.expand-env option in the flags. As Parsing stops on the first error, eg. unknown flag, we simply // try remaining parameters until we find config flag, or there are no params left. @@ -220,7 +228,7 @@ func parseConfigFileParameter(args []string) (configFile string, expandEnv bool) } // LoadConfig read YAML-formatted config from filename into cfg. -func LoadConfig(filename string, expandENV bool, cfg *mimir.Config) error { +func LoadConfig(filename string, expandEnv bool, cfg *mimir.Config) error { buf, err := ioutil.ReadFile(filename) if err != nil { return errors.Wrap(err, "Error reading config file") @@ -232,8 +240,8 @@ func LoadConfig(filename string, expandENV bool, cfg *mimir.Config) error { configHash.Reset() configHash.WithLabelValues(fmt.Sprintf("%x", hash)).Set(1) - if expandENV { - buf = expandEnv(buf) + if expandEnv { + buf = expandEnvironmentVariables(buf) } err = yaml.UnmarshalStrict(buf, cfg) @@ -253,10 +261,10 @@ func DumpYaml(cfg *mimir.Config) { } } -// expandEnv replaces ${var} or $var in config according to the values of the current environment variables. +// expandEnvironmentVariables replaces ${var} or $var in config according to the values of the current environment variables. // The replacement is case-sensitive. References to undefined variables are replaced by the empty string. // A default value can be given by using the form ${var:default value}. -func expandEnv(config []byte) []byte { +func expandEnvironmentVariables(config []byte) []byte { return []byte(os.Expand(string(config), func(key string) string { keyAndDefault := strings.SplitN(key, ":", 2) key = keyAndDefault[0] diff --git a/cmd/mimir/main_test.go b/cmd/mimir/main_test.go index 1c09132dfc6..9de92145db7 100644 --- a/cmd/mimir/main_test.go +++ b/cmd/mimir/main_test.go @@ -9,15 +9,18 @@ import ( "bytes" "flag" "io" - "io/ioutil" "os" "path/filepath" "strings" "sync" "testing" + "github.com/go-kit/log" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/grafana/mimir/pkg/mimir" + "github.com/grafana/mimir/pkg/util/fieldcategory" ) func TestFlagParsing(t *testing.T) { @@ -162,7 +165,7 @@ func TestHelp(t *testing.T) { // Restore stdout and stderr before reporting errors to make them visible. restoreIfNeeded() - expected, err := ioutil.ReadFile(tc.filename) + expected, err := os.ReadFile(tc.filename) require.NoError(t, err) assert.Equalf(t, string(expected), string(stdout), "%s %s output changed; try `make reference-help`", cmd, tc.arg) assert.Empty(t, stderr) @@ -273,7 +276,7 @@ func (co *capturedOutput) Done() (stdout []byte, stderr []byte) { return co.stdoutBuf.Bytes(), co.stderrBuf.Bytes() } -func TestExpandEnv(t *testing.T) { +func TestExpandEnvironmentVariables(t *testing.T) { var tests = []struct { in string out string @@ -299,7 +302,7 @@ func TestExpandEnv(t *testing.T) { test := test t.Run(test.in, func(t *testing.T) { _ = os.Setenv("y", "y") - output := expandEnv([]byte(test.in)) + output := expandEnvironmentVariables([]byte(test.in)) assert.Equal(t, test.out, string(output), "Input: %s", test.in) }) } @@ -309,7 +312,7 @@ func TestParseConfigFileParameter(t *testing.T) { var tests = []struct { args string configFile string - expandENV bool + expandEnv bool }{ {"", "", false}, {"--foo", "", false}, @@ -337,9 +340,31 @@ func TestParseConfigFileParameter(t *testing.T) { test := test t.Run(test.args, func(t *testing.T) { args := strings.Split(test.args, " ") - configFile, expandENV := parseConfigFileParameter(args) + configFile, expandEnv := parseConfigFileParameter(args) assert.Equal(t, test.configFile, configFile) - assert.Equal(t, test.expandENV, expandENV) + assert.Equal(t, test.expandEnv, expandEnv) }) } } + +func TestFieldCategoryOverridesNotStale(t *testing.T) { + overrides := make(map[string]struct{}) + fieldcategory.VisitOverrides(func(s string) { + overrides[s] = struct{}{} + }) + + fs := flag.NewFlagSet("test", flag.PanicOnError) + + var ( + cfg mimir.Config + mf mainFlags + ) + cfg.RegisterFlags(fs, log.NewNopLogger()) + mf.registerFlags(fs) + + fs.VisitAll(func(fl *flag.Flag) { + delete(overrides, fl.Name) + }) + + require.Empty(t, overrides, "There are category overrides for configuration options that no longer exist") +} diff --git a/cmd/mimir/usage.go b/cmd/mimir/usage.go index 96b2a7d5273..6f2d9e42045 100644 --- a/cmd/mimir/usage.go +++ b/cmd/mimir/usage.go @@ -13,10 +13,13 @@ import ( "github.com/grafana/mimir/pkg/util/fieldcategory" ) -// usage prints command-line usage, the printAll argument controls whether also non-basic flags will be included. -func usage(cfg *mimir.Config, printAll bool) error { +// usage prints command-line usage. If mf.printHelpAll is false then only basic flags are included, otherwise all flags are included. +func usage(mf *mainFlags, cfg *mimir.Config) error { fields := map[uintptr]reflect.StructField{} - if err := parseConfig(cfg, fields); err != nil { + if err := parseStructure(mf, fields); err != nil { + return err + } + if err := parseStructure(cfg, fields); err != nil { return err } @@ -43,7 +46,7 @@ func usage(cfg *mimir.Config, printAll bool) error { } } - if fieldCat != fieldcategory.Basic && !printAll { + if fieldCat != fieldcategory.Basic && !mf.printHelpAll { // Don't print help for this flag since we're supposed to print only basic flags return } @@ -79,7 +82,7 @@ func usage(cfg *mimir.Config, printAll bool) error { fmt.Fprint(fs.Output(), b.String(), "\n") }) - if !printAll { + if !mf.printHelpAll { fmt.Fprintf(fs.Output(), "\nTo see all flags, use -help-all\n") } @@ -102,14 +105,14 @@ func isZeroValue(fl *flag.Flag, value string) bool { return value == z.Interface().(flag.Value).String() } -// parseConfig parses a mimir.Config and populates fields. -func parseConfig(cfg interface{}, fields map[uintptr]reflect.StructField) error { - // The input config is expected to be a pointer to struct. - if reflect.TypeOf(cfg).Kind() != reflect.Ptr { - t := reflect.TypeOf(cfg) +// parseStructure parses a struct and populates fields. +func parseStructure(structure interface{}, fields map[uintptr]reflect.StructField) error { + // structure is expected to be a pointer to a struct + if reflect.TypeOf(structure).Kind() != reflect.Ptr { + t := reflect.TypeOf(structure) return fmt.Errorf("%s is a %s while a %s is expected", t, t.Kind(), reflect.Ptr) } - v := reflect.ValueOf(cfg).Elem() + v := reflect.ValueOf(structure).Elem() if v.Kind() != reflect.Struct { return fmt.Errorf("%s is a %s while a %s is expected", v, v.Kind(), reflect.Struct) } @@ -132,7 +135,7 @@ func parseConfig(cfg interface{}, fields map[uintptr]reflect.StructField) error continue } - if err := parseConfig(fieldValue.Addr().Interface(), fields); err != nil { + if err := parseStructure(fieldValue.Addr().Interface(), fields); err != nil { return err } } diff --git a/pkg/util/fieldcategory/overrides.go b/pkg/util/fieldcategory/overrides.go index 144de830d13..32abe422926 100644 --- a/pkg/util/fieldcategory/overrides.go +++ b/pkg/util/fieldcategory/overrides.go @@ -31,10 +31,6 @@ func (c Category) String() string { // Fields are primarily categorized via struct tags, but this can be impossible when third party libraries are involved // Only categorize fields here when you can't otherwise, since struct tags are less likely to become stale var overrides = map[string]Category{ - // cmd/mimir/main variables that aren't in structs - "debug.block-profile-rate": Advanced, - "debug.mutex-profile-fraction": Advanced, - "mem-ballast-size-bytes": Advanced, // weaveworks/common/server in server.Config "server.graceful-shutdown-timeout": Advanced, "server.grpc-conn-limit": Advanced, @@ -73,3 +69,9 @@ func GetOverride(fieldName string) (category Category, ok bool) { category, ok = overrides[fieldName] return } + +func VisitOverrides(f func(name string)) { + for override := range overrides { + f(override) + } +}