Do not resync blocks in running store gateways during rollout deployment and container restart (#5363)

* Do not resync blocks in running store gateways during rollout deployment and container restart

Signed-off-by: Justin Jung <[email protected]>

* Add changelog

Signed-off-by: Justin Jung <[email protected]>

* Change numScaleUpGateways in RingTopologyChangedAfterScaleUp test to be less than the replication factor

Signed-off-by: Justin Jung <[email protected]>

* Ignore RegisteredTimestamp in HasReplicationSetChangedWithoutStateAndAddress

Signed-off-by: Justin Jung <[email protected]>

---------

Signed-off-by: Justin Jung <[email protected]>

Author: justinjung04

CHANGELOG.md

@@ -27,6 +27,7 @@
 * [BUGFIX] Storage: Bucket index updater should ignore meta not found for partial blocks. #5343
 * [BUGFIX] Ring: Add JOINING state to read operation. #5346
 * [BUGFIX] Compactor: Partial block with only visit marker should be deleted even there is no deletion marker. #5342
+* [ENHANCEMENT] Do not resync blocks in running store gateways during rollout deployment and container restart. #5363
 
 ## 1.15.1 2023-04-26
 

pkg/ring/replication_set.go

@@ -127,24 +127,36 @@ func (r ReplicationSet) GetAddressesWithout(exclude string) []string {
 	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, ...).
+// HasReplicationSetChanged returns false if two replications sets are the same (with possibly different timestamps),
+// true if they differ in any way (number of instances, instance states, tokens, zones, ...).
 func HasReplicationSetChanged(before, after ReplicationSet) bool {
 	return hasReplicationSetChangedExcluding(before, after, func(i *InstanceDesc) {
 		i.Timestamp = 0
 	})
 }
 
-// HasReplicationSetChangedWithoutState returns true if two replications sets
+// HasReplicationSetChangedWithoutState returns false if two replications sets
 // are the same (with possibly different timestamps and instance states),
-// false if they differ in any other way (number of instances, tokens, zones, ...).
+// true if they differ in any other way (number of instances, tokens, zones, ...).
 func HasReplicationSetChangedWithoutState(before, after ReplicationSet) bool {
 	return hasReplicationSetChangedExcluding(before, after, func(i *InstanceDesc) {
 		i.Timestamp = 0
 		i.State = PENDING
 	})
 }
 
+// HasReplicationSetChangedWithoutStateAndAddress returns false if two replications sets
+// are the same (with possibly different timestamps, instance states and address),
+// true if they differ in any other way (number of instances, tokens, or zones).
+func HasReplicationSetChangedWithoutStateAndAddress(before, after ReplicationSet) bool {
+	return hasReplicationSetChangedExcluding(before, after, func(i *InstanceDesc) {
+		i.Timestamp = 0
+		i.State = PENDING
+		i.Addr = ""
+		i.RegisteredTimestamp = 0
+	})
+}
+
 // Do comparison of replicasets, but apply a function first
 // to be able to exclude (reset) some values
 func hasReplicationSetChangedExcluding(before, after ReplicationSet, exclude func(*InstanceDesc)) bool {

pkg/ring/replication_set_test.go

@@ -251,9 +251,10 @@ var (
 		},
 	}
 	replicationSetChangesTestCases = map[string]struct {
-		nextState                                  ReplicationSet
-		expectHasReplicationSetChanged             bool
-		expectHasReplicationSetChangedWithoutState bool
+		nextState                                            ReplicationSet
+		expectHasReplicationSetChanged                       bool
+		expectHasReplicationSetChangedWithoutState           bool
+		expectHasReplicationSetChangedWithoutStateAndAddress bool
 	}{
 		"timestamp changed": {
 			ReplicationSet{
@@ -265,6 +266,7 @@ var (
 			},
 			false,
 			false,
+			false,
 		},
 		"state changed": {
 			ReplicationSet{
@@ -276,6 +278,7 @@ var (
 			},
 			true,
 			false,
+			false,
 		},
 		"more instances": {
 			ReplicationSet{
@@ -288,6 +291,30 @@ var (
 			},
 			true,
 			true,
+			true,
+		},
+		"less instances": {
+			ReplicationSet{
+				Instances: []InstanceDesc{
+					{Addr: "127.0.0.1"},
+					{Addr: "127.0.0.2"},
+				},
+			},
+			true,
+			true,
+			true,
+		},
+		"replaced instance": {
+			ReplicationSet{
+				Instances: []InstanceDesc{
+					{Addr: "127.0.0.1"},
+					{Addr: "127.0.0.2"},
+					{Addr: "127.0.0.5"},
+				},
+			},
+			true,
+			true,
+			false,
 		},
 	}
 )
@@ -309,3 +336,12 @@ func TestHasReplicationSetChangedWithoutState_IgnoresTimeStampAndState(t *testin
 		})
 	}
 }
+
+func TestHasReplicationSetChangedWithoutStateAndAddress_IgnoresTimeStampAndStateAndAddress(t *testing.T) {
+	// Only testing difference to underlying Equal function
+	for testName, testData := range replicationSetChangesTestCases {
+		t.Run(testName, func(t *testing.T) {
+			assert.Equal(t, testData.expectHasReplicationSetChangedWithoutStateAndAddress, HasReplicationSetChangedWithoutStateAndAddress(replicationSetChangesInitialState, testData.nextState), "HasReplicationSetChangedWithoutStateAndAddress wrong result")
+		})
+	}
+}

pkg/storegateway/gateway.go

@@ -299,7 +299,8 @@ func (g *StoreGateway) running(ctx context.Context) error {
 			// replication set which we use to compare with the previous state.
 			currRingState, _ := g.ring.GetAllHealthy(BlocksOwnerSync) // nolint:errcheck
 
-			if ring.HasReplicationSetChanged(ringLastState, currRingState) {
+			// Ignore address when comparing to avoid block re-sync if tokens are persisted with tokens_file_path
+			if ring.HasReplicationSetChangedWithoutStateAndAddress(ringLastState, currRingState) {
 				ringLastState = currRingState
 				g.syncStores(ctx, syncReasonRingChange)
 			}

pkg/storegateway/gateway_test.go

@@ -382,7 +382,7 @@ func TestStoreGateway_BlocksSyncWithDefaultSharding_RingTopologyChangedAfterScal
 		shardingStrategy     = util.ShardingStrategyDefault
 		replicationFactor    = 3
 		numInitialGateways   = 4
-		numScaleUpGateways   = 6
+		numScaleUpGateways   = 2
 		expectedBlocksLoaded = 3 * numBlocks // blocks are replicated 3 times
 	)
 
@@ -615,7 +615,7 @@ func TestStoreGateway_SyncOnRingTopologyChanged(t *testing.T) {
 			},
 			expectedSync: true,
 		},
-		"should sync when an instance changes state": {
+		"should NOT sync when an instance changes state": {
 			setupRing: func(desc *ring.Desc) {
 				desc.AddIngester("instance-1", "127.0.0.1", "", ring.Tokens{1, 2, 3}, ring.ACTIVE, registeredAt)
 				desc.AddIngester("instance-2", "127.0.0.2", "", ring.Tokens{4, 5, 6}, ring.JOINING, registeredAt)
@@ -625,7 +625,19 @@ func TestStoreGateway_SyncOnRingTopologyChanged(t *testing.T) {
 				instance.State = ring.ACTIVE
 				desc.Ingesters["instance-2"] = instance
 			},
-			expectedSync: true,
+			expectedSync: false,
+		},
+		"should NOT sync when an instance address is replaced": {
+			setupRing: func(desc *ring.Desc) {
+				desc.AddIngester("instance-1", "127.0.0.1", "", ring.Tokens{1, 2, 3}, ring.ACTIVE, registeredAt)
+				desc.AddIngester("instance-2", "127.0.0.2", "", ring.Tokens{4, 5, 6}, ring.JOINING, registeredAt)
+			},
+			updateRing: func(desc *ring.Desc) {
+				instance := desc.Ingesters["instance-2"]
+				instance.Addr = "127.0.0.3"
+				desc.Ingesters["instance-2"] = instance
+			},
+			expectedSync: false,
 		},
 		"should sync when an healthy instance becomes unhealthy": {
 			setupRing: func(desc *ring.Desc) {