Repository: grafana/dskit (go)

Issue:
When lifecycler reuses existing LEAVING entry, it waits for another heartbeat period to write ACTIVE state to ring
When lifecycler finds existing entry in the ring, it will reuse state and tokens from it. If the state was LEAVING and entry has enough tokens, lifecycler immediately switches its internal state to ACTIVE, but it doesn't update the ring with this state, and instead waits for next heartbeat.

With long heartbeats (eg. 2 minutes), this makes the time until ACTIVE state of lifecycler is visible to other components longer.

This also affects how soon lifecycler reports "ready" in its `CheckReady` [function](https://github.com/grafana/dskit/blob/789ec0ca4a3b372335fad1c73147366a23ddc8b6/ring/lifecycler.go#L192), because it actually checks the state in the ring. (This is correct behaviour, if it only consulted its internal state, we could report "ready" before other components saw it as active, which could result in an outage in case of Mimir ingesters for example).


Pull request description:
Don't wait until next heartbeat to update ring on initialization.
**What this PR does**:
This PR changes lifecycler initialization such that it always updates ring immediately when it finds existing ring entry that it can reuse, instead of waiting for next heartbeat, since heartbeat period can be configured with long time.

**Which issue(s) this PR fixes**:

Fixes #174

**Checklist**
- [x] Tests updated
- [x] `CHANGELOG.md` updated - the order of entries should be `[CHANGE]`, `[FEATURE]`, `[ENHANCEMENT]`, `[BUGFIX]`


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 128b35e..63ed28f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -57,3 +57,4 @@
 * [BUGFIX] Multi KV: fix panic when using function to modify primary KV store in runtime. #153
 * [BUGFIX] Lifecycler: if the ring backend storage is reset, the instance adds itself back to the ring with an updated registration timestamp set to current time. #165
 * [BUGFIX] Ring: fix bug where hash ring instances may appear unhealthy in the web UI even though they are not. #172
+* [BUGFIX] Lifecycler: if existing ring entry is reused, ring is updated immediately, and not on next heartbeat. #175
diff --git a/ring/lifecycler.go b/ring/lifecycler.go
index da5f603..dfef6af 100644
--- a/ring/lifecycler.go
+++ b/ring/lifecycler.go
@@ -600,6 +600,10 @@ func (i *Lifecycler) initRing(ctx context.Context) error {
 			instanceDesc.State = ACTIVE
 		}
 
+		// We're taking over this entry, update instanceDesc with our values
+		instanceDesc.Addr = i.Addr
+		instanceDesc.Zone = i.Zone
+
 		// We exist in the ring, so assume the ring is right and copy out tokens & state out of there.
 		i.setState(instanceDesc.State)
 		tokens, _ := ringDesc.TokensFor(i.ID)
@@ -607,10 +611,9 @@ func (i *Lifecycler) initRing(ctx context.Context) error {
 
 		level.Info(i.logger).Log("msg", "existing entry found in ring", "state", i.GetState(), "tokens", len(tokens), "ring", i.RingName)
 
-		// Update the ring if the instance has been changed and the heartbeat is disabled.
-		// We dont need to update KV here when heartbeat is enabled as this info will eventually be update on KV
-		// on the next heartbeat
-		if i.cfg.HeartbeatPeriod == 0 && !instanceDesc.Equal(ringDesc.Ingesters[i.ID]) {
+		// Update the ring if the instance has been changed. We don't want to rely on heartbeat update, as heartbeat
+		// can be configured to long time, and until then lifecycler would not report this instance as ready in CheckReady.
+		if !instanceDesc.Equal(ringDesc.Ingesters[i.ID]) {
 			// Update timestamp to give gossiping client a chance register ring change.
 			instanceDesc.Timestamp = time.Now().Unix()
 			ringDesc.Ingesters[i.ID] = instanceDesc


Test patch:
diff --git a/ring/lifecycler_test.go b/ring/lifecycler_test.go
index 9358604..ad0f70f 100644
--- a/ring/lifecycler_test.go
+++ b/ring/lifecycler_test.go
@@ -648,6 +648,61 @@ func TestRestartIngester_DisabledHeartbeat_unregister_on_shutdown_false(t *testi
 	require.NoError(t, services.StopAndAwaitTerminated(context.Background(), l2))
 }
 
+func TestRestartIngester_NoUnregister_LongHeartbeat(t *testing.T) {
+	ringStore, closer := consul.NewInMemoryClient(GetCodec(), log.NewNopLogger(), nil)
+	t.Cleanup(func() { assert.NoError(t, closer.Close()) })
+
+	origTokens := GenerateTokens(100, nil)
+
+	const id = "test"
+	registeredAt := time.Now().Add(-1 * time.Hour)
+
+	err := ringStore.CAS(context.Background(), ringKey, func(in interface{}) (out interface{}, retry bool, err error) {
+		// Create ring with LEAVING entry with some tokens
+		r := GetOrCreateRingDesc(in)
+		r.AddIngester(id, "3.3.3.3:333", "old", origTokens, LEAVING, registeredAt)
+		return r, true, err
+	})
+	require.NoError(t, err)
+
+	var lifecyclerConfig LifecyclerConfig
+	flagext.DefaultValues(&lifecyclerConfig)
+	lifecyclerConfig.Addr = "1.1.1.1"
+	lifecyclerConfig.Port = 111
+	lifecyclerConfig.Zone = "new"
+	lifecyclerConfig.RingConfig.KVStore.Mock = ringStore
+	lifecyclerConfig.NumTokens = len(origTokens)
+	lifecyclerConfig.ID = id
+	lifecyclerConfig.HeartbeatPeriod = 5 * time.Minute // Long hearbeat period.
+	lifecyclerConfig.MinReadyDuration = 0              // Disable waiting extra time for Ready
+	lifecyclerConfig.JoinAfter = 1 * time.Minute       // Use long value to make sure that we don't use "join" code path.
+
+	l, err := NewLifecycler(lifecyclerConfig, &noopFlushTransferer{}, "test", ringKey, false, log.NewNopLogger(), nil)
+	require.NoError(t, err)
+
+	require.NoError(t, services.StartAndAwaitRunning(context.Background(), l))
+	defer services.StopAndAwaitTerminated(context.Background(), l) //nolint:errcheck
+
+	test.Poll(t, 1*time.Second, nil, func() interface{} {
+		return l.CheckReady(context.Background())
+	})
+
+	// Lifecycler should be in ACTIVE state, using tokens from the ring.
+	require.Equal(t, ACTIVE, l.GetState())
+	require.Equal(t, Tokens(origTokens), l.getTokens())
+	require.Equal(t, registeredAt.Truncate(time.Second), l.getRegisteredAt())
+
+	// check that ring entry has updated address and state
+	desc, err := ringStore.Get(context.Background(), ringKey)
+	require.NoError(t, err)
+
+	r := GetOrCreateRingDesc(desc)
+	require.Equal(t, ACTIVE, r.Ingesters[id].State)
+	require.Equal(t, "1.1.1.1:111", r.Ingesters[id].Addr)
+	require.Equal(t, "new", r.Ingesters[id].Zone)
+	require.Equal(t, registeredAt.Unix(), r.Ingesters[id].RegisteredTimestamp)
+}
+
 func TestTokensOnDisk(t *testing.T) {
 	ringStore, closer := consul.NewInMemoryClient(GetCodec(), log.NewNopLogger(), nil)
 	t.Cleanup(func() { assert.NoError(t, closer.Close()) })