diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ac212caa38..68ee6088218 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,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 diff --git a/pkg/ring/replication_set.go b/pkg/ring/replication_set.go index 461429d6fa8..2a8fb7a7bf1 100644 --- a/pkg/ring/replication_set.go +++ b/pkg/ring/replication_set.go @@ -127,17 +127,17 @@ 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 @@ -145,6 +145,18 @@ func HasReplicationSetChangedWithoutState(before, after ReplicationSet) bool { }) } +// 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 { diff --git a/pkg/ring/replication_set_test.go b/pkg/ring/replication_set_test.go index 42ecc0f1122..4d07887fe35 100644 --- a/pkg/ring/replication_set_test.go +++ b/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") + }) + } +} diff --git a/pkg/storegateway/gateway.go b/pkg/storegateway/gateway.go index 87ca44a337f..8508763dae3 100644 --- a/pkg/storegateway/gateway.go +++ b/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) } diff --git a/pkg/storegateway/gateway_test.go b/pkg/storegateway/gateway_test.go index 4f8ebfe4eeb..a5fdb518e34 100644 --- a/pkg/storegateway/gateway_test.go +++ b/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) {