From 6d8dc726ceeb794bee862b0c249187280bfe7121 Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Tue, 29 Sep 2020 12:58:01 +0200
Subject: [PATCH 01/17] Added ingesters shuffle sharding support on the read
 path

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 docs/blocks-storage/querier.md              |   8 +
 docs/configuration/config-file-reference.md |   7 +
 pkg/cortex/modules.go                       |   1 +
 pkg/distributor/distributor.go              |  56 ++-
 pkg/distributor/query.go                    |  64 +++-
 pkg/querier/querier.go                      |   3 +
 pkg/ring/replication_set.go                 |   9 +
 pkg/ring/ring.go                            |  48 ++-
 pkg/ring/ring_test.go                       | 378 +++++++++++++++++++-
 9 files changed, 531 insertions(+), 43 deletions(-)

diff --git a/docs/blocks-storage/querier.md b/docs/blocks-storage/querier.md
index 6d805213cf3..f95a8d7d302 100644
--- a/docs/blocks-storage/querier.md
+++ b/docs/blocks-storage/querier.md
@@ -173,6 +173,14 @@ querier:
   # Default value 0 means secondary store is always queried.
   # CLI flag: -querier.use-second-store-before-time
   [use_second_store_before_time: <time> | default = 0]
+
+  # When distributor's sharding strategy is shuffle-sharding and this setting is
+  # > 0, queriers fetch in-memory series from the minimum set of required
+  # ingesters, selecting only ingesters which may have received series since
+  # 'now - lookback period'. If this setting is 0, queriers always query all
+  # ingesters.
+  # CLI flag: -querier.shuffle-sharding-ingesters-lookback-period
+  [shuffle_sharding_ingesters_lookback_period: <duration> | default = 0s]
 ```
 
 ### `blocks_storage_config`
diff --git a/docs/configuration/config-file-reference.md b/docs/configuration/config-file-reference.md
index a876dde97fa..eb2706e8f4a 100644
--- a/docs/configuration/config-file-reference.md
+++ b/docs/configuration/config-file-reference.md
@@ -740,6 +740,13 @@ store_gateway_client:
 # Default value 0 means secondary store is always queried.
 # CLI flag: -querier.use-second-store-before-time
 [use_second_store_before_time: <time> | default = 0]
+
+# When distributor's sharding strategy is shuffle-sharding and this setting is >
+# 0, queriers fetch in-memory series from the minimum set of required ingesters,
+# selecting only ingesters which may have received series since 'now - lookback
+# period'. If this setting is 0, queriers always query all ingesters.
+# CLI flag: -querier.shuffle-sharding-ingesters-lookback-period
+[shuffle_sharding_ingesters_lookback_period: <duration> | default = 0s]
 ```
 
 ### `query_frontend_config`
diff --git a/pkg/cortex/modules.go b/pkg/cortex/modules.go
index 1c85d8f02e8..98825db6024 100644
--- a/pkg/cortex/modules.go
+++ b/pkg/cortex/modules.go
@@ -169,6 +169,7 @@ func (t *Cortex) initOverrides() (serv services.Service, err error) {
 
 func (t *Cortex) initDistributorService() (serv services.Service, err error) {
 	t.Cfg.Distributor.DistributorRing.ListenPort = t.Cfg.Server.GRPCListenPort
+	t.Cfg.Distributor.ShuffleShardingLookbackPeriod = t.Cfg.Querier.ShuffleShardingIngestersLookbackPeriod
 
 	// Check whether the distributor can join the distributors ring, which is
 	// whenever it's not running as an internal dependency (ie. querier or
diff --git a/pkg/distributor/distributor.go b/pkg/distributor/distributor.go
index 8520cdfa8fd..ceff247fc9e 100644
--- a/pkg/distributor/distributor.go
+++ b/pkg/distributor/distributor.go
@@ -170,6 +170,9 @@ type Config struct {
 	// when true the distributor does not validate the label name, Cortex doesn't directly use
 	// this (and should never use it) but this feature is used by other projects built on top of it
 	SkipLabelNameValidation bool `yaml:"-"`
+
+	// This config is dynamically injected because defined in the querier config.
+	ShuffleShardingLookbackPeriod time.Duration `yaml:"-"`
 }
 
 // RegisterFlags adds the flags required to config this to the given FlagSet
@@ -622,16 +625,8 @@ func (d *Distributor) send(ctx context.Context, ingester ring.IngesterDesc, time
 	return err
 }
 
-// ForAllIngesters runs f, in parallel, for all ingesters
-func (d *Distributor) ForAllIngesters(ctx context.Context, reallyAll bool, f func(context.Context, client.IngesterClient) (interface{}, error)) ([]interface{}, error) {
-	replicationSet, err := d.ingestersRing.GetAll(ring.Read)
-	if err != nil {
-		return nil, err
-	}
-	if reallyAll {
-		replicationSet.MaxErrors = 0
-	}
-
+// ForReplicationSet runs f, in parallel, for all ingesters in the input replication set.
+func (d *Distributor) ForReplicationSet(ctx context.Context, replicationSet ring.ReplicationSet, f func(context.Context, client.IngesterClient) (interface{}, error)) ([]interface{}, error) {
 	return replicationSet.Do(ctx, d.cfg.ExtraQueryDelay, func(ctx context.Context, ing *ring.IngesterDesc) (interface{}, error) {
 		client, err := d.ingesterPool.GetClientFor(ing.Addr)
 		if err != nil {
@@ -644,10 +639,15 @@ func (d *Distributor) ForAllIngesters(ctx context.Context, reallyAll bool, f fun
 
 // LabelValuesForLabelName returns all of the label values that are associated with a given label name.
 func (d *Distributor) LabelValuesForLabelName(ctx context.Context, labelName model.LabelName) ([]string, error) {
+	replicationSet, err := d.getIngestersForMetadata(ctx)
+	if err != nil {
+		return nil, err
+	}
+
 	req := &client.LabelValuesRequest{
 		LabelName: string(labelName),
 	}
-	resps, err := d.ForAllIngesters(ctx, false, func(ctx context.Context, client client.IngesterClient) (interface{}, error) {
+	resps, err := d.ForReplicationSet(ctx, replicationSet, func(ctx context.Context, client client.IngesterClient) (interface{}, error) {
 		return client.LabelValues(ctx, req)
 	})
 	if err != nil {
@@ -670,8 +670,13 @@ func (d *Distributor) LabelValuesForLabelName(ctx context.Context, labelName mod
 
 // LabelNames returns all of the label names.
 func (d *Distributor) LabelNames(ctx context.Context) ([]string, error) {
+	replicationSet, err := d.getIngestersForMetadata(ctx)
+	if err != nil {
+		return nil, err
+	}
+
 	req := &client.LabelNamesRequest{}
-	resps, err := d.ForAllIngesters(ctx, false, func(ctx context.Context, client client.IngesterClient) (interface{}, error) {
+	resps, err := d.ForReplicationSet(ctx, replicationSet, func(ctx context.Context, client client.IngesterClient) (interface{}, error) {
 		return client.LabelNames(ctx, req)
 	})
 	if err != nil {
@@ -698,12 +703,17 @@ func (d *Distributor) LabelNames(ctx context.Context) ([]string, error) {
 
 // MetricsForLabelMatchers gets the metrics that match said matchers
 func (d *Distributor) MetricsForLabelMatchers(ctx context.Context, from, through model.Time, matchers ...*labels.Matcher) ([]metric.Metric, error) {
+	replicationSet, err := d.getIngestersForMetadata(ctx)
+	if err != nil {
+		return nil, err
+	}
+
 	req, err := ingester_client.ToMetricsForLabelMatchersRequest(from, through, matchers)
 	if err != nil {
 		return nil, err
 	}
 
-	resps, err := d.ForAllIngesters(ctx, false, func(ctx context.Context, client client.IngesterClient) (interface{}, error) {
+	resps, err := d.ForReplicationSet(ctx, replicationSet, func(ctx context.Context, client client.IngesterClient) (interface{}, error) {
 		return client.MetricsForLabelMatchers(ctx, req)
 	})
 	if err != nil {
@@ -729,9 +739,14 @@ func (d *Distributor) MetricsForLabelMatchers(ctx context.Context, from, through
 
 // MetricsMetadata returns all metric metadata of a user.
 func (d *Distributor) MetricsMetadata(ctx context.Context) ([]scrape.MetricMetadata, error) {
+	replicationSet, err := d.getIngestersForMetadata(ctx)
+	if err != nil {
+		return nil, err
+	}
+
 	req := &ingester_client.MetricsMetadataRequest{}
 	// TODO(gotjosh): We only need to look in all the ingesters if shardByAllLabels is enabled.
-	resps, err := d.ForAllIngesters(ctx, false, func(ctx context.Context, client client.IngesterClient) (interface{}, error) {
+	resps, err := d.ForReplicationSet(ctx, replicationSet, func(ctx context.Context, client client.IngesterClient) (interface{}, error) {
 		return client.MetricsMetadata(ctx, req)
 	})
 	if err != nil {
@@ -764,8 +779,17 @@ func (d *Distributor) MetricsMetadata(ctx context.Context) ([]scrape.MetricMetad
 
 // UserStats returns statistics about the current user.
 func (d *Distributor) UserStats(ctx context.Context) (*UserStats, error) {
+	// We always need to query all ingesters.
+	replicationSet, err := d.ingestersRing.GetAll(ring.Read)
+	if err != nil {
+		return nil, err
+	}
+
+	// Make sure we get a successful response from all of them.
+	replicationSet.MaxErrors = 0
+
 	req := &client.UserStatsRequest{}
-	resps, err := d.ForAllIngesters(ctx, true, func(ctx context.Context, client client.IngesterClient) (interface{}, error) {
+	resps, err := d.ForReplicationSet(ctx, replicationSet, func(ctx context.Context, client client.IngesterClient) (interface{}, error) {
 		return client.UserStats(ctx, req)
 	})
 	if err != nil {
@@ -801,7 +825,7 @@ func (d *Distributor) AllUserStats(ctx context.Context) ([]UserIDStats, error) {
 
 	req := &client.UserStatsRequest{}
 	ctx = user.InjectOrgID(ctx, "1") // fake: ingester insists on having an org ID
-	// Not using d.ForAllIngesters(), so we can fail after first error.
+	// Not using d.ForReplicationSet(), so we can fail after first error.
 	replicationSet, err := d.ingestersRing.GetAll(ring.Read)
 	if err != nil {
 		return nil, err
diff --git a/pkg/distributor/query.go b/pkg/distributor/query.go
index eb1d68d43d8..8edef1a0e49 100644
--- a/pkg/distributor/query.go
+++ b/pkg/distributor/query.go
@@ -22,7 +22,12 @@ import (
 func (d *Distributor) Query(ctx context.Context, from, to model.Time, matchers ...*labels.Matcher) (model.Matrix, error) {
 	var matrix model.Matrix
 	err := instrument.CollectedRequest(ctx, "Distributor.Query", queryDuration, instrument.ErrorCode, func(ctx context.Context) error {
-		replicationSet, req, err := d.queryPrep(ctx, from, to, matchers...)
+		req, err := ingester_client.ToQueryRequest(from, to, matchers)
+		if err != nil {
+			return err
+		}
+
+		replicationSet, err := d.getIngestersForQuery(ctx, matchers...)
 		if err != nil {
 			return err
 		}
@@ -44,7 +49,12 @@ func (d *Distributor) Query(ctx context.Context, from, to model.Time, matchers .
 func (d *Distributor) QueryStream(ctx context.Context, from, to model.Time, matchers ...*labels.Matcher) (*ingester_client.QueryStreamResponse, error) {
 	var result *ingester_client.QueryStreamResponse
 	err := instrument.CollectedRequest(ctx, "Distributor.QueryStream", queryDuration, instrument.ErrorCode, func(ctx context.Context) error {
-		replicationSet, req, err := d.queryPrep(ctx, from, to, matchers...)
+		req, err := ingester_client.ToQueryRequest(from, to, matchers)
+		if err != nil {
+			return err
+		}
+
+		replicationSet, err := d.getIngestersForQuery(ctx, matchers...)
 		if err != nil {
 			return err
 		}
@@ -62,26 +72,54 @@ func (d *Distributor) QueryStream(ctx context.Context, from, to model.Time, matc
 	return result, err
 }
 
-func (d *Distributor) queryPrep(ctx context.Context, from, to model.Time, matchers ...*labels.Matcher) (ring.ReplicationSet, *client.QueryRequest, error) {
-	var replicationSet ring.ReplicationSet
+// getIngestersForQuery returns a replication set including all ingesters that should be queried
+// to fetch series matching input label matchers.
+func (d *Distributor) getIngestersForQuery(ctx context.Context, matchers ...*labels.Matcher) (ring.ReplicationSet, error) {
 	userID, err := user.ExtractOrgID(ctx)
 	if err != nil {
-		return replicationSet, nil, err
+		return ring.ReplicationSet{}, err
 	}
 
-	req, err := ingester_client.ToQueryRequest(from, to, matchers)
-	if err != nil {
-		return replicationSet, nil, err
+	// If shuffle sharding is enabled we should only query ingesters which are
+	// part of the tenant's subring.
+	if d.cfg.ShardingStrategy == ShardingStrategyShuffle {
+		shardSize := d.limits.IngestionTenantShardSize(userID)
+		lookbackPeriod := d.cfg.ShuffleShardingLookbackPeriod
+
+		if shardSize > 0 && lookbackPeriod > 0 {
+			return d.ingestersRing.ShuffleShardWithLookback(userID, shardSize, lookbackPeriod).GetAll(ring.Read)
+		}
 	}
 
-	// Get ingesters by metricName if one exists, otherwise get all ingesters
+	// If "shard by all labels" is disabled, we can get ingesters by metricName if exists.
 	metricNameMatcher, _, ok := extract.MetricNameMatcherFromMatchers(matchers)
 	if !d.cfg.ShardByAllLabels && ok && metricNameMatcher.Type == labels.MatchEqual {
-		replicationSet, err = d.ingestersRing.Get(shardByMetricName(userID, metricNameMatcher.Value), ring.Read, nil)
-	} else {
-		replicationSet, err = d.ingestersRing.GetAll(ring.Read)
+		return d.ingestersRing.Get(shardByMetricName(userID, metricNameMatcher.Value), ring.Read, nil)
 	}
-	return replicationSet, req, err
+
+	return d.ingestersRing.GetAll(ring.Read)
+}
+
+// getIngestersForMetadata returns a replication set including all ingesters that should be queried
+// to fetch metadata (eg. label names/values or series).
+func (d *Distributor) getIngestersForMetadata(ctx context.Context) (ring.ReplicationSet, error) {
+	userID, err := user.ExtractOrgID(ctx)
+	if err != nil {
+		return ring.ReplicationSet{}, err
+	}
+
+	// If shuffle sharding is enabled we should only query ingesters which are
+	// part of the tenant's subring.
+	if d.cfg.ShardingStrategy == ShardingStrategyShuffle {
+		shardSize := d.limits.IngestionTenantShardSize(userID)
+		lookbackPeriod := d.cfg.ShuffleShardingLookbackPeriod
+
+		if shardSize > 0 {
+			return d.ingestersRing.ShuffleShardWithLookback(userID, shardSize, lookbackPeriod).GetAll(ring.Read)
+		}
+	}
+
+	return d.ingestersRing.GetAll(ring.Read)
 }
 
 // queryIngesters queries the ingesters via the older, sample-based API.
diff --git a/pkg/querier/querier.go b/pkg/querier/querier.go
index 5bd089fa411..ec4ea5f4b44 100644
--- a/pkg/querier/querier.go
+++ b/pkg/querier/querier.go
@@ -64,6 +64,8 @@ type Config struct {
 
 	SecondStoreEngine        string       `yaml:"second_store_engine"`
 	UseSecondStoreBeforeTime flagext.Time `yaml:"use_second_store_before_time"`
+
+	ShuffleShardingIngestersLookbackPeriod time.Duration `yaml:"shuffle_sharding_ingesters_lookback_period"`
 }
 
 var (
@@ -88,6 +90,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.DurationVar(&cfg.LookbackDelta, "querier.lookback-delta", 5*time.Minute, "Time since the last sample after which a time series is considered stale and ignored by expression evaluations.")
 	f.StringVar(&cfg.SecondStoreEngine, "querier.second-store-engine", "", "Second store engine to use for querying. Empty = disabled.")
 	f.Var(&cfg.UseSecondStoreBeforeTime, "querier.use-second-store-before-time", "If specified, second store is only used for queries before this timestamp. Default value 0 means secondary store is always queried.")
+	f.DurationVar(&cfg.ShuffleShardingIngestersLookbackPeriod, "querier.shuffle-sharding-ingesters-lookback-period", 0, "When distributor's sharding strategy is shuffle-sharding and this setting is > 0, queriers fetch in-memory series from the minimum set of required ingesters, selecting only ingesters which may have received series since 'now - lookback period'. If this setting is 0, queriers always query all ingesters.")
 }
 
 // Validate the config
diff --git a/pkg/ring/replication_set.go b/pkg/ring/replication_set.go
index 686c012f8e4..ef5e3c3c5e4 100644
--- a/pkg/ring/replication_set.go
+++ b/pkg/ring/replication_set.go
@@ -85,6 +85,15 @@ func (r ReplicationSet) Includes(addr string) bool {
 	return false
 }
 
+// GetAddresses returns the addresses of all instances within the replication set.
+func (r ReplicationSet) GetAddresses() []string {
+	addrs := make([]string, 0, len(r.Ingesters))
+	for _, desc := range r.Ingesters {
+		addrs = append(addrs, desc.Addr)
+	}
+	return addrs
+}
+
 // HasReplicationSetChanged returns true if two replications sets are the same (with possibly different timestamps),
 // false if they differ in any way (number of instances, instance states, tokens, zones, ...).
 func HasReplicationSetChanged(before, after ReplicationSet) bool {
diff --git a/pkg/ring/ring.go b/pkg/ring/ring.go
index 91552d848ec..ee3762dec50 100644
--- a/pkg/ring/ring.go
+++ b/pkg/ring/ring.go
@@ -52,6 +52,10 @@ type ReadRing interface {
 	// and size (number of instances).
 	ShuffleShard(identifier string, size int) ReadRing
 
+	// ShuffleShardWithLookback is like ShuffleShard() but the returned subring includes
+	// all instances that have been part of the identifier's shard since "now - lookbackPeriod".
+	ShuffleShardWithLookback(identifier string, size int, lookbackPeriod time.Duration) ReadRing
+
 	// HasInstance returns whether the ring contains an instance matching the provided instanceID.
 	HasInstance(instanceID string) bool
 }
@@ -471,15 +475,32 @@ func (r *Ring) ShuffleShard(identifier string, size int) ReadRing {
 		return cached
 	}
 
-	var result *Ring
+	result := r.shuffleShard(identifier, size, 0)
+
+	r.setCachedShuffledSubring(identifier, size, result.(*Ring))
+	return result
+}
+
+// ShuffleShardWithLookback is like ShuffleShard() but the returned subring includes all instances
+// that have been part of the identifier's shard since "now - lookbackPeriod".
+//
+// The returned subring may be unbalanced with regard to zones and should never be used for write
+// operations (read only).
+//
+// This function doesn't support caching.
+func (r *Ring) ShuffleShardWithLookback(identifier string, size int, lookbackPeriod time.Duration) ReadRing {
+	// Nothing to do if the shard size is not smaller then the actual ring.
+	if size <= 0 || r.IngesterCount() <= size {
+		return r
+	}
+
+	return r.shuffleShard(identifier, size, lookbackPeriod)
+}
+
+func (r *Ring) shuffleShard(identifier string, size int, lookbackPeriod time.Duration) ReadRing {
+	lookbackUntil := time.Now().Add(-lookbackPeriod).Unix()
 
-	// This deferred function will store newly computed ring into cache.
-	// This needs to happen *after* releasing Read lock (otherwise we get a deadlock).
-	defer func() {
-		// We cannot defer setCachedShuffledSubring directly, as result is still nil at this point,
-		// and that would be passed to the function, instead of updated value.
-		r.setCachedShuffledSubring(identifier, size, result)
-	}()
+	var result *Ring
 
 	r.mtx.RLock()
 	defer r.mtx.RUnlock()
@@ -537,7 +558,16 @@ func (r *Ring) ShuffleShard(identifier string, size int) ReadRing {
 					continue
 				}
 
-				shard[tokens[p].Ingester] = r.ringDesc.Ingesters[tokens[p].Ingester]
+				instance := r.ringDesc.Ingesters[tokens[p].Ingester]
+
+				// If the lookback is enabled and this instance has been registered within the lookback period
+				// then we should include it in the subring but continuing selecting instances.
+				if lookbackPeriod > 0 && instance.RegisteredTimestamp >= lookbackUntil {
+					shard[tokens[p].Ingester] = instance
+					continue
+				}
+
+				shard[tokens[p].Ingester] = instance
 				found = true
 				break
 			}
diff --git a/pkg/ring/ring_test.go b/pkg/ring/ring_test.go
index 006956136d9..73a8ed1f7fd 100644
--- a/pkg/ring/ring_test.go
+++ b/pkg/ring/ring_test.go
@@ -6,6 +6,7 @@ import (
 	"math"
 	"math/rand"
 	"strconv"
+	"strings"
 	"testing"
 	"time"
 
@@ -759,6 +760,356 @@ func TestRing_ShuffleShard_ConsistencyOnZonesChanged(t *testing.T) {
 	}
 }
 
+func TestRing_ShuffleShardWithLookback(t *testing.T) {
+	type eventType int
+
+	const (
+		add eventType = iota
+		remove
+		test
+
+		lookbackPeriod = time.Hour
+		userID         = "user-1"
+	)
+
+	var (
+		now = time.Now()
+	)
+
+	type event struct {
+		what         eventType
+		instanceID   string
+		instanceDesc IngesterDesc
+		shardSize    int
+		expected     []string
+	}
+
+	tests := map[string]struct {
+		timeline []event
+	}{
+		"single zone, shard size = 1, recently bootstrapped cluster": {
+			timeline: []event{
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-time.Minute))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now.Add(-time.Minute))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, 2) + 1}, now.Add(-time.Minute))},
+				{what: test, shardSize: 1, expected: []string{"instance-1", "instance-2", "instance-3"}},
+			},
+		},
+		"single zone, shard size = 1, instances scale up": {
+			timeline: []event{
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, 2) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: test, shardSize: 1, expected: []string{"instance-1"}},
+				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, 0) + 2}, now.Add(-10*time.Minute))},
+				{what: test, shardSize: 1, expected: []string{"instance-4" /* lookback: */, "instance-1"}},
+				{what: add, instanceID: "instance-5", instanceDesc: generateRingInstanceWithInfo("instance-5", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-5*time.Minute))},
+				{what: test, shardSize: 1, expected: []string{"instance-5" /* lookback: */, "instance-4", "instance-1"}},
+			},
+		},
+		"single zone, shard size = 1, instances scale down": {
+			timeline: []event{
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, 2) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: test, shardSize: 1, expected: []string{"instance-1"}},
+				{what: remove, instanceID: "instance-3"},
+				{what: test, shardSize: 1, expected: []string{"instance-1"}},
+				{what: remove, instanceID: "instance-1"},
+				{what: test, shardSize: 1, expected: []string{"instance-2"}},
+			},
+		},
+		"single zone, shard size = 1, rollout with instances unregistered (removed and re-added one by one)": {
+			timeline: []event{
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, 2) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: test, shardSize: 1, expected: []string{"instance-1"}},
+				// Rollout instance-3.
+				{what: remove, instanceID: "instance-3"},
+				{what: test, shardSize: 1, expected: []string{"instance-1"}},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, 2) + 1}, now)},
+				{what: test, shardSize: 1, expected: []string{"instance-1"}},
+				// Rollout instance-2.
+				{what: remove, instanceID: "instance-2"},
+				{what: test, shardSize: 1, expected: []string{"instance-1"}},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now)},
+				{what: test, shardSize: 1, expected: []string{"instance-1"}},
+				// Rollout instance-1.
+				{what: remove, instanceID: "instance-1"},
+				{what: test, shardSize: 1, expected: []string{"instance-2", "instance-3"}}, // TODO instance-3 is a side effect (investigate better)
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now)},
+				{what: test, shardSize: 1, expected: []string{"instance-1" /* lookback: */, "instance-2", "instance-3"}},
+			},
+		},
+		"single zone, shard size = 2, rollout with instances unregistered (removed and re-added one by one)": {
+			timeline: []event{
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, 2) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, 3) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2"}},
+				// Rollout instance-4.
+				{what: remove, instanceID: "instance-4"},
+				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2"}},
+				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, 3) + 1}, now)},
+				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2"}},
+				// Rollout instance-3.
+				{what: remove, instanceID: "instance-3"},
+				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2"}},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, 2) + 1}, now)},
+				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2"}},
+				// Rollout instance-2.
+				{what: remove, instanceID: "instance-2"},
+				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-3", "instance-4"}}, // TODO instance-4 is a side effect (investigate better)
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now)},
+				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2" /* lookback: */, "instance-3", "instance-4"}},
+				// Rollout instance-1.
+				{what: remove, instanceID: "instance-1"},
+				{what: test, shardSize: 2, expected: []string{"instance-2" /* lookback: */, "instance-3", "instance-4"}},
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now)},
+				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2" /* lookback: */, "instance-3", "instance-4"}},
+			},
+		},
+		"single zone, increase shard size": {
+			timeline: []event{
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, 2) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: test, shardSize: 1, expected: []string{"instance-1"}},
+				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2"}},
+				{what: test, shardSize: 3, expected: []string{"instance-1", "instance-2", "instance-3"}},
+				{what: test, shardSize: 4, expected: []string{"instance-1", "instance-2", "instance-3"}},
+			},
+		},
+		"multi zone, shard size = 3, recently bootstrapped cluster": {
+			timeline: []event{
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-time.Minute))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-b", []uint32{userToken(userID, 1) + 1}, now.Add(-time.Minute))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-c", []uint32{userToken(userID, 2) + 1}, now.Add(-time.Minute))},
+				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, 3) + 1}, now.Add(-time.Minute))},
+				{what: add, instanceID: "instance-5", instanceDesc: generateRingInstanceWithInfo("instance-5", "zone-b", []uint32{userToken(userID, 4) + 1}, now.Add(-time.Minute))},
+				{what: add, instanceID: "instance-6", instanceDesc: generateRingInstanceWithInfo("instance-6", "zone-c", []uint32{userToken(userID, 5) + 1}, now.Add(-time.Minute))},
+				{what: test, shardSize: 3, expected: []string{"instance-1", "instance-2", "instance-3", "instance-4", "instance-5", "instance-6"}},
+			},
+		},
+		"multi zone, shard size = 3, instances scale up": {
+			timeline: []event{
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-b", []uint32{userToken(userID, 1) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-c", []uint32{userToken(userID, 2) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, 0) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-5", instanceDesc: generateRingInstanceWithInfo("instance-5", "zone-b", []uint32{userToken(userID, 1) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-6", instanceDesc: generateRingInstanceWithInfo("instance-6", "zone-c", []uint32{userToken(userID, 2) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: test, shardSize: 3, expected: []string{"instance-1", "instance-2", "instance-3"}},
+				// Scale up.
+				{what: add, instanceID: "instance-7", instanceDesc: generateRingInstanceWithInfo("instance-7", "zone-a", []uint32{userToken(userID, 0) + 1}, now)},
+				{what: add, instanceID: "instance-8", instanceDesc: generateRingInstanceWithInfo("instance-8", "zone-b", []uint32{userToken(userID, 1) + 1}, now)},
+				{what: add, instanceID: "instance-9", instanceDesc: generateRingInstanceWithInfo("instance-9", "zone-c", []uint32{userToken(userID, 2) + 1}, now)},
+				{what: test, shardSize: 3, expected: []string{"instance-7", "instance-8", "instance-9" /* lookback: */, "instance-1", "instance-2", "instance-3"}},
+			},
+		},
+		"multi zone, shard size = 3, instances scale down": {
+			timeline: []event{
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-b", []uint32{userToken(userID, 1) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-c", []uint32{userToken(userID, 2) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, 0) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-5", instanceDesc: generateRingInstanceWithInfo("instance-5", "zone-b", []uint32{userToken(userID, 1) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-6", instanceDesc: generateRingInstanceWithInfo("instance-6", "zone-c", []uint32{userToken(userID, 2) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-7", instanceDesc: generateRingInstanceWithInfo("instance-7", "zone-a", []uint32{userToken(userID, 0) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-8", instanceDesc: generateRingInstanceWithInfo("instance-8", "zone-b", []uint32{userToken(userID, 1) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-9", instanceDesc: generateRingInstanceWithInfo("instance-9", "zone-c", []uint32{userToken(userID, 2) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: test, shardSize: 3, expected: []string{"instance-1", "instance-2", "instance-3"}},
+				// Scale down.
+				{what: remove, instanceID: "instance-1"},
+				{what: remove, instanceID: "instance-2"},
+				{what: remove, instanceID: "instance-3"},
+				{what: test, shardSize: 3, expected: []string{"instance-7", "instance-8", "instance-9"}},
+			},
+		},
+		"multi zone, increase shard size": {
+			timeline: []event{
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-b", []uint32{userToken(userID, 1) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-c", []uint32{userToken(userID, 2) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, 0) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-5", instanceDesc: generateRingInstanceWithInfo("instance-5", "zone-b", []uint32{userToken(userID, 1) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-6", instanceDesc: generateRingInstanceWithInfo("instance-6", "zone-c", []uint32{userToken(userID, 2) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-7", instanceDesc: generateRingInstanceWithInfo("instance-7", "zone-a", []uint32{userToken(userID, 0) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-8", instanceDesc: generateRingInstanceWithInfo("instance-8", "zone-b", []uint32{userToken(userID, 1) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-9", instanceDesc: generateRingInstanceWithInfo("instance-9", "zone-c", []uint32{userToken(userID, 2) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: test, shardSize: 3, expected: []string{"instance-1", "instance-2", "instance-3"}},
+				{what: test, shardSize: 6, expected: []string{"instance-1", "instance-2", "instance-3", "instance-7", "instance-8", "instance-9"}},
+			},
+		},
+	}
+
+	for testName, testData := range tests {
+		t.Run(testName, func(t *testing.T) {
+			// Initialise the ring.
+			ringDesc := &Desc{Ingesters: map[string]IngesterDesc{}}
+			ring := Ring{
+				cfg: Config{
+					HeartbeatTimeout:     time.Hour,
+					ZoneAwarenessEnabled: true,
+				},
+				ringDesc:         ringDesc,
+				ringTokens:       ringDesc.getTokens(),
+				ringTokensByZone: ringDesc.getTokensByZone(),
+				ringZones:        getZones(ringDesc.getTokensByZone()),
+				strategy:         &DefaultReplicationStrategy{},
+			}
+
+			// Replay the events on the timeline.
+			for _, event := range testData.timeline {
+				switch event.what {
+				case add:
+					ringDesc.Ingesters[event.instanceID] = event.instanceDesc
+
+					ring.ringTokens = ringDesc.getTokens()
+					ring.ringTokensByZone = ringDesc.getTokensByZone()
+					ring.ringZones = getZones(ringDesc.getTokensByZone())
+				case remove:
+					delete(ringDesc.Ingesters, event.instanceID)
+
+					ring.ringTokens = ringDesc.getTokens()
+					ring.ringTokensByZone = ringDesc.getTokensByZone()
+					ring.ringZones = getZones(ringDesc.getTokensByZone())
+				case test:
+					rs, err := ring.ShuffleShardWithLookback(userID, event.shardSize, lookbackPeriod).GetAll(Read)
+					require.NoError(t, err)
+					assert.ElementsMatch(t, event.expected, rs.GetAddresses())
+				}
+			}
+		})
+	}
+}
+
+func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
+	// The goal of this test is NOT to ensure that the minimum required number of instances
+	// are returned at any given time, BUT at least all required instances are returned.
+	var (
+		numInitialInstances = []int{9, 30, 60, 90}
+		numInitialZones     = []int{1, 3}
+		numEvents           = 100
+		lookbackPeriod      = time.Hour
+		delayBetweenEvents  = 5 * time.Minute // 12 events / hour
+		userID              = "user-1"
+
+		// Make test execution reproducible.
+		entropy = rand.New(rand.NewSource(1))
+	)
+
+	for _, numInstances := range numInitialInstances {
+		for _, numZones := range numInitialZones {
+			testName := fmt.Sprintf("num instances = %d, num zones = %d", numInstances, numZones)
+
+			t.Run(testName, func(t *testing.T) {
+				// Initialise the ring.
+				ringDesc := &Desc{Ingesters: generateRingInstances(numInstances, numZones)}
+				ring := Ring{
+					cfg: Config{
+						HeartbeatTimeout:     time.Hour,
+						ZoneAwarenessEnabled: true,
+					},
+					ringDesc:         ringDesc,
+					ringTokens:       ringDesc.getTokens(),
+					ringTokensByZone: ringDesc.getTokensByZone(),
+					ringZones:        getZones(ringDesc.getTokensByZone()),
+					strategy:         &DefaultReplicationStrategy{},
+				}
+
+				// The simulation starts with the minimum shard size. Random events can later increase it.
+				shardSize := numZones
+
+				// The simulation assumes the initial ring contains instances registered
+				// since more than the lookback period.
+				currTime := time.Now().Add(lookbackPeriod).Add(time.Minute)
+
+				// Add the initial shard to the history.
+				rs, err := ring.ShuffleShard(userID, shardSize).GetAll(Read)
+				require.NoError(t, err)
+
+				history := map[time.Time]ReplicationSet{
+					currTime: rs,
+				}
+
+				// Simulate a progression of random events over the time and, at each iteration of the simuation,
+				// make sure the subring includes all non-removed instances picked from previous versions of the
+				// ring up until the lookback period.
+				for i := 1; i <= numEvents; i++ {
+					currTime = currTime.Add(delayBetweenEvents)
+
+					switch r := entropy.Intn(100); {
+					case r < 80:
+						// Scale up instances by 1.
+						id := len(ringDesc.Ingesters) + 1
+						instanceID := fmt.Sprintf("instance-%d", id)
+						zoneID := fmt.Sprintf("zone-%d", id%numZones)
+						ringDesc.Ingesters[instanceID] = generateRingInstanceWithInfo(instanceID, zoneID, GenerateTokens(128, nil), currTime)
+
+						ring.ringTokens = ringDesc.getTokens()
+						ring.ringTokensByZone = ringDesc.getTokensByZone()
+						ring.ringZones = getZones(ringDesc.getTokensByZone())
+					case r < 90:
+						// Scale down instances by 1.
+						idxToRemove := rand.Intn(len(ring.ringTokens))
+						idToRemove := ring.ringTokens[idxToRemove].Ingester
+						delete(ringDesc.Ingesters, idToRemove)
+
+						ring.ringTokens = ringDesc.getTokens()
+						ring.ringTokensByZone = ringDesc.getTokensByZone()
+						ring.ringZones = getZones(ringDesc.getTokensByZone())
+
+						// Remove the terminated instance from the history.
+						for ringTime, ringState := range history {
+							for idx, desc := range ringState.Ingesters {
+								// In this simulation instance ID == instance address.
+								if desc.Addr != idToRemove {
+									continue
+								}
+
+								ringState.Ingesters = append(ringState.Ingesters[:idx], ringState.Ingesters[idx+1:]...)
+								history[ringTime] = ringState
+								break
+							}
+						}
+					default:
+						// Scale up shard size (keeping the per-zone balance).
+						shardSize += numZones
+					}
+
+					// Add the current shard to the history.
+					rs, err = ring.ShuffleShard(userID, shardSize).GetAll(Read)
+					require.NoError(t, err)
+					history[currTime] = rs
+
+					// Ensure the shard with lookback includes all instances from previous states of the ring.
+					rsWithLookback, err := ring.ShuffleShardWithLookback(userID, shardSize, lookbackPeriod).GetAll(Read)
+					require.NoError(t, err)
+
+					for ringTime, ringState := range history {
+						if ringTime.Before(currTime.Add(-lookbackPeriod)) {
+							// This entry from the history is obsolete, we can remove it.
+							delete(history, ringTime)
+							continue
+						}
+
+						for _, expectedAddr := range ringState.GetAddresses() {
+							if !rsWithLookback.Includes(expectedAddr) {
+								t.Fatalf(
+									"subring generated after event %d is expected to include instance %s but it's missing (actual instances are: %s)",
+									i, expectedAddr, strings.Join(rsWithLookback.GetAddresses(), ", "))
+							}
+						}
+					}
+				}
+			})
+		}
+	}
+}
+
 func BenchmarkRing_ShuffleShard(b *testing.B) {
 	for _, numInstances := range []int{50, 100, 1000} {
 		for _, numZones := range []int{1, 3} {
@@ -832,13 +1183,20 @@ func generateRingInstances(numInstances, numZones int) map[string]IngesterDesc {
 }
 
 func generateRingInstance(id, zone int) (string, IngesterDesc) {
-	return fmt.Sprintf("instance-%d", id), IngesterDesc{
-		Addr:                fmt.Sprintf("127.0.0.%d", id),
+	instanceID := fmt.Sprintf("instance-%d", id)
+	zoneID := fmt.Sprintf("zone-%d", zone)
+
+	return instanceID, generateRingInstanceWithInfo(instanceID, zoneID, GenerateTokens(128, nil), time.Now())
+}
+
+func generateRingInstanceWithInfo(addr, zone string, tokens []uint32, registeredAt time.Time) IngesterDesc {
+	return IngesterDesc{
+		Addr:                addr,
 		Timestamp:           time.Now().Unix(),
-		RegisteredTimestamp: time.Now().Unix(),
+		RegisteredTimestamp: registeredAt.Unix(),
 		State:               ACTIVE,
-		Tokens:              GenerateTokens(128, nil),
-		Zone:                fmt.Sprintf("zone-%d", zone),
+		Tokens:              tokens,
+		Zone:                zone,
 	}
 }
 
@@ -1045,3 +1403,13 @@ func TestShuffleShardWithCaching(t *testing.T) {
 	// Zone-aware shuffle-shard gives all zones the same number of ingesters (at least one).
 	require.Equal(t, zones, newSubring.IngesterCount())
 }
+
+// User shuffle shard token.
+func userToken(user string, skip int) uint32 {
+	r := rand.New(rand.NewSource(util.ShuffleShardSeed(user)))
+
+	for ; skip > 0; skip-- {
+		_ = r.Uint32()
+	}
+	return r.Uint32()
+}

From 3791c04939dec47a6cc3f8a55fa9e160f8e50442 Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Wed, 30 Sep 2020 16:59:33 +0200
Subject: [PATCH 02/17] Committed broken test to signal there's an issue

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 pkg/distributor/query.go |  5 +--
 pkg/ring/model.go        | 12 +++++++
 pkg/ring/ring.go         | 12 +++----
 pkg/ring/ring_test.go    | 78 ++++++++++++++++++++++++++++------------
 4 files changed, 77 insertions(+), 30 deletions(-)

diff --git a/pkg/distributor/query.go b/pkg/distributor/query.go
index 8edef1a0e49..3978a347a7a 100644
--- a/pkg/distributor/query.go
+++ b/pkg/distributor/query.go
@@ -3,6 +3,7 @@ package distributor
 import (
 	"context"
 	"io"
+	"time"
 
 	"github.com/opentracing/opentracing-go"
 	"github.com/prometheus/common/model"
@@ -87,7 +88,7 @@ func (d *Distributor) getIngestersForQuery(ctx context.Context, matchers ...*lab
 		lookbackPeriod := d.cfg.ShuffleShardingLookbackPeriod
 
 		if shardSize > 0 && lookbackPeriod > 0 {
-			return d.ingestersRing.ShuffleShardWithLookback(userID, shardSize, lookbackPeriod).GetAll(ring.Read)
+			return d.ingestersRing.ShuffleShardWithLookback(userID, shardSize, lookbackPeriod, time.Now()).GetAll(ring.Read)
 		}
 	}
 
@@ -115,7 +116,7 @@ func (d *Distributor) getIngestersForMetadata(ctx context.Context) (ring.Replica
 		lookbackPeriod := d.cfg.ShuffleShardingLookbackPeriod
 
 		if shardSize > 0 {
-			return d.ingestersRing.ShuffleShardWithLookback(userID, shardSize, lookbackPeriod).GetAll(ring.Read)
+			return d.ingestersRing.ShuffleShardWithLookback(userID, shardSize, lookbackPeriod, time.Now()).GetAll(ring.Read)
 		}
 	}
 
diff --git a/pkg/ring/model.go b/pkg/ring/model.go
index 19ab91c9e53..c9e09726f27 100644
--- a/pkg/ring/model.go
+++ b/pkg/ring/model.go
@@ -25,6 +25,18 @@ func (ts ByAddr) Len() int           { return len(ts) }
 func (ts ByAddr) Swap(i, j int)      { ts[i], ts[j] = ts[j], ts[i] }
 func (ts ByAddr) Less(i, j int) bool { return ts[i].Addr < ts[j].Addr }
 
+// ByZoneAndAddr is a sortable list of IngesterDesc.
+type ByZoneAddr []IngesterDesc
+
+func (ts ByZoneAddr) Len() int      { return len(ts) }
+func (ts ByZoneAddr) Swap(i, j int) { ts[i], ts[j] = ts[j], ts[i] }
+func (ts ByZoneAddr) Less(i, j int) bool {
+	if ts[i].Zone != ts[j].Zone {
+		return ts[i].Zone < ts[j].Zone
+	}
+	return ts[i].Addr < ts[j].Addr
+}
+
 // ProtoDescFactory makes new Descs
 func ProtoDescFactory() proto.Message {
 	return NewDesc()
diff --git a/pkg/ring/ring.go b/pkg/ring/ring.go
index ee3762dec50..1cb48b4cddc 100644
--- a/pkg/ring/ring.go
+++ b/pkg/ring/ring.go
@@ -54,7 +54,7 @@ type ReadRing interface {
 
 	// ShuffleShardWithLookback is like ShuffleShard() but the returned subring includes
 	// all instances that have been part of the identifier's shard since "now - lookbackPeriod".
-	ShuffleShardWithLookback(identifier string, size int, lookbackPeriod time.Duration) ReadRing
+	ShuffleShardWithLookback(identifier string, size int, lookbackPeriod time.Duration, now time.Time) ReadRing
 
 	// HasInstance returns whether the ring contains an instance matching the provided instanceID.
 	HasInstance(instanceID string) bool
@@ -475,7 +475,7 @@ func (r *Ring) ShuffleShard(identifier string, size int) ReadRing {
 		return cached
 	}
 
-	result := r.shuffleShard(identifier, size, 0)
+	result := r.shuffleShard(identifier, size, 0, time.Now())
 
 	r.setCachedShuffledSubring(identifier, size, result.(*Ring))
 	return result
@@ -488,17 +488,17 @@ func (r *Ring) ShuffleShard(identifier string, size int) ReadRing {
 // operations (read only).
 //
 // This function doesn't support caching.
-func (r *Ring) ShuffleShardWithLookback(identifier string, size int, lookbackPeriod time.Duration) ReadRing {
+func (r *Ring) ShuffleShardWithLookback(identifier string, size int, lookbackPeriod time.Duration, now time.Time) ReadRing {
 	// Nothing to do if the shard size is not smaller then the actual ring.
 	if size <= 0 || r.IngesterCount() <= size {
 		return r
 	}
 
-	return r.shuffleShard(identifier, size, lookbackPeriod)
+	return r.shuffleShard(identifier, size, lookbackPeriod, now)
 }
 
-func (r *Ring) shuffleShard(identifier string, size int, lookbackPeriod time.Duration) ReadRing {
-	lookbackUntil := time.Now().Add(-lookbackPeriod).Unix()
+func (r *Ring) shuffleShard(identifier string, size int, lookbackPeriod time.Duration, now time.Time) ReadRing {
+	lookbackUntil := now.Add(-lookbackPeriod).Unix()
 
 	var result *Ring
 
diff --git a/pkg/ring/ring_test.go b/pkg/ring/ring_test.go
index 73a8ed1f7fd..da36f45492b 100644
--- a/pkg/ring/ring_test.go
+++ b/pkg/ring/ring_test.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"math"
 	"math/rand"
+	"sort"
 	"strconv"
 	"strings"
 	"testing"
@@ -837,9 +838,9 @@ func TestRing_ShuffleShardWithLookback(t *testing.T) {
 				{what: test, shardSize: 1, expected: []string{"instance-1"}},
 				// Rollout instance-1.
 				{what: remove, instanceID: "instance-1"},
-				{what: test, shardSize: 1, expected: []string{"instance-2", "instance-3"}}, // TODO instance-3 is a side effect (investigate better)
+				{what: test, shardSize: 1, expected: []string{"instance-2" /* side effect: */, "instance-3"}},
 				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now)},
-				{what: test, shardSize: 1, expected: []string{"instance-1" /* lookback: */, "instance-2", "instance-3"}},
+				{what: test, shardSize: 1, expected: []string{"instance-1" /* lookback: */, "instance-2" /* side effect: */, "instance-3"}},
 			},
 		},
 		"single zone, shard size = 2, rollout with instances unregistered (removed and re-added one by one)": {
@@ -861,14 +862,14 @@ func TestRing_ShuffleShardWithLookback(t *testing.T) {
 				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2"}},
 				// Rollout instance-2.
 				{what: remove, instanceID: "instance-2"},
-				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-3", "instance-4"}}, // TODO instance-4 is a side effect (investigate better)
+				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-3" /* side effect:*/, "instance-4"}},
 				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now)},
-				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2" /* lookback: */, "instance-3", "instance-4"}},
+				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2" /* lookback: */, "instance-3" /* side effect:*/, "instance-4"}},
 				// Rollout instance-1.
 				{what: remove, instanceID: "instance-1"},
-				{what: test, shardSize: 2, expected: []string{"instance-2" /* lookback: */, "instance-3", "instance-4"}},
+				{what: test, shardSize: 2, expected: []string{"instance-2" /* lookback: */, "instance-3" /* side effect:*/, "instance-4"}},
 				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now)},
-				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2" /* lookback: */, "instance-3", "instance-4"}},
+				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2" /* lookback: */, "instance-3" /* side effect:*/, "instance-4"}},
 			},
 		},
 		"single zone, increase shard size": {
@@ -977,7 +978,7 @@ func TestRing_ShuffleShardWithLookback(t *testing.T) {
 					ring.ringTokensByZone = ringDesc.getTokensByZone()
 					ring.ringZones = getZones(ringDesc.getTokensByZone())
 				case test:
-					rs, err := ring.ShuffleShardWithLookback(userID, event.shardSize, lookbackPeriod).GetAll(Read)
+					rs, err := ring.ShuffleShardWithLookback(userID, event.shardSize, lookbackPeriod, time.Now()).GetAll(Read)
 					require.NoError(t, err)
 					assert.ElementsMatch(t, event.expected, rs.GetAddresses())
 				}
@@ -990,17 +991,29 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
 	// The goal of this test is NOT to ensure that the minimum required number of instances
 	// are returned at any given time, BUT at least all required instances are returned.
 	var (
-		numInitialInstances = []int{9, 30, 60, 90}
-		numInitialZones     = []int{1, 3}
+		numInitialInstances = []int{9} // TODO , 30, 60, 90}
+		numInitialZones     = []int{3} // TODO, 1, 3}
 		numEvents           = 100
 		lookbackPeriod      = time.Hour
 		delayBetweenEvents  = 5 * time.Minute // 12 events / hour
 		userID              = "user-1"
-
-		// Make test execution reproducible.
-		entropy = rand.New(rand.NewSource(1))
 	)
 
+	// TODO randomise it but log the seed to have a way to reproduce tests if break
+	rand.Seed(2)
+
+	//printShard := func(rs ReplicationSet) {
+	//	fmt.Print("curr shard: ")
+	//
+	//	ingesters := rs.Ingesters
+	//	sort.Sort(ByZoneAddr(ingesters))
+	//
+	//	for _, i := range ingesters {
+	//		fmt.Print(i.Addr, "(", i.Zone, "), ")
+	//	}
+	//	fmt.Print("\n")
+	//}
+
 	for _, numInstances := range numInitialInstances {
 		for _, numZones := range numInitialZones {
 			testName := fmt.Sprintf("num instances = %d, num zones = %d", numInstances, numZones)
@@ -1030,6 +1043,7 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
 				// Add the initial shard to the history.
 				rs, err := ring.ShuffleShard(userID, shardSize).GetAll(Read)
 				require.NoError(t, err)
+				//printShard(rs)
 
 				history := map[time.Time]ReplicationSet{
 					currTime: rs,
@@ -1038,26 +1052,43 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
 				// Simulate a progression of random events over the time and, at each iteration of the simuation,
 				// make sure the subring includes all non-removed instances picked from previous versions of the
 				// ring up until the lookback period.
+				nextIngesterID := len(ringDesc.Ingesters)
+
 				for i := 1; i <= numEvents; i++ {
+					//fmt.Println("")
+
 					currTime = currTime.Add(delayBetweenEvents)
 
-					switch r := entropy.Intn(100); {
+					switch r := rand.Intn(100); {
 					case r < 80:
 						// Scale up instances by 1.
-						id := len(ringDesc.Ingesters) + 1
-						instanceID := fmt.Sprintf("instance-%d", id)
-						zoneID := fmt.Sprintf("zone-%d", id%numZones)
+						instanceID := fmt.Sprintf("instance-%d", nextIngesterID)
+						zoneID := fmt.Sprintf("zone-%d", nextIngesterID%numZones)
+						nextIngesterID++
+
+						//fmt.Println(currTime.String(), "scale up", instanceID, "zone", zoneID)
+
 						ringDesc.Ingesters[instanceID] = generateRingInstanceWithInfo(instanceID, zoneID, GenerateTokens(128, nil), currTime)
 
 						ring.ringTokens = ringDesc.getTokens()
 						ring.ringTokensByZone = ringDesc.getTokensByZone()
 						ring.ringZones = getZones(ringDesc.getTokensByZone())
 					case r < 90:
-						// Scale down instances by 1.
-						idxToRemove := rand.Intn(len(ring.ringTokens))
-						idToRemove := ring.ringTokens[idxToRemove].Ingester
+						// Scale down instances by 1. To make tests reproducible we get the instance IDs, sort them
+						// and then get a random index (using the random generator initialized with a constant seed).
+						ingesterIDs := make([]string, 0, len(ringDesc.Ingesters))
+						for id := range ringDesc.Ingesters {
+							ingesterIDs = append(ingesterIDs, id)
+						}
+
+						sort.Strings(ingesterIDs)
+
+						idxToRemove := rand.Intn(len(ingesterIDs))
+						idToRemove := ingesterIDs[idxToRemove]
 						delete(ringDesc.Ingesters, idToRemove)
 
+						//fmt.Println(currTime.String(), "scale down", idToRemove)
+
 						ring.ringTokens = ringDesc.getTokens()
 						ring.ringTokensByZone = ringDesc.getTokensByZone()
 						ring.ringZones = getZones(ringDesc.getTokensByZone())
@@ -1078,15 +1109,18 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
 					default:
 						// Scale up shard size (keeping the per-zone balance).
 						shardSize += numZones
+
+						//fmt.Println(currTime.String(), "shard size scaled up to", shardSize)
 					}
 
 					// Add the current shard to the history.
 					rs, err = ring.ShuffleShard(userID, shardSize).GetAll(Read)
 					require.NoError(t, err)
 					history[currTime] = rs
+					//printShard(rs)
 
 					// Ensure the shard with lookback includes all instances from previous states of the ring.
-					rsWithLookback, err := ring.ShuffleShardWithLookback(userID, shardSize, lookbackPeriod).GetAll(Read)
+					rsWithLookback, err := ring.ShuffleShardWithLookback(userID, shardSize, lookbackPeriod, currTime).GetAll(Read)
 					require.NoError(t, err)
 
 					for ringTime, ringState := range history {
@@ -1099,8 +1133,8 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
 						for _, expectedAddr := range ringState.GetAddresses() {
 							if !rsWithLookback.Includes(expectedAddr) {
 								t.Fatalf(
-									"subring generated after event %d is expected to include instance %s but it's missing (actual instances are: %s)",
-									i, expectedAddr, strings.Join(rsWithLookback.GetAddresses(), ", "))
+									"subring generated after event %d is expected to include instance %s from ring state at time %s but it's missing (actual instances are: %s)",
+									i, expectedAddr, ringTime.String(), strings.Join(rsWithLookback.GetAddresses(), ", "))
 							}
 						}
 					}

From 89f7ed49a683d90d1874e00782f435214cdd360f Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Thu, 8 Oct 2020 09:54:16 +0200
Subject: [PATCH 03/17] Fixed shuffle sharding consistency when zone-awareness
 is enabled and the shard size is increased or instances in a new zone are
 added

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 CHANGELOG.md          |   1 +
 pkg/ring/ring_test.go | 162 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 163 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 54b06549813..0b9da4a51a6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -92,6 +92,7 @@
 * [BUGFIX] Use a valid grpc header when logging IP addresses. #3307
 * [BUGFIX] Fixed the metric `cortex_prometheus_rule_group_duration_seconds` in the Ruler, it wouldn't report any values. #3310
 * [BUGFIX] Fixed gRPC connections leaking in rulers when rulers sharding is enabled and APIs called. #3314
+* [BUGFIX] Fixed shuffle sharding consistency when zone-awareness is enabled and the shard size is increased or instances in a new zone are added. #3299
 
 ## 1.4.0 / 2020-10-02
 
diff --git a/pkg/ring/ring_test.go b/pkg/ring/ring_test.go
index da36f45492b..76df6f96bf7 100644
--- a/pkg/ring/ring_test.go
+++ b/pkg/ring/ring_test.go
@@ -1144,6 +1144,168 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
 	}
 }
 
+func TestRing_ShuffleShard_ConsistencyOnShardSizeChanged(t *testing.T) {
+	// Create 30 instances in 3 zones.
+	ringInstances := map[string]IngesterDesc{}
+	for i := 0; i < 30; i++ {
+		name, desc := generateRingInstance(i, i%3)
+		ringInstances[name] = desc
+	}
+
+	// Init the ring description.
+	ringDesc := &Desc{Ingesters: ringInstances}
+	for id, instance := range ringDesc.Ingesters {
+		instance.Timestamp = time.Now().Unix()
+		instance.State = ACTIVE
+		ringDesc.Ingesters[id] = instance
+	}
+
+	ring := Ring{
+		cfg: Config{
+			HeartbeatTimeout:     time.Hour,
+			ZoneAwarenessEnabled: true,
+		},
+		ringDesc:         ringDesc,
+		ringTokens:       ringDesc.getTokens(),
+		ringTokensByZone: ringDesc.getTokensByZone(),
+		ringZones:        getZones(ringDesc.getTokensByZone()),
+		strategy:         &DefaultReplicationStrategy{},
+	}
+
+	// Get the replication set with shard size = 3.
+	firstShard := ring.ShuffleShard("tenant-id", 3)
+	assert.Equal(t, 3, firstShard.IngesterCount())
+
+	firstSet, err := firstShard.GetAll(Read)
+	require.NoError(t, err)
+
+	// Increase shard size to 6.
+	secondShard := ring.ShuffleShard("tenant-id", 6)
+	assert.Equal(t, 6, secondShard.IngesterCount())
+
+	secondSet, err := secondShard.GetAll(Read)
+	require.NoError(t, err)
+
+	for _, firstInstance := range firstSet.Ingesters {
+		assert.True(t, secondSet.Includes(firstInstance.Addr), "new replication set is expected to include previous instance %s", firstInstance.Addr)
+	}
+
+	// Increase shard size to 9.
+	thirdShard := ring.ShuffleShard("tenant-id", 9)
+	assert.Equal(t, 9, thirdShard.IngesterCount())
+
+	thirdSet, err := thirdShard.GetAll(Read)
+	require.NoError(t, err)
+
+	for _, secondInstance := range secondSet.Ingesters {
+		assert.True(t, thirdSet.Includes(secondInstance.Addr), "new replication set is expected to include previous instance %s", secondInstance.Addr)
+	}
+
+	// Decrease shard size to 6.
+	fourthShard := ring.ShuffleShard("tenant-id", 6)
+	assert.Equal(t, 6, fourthShard.IngesterCount())
+
+	fourthSet, err := fourthShard.GetAll(Read)
+	require.NoError(t, err)
+
+	// We expect to have the same exact instances we had when the shard size was 6.
+	for _, secondInstance := range secondSet.Ingesters {
+		assert.True(t, fourthSet.Includes(secondInstance.Addr), "new replication set is expected to include previous instance %s", secondInstance.Addr)
+	}
+
+	// Decrease shard size to 3.
+	fifthShard := ring.ShuffleShard("tenant-id", 3)
+	assert.Equal(t, 3, fifthShard.IngesterCount())
+
+	fifthSet, err := fifthShard.GetAll(Read)
+	require.NoError(t, err)
+
+	// We expect to have the same exact instances we had when the shard size was 3.
+	for _, firstInstance := range firstSet.Ingesters {
+		assert.True(t, fifthSet.Includes(firstInstance.Addr), "new replication set is expected to include previous instance %s", firstInstance.Addr)
+	}
+}
+
+func TestRing_ShuffleShard_ConsistencyOnZonesChanged(t *testing.T) {
+	// Create 20 instances in 2 zones.
+	ringInstances := map[string]IngesterDesc{}
+	for i := 0; i < 20; i++ {
+		name, desc := generateRingInstance(i, i%2)
+		ringInstances[name] = desc
+	}
+
+	// Init the ring description.
+	ringDesc := &Desc{Ingesters: ringInstances}
+	for id, instance := range ringDesc.Ingesters {
+		instance.Timestamp = time.Now().Unix()
+		instance.State = ACTIVE
+		ringDesc.Ingesters[id] = instance
+	}
+
+	ring := Ring{
+		cfg: Config{
+			HeartbeatTimeout:     time.Hour,
+			ZoneAwarenessEnabled: true,
+		},
+		ringDesc:         ringDesc,
+		ringTokens:       ringDesc.getTokens(),
+		ringTokensByZone: ringDesc.getTokensByZone(),
+		ringZones:        getZones(ringDesc.getTokensByZone()),
+		strategy:         &DefaultReplicationStrategy{},
+	}
+
+	// Get the replication set with shard size = 2.
+	firstShard := ring.ShuffleShard("tenant-id", 2)
+	assert.Equal(t, 2, firstShard.IngesterCount())
+
+	firstSet, err := firstShard.GetAll(Read)
+	require.NoError(t, err)
+
+	// Increase shard size to 4.
+	secondShard := ring.ShuffleShard("tenant-id", 4)
+	assert.Equal(t, 4, secondShard.IngesterCount())
+
+	secondSet, err := secondShard.GetAll(Read)
+	require.NoError(t, err)
+
+	for _, firstInstance := range firstSet.Ingesters {
+		assert.True(t, secondSet.Includes(firstInstance.Addr), "new replication set is expected to include previous instance %s", firstInstance.Addr)
+	}
+
+	// Scale up cluster, adding 10 instances in 1 new zone.
+	for i := 20; i < 30; i++ {
+		name, desc := generateRingInstance(i, 2)
+		ringInstances[name] = desc
+	}
+
+	ring.ringDesc.Ingesters = ringInstances
+	ring.ringTokens = ringDesc.getTokens()
+	ring.ringTokensByZone = ringDesc.getTokensByZone()
+	ring.ringZones = getZones(ringDesc.getTokensByZone())
+
+	// Increase shard size to 6.
+	thirdShard := ring.ShuffleShard("tenant-id", 6)
+	assert.Equal(t, 6, thirdShard.IngesterCount())
+
+	thirdSet, err := thirdShard.GetAll(Read)
+	require.NoError(t, err)
+
+	for _, secondInstance := range secondSet.Ingesters {
+		assert.True(t, thirdSet.Includes(secondInstance.Addr), "new replication set is expected to include previous instance %s", secondInstance.Addr)
+	}
+
+	// Increase shard size to 9.
+	fourthShard := ring.ShuffleShard("tenant-id", 9)
+	assert.Equal(t, 9, fourthShard.IngesterCount())
+
+	fourthSet, err := fourthShard.GetAll(Read)
+	require.NoError(t, err)
+
+	for _, thirdInstance := range thirdSet.Ingesters {
+		assert.True(t, fourthSet.Includes(thirdInstance.Addr), "new replication set is expected to include previous instance %s", thirdInstance.Addr)
+	}
+}
+
 func BenchmarkRing_ShuffleShard(b *testing.B) {
 	for _, numInstances := range []int{50, 100, 1000} {
 		for _, numZones := range []int{1, 3} {

From c691da8d51f6ead0a7484cda647289493a2abc5d Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Thu, 8 Oct 2020 11:24:05 +0200
Subject: [PATCH 04/17] Added ShuffleShardingIngestersLookbackPeriod validation

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 docs/blocks-storage/querier.md              |  2 +-
 docs/configuration/config-file-reference.md |  3 +-
 pkg/distributor/query.go                    |  2 +-
 pkg/querier/querier.go                      | 12 +++++--
 pkg/querier/querier_test.go                 | 39 +++++++++++++++++++++
 5 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/docs/blocks-storage/querier.md b/docs/blocks-storage/querier.md
index f95a8d7d302..a86222e2942 100644
--- a/docs/blocks-storage/querier.md
+++ b/docs/blocks-storage/querier.md
@@ -178,7 +178,7 @@ querier:
   # > 0, queriers fetch in-memory series from the minimum set of required
   # ingesters, selecting only ingesters which may have received series since
   # 'now - lookback period'. If this setting is 0, queriers always query all
-  # ingesters.
+  # ingesters (ingesters shuffle sharding on read path is disabled).
   # CLI flag: -querier.shuffle-sharding-ingesters-lookback-period
   [shuffle_sharding_ingesters_lookback_period: <duration> | default = 0s]
 ```
diff --git a/docs/configuration/config-file-reference.md b/docs/configuration/config-file-reference.md
index eb2706e8f4a..32b226db083 100644
--- a/docs/configuration/config-file-reference.md
+++ b/docs/configuration/config-file-reference.md
@@ -744,7 +744,8 @@ store_gateway_client:
 # When distributor's sharding strategy is shuffle-sharding and this setting is >
 # 0, queriers fetch in-memory series from the minimum set of required ingesters,
 # selecting only ingesters which may have received series since 'now - lookback
-# period'. If this setting is 0, queriers always query all ingesters.
+# period'. If this setting is 0, queriers always query all ingesters (ingesters
+# shuffle sharding on read path is disabled).
 # CLI flag: -querier.shuffle-sharding-ingesters-lookback-period
 [shuffle_sharding_ingesters_lookback_period: <duration> | default = 0s]
 ```
diff --git a/pkg/distributor/query.go b/pkg/distributor/query.go
index 3978a347a7a..95021ce0721 100644
--- a/pkg/distributor/query.go
+++ b/pkg/distributor/query.go
@@ -115,7 +115,7 @@ func (d *Distributor) getIngestersForMetadata(ctx context.Context) (ring.Replica
 		shardSize := d.limits.IngestionTenantShardSize(userID)
 		lookbackPeriod := d.cfg.ShuffleShardingLookbackPeriod
 
-		if shardSize > 0 {
+		if shardSize > 0 && lookbackPeriod > 0 {
 			return d.ingestersRing.ShuffleShardWithLookback(userID, shardSize, lookbackPeriod, time.Now()).GetAll(ring.Read)
 		}
 	}
diff --git a/pkg/querier/querier.go b/pkg/querier/querier.go
index ec4ea5f4b44..a9902486955 100644
--- a/pkg/querier/querier.go
+++ b/pkg/querier/querier.go
@@ -69,7 +69,8 @@ type Config struct {
 }
 
 var (
-	errBadLookbackConfigs = errors.New("bad settings, query_store_after >= query_ingesters_within which can result in queries not being sent")
+	errBadLookbackConfigs                             = errors.New("bad settings, query_store_after >= query_ingesters_within which can result in queries not being sent")
+	errShuffleShardingLookbackLessThanQueryStoreAfter = errors.New("the shuffle-sharding lookback period should be greater or equal than the configured 'query store after'")
 )
 
 // RegisterFlags adds the flags required to config this to the given FlagSet.
@@ -90,12 +91,11 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.DurationVar(&cfg.LookbackDelta, "querier.lookback-delta", 5*time.Minute, "Time since the last sample after which a time series is considered stale and ignored by expression evaluations.")
 	f.StringVar(&cfg.SecondStoreEngine, "querier.second-store-engine", "", "Second store engine to use for querying. Empty = disabled.")
 	f.Var(&cfg.UseSecondStoreBeforeTime, "querier.use-second-store-before-time", "If specified, second store is only used for queries before this timestamp. Default value 0 means secondary store is always queried.")
-	f.DurationVar(&cfg.ShuffleShardingIngestersLookbackPeriod, "querier.shuffle-sharding-ingesters-lookback-period", 0, "When distributor's sharding strategy is shuffle-sharding and this setting is > 0, queriers fetch in-memory series from the minimum set of required ingesters, selecting only ingesters which may have received series since 'now - lookback period'. If this setting is 0, queriers always query all ingesters.")
+	f.DurationVar(&cfg.ShuffleShardingIngestersLookbackPeriod, "querier.shuffle-sharding-ingesters-lookback-period", 0, "When distributor's sharding strategy is shuffle-sharding and this setting is > 0, queriers fetch in-memory series from the minimum set of required ingesters, selecting only ingesters which may have received series since 'now - lookback period'. If this setting is 0, queriers always query all ingesters (ingesters shuffle sharding on read path is disabled).")
 }
 
 // Validate the config
 func (cfg *Config) Validate() error {
-
 	// Ensure the config wont create a situation where no queriers are returned.
 	if cfg.QueryIngestersWithin != 0 && cfg.QueryStoreAfter != 0 {
 		if cfg.QueryStoreAfter >= cfg.QueryIngestersWithin {
@@ -103,6 +103,12 @@ func (cfg *Config) Validate() error {
 		}
 	}
 
+	if cfg.ShuffleShardingIngestersLookbackPeriod > 0 {
+		if cfg.ShuffleShardingIngestersLookbackPeriod < cfg.QueryStoreAfter {
+			return errShuffleShardingLookbackLessThanQueryStoreAfter
+		}
+	}
+
 	return nil
 }
 
diff --git a/pkg/querier/querier_test.go b/pkg/querier/querier_test.go
index f92df28a726..957f6e1b2aa 100644
--- a/pkg/querier/querier_test.go
+++ b/pkg/querier/querier_test.go
@@ -736,6 +736,45 @@ func TestStoreQueryable(t *testing.T) {
 	require.True(t, m.useQueryableCalled) // storeQueryable wraps QueryableWithFilter, so it must call its UseQueryable method.
 }
 
+func TestConfig_Validate(t *testing.T) {
+	tests := map[string]struct {
+		setup    func(cfg *Config)
+		expected error
+	}{
+		"should pass with default config": {
+			setup: func(cfg *Config) {},
+		},
+		"should pass if 'query store after' is enabled and shuffle-sharding is disabled": {
+			setup: func(cfg *Config) {
+				cfg.QueryStoreAfter = time.Hour
+			},
+		},
+		"should pass if 'query store after' is enabled and shuffle-sharding is enabled with greater value": {
+			setup: func(cfg *Config) {
+				cfg.QueryStoreAfter = time.Hour
+				cfg.ShuffleShardingIngestersLookbackPeriod = 2 * time.Hour
+			},
+		},
+		"should fail if 'query store after' is enabled and shuffle-sharding is enabled with lesser value": {
+			setup: func(cfg *Config) {
+				cfg.QueryStoreAfter = time.Hour
+				cfg.ShuffleShardingIngestersLookbackPeriod = time.Minute
+			},
+			expected: errShuffleShardingLookbackLessThanQueryStoreAfter,
+		},
+	}
+
+	for testName, testData := range tests {
+		t.Run(testName, func(t *testing.T) {
+			cfg := &Config{}
+			flagext.DefaultValues(cfg)
+			testData.setup(cfg)
+
+			assert.Equal(t, testData.expected, cfg.Validate())
+		})
+	}
+}
+
 type mockQueryableWithFilter struct {
 	useQueryableCalled bool
 }

From 1a0d1abd8372a88670d4b4a78274ed55e06b284b Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Thu, 8 Oct 2020 11:35:38 +0200
Subject: [PATCH 05/17] Tiny getIngestersForQuery() optimisation

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 pkg/distributor/query.go | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/pkg/distributor/query.go b/pkg/distributor/query.go
index 95021ce0721..963e67f4f14 100644
--- a/pkg/distributor/query.go
+++ b/pkg/distributor/query.go
@@ -93,9 +93,12 @@ func (d *Distributor) getIngestersForQuery(ctx context.Context, matchers ...*lab
 	}
 
 	// If "shard by all labels" is disabled, we can get ingesters by metricName if exists.
-	metricNameMatcher, _, ok := extract.MetricNameMatcherFromMatchers(matchers)
-	if !d.cfg.ShardByAllLabels && ok && metricNameMatcher.Type == labels.MatchEqual {
-		return d.ingestersRing.Get(shardByMetricName(userID, metricNameMatcher.Value), ring.Read, nil)
+	if !d.cfg.ShardByAllLabels {
+		metricNameMatcher, _, ok := extract.MetricNameMatcherFromMatchers(matchers)
+
+		if ok && metricNameMatcher.Type == labels.MatchEqual {
+			return d.ingestersRing.Get(shardByMetricName(userID, metricNameMatcher.Value), ring.Read, nil)
+		}
 	}
 
 	return d.ingestersRing.GetAll(ring.Read)

From 0d4249f048f49aaa15697853674764e9e1f8dfe5 Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Thu, 8 Oct 2020 12:36:14 +0200
Subject: [PATCH 06/17] Added unit tests on distributor changes

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 pkg/distributor/distributor.go      |   3 +-
 pkg/distributor/distributor_test.go | 414 +++++++++++++++++++---------
 2 files changed, 290 insertions(+), 127 deletions(-)

diff --git a/pkg/distributor/distributor.go b/pkg/distributor/distributor.go
index ceff247fc9e..51f977ab8ee 100644
--- a/pkg/distributor/distributor.go
+++ b/pkg/distributor/distributor.go
@@ -779,8 +779,7 @@ func (d *Distributor) MetricsMetadata(ctx context.Context) ([]scrape.MetricMetad
 
 // UserStats returns statistics about the current user.
 func (d *Distributor) UserStats(ctx context.Context) (*UserStats, error) {
-	// We always need to query all ingesters.
-	replicationSet, err := d.ingestersRing.GetAll(ring.Read)
+	replicationSet, err := d.getIngestersForMetadata(ctx)
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/distributor/distributor_test.go b/pkg/distributor/distributor_test.go
index 6276ea8c9be..27af909c5c5 100644
--- a/pkg/distributor/distributor_test.go
+++ b/pkg/distributor/distributor_test.go
@@ -445,22 +445,26 @@ func TestDistributor_PushHAInstances(t *testing.T) {
 }
 
 func TestDistributor_PushQuery(t *testing.T) {
+	const shuffleShardSize = 5
+
 	nameMatcher := mustEqualMatcher(model.MetricNameLabel, "foo")
 	barMatcher := mustEqualMatcher("bar", "baz")
 
 	type testcase struct {
-		name             string
-		numIngesters     int
-		happyIngesters   int
-		samples          int
-		metadata         int
-		matchers         []*labels.Matcher
-		expectedResponse model.Matrix
-		expectedError    error
-		shardByAllLabels bool
+		name                string
+		numIngesters        int
+		happyIngesters      int
+		samples             int
+		metadata            int
+		matchers            []*labels.Matcher
+		expectedIngesters   int
+		expectedResponse    model.Matrix
+		expectedError       error
+		shardByAllLabels    bool
+		shuffleShardEnabled bool
 	}
 
-	// We'll programatically build the test cases now, as we want complete
+	// We'll programmatically build the test cases now, as we want complete
 	// coverage along quite a few different axis.
 	testcases := []testcase{}
 
@@ -473,74 +477,98 @@ func TestDistributor_PushQuery(t *testing.T) {
 			// Test with between 0 and numIngesters "happy" ingesters.
 			for happyIngesters := 0; happyIngesters <= numIngesters; happyIngesters++ {
 
-				// When we're not sharding by metric name, queriers with more than one
-				// failed ingester should fail.
-				if shardByAllLabels && numIngesters-happyIngesters > 1 {
+				// Test either with shuffle-sharding enabled or disabled.
+				for _, shuffleShardEnabled := range []bool{false, true} {
+					scenario := fmt.Sprintf("shardByAllLabels=%v, numIngester=%d, happyIngester=%d, shuffleSharding=%v)", shardByAllLabels, numIngesters, happyIngesters, shuffleShardEnabled)
+
+					// The number of ingesters we expect to query depends whether shuffle sharding and/or
+					// shard by all labels are enabled.
+					var expectedIngesters int
+					if shuffleShardEnabled {
+						expectedIngesters = util.Min(shuffleShardSize, numIngesters)
+					} else if shardByAllLabels {
+						expectedIngesters = numIngesters
+					} else {
+						expectedIngesters = 3 // Replication factor
+					}
+
+					// When we're not sharding by metric name, queriers with more than one
+					// failed ingester should fail.
+					if shardByAllLabels && numIngesters-happyIngesters > 1 {
+						testcases = append(testcases, testcase{
+							name:                fmt.Sprintf("ExpectFail(%s)", scenario),
+							numIngesters:        numIngesters,
+							happyIngesters:      happyIngesters,
+							matchers:            []*labels.Matcher{nameMatcher, barMatcher},
+							expectedError:       errFail,
+							shardByAllLabels:    shardByAllLabels,
+							shuffleShardEnabled: shuffleShardEnabled,
+						})
+						continue
+					}
+
+					// When we have less ingesters than replication factor, any failed ingester
+					// will cause a failure.
+					if numIngesters < 3 && happyIngesters < 2 {
+						testcases = append(testcases, testcase{
+							name:                fmt.Sprintf("ExpectFail(%s)", scenario),
+							numIngesters:        numIngesters,
+							happyIngesters:      happyIngesters,
+							matchers:            []*labels.Matcher{nameMatcher, barMatcher},
+							expectedError:       errFail,
+							shardByAllLabels:    shardByAllLabels,
+							shuffleShardEnabled: shuffleShardEnabled,
+						})
+						continue
+					}
+
+					// If we're sharding by metric name and we have failed ingesters, we can't
+					// tell ahead of time if the query will succeed, as we don't know which
+					// ingesters will hold the results for the query.
+					if !shardByAllLabels && numIngesters-happyIngesters > 1 {
+						continue
+					}
+
+					// Reading all the samples back should succeed.
 					testcases = append(testcases, testcase{
-						name:             fmt.Sprintf("ExpectFail(shardByAllLabels=%v,numIngester=%d,happyIngester=%d)", shardByAllLabels, numIngesters, happyIngesters),
-						numIngesters:     numIngesters,
-						happyIngesters:   happyIngesters,
-						matchers:         []*labels.Matcher{nameMatcher, barMatcher},
-						expectedError:    errFail,
-						shardByAllLabels: shardByAllLabels,
+						name:                fmt.Sprintf("ReadAll(%s)", scenario),
+						numIngesters:        numIngesters,
+						happyIngesters:      happyIngesters,
+						samples:             10,
+						matchers:            []*labels.Matcher{nameMatcher, barMatcher},
+						expectedResponse:    expectedResponse(0, 10),
+						expectedIngesters:   expectedIngesters,
+						shardByAllLabels:    shardByAllLabels,
+						shuffleShardEnabled: shuffleShardEnabled,
 					})
-					continue
-				}
 
-				// When we have less ingesters than replication factor, any failed ingester
-				// will cause a failure.
-				if numIngesters < 3 && happyIngesters < 2 {
+					// As should reading none of the samples back.
 					testcases = append(testcases, testcase{
-						name:             fmt.Sprintf("ExpectFail(shardByAllLabels=%v,numIngester=%d,happyIngester=%d)", shardByAllLabels, numIngesters, happyIngesters),
-						numIngesters:     numIngesters,
-						happyIngesters:   happyIngesters,
-						matchers:         []*labels.Matcher{nameMatcher, barMatcher},
-						expectedError:    errFail,
-						shardByAllLabels: shardByAllLabels,
+						name:                fmt.Sprintf("ReadNone(%s)", scenario),
+						numIngesters:        numIngesters,
+						happyIngesters:      happyIngesters,
+						samples:             10,
+						matchers:            []*labels.Matcher{nameMatcher, mustEqualMatcher("not", "found")},
+						expectedResponse:    expectedResponse(0, 0),
+						expectedIngesters:   expectedIngesters,
+						shardByAllLabels:    shardByAllLabels,
+						shuffleShardEnabled: shuffleShardEnabled,
 					})
-					continue
-				}
 
-				// If we're sharding by metric name and we have failed ingesters, we can't
-				// tell ahead of time if the query will succeed, as we don't know which
-				// ingesters will hold the results for the query.
-				if !shardByAllLabels && numIngesters-happyIngesters > 1 {
-					continue
-				}
-
-				// Reading all the samples back should succeed.
-				testcases = append(testcases, testcase{
-					name:             fmt.Sprintf("ReadAll(shardByAllLabels=%v,numIngester=%d,happyIngester=%d)", shardByAllLabels, numIngesters, happyIngesters),
-					numIngesters:     numIngesters,
-					happyIngesters:   happyIngesters,
-					samples:          10,
-					matchers:         []*labels.Matcher{nameMatcher, barMatcher},
-					expectedResponse: expectedResponse(0, 10),
-					shardByAllLabels: shardByAllLabels,
-				})
-
-				// As should reading none of the samples back.
-				testcases = append(testcases, testcase{
-					name:             fmt.Sprintf("ReadNone(shardByAllLabels=%v,numIngester=%d,happyIngester=%d)", shardByAllLabels, numIngesters, happyIngesters),
-					numIngesters:     numIngesters,
-					happyIngesters:   happyIngesters,
-					samples:          10,
-					matchers:         []*labels.Matcher{nameMatcher, mustEqualMatcher("not", "found")},
-					expectedResponse: expectedResponse(0, 0),
-					shardByAllLabels: shardByAllLabels,
-				})
-
-				// And reading each sample individually.
-				for i := 0; i < 10; i++ {
-					testcases = append(testcases, testcase{
-						name:             fmt.Sprintf("ReadOne(shardByAllLabels=%v, sample=%d,numIngester=%d,happyIngester=%d)", shardByAllLabels, i, numIngesters, happyIngesters),
-						numIngesters:     numIngesters,
-						happyIngesters:   happyIngesters,
-						samples:          10,
-						matchers:         []*labels.Matcher{nameMatcher, mustEqualMatcher("sample", strconv.Itoa(i))},
-						expectedResponse: expectedResponse(i, i+1),
-						shardByAllLabels: shardByAllLabels,
-					})
+					// And reading each sample individually.
+					for i := 0; i < 10; i++ {
+						testcases = append(testcases, testcase{
+							name:                fmt.Sprintf("ReadOne(%s, sample=%d)", scenario, i),
+							numIngesters:        numIngesters,
+							happyIngesters:      happyIngesters,
+							samples:             10,
+							matchers:            []*labels.Matcher{nameMatcher, mustEqualMatcher("sample", strconv.Itoa(i))},
+							expectedResponse:    expectedResponse(i, i+1),
+							expectedIngesters:   expectedIngesters,
+							shardByAllLabels:    shardByAllLabels,
+							shuffleShardEnabled: shuffleShardEnabled,
+						})
+					}
 				}
 			}
 		}
@@ -548,11 +576,13 @@ func TestDistributor_PushQuery(t *testing.T) {
 
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
-			ds, _, r := prepare(t, prepConfig{
-				numIngesters:     tc.numIngesters,
-				happyIngesters:   tc.happyIngesters,
-				numDistributors:  1,
-				shardByAllLabels: tc.shardByAllLabels,
+			ds, ingesters, r := prepare(t, prepConfig{
+				numIngesters:        tc.numIngesters,
+				happyIngesters:      tc.happyIngesters,
+				numDistributors:     1,
+				shardByAllLabels:    tc.shardByAllLabels,
+				shuffleShardEnabled: tc.shuffleShardEnabled,
+				shuffleShardSize:    shuffleShardSize,
 			})
 			defer stopAll(ds, r)
 
@@ -576,6 +606,15 @@ func TestDistributor_PushQuery(t *testing.T) {
 			}
 			assert.NoError(t, err)
 			assert.Equal(t, tc.expectedResponse.String(), response.String())
+
+			// Check how many ingesters have been queried.
+			// Due to the quorum the distributor could cancel the last request towards ingesters
+			// if all other ones are successful, so we're good either has been queried X or X-1
+			// ingesters.
+			if tc.expectedError == nil {
+				assert.Contains(t, []int{tc.expectedIngesters, tc.expectedIngesters - 1}, countMockIngestersCalls(ingesters, "Query"))
+				assert.Contains(t, []int{tc.expectedIngesters, tc.expectedIngesters - 1}, countMockIngestersCalls(ingesters, "QueryStream"))
+			}
 		})
 	}
 }
@@ -808,6 +847,8 @@ func TestSlowQueries(t *testing.T) {
 }
 
 func TestDistributor_MetricsForLabelMatchers(t *testing.T) {
+	const numIngesters = 5
+
 	fixtures := []struct {
 		lbls      labels.Labels
 		value     float64
@@ -822,96 +863,169 @@ func TestDistributor_MetricsForLabelMatchers(t *testing.T) {
 	}
 
 	tests := map[string]struct {
-		matchers []*labels.Matcher
-		expected []metric.Metric
+		shuffleShardEnabled bool
+		shuffleShardSize    int
+		matchers            []*labels.Matcher
+		expectedResult      []metric.Metric
+		expectedIngesters   int
 	}{
 		"should return an empty response if no metric match": {
 			matchers: []*labels.Matcher{
 				mustNewMatcher(labels.MatchEqual, model.MetricNameLabel, "unknown"),
 			},
-			expected: []metric.Metric{},
+			expectedResult:    []metric.Metric{},
+			expectedIngesters: numIngesters,
 		},
 		"should filter metrics by single matcher": {
 			matchers: []*labels.Matcher{
 				mustNewMatcher(labels.MatchEqual, model.MetricNameLabel, "test_1"),
 			},
-			expected: []metric.Metric{
+			expectedResult: []metric.Metric{
 				{Metric: util.LabelsToMetric(fixtures[0].lbls)},
 				{Metric: util.LabelsToMetric(fixtures[1].lbls)},
 			},
+			expectedIngesters: numIngesters,
 		},
 		"should filter metrics by multiple matchers": {
 			matchers: []*labels.Matcher{
 				mustNewMatcher(labels.MatchEqual, "status", "200"),
 				mustNewMatcher(labels.MatchEqual, model.MetricNameLabel, "test_1"),
 			},
-			expected: []metric.Metric{
+			expectedResult: []metric.Metric{
 				{Metric: util.LabelsToMetric(fixtures[0].lbls)},
 			},
+			expectedIngesters: numIngesters,
 		},
 		"should return all matching metrics even if their FastFingerprint collide": {
 			matchers: []*labels.Matcher{
 				mustNewMatcher(labels.MatchEqual, model.MetricNameLabel, "fast_fingerprint_collision"),
 			},
-			expected: []metric.Metric{
+			expectedResult: []metric.Metric{
 				{Metric: util.LabelsToMetric(fixtures[3].lbls)},
 				{Metric: util.LabelsToMetric(fixtures[4].lbls)},
 			},
+			expectedIngesters: numIngesters,
+		},
+		"should query only ingesters belonging to tenant's subring if shuffle sharding is enabled": {
+			shuffleShardEnabled: true,
+			shuffleShardSize:    3,
+			matchers: []*labels.Matcher{
+				mustNewMatcher(labels.MatchEqual, model.MetricNameLabel, "test_1"),
+			},
+			expectedResult: []metric.Metric{
+				{Metric: util.LabelsToMetric(fixtures[0].lbls)},
+				{Metric: util.LabelsToMetric(fixtures[1].lbls)},
+			},
+			expectedIngesters: 3,
+		},
+		"should query all ingesters if shuffle sharding is enabled but shard size is 0": {
+			shuffleShardEnabled: true,
+			shuffleShardSize:    0,
+			matchers: []*labels.Matcher{
+				mustNewMatcher(labels.MatchEqual, model.MetricNameLabel, "test_1"),
+			},
+			expectedResult: []metric.Metric{
+				{Metric: util.LabelsToMetric(fixtures[0].lbls)},
+				{Metric: util.LabelsToMetric(fixtures[1].lbls)},
+			},
+			expectedIngesters: numIngesters,
 		},
 	}
 
-	// Create distributor
-	ds, _, r := prepare(t, prepConfig{
-		numIngesters:     3,
-		happyIngesters:   3,
-		numDistributors:  1,
-		shardByAllLabels: true,
-	})
-	defer stopAll(ds, r)
-
-	// Push fixtures
-	ctx := user.InjectOrgID(context.Background(), "test")
-
-	for _, series := range fixtures {
-		req := mockWriteRequest(series.lbls, series.value, series.timestamp)
-		_, err := ds[0].Push(ctx, req)
-		require.NoError(t, err)
-	}
-
-	// Run tests
 	for testName, testData := range tests {
 		t.Run(testName, func(t *testing.T) {
 			now := model.Now()
 
+			// Create distributor
+			ds, ingesters, r := prepare(t, prepConfig{
+				numIngesters:        numIngesters,
+				happyIngesters:      numIngesters,
+				numDistributors:     1,
+				shardByAllLabels:    true,
+				shuffleShardEnabled: testData.shuffleShardEnabled,
+				shuffleShardSize:    testData.shuffleShardSize,
+			})
+			defer stopAll(ds, r)
+
+			// Push fixtures
+			ctx := user.InjectOrgID(context.Background(), "test")
+
+			for _, series := range fixtures {
+				req := mockWriteRequest(series.lbls, series.value, series.timestamp)
+				_, err := ds[0].Push(ctx, req)
+				require.NoError(t, err)
+			}
+
 			metrics, err := ds[0].MetricsForLabelMatchers(ctx, now, now, testData.matchers...)
 			require.NoError(t, err)
-			assert.ElementsMatch(t, testData.expected, metrics)
+			assert.ElementsMatch(t, testData.expectedResult, metrics)
+
+			// Check how many ingesters have been queried.
+			// Due to the quorum the distributor could cancel the last request towards ingesters
+			// if all other ones are successful, so we're good either has been queried X or X-1
+			// ingesters.
+			assert.Contains(t, []int{testData.expectedIngesters, testData.expectedIngesters - 1}, countMockIngestersCalls(ingesters, "MetricsForLabelMatchers"))
 		})
 	}
 }
 
 func TestDistributor_MetricsMetadata(t *testing.T) {
-	// Create distributor
-	ds, _, r := prepare(t, prepConfig{
-		numIngesters:     3,
-		happyIngesters:   3,
-		numDistributors:  1,
-		shardByAllLabels: true,
-		limits:           nil,
-	})
-	defer stopAll(ds, r)
+	const numIngesters = 5
 
-	// Push metadata
-	ctx := user.InjectOrgID(context.Background(), "test")
+	tests := map[string]struct {
+		shuffleShardEnabled bool
+		shuffleShardSize    int
+		expectedIngesters   int
+	}{
+		"should query all ingesters if shuffle sharding is disabled": {
+			shuffleShardEnabled: false,
+			expectedIngesters:   numIngesters,
+		},
+		"should query all ingesters if shuffle sharding is enabled but shard size is 0": {
+			shuffleShardEnabled: true,
+			shuffleShardSize:    0,
+			expectedIngesters:   numIngesters,
+		},
+		"should query only ingesters belonging to tenant's subring if shuffle sharding is enabled": {
+			shuffleShardEnabled: true,
+			shuffleShardSize:    3,
+			expectedIngesters:   3,
+		},
+	}
 
-	req := makeWriteRequest(0, 0, 10)
-	_, err := ds[0].Push(ctx, req)
-	require.NoError(t, err)
+	for testName, testData := range tests {
+		t.Run(testName, func(t *testing.T) {
+			// Create distributor
+			ds, ingesters, r := prepare(t, prepConfig{
+				numIngesters:        numIngesters,
+				happyIngesters:      numIngesters,
+				numDistributors:     1,
+				shardByAllLabels:    true,
+				shuffleShardEnabled: testData.shuffleShardEnabled,
+				shuffleShardSize:    testData.shuffleShardSize,
+				limits:              nil,
+			})
+			defer stopAll(ds, r)
 
-	// Asert on metric metadata
-	metadata, err := ds[0].MetricsMetadata(ctx)
-	require.NoError(t, err)
-	assert.Equal(t, 10, len(metadata))
+			// Push metadata
+			ctx := user.InjectOrgID(context.Background(), "test")
+
+			req := makeWriteRequest(0, 0, 10)
+			_, err := ds[0].Push(ctx, req)
+			require.NoError(t, err)
+
+			// Assert on metric metadata
+			metadata, err := ds[0].MetricsMetadata(ctx)
+			require.NoError(t, err)
+			assert.Equal(t, 10, len(metadata))
+
+			// Check how many ingesters have been queried.
+			// Due to the quorum the distributor could cancel the last request towards ingesters
+			// if all other ones are successful, so we're good either has been queried X or X-1
+			// ingesters.
+			assert.Contains(t, []int{testData.expectedIngesters, testData.expectedIngesters - 1}, countMockIngestersCalls(ingesters, "MetricsMetadata"))
+		})
+	}
 }
 
 func mustNewMatcher(t labels.MatchType, n, v string) *labels.Matcher {
@@ -938,6 +1052,8 @@ type prepConfig struct {
 	numIngesters, happyIngesters int
 	queryDelay                   time.Duration
 	shardByAllLabels             bool
+	shuffleShardEnabled          bool
+	shuffleShardSize             int
 	limits                       *validation.Limits
 	numDistributors              int
 }
@@ -966,7 +1082,7 @@ func prepare(t *testing.T, cfg prepConfig) ([]*Distributor, []mockIngester, *rin
 			Zone:                "",
 			State:               ring.ACTIVE,
 			Timestamp:           time.Now().Unix(),
-			RegisteredTimestamp: time.Now().Unix(),
+			RegisteredTimestamp: time.Now().Add(-2 * time.Hour).Unix(),
 			Tokens:              []uint32{uint32((math.MaxUint32 / cfg.numIngesters) * i)},
 		}
 		ingestersByAddr[addr] = &ingesters[i]
@@ -1002,6 +1118,11 @@ func prepare(t *testing.T, cfg prepConfig) ([]*Distributor, []mockIngester, *rin
 
 	distributors := make([]*Distributor, 0, cfg.numDistributors)
 	for i := 0; i < cfg.numDistributors; i++ {
+		if cfg.limits == nil {
+			cfg.limits = &validation.Limits{}
+			flagext.DefaultValues(cfg.limits)
+		}
+
 		var distributorCfg Config
 		var clientConfig client.Config
 		flagext.DefaultValues(&distributorCfg, &clientConfig)
@@ -1014,10 +1135,13 @@ func prepare(t *testing.T, cfg prepConfig) ([]*Distributor, []mockIngester, *rin
 		distributorCfg.DistributorRing.KVStore.Mock = kvStore
 		distributorCfg.DistributorRing.InstanceAddr = "127.0.0.1"
 
-		if cfg.limits == nil {
-			cfg.limits = &validation.Limits{}
-			flagext.DefaultValues(cfg.limits)
+		if cfg.shuffleShardEnabled {
+			distributorCfg.ShardingStrategy = ShardingStrategyShuffle
+			distributorCfg.ShuffleShardingLookbackPeriod = time.Hour
+
+			cfg.limits.IngestionTenantShardSize = cfg.shuffleShardSize
 		}
+
 		overrides, err := validation.NewOverrides(*cfg.limits, nil)
 		require.NoError(t, err)
 
@@ -1143,6 +1267,7 @@ type mockIngester struct {
 	timeseries map[uint32]*client.PreallocTimeseries
 	metadata   map[uint32]map[client.MetricMetadata]struct{}
 	queryDelay time.Duration
+	calls      map[string]int
 }
 
 func (i *mockIngester) series() map[uint32]*client.PreallocTimeseries {
@@ -1157,6 +1282,8 @@ func (i *mockIngester) series() map[uint32]*client.PreallocTimeseries {
 }
 
 func (i *mockIngester) Check(ctx context.Context, in *grpc_health_v1.HealthCheckRequest, opts ...grpc.CallOption) (*grpc_health_v1.HealthCheckResponse, error) {
+	i.trackCall("Check")
+
 	return &grpc_health_v1.HealthCheckResponse{}, nil
 }
 
@@ -1168,6 +1295,8 @@ func (i *mockIngester) Push(ctx context.Context, req *client.WriteRequest, opts
 	i.Lock()
 	defer i.Unlock()
 
+	i.trackCall("Push")
+
 	if !i.happy {
 		return nil, errFail
 	}
@@ -1220,9 +1349,12 @@ func (i *mockIngester) Push(ctx context.Context, req *client.WriteRequest, opts
 
 func (i *mockIngester) Query(ctx context.Context, req *client.QueryRequest, opts ...grpc.CallOption) (*client.QueryResponse, error) {
 	time.Sleep(i.queryDelay)
+
 	i.Lock()
 	defer i.Unlock()
 
+	i.trackCall("Query")
+
 	if !i.happy {
 		return nil, errFail
 	}
@@ -1243,9 +1375,12 @@ func (i *mockIngester) Query(ctx context.Context, req *client.QueryRequest, opts
 
 func (i *mockIngester) QueryStream(ctx context.Context, req *client.QueryRequest, opts ...grpc.CallOption) (client.Ingester_QueryStreamClient, error) {
 	time.Sleep(i.queryDelay)
+
 	i.Lock()
 	defer i.Unlock()
 
+	i.trackCall("QueryStream")
+
 	if !i.happy {
 		return nil, errFail
 	}
@@ -1308,6 +1443,8 @@ func (i *mockIngester) MetricsForLabelMatchers(ctx context.Context, req *client.
 	i.Lock()
 	defer i.Unlock()
 
+	i.trackCall("MetricsForLabelMatchers")
+
 	if !i.happy {
 		return nil, errFail
 	}
@@ -1332,6 +1469,8 @@ func (i *mockIngester) MetricsMetadata(ctx context.Context, req *client.MetricsM
 	i.Lock()
 	defer i.Unlock()
 
+	i.trackCall("MetricsMetadata")
+
 	if !i.happy {
 		return nil, errFail
 	}
@@ -1346,6 +1485,21 @@ func (i *mockIngester) MetricsMetadata(ctx context.Context, req *client.MetricsM
 	return resp, nil
 }
 
+func (i *mockIngester) trackCall(name string) {
+	if i.calls == nil {
+		i.calls = map[string]int{}
+	}
+
+	i.calls[name]++
+}
+
+func (i *mockIngester) countCalls(name string) int {
+	i.Lock()
+	defer i.Unlock()
+
+	return i.calls[name]
+}
+
 type stream struct {
 	grpc.ClientStream
 	i       int
@@ -1565,3 +1719,13 @@ func TestSortLabels(t *testing.T) {
 		return strings.Compare(unsorted[i].Name, unsorted[j].Name) < 0
 	})
 }
+
+func countMockIngestersCalls(ingesters []mockIngester, name string) int {
+	count := 0
+	for _, ing := range ingesters {
+		if ing.countCalls(name) > 0 {
+			count++
+		}
+	}
+	return count
+}

From 90285ef15063d31acf29050553f29d021885d6de Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Thu, 8 Oct 2020 14:32:55 +0200
Subject: [PATCH 07/17] Added integration test

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 integration/e2e/metrics_options.go    |  6 ++++
 integration/e2e/service.go            |  8 ++++++
 integration/ingester_sharding_test.go | 41 +++++++++++++++++++++++++--
 3 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/integration/e2e/metrics_options.go b/integration/e2e/metrics_options.go
index 3ec98601538..b609af8ae2e 100644
--- a/integration/e2e/metrics_options.go
+++ b/integration/e2e/metrics_options.go
@@ -23,6 +23,7 @@ type MetricsOptions struct {
 	GetValue           GetMetricValueFunc
 	LabelMatchers      []*labels.Matcher
 	WaitMissingMetrics bool
+	SkipMissingMetrics bool
 }
 
 // WithMetricCount is an option to get the histogram/summary count as metric value.
@@ -43,6 +44,11 @@ func WaitMissingMetrics(opts *MetricsOptions) {
 	opts.WaitMissingMetrics = true
 }
 
+// SkipWaitMissingMetrics is an option to skip/ignore whenever an expected metric is missing.
+func SkipMissingMetrics(opts *MetricsOptions) {
+	opts.SkipMissingMetrics = true
+}
+
 func buildMetricsOptions(opts []MetricsOption) MetricsOptions {
 	result := DefaultMetricsOptions
 	for _, opt := range opts {
diff --git a/integration/e2e/service.go b/integration/e2e/service.go
index 0dc09c4c7c2..59ada2f94e8 100644
--- a/integration/e2e/service.go
+++ b/integration/e2e/service.go
@@ -577,12 +577,20 @@ func (s *HTTPService) SumMetrics(metricNames []string, opts ...MetricsOption) ([
 		// Get the metric family.
 		mf, ok := families[m]
 		if !ok {
+			if options.SkipMissingMetrics {
+				continue
+			}
+
 			return nil, errors.Wrapf(errMissingMetric, "metric=%s service=%s", m, s.name)
 		}
 
 		// Filter metrics.
 		metrics := filterMetrics(mf.GetMetric(), options)
 		if len(metrics) == 0 {
+			if options.SkipMissingMetrics {
+				continue
+			}
+
 			return nil, errors.Wrapf(errMissingMetric, "metric=%s service=%s", m, s.name)
 		}
 
diff --git a/integration/ingester_sharding_test.go b/integration/ingester_sharding_test.go
index 250397bddc2..4b60c5e8f2a 100644
--- a/integration/ingester_sharding_test.go
+++ b/integration/ingester_sharding_test.go
@@ -8,7 +8,9 @@ import (
 	"testing"
 	"time"
 
+	"github.com/prometheus/common/model"
 	"github.com/prometheus/prometheus/pkg/labels"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/cortexproject/cortex/integration/e2e"
@@ -43,9 +45,16 @@ func TestIngesterSharding(t *testing.T) {
 			defer s.Close()
 
 			flags := BlocksStorageFlags
+			flags["-distributor.shard-by-all-labels"] = "true"
 			flags["-distributor.sharding-strategy"] = testData.shardingStrategy
 			flags["-distributor.ingestion-tenant-shard-size"] = strconv.Itoa(testData.tenantShardSize)
 
+			if testData.shardingStrategy == "shuffle-sharding" {
+				// Enable shuffle sharding on read path but not lookback, otherwise all ingesters would be
+				// queried being just registered.
+				flags["-querier.shuffle-sharding-ingesters-lookback-period"] = "1ns"
+			}
+
 			// Start dependencies.
 			consul := e2edb.NewConsul()
 			minio := e2edb.NewMinio(9000, flags["-blocks-storage.s3.bucket-name"])
@@ -56,6 +65,7 @@ func TestIngesterSharding(t *testing.T) {
 			ingester1 := e2ecortex.NewIngester("ingester-1", consul.NetworkHTTPEndpoint(), flags, "")
 			ingester2 := e2ecortex.NewIngester("ingester-2", consul.NetworkHTTPEndpoint(), flags, "")
 			ingester3 := e2ecortex.NewIngester("ingester-3", consul.NetworkHTTPEndpoint(), flags, "")
+			ingesters := e2ecortex.NewCompositeCortexService(ingester1, ingester2, ingester3)
 			querier := e2ecortex.NewQuerier("querier", consul.NetworkHTTPEndpoint(), flags, "")
 			require.NoError(t, s.StartAndWaitReady(distributor, ingester1, ingester2, ingester3, querier))
 
@@ -70,15 +80,19 @@ func TestIngesterSharding(t *testing.T) {
 
 			// Push series.
 			now := time.Now()
+			expectedVectors := map[string]model.Vector{}
 
-			client, err := e2ecortex.NewClient(distributor.HTTPEndpoint(), "", "", "", userID)
+			client, err := e2ecortex.NewClient(distributor.HTTPEndpoint(), querier.HTTPEndpoint(), "", "", userID)
 			require.NoError(t, err)
 
 			for i := 1; i <= numSeriesToPush; i++ {
-				series, _ := generateSeries(fmt.Sprintf("series_%d", i), now)
+				metricName := fmt.Sprintf("series_%d", i)
+				series, expectedVector := generateSeries(metricName, now)
 				res, err := client.Push(series)
 				require.NoError(t, err)
 				require.Equal(t, 200, res.StatusCode)
+
+				expectedVectors[metricName] = expectedVector
 			}
 
 			// Extract metrics from ingesters.
@@ -99,6 +113,29 @@ func TestIngesterSharding(t *testing.T) {
 			require.Equal(t, testData.expectedIngestersWithSeries, numIngestersWithSeries)
 			require.Equal(t, numSeriesToPush, totalIngestedSeries)
 
+			// Query back series.
+			for metricName, expectedVector := range expectedVectors {
+				result, err := client.Query(metricName, now)
+				require.NoError(t, err)
+				require.Equal(t, model.ValVector, result.Type())
+				assert.Equal(t, expectedVector, result.(model.Vector))
+			}
+
+			// 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" {
+				expectedIngesters = testData.tenantShardSize
+			}
+
+			expectedCalls := expectedIngesters * len(expectedVectors)
+			require.NoError(t, ingesters.WaitSumMetricsWithOptions(
+				e2e.Equals(float64(expectedCalls)),
+				[]string{"cortex_request_duration_seconds"},
+				e2e.WithMetricCount,
+				e2e.SkipMissingMetrics, // Some ingesters may have received no request at all.
+				e2e.WithLabelMatchers(labels.MustNewMatcher(labels.MatchEqual, "route", "/cortex.Ingester/QueryStream"))))
+
 			// Ensure no service-specific metrics prefix is used by the wrong service.
 			assertServiceMetricsPrefixes(t, Distributor, distributor)
 			assertServiceMetricsPrefixes(t, Ingester, ingester1)

From cb186c4559c625c1ab98a2b6c4ffedc6246afb1c Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Thu, 8 Oct 2020 14:36:30 +0200
Subject: [PATCH 08/17] Fixed race in unit test

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 pkg/distributor/distributor_test.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pkg/distributor/distributor_test.go b/pkg/distributor/distributor_test.go
index 27af909c5c5..3a6bac9da8c 100644
--- a/pkg/distributor/distributor_test.go
+++ b/pkg/distributor/distributor_test.go
@@ -1722,8 +1722,8 @@ func TestSortLabels(t *testing.T) {
 
 func countMockIngestersCalls(ingesters []mockIngester, name string) int {
 	count := 0
-	for _, ing := range ingesters {
-		if ing.countCalls(name) > 0 {
+	for i := 0; i < len(ingesters); i++ {
+		if ingesters[i].countCalls(name) > 0 {
 			count++
 		}
 	}

From 6ffc73f5e4fb665e945dcf6694f8942052f2e8d4 Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Thu, 8 Oct 2020 14:38:53 +0200
Subject: [PATCH 09/17] Added CHANGELOG entry

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 CHANGELOG.md | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0b9da4a51a6..d052a9f277c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -48,7 +48,8 @@
 * [CHANGE] Renamed `-<prefix>.redis.enable-tls` CLI flag to `-<prefix>.redis.tls-enabled`, and its respective YAML config option from `enable_tls` to `tls_enabled`. #3298
 * [CHANGE] Increased default `-<prefix>.redis.timeout` from `100ms` to `500ms`. #3301
 * [CHANGE] `cortex_alertmanager_config_invalid` has been removed in favor of `cortex_alertmanager_config_last_reload_successful`. #3289
-* [FEATURE] Added support for shuffle-sharding queriers in the query-frontend. When configured (`-frontend.max-queriers-per-tenant` globally, or using per-tenant limit `max_queriers_per_tenant`), each tenants's requests will be handled by different set of queriers. #3113 #3257
+* [FEATURE] Shuffle sharding: added support for shuffle-sharding queriers in the query-frontend. When configured (`-frontend.max-queriers-per-tenant` globally, or using per-tenant limit `max_queriers_per_tenant`), each tenants's requests will be handled by different set of queriers. #3113 #3257
+* [FEATURE] Shuffle sharding: added support for shuffle-sharding ingesters on the read path. When ingesters shuffle-sharding is enabled and `-querier.shuffle-sharding-ingesters-lookback-period` is set, queriers will fetch in-memory series from the minimum set of required ingesters, selecting only ingesters which may have received series since 'now - lookback period'. #3252
 * [FEATURE] Query-frontend: added `compression` config to support results cache with compression. #3217
 * [ENHANCEMENT] Allow to specify multiple comma-separated Cortex services to `-target` CLI option (or its respective YAML config option). For example, `-target=all,compactor` can be used to start Cortex single-binary with compactor as well. #3275
 * [ENHANCEMENT] Expose additional HTTP configs for the S3 backend client. New flag are listed below: #3244

From 8c39891fc925682196efa84c12781b2f6304a07e Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Thu, 8 Oct 2020 14:46:54 +0200
Subject: [PATCH 10/17] Removed ring.ByZoneAndAddr because unused

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 pkg/ring/model.go | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/pkg/ring/model.go b/pkg/ring/model.go
index c9e09726f27..19ab91c9e53 100644
--- a/pkg/ring/model.go
+++ b/pkg/ring/model.go
@@ -25,18 +25,6 @@ func (ts ByAddr) Len() int           { return len(ts) }
 func (ts ByAddr) Swap(i, j int)      { ts[i], ts[j] = ts[j], ts[i] }
 func (ts ByAddr) Less(i, j int) bool { return ts[i].Addr < ts[j].Addr }
 
-// ByZoneAndAddr is a sortable list of IngesterDesc.
-type ByZoneAddr []IngesterDesc
-
-func (ts ByZoneAddr) Len() int      { return len(ts) }
-func (ts ByZoneAddr) Swap(i, j int) { ts[i], ts[j] = ts[j], ts[i] }
-func (ts ByZoneAddr) Less(i, j int) bool {
-	if ts[i].Zone != ts[j].Zone {
-		return ts[i].Zone < ts[j].Zone
-	}
-	return ts[i].Addr < ts[j].Addr
-}
-
 // ProtoDescFactory makes new Descs
 func ProtoDescFactory() proto.Message {
 	return NewDesc()

From d8abef73cababa6e7db3b309a85297309e41221f Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Fri, 9 Oct 2020 17:39:28 +0200
Subject: [PATCH 11/17] Addressed review comments

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 docs/blocks-storage/querier.md              |  5 +++--
 docs/configuration/config-file-reference.md |  5 +++--
 pkg/querier/querier.go                      |  2 +-
 pkg/ring/ring.go                            | 10 +++-------
 4 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/docs/blocks-storage/querier.md b/docs/blocks-storage/querier.md
index a86222e2942..633d6edcee5 100644
--- a/docs/blocks-storage/querier.md
+++ b/docs/blocks-storage/querier.md
@@ -177,8 +177,9 @@ querier:
   # When distributor's sharding strategy is shuffle-sharding and this setting is
   # > 0, queriers fetch in-memory series from the minimum set of required
   # ingesters, selecting only ingesters which may have received series since
-  # 'now - lookback period'. If this setting is 0, queriers always query all
-  # ingesters (ingesters shuffle sharding on read path is disabled).
+  # 'now - lookback period'. The lookback period should be greater or equal than
+  # the configured 'query store after'. If this setting is 0, queriers always
+  # query all ingesters (ingesters shuffle sharding on read path is disabled).
   # CLI flag: -querier.shuffle-sharding-ingesters-lookback-period
   [shuffle_sharding_ingesters_lookback_period: <duration> | default = 0s]
 ```
diff --git a/docs/configuration/config-file-reference.md b/docs/configuration/config-file-reference.md
index 32b226db083..f9f419a74e4 100644
--- a/docs/configuration/config-file-reference.md
+++ b/docs/configuration/config-file-reference.md
@@ -744,8 +744,9 @@ store_gateway_client:
 # When distributor's sharding strategy is shuffle-sharding and this setting is >
 # 0, queriers fetch in-memory series from the minimum set of required ingesters,
 # selecting only ingesters which may have received series since 'now - lookback
-# period'. If this setting is 0, queriers always query all ingesters (ingesters
-# shuffle sharding on read path is disabled).
+# period'. The lookback period should be greater or equal than the configured
+# 'query store after'. If this setting is 0, queriers always query all ingesters
+# (ingesters shuffle sharding on read path is disabled).
 # CLI flag: -querier.shuffle-sharding-ingesters-lookback-period
 [shuffle_sharding_ingesters_lookback_period: <duration> | default = 0s]
 ```
diff --git a/pkg/querier/querier.go b/pkg/querier/querier.go
index a9902486955..ef7566d35cd 100644
--- a/pkg/querier/querier.go
+++ b/pkg/querier/querier.go
@@ -91,7 +91,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.DurationVar(&cfg.LookbackDelta, "querier.lookback-delta", 5*time.Minute, "Time since the last sample after which a time series is considered stale and ignored by expression evaluations.")
 	f.StringVar(&cfg.SecondStoreEngine, "querier.second-store-engine", "", "Second store engine to use for querying. Empty = disabled.")
 	f.Var(&cfg.UseSecondStoreBeforeTime, "querier.use-second-store-before-time", "If specified, second store is only used for queries before this timestamp. Default value 0 means secondary store is always queried.")
-	f.DurationVar(&cfg.ShuffleShardingIngestersLookbackPeriod, "querier.shuffle-sharding-ingesters-lookback-period", 0, "When distributor's sharding strategy is shuffle-sharding and this setting is > 0, queriers fetch in-memory series from the minimum set of required ingesters, selecting only ingesters which may have received series since 'now - lookback period'. If this setting is 0, queriers always query all ingesters (ingesters shuffle sharding on read path is disabled).")
+	f.DurationVar(&cfg.ShuffleShardingIngestersLookbackPeriod, "querier.shuffle-sharding-ingesters-lookback-period", 0, "When distributor's sharding strategy is shuffle-sharding and this setting is > 0, queriers fetch in-memory series from the minimum set of required ingesters, selecting only ingesters which may have received series since 'now - lookback period'. The lookback period should be greater or equal than the configured 'query store after'. If this setting is 0, queriers always query all ingesters (ingesters shuffle sharding on read path is disabled).")
 }
 
 // Validate the config
diff --git a/pkg/ring/ring.go b/pkg/ring/ring.go
index 1cb48b4cddc..dc61136ea7f 100644
--- a/pkg/ring/ring.go
+++ b/pkg/ring/ring.go
@@ -477,7 +477,7 @@ func (r *Ring) ShuffleShard(identifier string, size int) ReadRing {
 
 	result := r.shuffleShard(identifier, size, 0, time.Now())
 
-	r.setCachedShuffledSubring(identifier, size, result.(*Ring))
+	r.setCachedShuffledSubring(identifier, size, result)
 	return result
 }
 
@@ -497,11 +497,9 @@ func (r *Ring) ShuffleShardWithLookback(identifier string, size int, lookbackPer
 	return r.shuffleShard(identifier, size, lookbackPeriod, now)
 }
 
-func (r *Ring) shuffleShard(identifier string, size int, lookbackPeriod time.Duration, now time.Time) ReadRing {
+func (r *Ring) shuffleShard(identifier string, size int, lookbackPeriod time.Duration, now time.Time) *Ring {
 	lookbackUntil := now.Add(-lookbackPeriod).Unix()
 
-	var result *Ring
-
 	r.mtx.RLock()
 	defer r.mtx.RUnlock()
 
@@ -585,7 +583,7 @@ func (r *Ring) shuffleShard(identifier string, size int, lookbackPeriod time.Dur
 	shardDesc := &Desc{Ingesters: shard}
 	shardTokensByZone := shardDesc.getTokensByZone()
 
-	result = &Ring{
+	return &Ring{
 		cfg:              r.cfg,
 		strategy:         r.strategy,
 		ringDesc:         shardDesc,
@@ -596,8 +594,6 @@ func (r *Ring) shuffleShard(identifier string, size int, lookbackPeriod time.Dur
 		// For caching to work, remember these values.
 		lastTopologyChange: r.lastTopologyChange,
 	}
-
-	return result
 }
 
 // GetInstanceState returns the current state of an instance or an error if the

From 33bd6dd9cb7e719059e4a05978e7f2b2521cd76c Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Fri, 9 Oct 2020 17:47:24 +0200
Subject: [PATCH 12/17] Fixed
 TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy after wrong rebase

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 pkg/ring/ring_test.go | 34 +++++++---------------------------
 1 file changed, 7 insertions(+), 27 deletions(-)

diff --git a/pkg/ring/ring_test.go b/pkg/ring/ring_test.go
index 76df6f96bf7..302aff694b5 100644
--- a/pkg/ring/ring_test.go
+++ b/pkg/ring/ring_test.go
@@ -991,28 +991,18 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
 	// The goal of this test is NOT to ensure that the minimum required number of instances
 	// are returned at any given time, BUT at least all required instances are returned.
 	var (
-		numInitialInstances = []int{9} // TODO , 30, 60, 90}
-		numInitialZones     = []int{3} // TODO, 1, 3}
+		numInitialInstances = []int{9, 30, 60, 90}
+		numInitialZones     = []int{3, 1, 3}
 		numEvents           = 100
 		lookbackPeriod      = time.Hour
 		delayBetweenEvents  = 5 * time.Minute // 12 events / hour
 		userID              = "user-1"
 	)
 
-	// TODO randomise it but log the seed to have a way to reproduce tests if break
-	rand.Seed(2)
-
-	//printShard := func(rs ReplicationSet) {
-	//	fmt.Print("curr shard: ")
-	//
-	//	ingesters := rs.Ingesters
-	//	sort.Sort(ByZoneAddr(ingesters))
-	//
-	//	for _, i := range ingesters {
-	//		fmt.Print(i.Addr, "(", i.Zone, "), ")
-	//	}
-	//	fmt.Print("\n")
-	//}
+	// Randomise the seed but log it in case we need to reproduce the test on failure.
+	seed := time.Now().UnixNano()
+	rand.Seed(seed)
+	t.Log("random generator seed:", seed)
 
 	for _, numInstances := range numInitialInstances {
 		for _, numZones := range numInitialZones {
@@ -1043,7 +1033,6 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
 				// Add the initial shard to the history.
 				rs, err := ring.ShuffleShard(userID, shardSize).GetAll(Read)
 				require.NoError(t, err)
-				//printShard(rs)
 
 				history := map[time.Time]ReplicationSet{
 					currTime: rs,
@@ -1052,11 +1041,9 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
 				// Simulate a progression of random events over the time and, at each iteration of the simuation,
 				// make sure the subring includes all non-removed instances picked from previous versions of the
 				// ring up until the lookback period.
-				nextIngesterID := len(ringDesc.Ingesters)
+				nextIngesterID := len(ringDesc.Ingesters) + 1
 
 				for i := 1; i <= numEvents; i++ {
-					//fmt.Println("")
-
 					currTime = currTime.Add(delayBetweenEvents)
 
 					switch r := rand.Intn(100); {
@@ -1066,8 +1053,6 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
 						zoneID := fmt.Sprintf("zone-%d", nextIngesterID%numZones)
 						nextIngesterID++
 
-						//fmt.Println(currTime.String(), "scale up", instanceID, "zone", zoneID)
-
 						ringDesc.Ingesters[instanceID] = generateRingInstanceWithInfo(instanceID, zoneID, GenerateTokens(128, nil), currTime)
 
 						ring.ringTokens = ringDesc.getTokens()
@@ -1087,8 +1072,6 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
 						idToRemove := ingesterIDs[idxToRemove]
 						delete(ringDesc.Ingesters, idToRemove)
 
-						//fmt.Println(currTime.String(), "scale down", idToRemove)
-
 						ring.ringTokens = ringDesc.getTokens()
 						ring.ringTokensByZone = ringDesc.getTokensByZone()
 						ring.ringZones = getZones(ringDesc.getTokensByZone())
@@ -1109,15 +1092,12 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
 					default:
 						// Scale up shard size (keeping the per-zone balance).
 						shardSize += numZones
-
-						//fmt.Println(currTime.String(), "shard size scaled up to", shardSize)
 					}
 
 					// Add the current shard to the history.
 					rs, err = ring.ShuffleShard(userID, shardSize).GetAll(Read)
 					require.NoError(t, err)
 					history[currTime] = rs
-					//printShard(rs)
 
 					// Ensure the shard with lookback includes all instances from previous states of the ring.
 					rsWithLookback, err := ring.ShuffleShardWithLookback(userID, shardSize, lookbackPeriod, currTime).GetAll(Read)

From 2b8032c7141ace87ba850133e0bdfd1bdabb8bc9 Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Mon, 12 Oct 2020 10:03:24 +0200
Subject: [PATCH 13/17] Fixed tests after rebase

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 pkg/ring/ring_test.go | 288 +++++++++---------------------------------
 1 file changed, 63 insertions(+), 225 deletions(-)

diff --git a/pkg/ring/ring_test.go b/pkg/ring/ring_test.go
index 302aff694b5..71db3e86fa9 100644
--- a/pkg/ring/ring_test.go
+++ b/pkg/ring/ring_test.go
@@ -790,29 +790,29 @@ func TestRing_ShuffleShardWithLookback(t *testing.T) {
 	}{
 		"single zone, shard size = 1, recently bootstrapped cluster": {
 			timeline: []event{
-				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-time.Minute))},
-				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now.Add(-time.Minute))},
-				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, 2) + 1}, now.Add(-time.Minute))},
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 1}, now.Add(-time.Minute))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, "zone-a", 1) + 1}, now.Add(-time.Minute))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, "zone-a", 2) + 1}, now.Add(-time.Minute))},
 				{what: test, shardSize: 1, expected: []string{"instance-1", "instance-2", "instance-3"}},
 			},
 		},
 		"single zone, shard size = 1, instances scale up": {
 			timeline: []event{
-				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 3}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, 2) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, "zone-a", 1) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, "zone-a", 2) + 1}, now.Add(-2*lookbackPeriod))},
 				{what: test, shardSize: 1, expected: []string{"instance-1"}},
-				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, 0) + 2}, now.Add(-10*time.Minute))},
+				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 2}, now.Add(-10*time.Minute))},
 				{what: test, shardSize: 1, expected: []string{"instance-4" /* lookback: */, "instance-1"}},
-				{what: add, instanceID: "instance-5", instanceDesc: generateRingInstanceWithInfo("instance-5", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-5*time.Minute))},
+				{what: add, instanceID: "instance-5", instanceDesc: generateRingInstanceWithInfo("instance-5", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 1}, now.Add(-5*time.Minute))},
 				{what: test, shardSize: 1, expected: []string{"instance-5" /* lookback: */, "instance-4", "instance-1"}},
 			},
 		},
 		"single zone, shard size = 1, instances scale down": {
 			timeline: []event{
-				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, 2) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, "zone-a", 1) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, "zone-a", 2) + 1}, now.Add(-2*lookbackPeriod))},
 				{what: test, shardSize: 1, expected: []string{"instance-1"}},
 				{what: remove, instanceID: "instance-3"},
 				{what: test, shardSize: 1, expected: []string{"instance-1"}},
@@ -822,61 +822,61 @@ func TestRing_ShuffleShardWithLookback(t *testing.T) {
 		},
 		"single zone, shard size = 1, rollout with instances unregistered (removed and re-added one by one)": {
 			timeline: []event{
-				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, 2) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, "zone-a", 1) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, "zone-a", 2) + 1}, now.Add(-2*lookbackPeriod))},
 				{what: test, shardSize: 1, expected: []string{"instance-1"}},
 				// Rollout instance-3.
 				{what: remove, instanceID: "instance-3"},
 				{what: test, shardSize: 1, expected: []string{"instance-1"}},
-				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, 2) + 1}, now)},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, "zone-a", 2) + 1}, now)},
 				{what: test, shardSize: 1, expected: []string{"instance-1"}},
 				// Rollout instance-2.
 				{what: remove, instanceID: "instance-2"},
 				{what: test, shardSize: 1, expected: []string{"instance-1"}},
-				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now)},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, "zone-a", 1) + 1}, now)},
 				{what: test, shardSize: 1, expected: []string{"instance-1"}},
 				// Rollout instance-1.
 				{what: remove, instanceID: "instance-1"},
 				{what: test, shardSize: 1, expected: []string{"instance-2" /* side effect: */, "instance-3"}},
-				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now)},
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 1}, now)},
 				{what: test, shardSize: 1, expected: []string{"instance-1" /* lookback: */, "instance-2" /* side effect: */, "instance-3"}},
 			},
 		},
 		"single zone, shard size = 2, rollout with instances unregistered (removed and re-added one by one)": {
 			timeline: []event{
-				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, 2) + 1}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, 3) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, "zone-a", 1) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, "zone-a", 2) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, "zone-a", 3) + 1}, now.Add(-2*lookbackPeriod))},
 				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2"}},
 				// Rollout instance-4.
 				{what: remove, instanceID: "instance-4"},
 				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2"}},
-				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, 3) + 1}, now)},
+				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, "zone-a", 3) + 1}, now)},
 				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2"}},
 				// Rollout instance-3.
 				{what: remove, instanceID: "instance-3"},
 				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2"}},
-				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, 2) + 1}, now)},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, "zone-a", 2) + 1}, now)},
 				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2"}},
 				// Rollout instance-2.
 				{what: remove, instanceID: "instance-2"},
 				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-3" /* side effect:*/, "instance-4"}},
-				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now)},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, "zone-a", 1) + 1}, now)},
 				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2" /* lookback: */, "instance-3" /* side effect:*/, "instance-4"}},
 				// Rollout instance-1.
 				{what: remove, instanceID: "instance-1"},
 				{what: test, shardSize: 2, expected: []string{"instance-2" /* lookback: */, "instance-3" /* side effect:*/, "instance-4"}},
-				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now)},
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 1}, now)},
 				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2" /* lookback: */, "instance-3" /* side effect:*/, "instance-4"}},
 			},
 		},
 		"single zone, increase shard size": {
 			timeline: []event{
-				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, 1) + 1}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, 2) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-a", []uint32{userToken(userID, "zone-a", 1) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-a", []uint32{userToken(userID, "zone-a", 2) + 1}, now.Add(-2*lookbackPeriod))},
 				{what: test, shardSize: 1, expected: []string{"instance-1"}},
 				{what: test, shardSize: 2, expected: []string{"instance-1", "instance-2"}},
 				{what: test, shardSize: 3, expected: []string{"instance-1", "instance-2", "instance-3"}},
@@ -885,42 +885,42 @@ func TestRing_ShuffleShardWithLookback(t *testing.T) {
 		},
 		"multi zone, shard size = 3, recently bootstrapped cluster": {
 			timeline: []event{
-				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-time.Minute))},
-				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-b", []uint32{userToken(userID, 1) + 1}, now.Add(-time.Minute))},
-				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-c", []uint32{userToken(userID, 2) + 1}, now.Add(-time.Minute))},
-				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, 3) + 1}, now.Add(-time.Minute))},
-				{what: add, instanceID: "instance-5", instanceDesc: generateRingInstanceWithInfo("instance-5", "zone-b", []uint32{userToken(userID, 4) + 1}, now.Add(-time.Minute))},
-				{what: add, instanceID: "instance-6", instanceDesc: generateRingInstanceWithInfo("instance-6", "zone-c", []uint32{userToken(userID, 5) + 1}, now.Add(-time.Minute))},
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 1}, now.Add(-time.Minute))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-b", []uint32{userToken(userID, "zone-b", 1) + 1}, now.Add(-time.Minute))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-c", []uint32{userToken(userID, "zone-c", 2) + 1}, now.Add(-time.Minute))},
+				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, "zone-a", 3) + 1}, now.Add(-time.Minute))},
+				{what: add, instanceID: "instance-5", instanceDesc: generateRingInstanceWithInfo("instance-5", "zone-b", []uint32{userToken(userID, "zone-b", 4) + 1}, now.Add(-time.Minute))},
+				{what: add, instanceID: "instance-6", instanceDesc: generateRingInstanceWithInfo("instance-6", "zone-c", []uint32{userToken(userID, "zone-c", 5) + 1}, now.Add(-time.Minute))},
 				{what: test, shardSize: 3, expected: []string{"instance-1", "instance-2", "instance-3", "instance-4", "instance-5", "instance-6"}},
 			},
 		},
 		"multi zone, shard size = 3, instances scale up": {
 			timeline: []event{
-				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 2}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-b", []uint32{userToken(userID, 1) + 2}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-c", []uint32{userToken(userID, 2) + 2}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, 0) + 3}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-5", instanceDesc: generateRingInstanceWithInfo("instance-5", "zone-b", []uint32{userToken(userID, 1) + 3}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-6", instanceDesc: generateRingInstanceWithInfo("instance-6", "zone-c", []uint32{userToken(userID, 2) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-b", []uint32{userToken(userID, "zone-b", 1) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-c", []uint32{userToken(userID, "zone-c", 2) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-5", instanceDesc: generateRingInstanceWithInfo("instance-5", "zone-b", []uint32{userToken(userID, "zone-b", 1) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-6", instanceDesc: generateRingInstanceWithInfo("instance-6", "zone-c", []uint32{userToken(userID, "zone-c", 2) + 3}, now.Add(-2*lookbackPeriod))},
 				{what: test, shardSize: 3, expected: []string{"instance-1", "instance-2", "instance-3"}},
 				// Scale up.
-				{what: add, instanceID: "instance-7", instanceDesc: generateRingInstanceWithInfo("instance-7", "zone-a", []uint32{userToken(userID, 0) + 1}, now)},
-				{what: add, instanceID: "instance-8", instanceDesc: generateRingInstanceWithInfo("instance-8", "zone-b", []uint32{userToken(userID, 1) + 1}, now)},
-				{what: add, instanceID: "instance-9", instanceDesc: generateRingInstanceWithInfo("instance-9", "zone-c", []uint32{userToken(userID, 2) + 1}, now)},
+				{what: add, instanceID: "instance-7", instanceDesc: generateRingInstanceWithInfo("instance-7", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 1}, now)},
+				{what: add, instanceID: "instance-8", instanceDesc: generateRingInstanceWithInfo("instance-8", "zone-b", []uint32{userToken(userID, "zone-b", 1) + 1}, now)},
+				{what: add, instanceID: "instance-9", instanceDesc: generateRingInstanceWithInfo("instance-9", "zone-c", []uint32{userToken(userID, "zone-c", 2) + 1}, now)},
 				{what: test, shardSize: 3, expected: []string{"instance-7", "instance-8", "instance-9" /* lookback: */, "instance-1", "instance-2", "instance-3"}},
 			},
 		},
 		"multi zone, shard size = 3, instances scale down": {
 			timeline: []event{
-				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-b", []uint32{userToken(userID, 1) + 1}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-c", []uint32{userToken(userID, 2) + 1}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, 0) + 3}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-5", instanceDesc: generateRingInstanceWithInfo("instance-5", "zone-b", []uint32{userToken(userID, 1) + 3}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-6", instanceDesc: generateRingInstanceWithInfo("instance-6", "zone-c", []uint32{userToken(userID, 2) + 3}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-7", instanceDesc: generateRingInstanceWithInfo("instance-7", "zone-a", []uint32{userToken(userID, 0) + 2}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-8", instanceDesc: generateRingInstanceWithInfo("instance-8", "zone-b", []uint32{userToken(userID, 1) + 2}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-9", instanceDesc: generateRingInstanceWithInfo("instance-9", "zone-c", []uint32{userToken(userID, 2) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-b", []uint32{userToken(userID, "zone-b", 1) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-c", []uint32{userToken(userID, "zone-c", 2) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-5", instanceDesc: generateRingInstanceWithInfo("instance-5", "zone-b", []uint32{userToken(userID, "zone-b", 1) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-6", instanceDesc: generateRingInstanceWithInfo("instance-6", "zone-c", []uint32{userToken(userID, "zone-c", 2) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-7", instanceDesc: generateRingInstanceWithInfo("instance-7", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-8", instanceDesc: generateRingInstanceWithInfo("instance-8", "zone-b", []uint32{userToken(userID, "zone-b", 1) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-9", instanceDesc: generateRingInstanceWithInfo("instance-9", "zone-c", []uint32{userToken(userID, "zone-c", 2) + 2}, now.Add(-2*lookbackPeriod))},
 				{what: test, shardSize: 3, expected: []string{"instance-1", "instance-2", "instance-3"}},
 				// Scale down.
 				{what: remove, instanceID: "instance-1"},
@@ -931,15 +931,15 @@ func TestRing_ShuffleShardWithLookback(t *testing.T) {
 		},
 		"multi zone, increase shard size": {
 			timeline: []event{
-				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, 0) + 1}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-b", []uint32{userToken(userID, 1) + 1}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-c", []uint32{userToken(userID, 2) + 1}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, 0) + 3}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-5", instanceDesc: generateRingInstanceWithInfo("instance-5", "zone-b", []uint32{userToken(userID, 1) + 3}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-6", instanceDesc: generateRingInstanceWithInfo("instance-6", "zone-c", []uint32{userToken(userID, 2) + 3}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-7", instanceDesc: generateRingInstanceWithInfo("instance-7", "zone-a", []uint32{userToken(userID, 0) + 2}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-8", instanceDesc: generateRingInstanceWithInfo("instance-8", "zone-b", []uint32{userToken(userID, 1) + 2}, now.Add(-2*lookbackPeriod))},
-				{what: add, instanceID: "instance-9", instanceDesc: generateRingInstanceWithInfo("instance-9", "zone-c", []uint32{userToken(userID, 2) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-1", instanceDesc: generateRingInstanceWithInfo("instance-1", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-2", instanceDesc: generateRingInstanceWithInfo("instance-2", "zone-b", []uint32{userToken(userID, "zone-b", 1) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-3", instanceDesc: generateRingInstanceWithInfo("instance-3", "zone-c", []uint32{userToken(userID, "zone-c", 2) + 1}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-4", instanceDesc: generateRingInstanceWithInfo("instance-4", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-5", instanceDesc: generateRingInstanceWithInfo("instance-5", "zone-b", []uint32{userToken(userID, "zone-b", 1) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-6", instanceDesc: generateRingInstanceWithInfo("instance-6", "zone-c", []uint32{userToken(userID, "zone-c", 2) + 3}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-7", instanceDesc: generateRingInstanceWithInfo("instance-7", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-8", instanceDesc: generateRingInstanceWithInfo("instance-8", "zone-b", []uint32{userToken(userID, "zone-b", 1) + 2}, now.Add(-2*lookbackPeriod))},
+				{what: add, instanceID: "instance-9", instanceDesc: generateRingInstanceWithInfo("instance-9", "zone-c", []uint32{userToken(userID, "zone-c", 2) + 2}, now.Add(-2*lookbackPeriod))},
 				{what: test, shardSize: 3, expected: []string{"instance-1", "instance-2", "instance-3"}},
 				{what: test, shardSize: 6, expected: []string{"instance-1", "instance-2", "instance-3", "instance-7", "instance-8", "instance-9"}},
 			},
@@ -1124,168 +1124,6 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
 	}
 }
 
-func TestRing_ShuffleShard_ConsistencyOnShardSizeChanged(t *testing.T) {
-	// Create 30 instances in 3 zones.
-	ringInstances := map[string]IngesterDesc{}
-	for i := 0; i < 30; i++ {
-		name, desc := generateRingInstance(i, i%3)
-		ringInstances[name] = desc
-	}
-
-	// Init the ring description.
-	ringDesc := &Desc{Ingesters: ringInstances}
-	for id, instance := range ringDesc.Ingesters {
-		instance.Timestamp = time.Now().Unix()
-		instance.State = ACTIVE
-		ringDesc.Ingesters[id] = instance
-	}
-
-	ring := Ring{
-		cfg: Config{
-			HeartbeatTimeout:     time.Hour,
-			ZoneAwarenessEnabled: true,
-		},
-		ringDesc:         ringDesc,
-		ringTokens:       ringDesc.getTokens(),
-		ringTokensByZone: ringDesc.getTokensByZone(),
-		ringZones:        getZones(ringDesc.getTokensByZone()),
-		strategy:         &DefaultReplicationStrategy{},
-	}
-
-	// Get the replication set with shard size = 3.
-	firstShard := ring.ShuffleShard("tenant-id", 3)
-	assert.Equal(t, 3, firstShard.IngesterCount())
-
-	firstSet, err := firstShard.GetAll(Read)
-	require.NoError(t, err)
-
-	// Increase shard size to 6.
-	secondShard := ring.ShuffleShard("tenant-id", 6)
-	assert.Equal(t, 6, secondShard.IngesterCount())
-
-	secondSet, err := secondShard.GetAll(Read)
-	require.NoError(t, err)
-
-	for _, firstInstance := range firstSet.Ingesters {
-		assert.True(t, secondSet.Includes(firstInstance.Addr), "new replication set is expected to include previous instance %s", firstInstance.Addr)
-	}
-
-	// Increase shard size to 9.
-	thirdShard := ring.ShuffleShard("tenant-id", 9)
-	assert.Equal(t, 9, thirdShard.IngesterCount())
-
-	thirdSet, err := thirdShard.GetAll(Read)
-	require.NoError(t, err)
-
-	for _, secondInstance := range secondSet.Ingesters {
-		assert.True(t, thirdSet.Includes(secondInstance.Addr), "new replication set is expected to include previous instance %s", secondInstance.Addr)
-	}
-
-	// Decrease shard size to 6.
-	fourthShard := ring.ShuffleShard("tenant-id", 6)
-	assert.Equal(t, 6, fourthShard.IngesterCount())
-
-	fourthSet, err := fourthShard.GetAll(Read)
-	require.NoError(t, err)
-
-	// We expect to have the same exact instances we had when the shard size was 6.
-	for _, secondInstance := range secondSet.Ingesters {
-		assert.True(t, fourthSet.Includes(secondInstance.Addr), "new replication set is expected to include previous instance %s", secondInstance.Addr)
-	}
-
-	// Decrease shard size to 3.
-	fifthShard := ring.ShuffleShard("tenant-id", 3)
-	assert.Equal(t, 3, fifthShard.IngesterCount())
-
-	fifthSet, err := fifthShard.GetAll(Read)
-	require.NoError(t, err)
-
-	// We expect to have the same exact instances we had when the shard size was 3.
-	for _, firstInstance := range firstSet.Ingesters {
-		assert.True(t, fifthSet.Includes(firstInstance.Addr), "new replication set is expected to include previous instance %s", firstInstance.Addr)
-	}
-}
-
-func TestRing_ShuffleShard_ConsistencyOnZonesChanged(t *testing.T) {
-	// Create 20 instances in 2 zones.
-	ringInstances := map[string]IngesterDesc{}
-	for i := 0; i < 20; i++ {
-		name, desc := generateRingInstance(i, i%2)
-		ringInstances[name] = desc
-	}
-
-	// Init the ring description.
-	ringDesc := &Desc{Ingesters: ringInstances}
-	for id, instance := range ringDesc.Ingesters {
-		instance.Timestamp = time.Now().Unix()
-		instance.State = ACTIVE
-		ringDesc.Ingesters[id] = instance
-	}
-
-	ring := Ring{
-		cfg: Config{
-			HeartbeatTimeout:     time.Hour,
-			ZoneAwarenessEnabled: true,
-		},
-		ringDesc:         ringDesc,
-		ringTokens:       ringDesc.getTokens(),
-		ringTokensByZone: ringDesc.getTokensByZone(),
-		ringZones:        getZones(ringDesc.getTokensByZone()),
-		strategy:         &DefaultReplicationStrategy{},
-	}
-
-	// Get the replication set with shard size = 2.
-	firstShard := ring.ShuffleShard("tenant-id", 2)
-	assert.Equal(t, 2, firstShard.IngesterCount())
-
-	firstSet, err := firstShard.GetAll(Read)
-	require.NoError(t, err)
-
-	// Increase shard size to 4.
-	secondShard := ring.ShuffleShard("tenant-id", 4)
-	assert.Equal(t, 4, secondShard.IngesterCount())
-
-	secondSet, err := secondShard.GetAll(Read)
-	require.NoError(t, err)
-
-	for _, firstInstance := range firstSet.Ingesters {
-		assert.True(t, secondSet.Includes(firstInstance.Addr), "new replication set is expected to include previous instance %s", firstInstance.Addr)
-	}
-
-	// Scale up cluster, adding 10 instances in 1 new zone.
-	for i := 20; i < 30; i++ {
-		name, desc := generateRingInstance(i, 2)
-		ringInstances[name] = desc
-	}
-
-	ring.ringDesc.Ingesters = ringInstances
-	ring.ringTokens = ringDesc.getTokens()
-	ring.ringTokensByZone = ringDesc.getTokensByZone()
-	ring.ringZones = getZones(ringDesc.getTokensByZone())
-
-	// Increase shard size to 6.
-	thirdShard := ring.ShuffleShard("tenant-id", 6)
-	assert.Equal(t, 6, thirdShard.IngesterCount())
-
-	thirdSet, err := thirdShard.GetAll(Read)
-	require.NoError(t, err)
-
-	for _, secondInstance := range secondSet.Ingesters {
-		assert.True(t, thirdSet.Includes(secondInstance.Addr), "new replication set is expected to include previous instance %s", secondInstance.Addr)
-	}
-
-	// Increase shard size to 9.
-	fourthShard := ring.ShuffleShard("tenant-id", 9)
-	assert.Equal(t, 9, fourthShard.IngesterCount())
-
-	fourthSet, err := fourthShard.GetAll(Read)
-	require.NoError(t, err)
-
-	for _, thirdInstance := range thirdSet.Ingesters {
-		assert.True(t, fourthSet.Includes(thirdInstance.Addr), "new replication set is expected to include previous instance %s", thirdInstance.Addr)
-	}
-}
-
 func BenchmarkRing_ShuffleShard(b *testing.B) {
 	for _, numInstances := range []int{50, 100, 1000} {
 		for _, numZones := range []int{1, 3} {
@@ -1581,8 +1419,8 @@ func TestShuffleShardWithCaching(t *testing.T) {
 }
 
 // User shuffle shard token.
-func userToken(user string, skip int) uint32 {
-	r := rand.New(rand.NewSource(util.ShuffleShardSeed(user)))
+func userToken(user, zone string, skip int) uint32 {
+	r := rand.New(rand.NewSource(util.ShuffleShardSeed(user, zone)))
 
 	for ; skip > 0; skip-- {
 		_ = r.Uint32()

From f5211d0f66bb87c9a22704362c8e272f38340e86 Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Tue, 13 Oct 2020 12:25:01 +0200
Subject: [PATCH 14/17] Added unit test on ReplicationSet.GetAddresses()

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 pkg/ring/replication_set.go      |  3 ++-
 pkg/ring/replication_set_test.go | 35 ++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)
 create mode 100644 pkg/ring/replication_set_test.go

diff --git a/pkg/ring/replication_set.go b/pkg/ring/replication_set.go
index ef5e3c3c5e4..3b15541f3ad 100644
--- a/pkg/ring/replication_set.go
+++ b/pkg/ring/replication_set.go
@@ -85,7 +85,8 @@ func (r ReplicationSet) Includes(addr string) bool {
 	return false
 }
 
-// GetAddresses returns the addresses of all instances within the replication set.
+// GetAddresses returns the addresses of all instances within the replication set. Returned slice
+// order is not guaranteed.
 func (r ReplicationSet) GetAddresses() []string {
 	addrs := make([]string, 0, len(r.Ingesters))
 	for _, desc := range r.Ingesters {
diff --git a/pkg/ring/replication_set_test.go b/pkg/ring/replication_set_test.go
new file mode 100644
index 00000000000..db11459cf10
--- /dev/null
+++ b/pkg/ring/replication_set_test.go
@@ -0,0 +1,35 @@
+package ring
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestReplicationSet_GetAddresses(t *testing.T) {
+	tests := map[string]struct {
+		rs       ReplicationSet
+		expected []string
+	}{
+		"should return an empty slice on empty replication set": {
+			rs:       ReplicationSet{},
+			expected: []string{},
+		},
+		"should return instances addresses (no order guaranteed)": {
+			rs: ReplicationSet{
+				Ingesters: []IngesterDesc{
+					{Addr: "127.0.0.1"},
+					{Addr: "127.0.0.2"},
+					{Addr: "127.0.0.3"},
+				},
+			},
+			expected: []string{"127.0.0.1", "127.0.0.2", "127.0.0.3"},
+		},
+	}
+
+	for testName, testData := range tests {
+		t.Run(testName, func(t *testing.T) {
+			assert.ElementsMatch(t, testData.expected, testData.rs.GetAddresses())
+		})
+	}
+}

From 4104dc6a5796bcdd77d2ac82d05295184021d614 Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Tue, 13 Oct 2020 12:28:10 +0200
Subject: [PATCH 15/17] Improved TestRing_ShuffleShardWithLookback

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 pkg/ring/ring_test.go | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/pkg/ring/ring_test.go b/pkg/ring/ring_test.go
index 71db3e86fa9..ca7841187ed 100644
--- a/pkg/ring/ring_test.go
+++ b/pkg/ring/ring_test.go
@@ -905,7 +905,9 @@ func TestRing_ShuffleShardWithLookback(t *testing.T) {
 				{what: test, shardSize: 3, expected: []string{"instance-1", "instance-2", "instance-3"}},
 				// Scale up.
 				{what: add, instanceID: "instance-7", instanceDesc: generateRingInstanceWithInfo("instance-7", "zone-a", []uint32{userToken(userID, "zone-a", 0) + 1}, now)},
+				{what: test, shardSize: 3, expected: []string{"instance-7", "instance-2", "instance-3" /* lookback: */, "instance-1"}},
 				{what: add, instanceID: "instance-8", instanceDesc: generateRingInstanceWithInfo("instance-8", "zone-b", []uint32{userToken(userID, "zone-b", 1) + 1}, now)},
+				{what: test, shardSize: 3, expected: []string{"instance-7", "instance-8", "instance-3" /* lookback: */, "instance-1", "instance-2"}},
 				{what: add, instanceID: "instance-9", instanceDesc: generateRingInstanceWithInfo("instance-9", "zone-c", []uint32{userToken(userID, "zone-c", 2) + 1}, now)},
 				{what: test, shardSize: 3, expected: []string{"instance-7", "instance-8", "instance-9" /* lookback: */, "instance-1", "instance-2", "instance-3"}},
 			},
@@ -924,7 +926,9 @@ func TestRing_ShuffleShardWithLookback(t *testing.T) {
 				{what: test, shardSize: 3, expected: []string{"instance-1", "instance-2", "instance-3"}},
 				// Scale down.
 				{what: remove, instanceID: "instance-1"},
+				{what: test, shardSize: 3, expected: []string{"instance-7", "instance-2", "instance-3"}},
 				{what: remove, instanceID: "instance-2"},
+				{what: test, shardSize: 3, expected: []string{"instance-7", "instance-8", "instance-3"}},
 				{what: remove, instanceID: "instance-3"},
 				{what: test, shardSize: 3, expected: []string{"instance-7", "instance-8", "instance-9"}},
 			},

From 44b50fd64607a13ccbcd5eba96aa885ff255a34b Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Tue, 13 Oct 2020 14:28:05 +0200
Subject: [PATCH 16/17] Fixed
 TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 pkg/ring/ring_test.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pkg/ring/ring_test.go b/pkg/ring/ring_test.go
index ca7841187ed..3ba76e99832 100644
--- a/pkg/ring/ring_test.go
+++ b/pkg/ring/ring_test.go
@@ -1035,7 +1035,7 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
 				currTime := time.Now().Add(lookbackPeriod).Add(time.Minute)
 
 				// Add the initial shard to the history.
-				rs, err := ring.ShuffleShard(userID, shardSize).GetAll(Read)
+				rs, err := ring.shuffleShard(userID, shardSize, 0, time.Now()).GetAll(Read)
 				require.NoError(t, err)
 
 				history := map[time.Time]ReplicationSet{
@@ -1099,7 +1099,7 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
 					}
 
 					// Add the current shard to the history.
-					rs, err = ring.ShuffleShard(userID, shardSize).GetAll(Read)
+					rs, err = ring.shuffleShard(userID, shardSize, 0, time.Now()).GetAll(Read)
 					require.NoError(t, err)
 					history[currTime] = rs
 

From a08b64c5b8017dbc1ae52e54d0ee95f85aa17ed0 Mon Sep 17 00:00:00 2001
From: Marco Pracucci <marco@pracucci.com>
Date: Tue, 13 Oct 2020 15:44:53 +0200
Subject: [PATCH 17/17] Increased unit tests timeout

Signed-off-by: Marco Pracucci <marco@pracucci.com>
---
 pkg/ring/ring_test.go | 12 ++++++------
 tools/test            |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/pkg/ring/ring_test.go b/pkg/ring/ring_test.go
index 3ba76e99832..3529fadde1e 100644
--- a/pkg/ring/ring_test.go
+++ b/pkg/ring/ring_test.go
@@ -996,23 +996,23 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) {
 	// are returned at any given time, BUT at least all required instances are returned.
 	var (
 		numInitialInstances = []int{9, 30, 60, 90}
-		numInitialZones     = []int{3, 1, 3}
+		numInitialZones     = []int{1, 3}
 		numEvents           = 100
 		lookbackPeriod      = time.Hour
 		delayBetweenEvents  = 5 * time.Minute // 12 events / hour
 		userID              = "user-1"
 	)
 
-	// Randomise the seed but log it in case we need to reproduce the test on failure.
-	seed := time.Now().UnixNano()
-	rand.Seed(seed)
-	t.Log("random generator seed:", seed)
-
 	for _, numInstances := range numInitialInstances {
 		for _, numZones := range numInitialZones {
 			testName := fmt.Sprintf("num instances = %d, num zones = %d", numInstances, numZones)
 
 			t.Run(testName, func(t *testing.T) {
+				// Randomise the seed but log it in case we need to reproduce the test on failure.
+				seed := time.Now().UnixNano()
+				rand.Seed(seed)
+				t.Log("random generator seed:", seed)
+
 				// Initialise the ring.
 				ringDesc := &Desc{Ingesters: generateRingInstances(numInstances, numZones)}
 				ring := Ring{
diff --git a/tools/test b/tools/test
index bea0d3f17cd..0a089fc2c7c 100755
--- a/tools/test
+++ b/tools/test
@@ -7,7 +7,7 @@ SLOW=
 TAGS=
 PARALLEL=
 RACE="-race -covermode=atomic"
-TIMEOUT=5m
+TIMEOUT=10m
 VERBOSE=
 
 usage() {