Unable spread minimizing token generation when tokens file path is set

CHANGELOG.md

@@ -14,7 +14,7 @@
   * `-ingester.read-path-cpu-utilization-limit`
   * `-ingester.read-path-memory-utilization-limit`
 * [FEATURE] Ruler: Support filtering results from rule status endpoint by `file`, `rule_group` and `rule_name`. #5291
-* [FEATURE] Ingester: add experimental support for creating tokens by using spread minimizing strategy. This can be enabled with `-ingester.ring.token-generation-strategy: spread-minimizing` and `-ingester.ring.spread-minimizing-zones: <all available zones>`. #5308
+* [FEATURE] Ingester: add experimental support for creating tokens by using spread minimizing strategy. This can be enabled with `-ingester.ring.token-generation-strategy: spread-minimizing` and `-ingester.ring.spread-minimizing-zones: <all available zones>`. In that case `-ingester.ring.tokens-file-path` must be empty. #5308 #5324
 * [ENHANCEMENT] Cardinality API: When zone aware replication is enabled, the label values cardinality API can now tolerate single zone failure #5178
 * [ENHANCEMENT] Distributor: optimize sending requests to ingesters when incoming requests don't need to be modified. For now this feature can be disabled by setting `-timeseries-unmarshal-caching-optimization-enabled=false`. #5137
 * [ENHANCEMENT] Add advanced CLI flags to control gRPC client behaviour: #5161

cmd/mimir/config-descriptor.json

@@ -2402,7 +2402,7 @@
               "kind": "field",
               "name": "tokens_file_path",
               "required": false,
-              "desc": "File path where tokens are stored. If empty, tokens are not stored at shutdown and restored at startup.",
+              "desc": "File path where tokens are stored. If empty, tokens are not stored at shutdown and restored at startup. Must be empty if -ingester.ring.token-generation-strategy is set to \"spread-minimizing\".",
               "fieldValue": null,
               "fieldDefaultValue": "",
               "fieldFlag": "ingester.ring.tokens-file-path",

cmd/mimir/help-all.txt.tmpl

@@ -1346,7 +1346,7 @@ Usage of ./cmd/mimir/mimir:
   -ingester.ring.token-generation-strategy string
     	[experimental] Specifies the strategy used for generating tokens for ingesters. Supported values are: random,spread-minimizing. (default "random")
   -ingester.ring.tokens-file-path string
-    	File path where tokens are stored. If empty, tokens are not stored at shutdown and restored at startup.
+    	File path where tokens are stored. If empty, tokens are not stored at shutdown and restored at startup. Must be empty if -ingester.ring.token-generation-strategy is set to "spread-minimizing".
   -ingester.ring.unregister-on-shutdown
     	Unregister from the ring upon clean shutdown. It can be useful to disable for rolling restarts with consistent naming. (default true)
   -ingester.ring.zone-awareness-enabled

cmd/mimir/help.txt.tmpl

@@ -394,7 +394,7 @@ Usage of ./cmd/mimir/mimir:
   -ingester.ring.store string
     	Backend storage to use for the ring. Supported values are: consul, etcd, inmemory, memberlist, multi. (default "memberlist")
   -ingester.ring.tokens-file-path string
-    	File path where tokens are stored. If empty, tokens are not stored at shutdown and restored at startup.
+    	File path where tokens are stored. If empty, tokens are not stored at shutdown and restored at startup. Must be empty if -ingester.ring.token-generation-strategy is set to "spread-minimizing".
   -ingester.ring.zone-awareness-enabled
     	True to enable the zone-awareness and replicate ingested samples across different availability zones. This option needs be set on ingesters, distributors, queriers and rulers when running in microservices mode.
   -log.format value

docs/sources/mimir/references/configuration-parameters/index.md

@@ -849,7 +849,8 @@ ring:
   [excluded_zones: <string> | default = ""]
 
   # File path where tokens are stored. If empty, tokens are not stored at
-  # shutdown and restored at startup.
+  # shutdown and restored at startup. Must be empty if
+  # -ingester.ring.token-generation-strategy is set to "spread-minimizing".
   # CLI flag: -ingester.ring.tokens-file-path
   [tokens_file_path: <string> | default = ""]
 

pkg/ingester/ingester_ring.go

@@ -21,6 +21,7 @@ const (
 	// sharedOptionWithRingClient is a message appended to all config options that should be also
 	// set on the components running the ingester ring client.
 	sharedOptionWithRingClient      = " This option needs be set on ingesters, distributors, queriers and rulers when running in microservices mode."
+	tokensFilePathFlag              = "tokens-file-path"
 	tokenGenerationStrategyFlag     = "token-generation-strategy"
 	randomTokenGeneration           = "random"
 	spreadMinimizingTokenGeneration = "spread-minimizing"
@@ -69,6 +70,9 @@ func (cfg *RingConfig) Validate() error {
 	}
 
 	if cfg.TokenGenerationStrategy == spreadMinimizingTokenGeneration {
+		if cfg.TokensFilePath != "" {
+			return fmt.Errorf("%q token generation strategy requires %q to be empty", spreadMinimizingTokenGeneration, tokensFilePathFlag)
+		}
 		_, err := ring.NewSpreadMinimizingTokenGenerator(cfg.InstanceID, cfg.InstanceZone, cfg.SpreadMinimizingZones, nil)
 		return err
 	}
@@ -96,7 +100,7 @@ func (cfg *RingConfig) RegisterFlags(f *flag.FlagSet, logger log.Logger) {
 	f.BoolVar(&cfg.ZoneAwarenessEnabled, prefix+"zone-awareness-enabled", false, "True to enable the zone-awareness and replicate ingested samples across different availability zones."+sharedOptionWithRingClient)
 	f.Var(&cfg.ExcludedZones, prefix+"excluded-zones", "Comma-separated list of zones to exclude from the ring. Instances in excluded zones will be filtered out from the ring."+sharedOptionWithRingClient)
 
-	f.StringVar(&cfg.TokensFilePath, prefix+"tokens-file-path", "", "File path where tokens are stored. If empty, tokens are not stored at shutdown and restored at startup.")
+	f.StringVar(&cfg.TokensFilePath, prefix+tokensFilePathFlag, "", fmt.Sprintf("File path where tokens are stored. If empty, tokens are not stored at shutdown and restored at startup. Must be empty if -%s is set to %q.", prefix+tokenGenerationStrategyFlag, spreadMinimizingTokenGeneration))
 	f.IntVar(&cfg.NumTokens, prefix+"num-tokens", 128, "Number of tokens for each ingester.")
 
 	// Instance flags

pkg/ingester/ingester_ring_test.go

@@ -3,6 +3,7 @@
 package ingester
 
 import (
+	"fmt"
 	"testing"
 	"time"
 
@@ -13,6 +14,16 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+const (
+	instanceID   = "instance-10"
+	instanceZone = "zone-a"
+	wrongZone    = "zone-d"
+)
+
+var (
+	spreadMinimizingZones = []string{"zone-a", "zone-b", "zone-c"}
+)
+
 func TestRingConfig_DefaultConfigToLifecyclerConfig(t *testing.T) {
 	cfg := RingConfig{}
 	expected := ring.LifecyclerConfig{}
@@ -98,42 +109,50 @@ func TestRingConfig_CustomConfigToLifecyclerConfig(t *testing.T) {
 	assert.Equal(t, expected, cfg.ToLifecyclerConfig(logger))
 }
 
-func TestRingConfig_CustomTokenGenerator(t *testing.T) {
-	const (
-		instanceID   = "instance-10"
-		instanceZone = "zone-a"
-		wrongZone    = "zone-d"
-	)
-	spreadMinimizingZones := []string{"zone-a", "zone-b", "zone-c"}
-
+func TestRingConfig_Validate(t *testing.T) {
 	tests := map[string]struct {
 		zone                    string
 		tokenGenerationStrategy string
 		spreadMinimizingZones   []string
-		expectedResultStrategy  string
-		expectedError           bool
+		expectedError           error
+		tokensFilePath          string
 	}{
-		"spread-minimizing and correct zones give a SpreadMinimizingTokenGenerator": {
+		"spread-minimizing and correct zones pass validation": {
 			zone:                    instanceZone,
 			tokenGenerationStrategy: spreadMinimizingTokenGeneration,
 			spreadMinimizingZones:   spreadMinimizingZones,
-			expectedResultStrategy:  spreadMinimizingTokenGeneration,
 		},
-		"spread-minimizing and a wrong zone give a RandomTokenGenerator": {
+		"spread-minimizing and no spread-minimizing-zones zone don't pass validation": {
+			zone:                    instanceZone,
+			tokenGenerationStrategy: spreadMinimizingTokenGeneration,
+			expectedError:           fmt.Errorf("number of zones 0 is not correct: it must be greater than 0 and less or equal than 8"),
+		},
+		"spread-minimizing and a wrong zone don't pass validation": {
 			zone:                    wrongZone,
 			tokenGenerationStrategy: spreadMinimizingTokenGeneration,
 			spreadMinimizingZones:   spreadMinimizingZones,
-			expectedError:           true,
+			expectedError:           fmt.Errorf("zone %s is not valid", wrongZone),
 		},
-		"random gives a RandomTokenGenerator": {
+		"spread-minimizing and tokens-file-path set don't pass validation": {
+			zone:                    instanceZone,
+			tokenGenerationStrategy: spreadMinimizingTokenGeneration,
+			spreadMinimizingZones:   spreadMinimizingZones,
+			tokensFilePath:          "/path/tokens",
+			expectedError:           fmt.Errorf("%q token generation strategy requires %q to be empty", spreadMinimizingTokenGeneration, tokensFilePathFlag),
+		},
+		"random passes validation": {
 			zone:                    instanceZone,
 			tokenGenerationStrategy: randomTokenGeneration,
-			expectedResultStrategy:  randomTokenGeneration,
 		},
-		"unknown token generation strategy gives nil": {
+		"random and tokens-file-path set pass validation": {
+			zone:                    instanceZone,
+			tokenGenerationStrategy: randomTokenGeneration,
+			tokensFilePath:          "/path/tokens",
+		},
+		"unknown token generation doesn't pass validation": {
 			zone:                    instanceZone,
 			tokenGenerationStrategy: "bla-bla-tokens",
-			expectedError:           true,
+			expectedError:           fmt.Errorf("unsupported token generation strategy (%q) has been chosen for %s", "bla-bla-tokens", tokenGenerationStrategyFlag),
 		},
 	}
 
@@ -143,12 +162,43 @@ func TestRingConfig_CustomTokenGenerator(t *testing.T) {
 		cfg.InstanceZone = testData.zone
 		cfg.TokenGenerationStrategy = testData.tokenGenerationStrategy
 		cfg.SpreadMinimizingZones = testData.spreadMinimizingZones
+		cfg.TokensFilePath = testData.tokensFilePath
 		err := cfg.Validate()
-		if testData.expectedError {
+		if testData.expectedError == nil {
+			require.NoError(t, err)
+		} else {
 			require.Error(t, err)
-			continue
+			require.Equal(t, testData.expectedError, err)
 		}
-		require.NoError(t, err)
+	}
+}
+
+func TestRingConfig_CustomTokenGenerator(t *testing.T) {
+	tests := map[string]struct {
+		zone                    string
+		tokenGenerationStrategy string
+		spreadMinimizingZones   []string
+		expectedResultStrategy  string
+	}{
+		"spread-minimizing and correct zones give a SpreadMinimizingTokenGenerator": {
+			zone:                    instanceZone,
+			tokenGenerationStrategy: spreadMinimizingTokenGeneration,
+			spreadMinimizingZones:   spreadMinimizingZones,
+			expectedResultStrategy:  spreadMinimizingTokenGeneration,
+		},
+		"random gives a RandomTokenGenerator": {
+			zone:                    instanceZone,
+			tokenGenerationStrategy: randomTokenGeneration,
+			expectedResultStrategy:  randomTokenGeneration,
+		},
+	}
+
+	for _, testData := range tests {
+		cfg := RingConfig{}
+		cfg.InstanceID = instanceID
+		cfg.InstanceZone = testData.zone
+		cfg.TokenGenerationStrategy = testData.tokenGenerationStrategy
+		cfg.SpreadMinimizingZones = testData.spreadMinimizingZones
 		lifecyclerConfig := cfg.ToLifecyclerConfig(log.NewNopLogger())
 		if testData.expectedResultStrategy == randomTokenGeneration {
 			tokenGenerator, ok := lifecyclerConfig.RingTokenGenerator.(*ring.RandomTokenGenerator)