Add proposal document for ingester unregister flexibility #7231
Conversation
|
|
|
thanks for taking the time to write this @LasseHels! It makes sense to me in terms of allowing to trade of query consistency for availability. Mimir wasn't built with this use case in mind, so this solution seems a bit counter-intuitive. But if we make this clear in the documentation for anyone who uses it, it is an acceptable solution. I've also asked other maintainers to take a look. |
There was a problem hiding this comment.
Thank you very much for writing the summary of the previous discussion in this short and digestible proposal. Suggested change looks tiny, and I would be curious to learn how it works in practice.
Have you tried to implement it and test if it works in your situation? If it works, I have no objections to adding such endpoint to Mimir.
| **Cons**: | ||
| * Queries may return incomplete results as samples held by the evicted ingesters are temporarily inaccessible. | ||
| The samples become accessible again when the ingesters join the ring after having replayed their WAL. | ||
| * Ownership of evicted series (i.e., series whose owner was evicted) is split between two ingesters. This is likely |
There was a problem hiding this comment.
To confirm my understanding, this is a result of resharding, right?
There was a problem hiding this comment.
this is a result of resharding, right?
Yes. Consider this example:
- Ingester
a-1owns seriesup{app="database"}. a-1ingests a couple of samples from seriesup{app="database"}.a-1is evicted.- As soon as
a-1is evicted, ownership ofup{app="database"}is implicitly passed toa-5. - While
a-1is restarting,a-5ingests samples from theup{app="database"}series. - Ownership of series
up{app="database"}remains witha-5even aftera-1has joined the ring once again1. - Ownership of series
up{app="database"}is now split since botha-1anda-5hold samples for the series.
Footnotes
-
Unless tokens are persisted on disk via
-ingester.ring.tokens-file-path, in which case ownership of the series will be passed back toa-1. ↩
There was a problem hiding this comment.
Right, thank you. I find this aspect of the proposal quite worrisome, to be honest, but whether it is a problem or not depends on your specifics.
(I will "unresolve" this conversation, as it has context that others may find useful.)
There was a problem hiding this comment.
@pstibrany Is there anything in particular with split series ownership that concerns you?
If my understanding is correct, split ownership can already happen if ingesters are configured (in the standard way) to leave the ring on shutdown. I suppose the endpoint makes it easier to trigger a state of split ownership.
There was a problem hiding this comment.
split ownership can already happen if ingesters are configured (in the standard way) to leave the ring on shutdown
That's correct. However, we don't recommend to leave ring on shutdown (e.g. we don't do it in our Jsonnet and Helm). The only reason why the default is still -ingester.ring.unregister-on-shutdown=true is because it's easier for the first time Mimir operator (at the beginning it's difficult to understand the implications of not leaving the ring on shutdown).
There was a problem hiding this comment.
@pstibrany Is there anything in particular with split series ownership that concerns you?
Resharding of the series and related increased usage of resources (memory, cpu) on other ingesters. If ingesters leave ring frequently, surviving ingesters may accumulate lot of series in memory that otherwise belong to other ingesters. It's something to keep in mind when planning for capacity. You save cost thanks to using spot-instances, but you may need to plan for larger instances instead. As you mention, it depends on how often ingester evictions happen.
We've not implemented it yet. We wanted to ensure that we had at least some buy-in from the Mimir team before starting with the implementation. |
There was a problem hiding this comment.
With the maintainer hat
I agree with Peter that the proposed change looks small. I also think you don't need to get these changes in Mimir main to start testing it. Would be nice (but not a blocker) if you could just do the changes, deploy them in your cluster and share some data points about how it works for you.
Mimir is quite opininated about guaranteeing we never return partial query results. The only reason why this proposal doesn't break such stance is because the proposed API is generic and it's how it will get used that may cause partial query results (but it's no different than clicking the "Forget" button the ring page).
With the operator hat
I'm personally not much convinced about the approach you're going to take. I understand cutting costs, but I'm not sure doubting whether the query results you get are consistent or not may provide a bad query experience.
I also think that in case of a significant % of ingesters removed from the ring, the others will suffer a thundering effect that will cause an outage anyway. By experience, recovering from such outage may be tricky. If remaining ingesters ingested a volume of data they can't practically handle, the only way to recover is vertically scaling, because all data written to WAL will still be there in a subsequent restart (alternative is delete the WAL, which is quite bad).
That being said, these are my personal considerations on how you plan to use the API and not the API itself for which I have no concerns.
|
@pstibrany @pracucci Thanks for your feedback. We will get started with an implementation and report back when we've tried it out, which could take a while. |
See #5901 (comment). @pstibrany Now that the parent pull request has been merged, should we merge this one also? |
Let's do that. |
|
Looks like a CI check is failing, let me see if I can fix that. |
Head branch was pushed to by a user without write access
Fixed! @pstibrany I think auto-merge was disabled because I pushed a new commit to fix the pipeline. We should be good to merge now. |
What this PR does
This pull request adds a document which summarises #5901 and introduces a solution proposal.
Checklist
Tests updated.CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.