Distributor default sharding should support default 0 value. (#5759)

* Distributor: allow 0 value in config value

Signed-off-by: Ryan West <[email protected]>

* Updating CHANGELOG

Signed-off-by: Ryan West <[email protected]>

* Removing log line, specifying shuffle sharding on validate, and moving code back to L680

Signed-off-by: Ryan West <[email protected]>

* Improving the error message readability

Signed-off-by: Ryan West <[email protected]>

---------

Signed-off-by: Ryan West <[email protected]>

Author: Kramer0x0

CHANGELOG.md

@@ -20,6 +20,7 @@
 * [BUGFIX] Distributor: Do not use label with empty values for sharding #5717
 * [BUGFIX] Query Frontend: queries with negative offset should check whether it is cacheable or not. #5719
 * [BUGFIX] Redis Cache: pass `cache_size` config correctly. #5734
+* [BUGFIX] Distributor: Shuffle-Sharding with IngestionTenantShardSize == 0, default sharding strategy should be used #5189
 
 
 ## 1.16.0 2023-11-20

integration/ingester_sharding_test.go

@@ -27,16 +27,28 @@ func TestIngesterSharding(t *testing.T) {
 		tenantShardSize             int
 		expectedIngestersWithSeries int
 	}{
-		"default sharding strategy should spread series across all ingesters": {
+		//Default Sharding Strategy
+		"default sharding strategy should be ignored and spread across all ingesters": {
 			shardingStrategy:            "default",
 			tenantShardSize:             2, // Ignored by default strategy.
 			expectedIngestersWithSeries: 3,
 		},
+		"default sharding strategy should spread series across all ingesters": {
+			shardingStrategy:            "default",
+			tenantShardSize:             0, // Ignored by default strategy.
+			expectedIngestersWithSeries: 3,
+		},
+		//Shuffle Sharding Strategy
 		"shuffle-sharding strategy should spread series across the configured shard size number of ingesters": {
 			shardingStrategy:            "shuffle-sharding",
 			tenantShardSize:             2,
 			expectedIngestersWithSeries: 2,
 		},
+		"Tenant Shard Size of 0 should leverage all ingesters": {
+			shardingStrategy:            "shuffle-sharding",
+			tenantShardSize:             0,
+			expectedIngestersWithSeries: 3,
+		},
 	}
 
 	for testName, testData := range tests {
@@ -125,7 +137,7 @@ func TestIngesterSharding(t *testing.T) {
 			// We expect that only ingesters belonging to tenant's shard have been queried if
 			// shuffle sharding is enabled.
 			expectedIngesters := ingesters.NumInstances()
-			if testData.shardingStrategy == "shuffle-sharding" {
+			if testData.shardingStrategy == "shuffle-sharding" && testData.tenantShardSize > 0 {
 				expectedIngesters = testData.tenantShardSize
 			}
 

pkg/distributor/distributor.go

@@ -49,7 +49,7 @@ var (
 
 	// Validation errors.
 	errInvalidShardingStrategy = errors.New("invalid sharding strategy")
-	errInvalidTenantShardSize  = errors.New("invalid tenant shard size, the value must be greater than 0")
+	errInvalidTenantShardSize  = errors.New("invalid tenant shard size. The value must be greater than or equal to 0")
 
 	// Distributor instance limits errors.
 	errTooManyInflightPushRequests    = errors.New("too many inflight push requests in distributor")
@@ -178,7 +178,7 @@ func (cfg *Config) Validate(limits validation.Limits) error {
 		return errInvalidShardingStrategy
 	}
 
-	if cfg.ShardingStrategy == util.ShardingStrategyShuffle && limits.IngestionTenantShardSize <= 0 {
+	if cfg.ShardingStrategy == util.ShardingStrategyShuffle && limits.IngestionTenantShardSize < 0 {
 		return errInvalidTenantShardSize
 	}
 

pkg/distributor/distributor_test.go

@@ -76,14 +76,14 @@ func TestConfig_Validate(t *testing.T) {
 			initLimits: func(_ *validation.Limits) {},
 			expected:   errInvalidShardingStrategy,
 		},
-		"should fail if the default shard size is 0 on when sharding strategy = shuffle-sharding": {
+		"should pass sharding strategy when IngestionTenantShardSize = 0": {
 			initConfig: func(cfg *Config) {
 				cfg.ShardingStrategy = "shuffle-sharding"
 			},
 			initLimits: func(limits *validation.Limits) {
 				limits.IngestionTenantShardSize = 0
 			},
-			expected: errInvalidTenantShardSize,
+			expected: nil,
 		},
 		"should pass if the default shard size > 0 on when sharding strategy = shuffle-sharding": {
 			initConfig: func(cfg *Config) {
@@ -94,6 +94,15 @@ func TestConfig_Validate(t *testing.T) {
 			},
 			expected: nil,
 		},
+		"should fail because the ingestionTenantShardSize is a non-positive number": {
+			initConfig: func(cfg *Config) {
+				cfg.ShardingStrategy = "shuffle-sharding"
+			},
+			initLimits: func(limits *validation.Limits) {
+				limits.IngestionTenantShardSize = -1
+			},
+			expected: errInvalidTenantShardSize,
+		},
 	}
 
 	for testName, testData := range tests {