Repository navigation
Ring: don't publish instance state before tokens are generated - #7901
Open
danielblando wants to merge 1 commit into
Open
danielblando wants to merge 1 commit into
danielblando wants to merge 1 commit into
Conversation
Signed-off-by: Daniel Blando <[email protected]>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does:
An ingester can end up registered in the ring owning no tokens, and never recover. It serves no
writes, serves no reads, and never passes its readiness check. Restarting does not help. Getting
into this state needs
-ingester.tokens-generator-strategy=minimize-spread, which is not thedefault, but once an instance is in it, it cannot get out regardless of the strategy.
How it happens:
autoJoinsets the instance to its target state at the top of its CAS callback, beforegenerating any tokens.
minimize-spread, generation can legitimately come back empty. The generator admits onlythe earliest-registered tokenless instance in a zone and returns nothing to the rest when
forceis false, which is howautoJoincalls it. The defaultrandomgenerator alwaysreturns the full count, so this branch is unreachable there.
no tokens. The heartbeat writes
i.GetState()to the ring on every tick, so that combinationgets persisted. The callback re-runs on each CAS retry, so this is not a brief race.
initRingfinds the entry and adopts its state from the ring.autoJoinonly runs while the instance isPENDING, on a timer that fires once per process, soit is never reached again. The restart is what re-adopts the bad state, which is why restarting
does not clear it.
Under
minimize-spreadthe damage is not confined to that instance. It holds the head of itszone's queue, so no other tokenless instance in the zone can generate either. We hit this on a cell
where 13 ingesters in one zone went tokenless and none recovered. The two oldest sat that way for
about 19 hours with zero restarts, because they never got far enough to fail.
The fix:
autoJoinsets the state only after tokens are secured, on both paths that publish the instance.This closes the window rather than narrowing it. What gets written to the ring is unchanged:
setStatenow runs immediately before the existingi.GetState()read on each success path, andnothing between the old and new position reads the instance state.
initRingresets an instance found in the ring without tokens back toPENDING, mirroring theJOININGreset directly above it. Instances already stuck recover on restart, instead of needingthe ring edited by hand. Forgetting such an entry only sticks while the process is down, since a
live one re-creates it on its next heartbeat.
Which issue(s) this PR fixes:
Fixes #7899
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]docs/configuration/v1-guarantees.mdupdated if this PR introduces experimental flags