Make sure unregistered ingester joining the ring after WAL replay

[alexqyle]

What this PR does:

Currently, when starting unregistered ingester with token file on disk, ingester would join the ring with pending state along with tokens read from file first. After replaying WAL, ingester would be turned into active state. In this case, ingester would receive write requests from distributor because it is in the ring with tokens. Since ingester is in pending state, all incoming write requests would be rejected with 5xx. Under such situation, end user cannot start multiple ingesters with token file at the same to avoid remote write 5xx in distributor.

This change is meant to fix this problem. So that multiple ingesters with token file can be spinned up at same time without causing remote write 5xx. The fix is that an unregistered instance would not join the ring if token file exists while autoJoinOnStartup is set to false until autoJoinAfter is triggered in loop. Also, heartbeat would not start immediately if instance did not join the ring on init. Heartbeat would start either in autoJoinAfter or observeChan when instance being added into the ring.

Which issue(s) this PR fixes:

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[danielblando]

Can you add more details on the issue you are trying to solve? From what we discuss offline I am a bit confused.

While the openTSBD is happening,
It seems the old code would mark ingester from LEAVING to PENDING on the ring and start heartbeat.
With this change we would leave the ingester on the ring as LEAVING without heartbeat.
Correct?

[danielblando]

Change makes sense to me.
Thanks

[danielblando]

Can you add an ENHANCEMENT on changelog?

[damnever]

Shouldn't the DefaultReplicationStrategy do its job to filter out unhealthy instances?

[alexqyle]

Shouldn't the DefaultReplicationStrategy do its job to filter out unhealthy instances?

The problem for the original code was that if ingesters joined the ring with pending state and loaded tokens from token file exists on disk, distributor would include those ingesters in replication sets. For example, with replication factor of 3, there would be 3 ingesters in replication set. Those 3 ingesters might contain more than 2 ingesters that had tokens and in pending state. After getting replication set, ring code would apply filter on those 3 ingesters. It would find 2 out of 3 were unhealthy because they were in pending state and returned error to fail fast since minimum success would not meet.

[damnever]

Shouldn't the DefaultReplicationStrategy do its job to filter out unhealthy instances?

The problem for the original code was that if ingesters joined the ring with pending state and loaded tokens from token file exists on disk, distributor would include those ingesters in replication sets. For example, with replication factor of 3, there would be 3 ingesters in replication set. Those 3 ingesters might contain more than 2 ingesters that had tokens and in pending state. After getting replication set, ring code would apply filter on those 3 ingesters. It would find 2 out of 3 were unhealthy because they were in pending state and returned error to fail fast since minimum success would not meet.

Based on my understanding, if extend-writes=true, then ShouldExtendReplicaSetOnState should extend the replicate set if an ingester is in the PENDING state. Otherwise, we should disable ingester.unregister-on-shutdown, and the PENDING state does not matter since we would expect that series not to spread to other ingesters.