From 6d13dc8430595f535d7dc13c5fbc37035431616c Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Mon, 21 Mar 2022 08:48:19 +0100 Subject: [PATCH] mimirtool config: small migration and flags output convenience (#1510) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Output only YAML when only YAML was provided (same for CLI flags) Signed-off-by: Dimitar Dimitrov * Set frontend.results_cache.backend when results cache was enabled in cortex Signed-off-by: Dimitar Dimitrov * Sort output flags Signed-off-by: Dimitar Dimitrov * My own nitpicks Signed-off-by: Dimitar Dimitrov * Update pkg/mimirtool/config/convert.go Co-authored-by: Mauro Stettler * Update pkg/mimirtool/config/convert_test.go Co-authored-by: Mauro Stettler Co-authored-by: Peter Štibraný --- pkg/mimirtool/commands/config.go | 3 + pkg/mimirtool/config/convert.go | 84 ++++++++----------- pkg/mimirtool/config/convert_test.go | 40 ++++++++- pkg/mimirtool/config/cortex.go | 10 +++ pkg/mimirtool/config/gem.go | 2 + pkg/mimirtool/config/inspect.go | 6 +- .../query-frontend-results-cache-new.yaml | 4 + .../query-frontend-results-cache-old.yaml | 2 + 8 files changed, 97 insertions(+), 54 deletions(-) create mode 100644 pkg/mimirtool/config/testdata/query-frontend-results-cache-new.yaml create mode 100644 pkg/mimirtool/config/testdata/query-frontend-results-cache-old.yaml diff --git a/pkg/mimirtool/commands/config.go b/pkg/mimirtool/commands/config.go index ffe1ada7af7..04eddfbee55 100644 --- a/pkg/mimirtool/commands/config.go +++ b/pkg/mimirtool/commands/config.go @@ -8,6 +8,7 @@ import ( "io" "io/fs" "os" + "sort" "strings" "github.com/grafana/dskit/multierror" @@ -137,6 +138,8 @@ func (c *ConfigCommand) output(yamlContents []byte, flags []string, notices conf } defer closeFile() + sort.Strings(flags) + _, err = fmt.Fprintln(outYAMLWriter, string(yamlContents)) _, err2 := fmt.Fprintln(outFlagsWriter, strings.Join(flags, "\n")) err3 := c.writeNotices(notices, outNoticesWriter) diff --git a/pkg/mimirtool/config/convert.go b/pkg/mimirtool/config/convert.go index 20b6dfdd44e..ba07b893061 100644 --- a/pkg/mimirtool/config/convert.go +++ b/pkg/mimirtool/config/convert.go @@ -5,7 +5,6 @@ package config import ( "flag" "fmt" - "os" "strings" "github.com/pkg/errors" @@ -103,14 +102,15 @@ func Convert( return nil, nil, ConversionNotices{}, errors.Wrap(err, "could not set unset parameters to default values") } } - pruneNils(target) var newFlags []string - if len(flags) > 0 { - newFlags, err = convertFlags(flags, m, target, sourceFactory, targetFactory) - if err != nil { - return nil, nil, ConversionNotices{}, errors.Wrap(err, "could not convert passed CLI args") - } + if len(contents) == 0 { + newFlags, err = extractAllAsFlags(target) + } else if len(flags) > 0 { + newFlags, err = extractInputFlags(target, flags, m, sourceFactory, targetFactory) + } + if err != nil { + return nil, nil, ConversionNotices{}, err } yamlBytes, err := yaml.Marshal(target) @@ -176,42 +176,53 @@ func changeNilsToDefaults(target *InspectedEntry) error { }) } -func convertFlags(flags []string, m Mapper, target Parameters, sourceFactory, targetFactory InspectedEntryFactory) ([]string, error) { - flagsNewPaths, err := mapOldFlagsToNewPaths(flags, m, sourceFactory, targetFactory) +func extractAllAsFlags(target *InspectedEntry) ([]string, error) { + return extractFlags(target, func(_ string, v Value) bool { return !v.IsUnset() }) +} + +func extractInputFlags(target *InspectedEntry, inputFlags []string, m Mapper, sourceFactory, targetFactory InspectedEntryFactory) ([]string, error) { + flagsNewPaths, err := mapOldFlagsToNewPaths(inputFlags, m, sourceFactory, targetFactory) if err != nil { return nil, err } - var newFlagsWithValues []string - err = target.Walk(func(path string, value Value) error { - if _, ok := flagsNewPaths[path]; !ok { - return nil - } + + return extractFlags(target, func(path string, _ Value) bool { + _, ok := flagsNewPaths[path] + return ok + }) +} + +func extractFlags(target *InspectedEntry, shouldExtract func(path string, v Value) bool) ([]string, error) { + var flagsWithValues, toDelete []string + + err := target.Walk(func(path string, value Value) error { flagName, err := target.GetFlag(path) if err != nil { return err } + if flagName == "" { + return nil + } - newFlagsWithValues = append(newFlagsWithValues, fmt.Sprintf("-%s=%s", flagName, value)) + if !shouldExtract(path, value) { + return nil + } + + flagsWithValues = append(flagsWithValues, fmt.Sprintf("-%s=%s", flagName, value)) + toDelete = append(toDelete, path) return nil }) if err != nil { return nil, err } - // remove the parameters from the YAML, only keep the flags - for f := range flagsNewPaths { - err = target.Delete(f) + for _, path := range toDelete { + err = target.Delete(path) if err != nil { - if errors.Is(err, ErrParameterNotFound) { - // This might happen when the flag was using the default value and was pruned before convertFlags was called. - // There's nothing to do. - continue - } return nil, err } } - - return newFlagsWithValues, nil + return flagsWithValues, nil } // addFlags parses the flags and add their values to the config @@ -322,29 +333,6 @@ func prepareSourceDefaults(m Mapper, sourceFactory, targetFactory InspectedEntry return mappedSourceDefaults, err } -// pruneNils removes parameters from params that are nil. pruneNils prints any errors during pruning to os.Stderr -func pruneNils(params Parameters) { - var pathsToDelete []string - - err := params.Walk(func(path string, value Value) error { - if value.IsUnset() { - pathsToDelete = append(pathsToDelete, path) - } - return nil - }) - if err != nil { - panic(err) - } - - for _, p := range pathsToDelete { - err = params.Delete(p) - if err != nil { - err = errors.Wrap(err, "could not delete parameter with default value from config") - _, _ = fmt.Fprintln(os.Stderr, err) - } - } -} - func reportDeletedFlags(contents []byte, flags []string, sourceFactory InspectedEntryFactory) (removedFieldPaths, removedFlags []string, _ error) { // Find YAML options that user is using, but are no longer supported. { diff --git a/pkg/mimirtool/config/convert_test.go b/pkg/mimirtool/config/convert_test.go index 0b1b497e07d..a06ee39c7b0 100644 --- a/pkg/mimirtool/config/convert_test.go +++ b/pkg/mimirtool/config/convert_test.go @@ -21,6 +21,7 @@ type conversionInput struct { } func testCortexAndGEM(t *testing.T, tc conversionInput, assert func(t *testing.T, outYAML []byte, outFlags []string, notices ConversionNotices, err error)) { + t.Parallel() t.Run("cortex->mimir", func(t *testing.T) { t.Parallel() mimirYAML, mimirFlags, mimirNotices, mimirErr := Convert(tc.inYAML, tc.inFlags, CortexToMimirMapper(), DefaultCortexConfig, DefaultMimirConfig, tc.useNewDefaults, tc.outputDefaults) @@ -246,12 +247,16 @@ func TestConvert_Cortex(t *testing.T) { inFile: "testdata/duration-list-old.yaml", outFile: "testdata/duration-list-new.yaml", }, + { + name: "new frontend.results_cache.backend == memcached when old query_range.cache_results == true", + inFile: "testdata/query-frontend-results-cache-old.yaml", + outFile: "testdata/query-frontend-results-cache-new.yaml", + }, } for _, tc := range testCases { tc := tc t.Run(tc.name, func(t *testing.T) { - t.Parallel() inBytes, expectedOut := loadFile(t, tc.inFile), loadFile(t, tc.outFile) inFlags, expectedOutFlags := loadFlags(t, tc.inFlagsFile), loadFlags(t, tc.outFlagsFile) if inFlags == nil { @@ -348,7 +353,6 @@ func TestConvert_InvalidConfigs(t *testing.T) { for _, tc := range testCases { tc := tc t.Run(tc.name, func(t *testing.T) { - t.Parallel() inBytes := loadFile(t, tc.inFile) inFlags := loadFlags(t, tc.inFlagsFile) @@ -606,7 +610,6 @@ func TestConvert_UseNewDefaults(t *testing.T) { for _, tc := range testCases { tc := tc t.Run(tc.name, func(t *testing.T) { - t.Parallel() // We pass the common flags in, but ignore the output. // This is so that the always-present options in common-flags.txt get output in the flags instead of // in the out YAML. This helps to keep the test cases and expected YAML clean of @@ -639,7 +642,6 @@ func TestConvert_NotInYAMLIsNotPrinted(t *testing.T) { for _, showDefaults := range []bool{true, false} { showDefaults, useNewDefaults := showDefaults, useNewDefaults t.Run(fmt.Sprintf("useNewDefault=%t_showDefaults=%t", useNewDefaults, showDefaults), func(t *testing.T) { - t.Parallel() in := conversionInput{ useNewDefaults: useNewDefaults, outputDefaults: showDefaults, @@ -655,6 +657,36 @@ func TestConvert_NotInYAMLIsNotPrinted(t *testing.T) { } } +func TestConvert_PassingOnlyYAMLReturnsOnlyYAML(t *testing.T) { + inYAML := []byte("distributor: { remote_timeout: 11s }") + expectedOutYAML := []byte(`{distributor: { remote_timeout: 11s }, server: { http_listen_port: 80 }}`) + + in := conversionInput{ + inYAML: inYAML, + } + + testCortexAndGEM(t, in, func(t *testing.T, outYAML []byte, outFlags []string, notices ConversionNotices, err error) { + assert.NoError(t, err) + assert.YAMLEq(t, string(expectedOutYAML), string(outYAML)) + assert.Empty(t, outFlags) + }) +} + +func TestConvert_PassingOnlyFlagsReturnsOnlyFlags(t *testing.T) { + inFlags := []string{"-distributor.remote-timeout=11s"} + expectedOutFlags := append([]string{"-server.http-listen-port=80"}, inFlags...) + + in := conversionInput{ + inFlags: inFlags, + } + + testCortexAndGEM(t, in, func(t *testing.T, outYAML []byte, outFlags []string, notices ConversionNotices, err error) { + assert.NoError(t, err) + assert.YAMLEq(t, "{}", string(outYAML)) + assert.ElementsMatch(t, expectedOutFlags, outFlags) + }) +} + func loadFile(t testing.TB, fileName string) []byte { t.Helper() diff --git a/pkg/mimirtool/config/cortex.go b/pkg/mimirtool/config/cortex.go index 412fc01c53e..b00e2f23819 100644 --- a/pkg/mimirtool/config/cortex.go +++ b/pkg/mimirtool/config/cortex.go @@ -38,6 +38,8 @@ func CortexToMimirMapper() Mapper { mapRulerAlertmanagerS3Buckets("alertmanager.storage", "alertmanager_storage"), mapRulerAlertmanagerS3Buckets("ruler.storage", "ruler_storage"), // Prevent server.http_listen_port from being updated with a new default and always output it. MapperFunc(mapServerHTTPListenPort), + // Set frontend.results_cache.backend when results cache was enabled in cortex + MapperFunc(mapQueryFrontendBackend), } } @@ -528,6 +530,14 @@ func mapServerHTTPListenPort(source, target Parameters) error { return nil } +func mapQueryFrontendBackend(source, target Parameters) error { + v, _ := source.GetValue("query_range.cache_results") + if v.AsBool() { + return target.SetValue("frontend.results_cache.backend", StringValue("memcached")) + } + return nil +} + // YAML Paths for config options removed since Cortex 1.11.0. var removedConfigPaths = []string{ "flusher.concurrent_flushes", // -flusher.concurrent-flushes diff --git a/pkg/mimirtool/config/gem.go b/pkg/mimirtool/config/gem.go index 4a35b123f66..c811ef5a8ec 100644 --- a/pkg/mimirtool/config/gem.go +++ b/pkg/mimirtool/config/gem.go @@ -40,6 +40,8 @@ func GEM170ToGEM200Mapper() Mapper { mapRulerAlertmanagerS3Buckets("alertmanager.storage", "alertmanager_storage"), mapRulerAlertmanagerS3Buckets("ruler.storage", "ruler_storage"), // Prevent server.http_listen_port from being updated with a new default and always output it. MapperFunc(mapServerHTTPListenPort), + // Set frontend.results_cache.backend when results cache was enabled in cortex + MapperFunc(mapQueryFrontendBackend), } } diff --git a/pkg/mimirtool/config/inspect.go b/pkg/mimirtool/config/inspect.go index 7bcb3ee7ba3..abdb104c320 100644 --- a/pkg/mimirtool/config/inspect.go +++ b/pkg/mimirtool/config/inspect.go @@ -209,7 +209,9 @@ func (i *InspectedEntry) asMap() map[string]interface{} { } } } else if e.Name != notInYaml { - combined[e.Name] = e.asMap() + if subMap := e.asMap(); len(subMap) > 0 { + combined[e.Name] = subMap + } } } return combined @@ -610,7 +612,7 @@ func (d *duration) UnmarshalJSON(data []byte) error { // stringSlice combines the behaviour of flagext.StringSlice and flagext.StringSliceCSV. // Its fmt.Stringer implementation returns a comma-joined string - this is handy for -// outputting the slice as the value of a flag during convertFlags. +// outputting the slice as the value of a flag during extractFlags. // Its yaml.Unmarshaler implementation supports reading in both YAML sequences and comma-delimited strings. // Its yaml.Marshaler implementation marshals the slice as a regular go slice. type stringSlice []string diff --git a/pkg/mimirtool/config/testdata/query-frontend-results-cache-new.yaml b/pkg/mimirtool/config/testdata/query-frontend-results-cache-new.yaml new file mode 100644 index 00000000000..54edb305d52 --- /dev/null +++ b/pkg/mimirtool/config/testdata/query-frontend-results-cache-new.yaml @@ -0,0 +1,4 @@ +frontend: + results_cache: + backend: memcached + cache_results: true diff --git a/pkg/mimirtool/config/testdata/query-frontend-results-cache-old.yaml b/pkg/mimirtool/config/testdata/query-frontend-results-cache-old.yaml new file mode 100644 index 00000000000..a30688ca93c --- /dev/null +++ b/pkg/mimirtool/config/testdata/query-frontend-results-cache-old.yaml @@ -0,0 +1,2 @@ +query_range: + cache_results: true