Allow ingesters to stay in the ring during restart #3305
Conversation
csmarchbanks
force-pushed
the
no-extend-writes
branch
from
0fc2e68 to
fbabbe7
Compare
csmarchbanks
force-pushed
the
no-extend-writes
branch
from
fbabbe7 to
dfb5be5
Compare
csmarchbanks
changed the title
csmarchbanks
force-pushed
the
no-extend-writes
branch
from
005d38e to
726f518
Compare
csmarchbanks
force-pushed
the
no-extend-writes
branch
4 times, most recently
from
e2b1a6a to
030d189
Compare
|
Thanks for the review! I implemented all of the suggestions - the rename did mean changing some tests around that were depending on SkipUnregister being default false. |
By keeping an ingester in the ring and not writing to additional ingesters while it is being restarted, keeps small series from being created. This will allow users running with block storage or a WAL to avoid excess load and memory during a rolling restart. Signed-off-by: Chris Marchbanks <[email protected]>
Signed-off-by: Chris Marchbanks <[email protected]>
csmarchbanks
force-pushed
the
no-extend-writes
branch
from
030d189 to
31edf47
Compare
|
Friendly reminder for @pracucci to review as requested :) |
pkg/ring/lifecycler.go
Outdated
| @@ -778,6 +781,16 @@ func (i *Lifecycler) SetFlushOnShutdown(flushOnShutdown bool) { | |||
| i.flushOnShutdown.Store(flushOnShutdown) | |||
| } | |||
|
|
|||
| // SkipUnregister returns if unregistering is skipped on shutdown. | |||
| func (i *Lifecycler) UnregisterFromRing() bool { | |||
There was a problem hiding this comment.
suggestion(non-blocking,if-minor): I would rename this function to parallel SetUnregisterFromRing. In my opinion UnregisterFromRing() connotes that it will actually perform the operation to unregister from the ring, not return the config value.
| func (i *Lifecycler) UnregisterFromRing() bool { | |
| func (i *Lifecycler) GetUnregisterFromRing() bool { |
There was a problem hiding this comment.
I changed it to Should which read nicer to me than Get.
Signed-off-by: Chris Marchbanks <[email protected]>
csmarchbanks
force-pushed
the
no-extend-writes
branch
from
31edf47 to
1823579
Compare
|
Ref #467 which I think this fixes, at least for the WAL restart-in-place case. |
|
I'll mention another downside: you lose redundancy. With replication-factor=3, if you turn off "extend writes" then do a rolling update, you are down to 2 copies of some data. If one of those is then lost then queries will be unstable due to #731. |
By keeping an ingester in the ring and not writing to additional
ingesters while it is being restarted, will stop small chunks from being
persisted to other ingesters. This will allow users running with a WAL
to avoid excess load and memory during a rolling restart.
I am curious what others think of this, if accepted, I think a similar
approach could be applied to the store-gateway (issue #2823). The
downside is that scaling a cortex cluster down while running with skip
unregister will involve manual forgetting of the ingester (or automation
to do this).
Fixes: #3304
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]