mimirtool config: small migration and flags output convenience (#1510)

* Output only YAML when only YAML was provided (same for CLI flags)

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

* Set frontend.results_cache.backend when results cache was enabled in cortex

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

* Sort output flags

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

* My own nitpicks

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

* Update pkg/mimirtool/config/convert.go

Co-authored-by: Mauro Stettler <mauro.stettler@gmail.com>

* Update pkg/mimirtool/config/convert_test.go

Co-authored-by: Mauro Stettler <mauro.stettler@gmail.com>
Co-authored-by: Peter Štibraný <pstibrany@gmail.com>

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)

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.
 	{

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()
 

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

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),
 	}
 }
 

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

pkg/mimirtool/config/testdata/query-frontend-results-cache-new.yaml

@@ -0,0 +1,4 @@
+frontend:
+  results_cache:
+    backend: memcached
+  cache_results: true

pkg/mimirtool/config/testdata/query-frontend-results-cache-old.yaml

@@ -0,0 +1,2 @@
+query_range:
+  cache_results: true