Add compactor auto-forget from ring on unhealthy #6563

eeldaly · 2025-01-28T18:30:17Z

What this PR does:

Add auto_forget_unhealthy_periods for ring to auto forget unhealthy compactors.
Add LifecyclerDelegate interface to normal Lifecycler with OnRingInstanceHeartbeat

Which issue(s) this PR fixes:
Fixes #6533

Checklist

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

Signed-off-by: Essam Eldaly <eeldaly@amazon.com>

Signed-off-by: Ben Ye <benye@amazon.com>

Signed-off-by: Essam Eldaly <eeldaly@amazon.com>

yeya24 · 2025-01-28T19:01:13Z

pkg/ring/ring.go

@@ -168,6 +169,7 @@ func (cfg *Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
 	f.IntVar(&cfg.ReplicationFactor, prefix+"distributor.replication-factor", 3, "The number of ingesters to write to and read from.")
 	f.BoolVar(&cfg.ZoneAwarenessEnabled, prefix+"distributor.zone-awareness-enabled", false, "True to enable the zone-awareness and replicate ingested samples across different availability zones.")
 	f.Var(&cfg.ExcludedZones, prefix+"distributor.excluded-zones", "Comma-separated list of zones to exclude from the ring. Instances in excluded zones will be filtered out from the ring.")
+	f.IntVar(&cfg.AutoForgetUnhealthyPeriods, prefix+"ring.auto-forget-unhealthy-periods", -1, "The number of heartbeat periods to occur after an ingester becomes unhealthy before it is forgotten from the ring. -1 to disable")


Is this flag needed? Is it used to auto forget Ingesters? I don't know if this is what we want to do in this PR at least

yeya24 · 2025-01-28T19:07:07Z

pkg/ring/ring.go

@@ -655,6 +659,43 @@ func (r *Ring) countTokens() (map[string]uint32, map[string]int64) {
 	return numTokens, owned
 }

+// autoForgetUnhealthy forgets unhealthy ingesters from the ring.
+func (r *Ring) AutoForgetUnhealthy(ctx context.Context) {


Instead of adding AutoForgetUnhealthy here I prefer to reuse the existing NewAutoForgetDelegate.
To make that work, we need to call i.delegate.OnRingInstanceHeartbeat(i, ctx) inside i.updateConsul and pass the same parameters.

+1

it should be similar to how

cortex/pkg/ring/basic_lifecycler_delegates.go

Line 143 in de5cfe1

func (d *AutoForgetDelegate) OnRingInstanceHeartbeat(lifecycler *BasicLifecycler, ringDesc *Desc, instanceDesc *InstanceDesc) {

works

After talking to @eeldaly offline I realized that the parameter is a bit different so probably hard to add the same handler.

This seems cleaner to me. We can switch AutoForgetDelegate to call this function instead.

func AutoForget(ringDesc *Desc, forgetPeriod time.Duration, logger log.Logger) { for id, instance := range ringDesc.Ingesters { lastHeartbeat := time.Unix(instance.GetTimestamp(), 0) if time.Since(lastHeartbeat) > forgetPeriod { level.Warn(d.logger).Log("msg", "auto-forgetting instance from the ring because it is unhealthy for a long time", "instance", id, "last_heartbeat", lastHeartbeat.String(), "forget_period", forgetPeriod) ringDesc.RemoveIngester(id) } } }

yeya24 · 2025-01-28T19:08:13Z

pkg/ring/lifecycler.go

@@ -601,6 +624,9 @@ func (i *Lifecycler) heartbeat(ctx context.Context) {
 	i.lifecyclerMetrics.consulHeartbeats.Inc()
 	ctx, cancel := context.WithTimeout(ctx, i.cfg.HeartbeatPeriod)
 	defer cancel()
+	if i.delegate != nil {


Instead of explicitly checking i.deletegate != nil, can we define a noopDelegate as the default which does nothing?

yeya24 · 2025-01-28T19:10:25Z

pkg/ring/lifecycler.go

+type LifecyclerDelegate interface {
+	// OnRingInstanceHeartbeat is called while the instance is updating its heartbeat
+	// in the ring.
+	OnRingInstanceHeartbeat(lifecycler *Lifecycler, ctx context.Context)


I'd love to see that the same function parameter to be reused. The current interface is a bit weird and we are not using the lifecycler at all in the compactor implementation

yeya24

Btw I think you need a rebase. It contains some commits from your previous changes

yeya24 · 2025-01-28T19:14:18Z

CHANGELOG.md

@@ -67,6 +67,7 @@
 * [ENHANCEMENT] Added metric name in limiter per-metric exceeded errors. #6416
 * [ENHANCEMENT] StoreGateway: Added `cortex_bucket_store_indexheader_load_duration_seconds` and `cortex_bucket_store_indexheader_download_duration_seconds` metrics for time of downloading and loading index header files. #6445
 * [ENHANCEMENT] Blocks Storage: Allow use of non-dualstack endpoints for S3 blocks storage via `-blocks-storage.s3.disable-dualstack`. #6522
+* [ENHANCEMENT] Add `auto_forget_unhealthy_periods` for ring to auto forget unhealthy compactors. #6533


This needs to add under ## master / unreleased

danielblando · 2025-01-28T22:01:07Z

pkg/compactor/lifecycle.go

+	"github.com/cortexproject/cortex/pkg/ring"
+)
+
+func (c *Compactor) OnRingInstanceHeartbeat(_ *ring.Lifecycler, ctx context.Context) {


I would do similar to the basicLifecycler.

Create the generic delegate for the ring and make compactor use it.

danielblando · 2025-01-28T22:03:13Z

pkg/ring/ring.go

@@ -655,6 +659,43 @@ func (r *Ring) countTokens() (map[string]uint32, map[string]int64) {
 	return numTokens, owned
 }

+// autoForgetUnhealthy forgets unhealthy ingesters from the ring.
+func (r *Ring) AutoForgetUnhealthy(ctx context.Context) {


+1

it should be similar to how

cortex/pkg/ring/basic_lifecycler_delegates.go

Line 143 in de5cfe1

func (d *AutoForgetDelegate) OnRingInstanceHeartbeat(lifecycler *BasicLifecycler, ringDesc *Desc, instanceDesc *InstanceDesc) {

works

…LifecyclerDelegate Signed-off-by: Essam Eldaly <eeldaly@amazon.com>

Signed-off-by: Essam Eldaly <eeldaly@amazon.com>

yeya24 · 2025-01-30T23:00:41Z

pkg/ring/lifecycler.go

@@ -972,6 +1001,7 @@ func (i *Lifecycler) updateConsul(ctx context.Context) error {
 			instanceDesc.Zone = i.Zone
 			instanceDesc.RegisteredTimestamp = i.getRegisteredAt().Unix()
 			ringDesc.Ingesters[i.ID] = instanceDesc
+			i.delegate.OnRingInstanceHeartbeat(i, ringDesc)


We should move this line outside the else block. We should call OnRingInstanceHeartbeat no matter the instance exists in the ring or not. Same behavior as UpdateInstance in the basic lifecycler

yeya24 · 2025-01-30T23:01:23Z

pkg/ring/lifecycler_delegates.go

+
+func (d *LifecyclerAutoForgetDelegate) OnRingInstanceHeartbeat(lifecycler *Lifecycler, ringDesc *Desc) {
+	AutoForgetFromRing(ringDesc, d.forgetPeriod, d.logger)
+	d.next.OnRingInstanceHeartbeat(lifecycler, ringDesc)


As we know there is not really a next delegate doing anything, I feel we can safely remove next here. I don't see a usecase to really chain this here for now.

I would leave this next here @yeya24
It will be painful if someone add a new delegate and try to debug why it is not working. Easier just follow the pattern. We can refactor all these delegates when we are joining both lifecycler into to one

danielblando

Can you add a test in compactor for it?

danielblando · 2025-01-30T23:39:23Z

pkg/compactor/compactor.go

@@ -333,6 +334,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.BoolVar(&cfg.SkipBlocksWithOutOfOrderChunksEnabled, "compactor.skip-blocks-with-out-of-order-chunks-enabled", false, "When enabled, mark blocks containing index with out-of-order chunks for no compact instead of halting the compaction.")
 	f.IntVar(&cfg.BlockFilesConcurrency, "compactor.block-files-concurrency", 10, "Number of goroutines to use when fetching/uploading block files from object storage.")
 	f.IntVar(&cfg.BlocksFetchConcurrency, "compactor.blocks-fetch-concurrency", 3, "Number of goroutines to use when fetching blocks from object storage when compacting.")
+	f.DurationVar(&cfg.AutoForgetDelay, "compactor.auto-forget-delay", 15*time.Minute, "Time since last heartbeat before compactor will be removed from ring. 0 to disable")


In ruler, alertmanager and sg, we dont have a config for this. We usually just do 2x heartbeat timeout.

I am fine adding a new config, but lets add the default as 2*heartbeat_timeout.

danielblando · 2025-01-30T23:49:28Z

pkg/ring/lifecycler_delegates.go

+
+func (d *LifecyclerAutoForgetDelegate) OnRingInstanceHeartbeat(lifecycler *Lifecycler, ringDesc *Desc) {
+	AutoForgetFromRing(ringDesc, d.forgetPeriod, d.logger)
+	d.next.OnRingInstanceHeartbeat(lifecycler, ringDesc)


I would leave this next here @yeya24
It will be painful if someone add a new delegate and try to debug why it is not working. Easier just follow the pattern. We can refactor all these delegates when we are joining both lifecycler into to one

eeldaly and others added 6 commits December 20, 2024 13:45

Add metric name in limiter per-metric exceeded errors

67ac0ba

Signed-off-by: Essam Eldaly <eeldaly@amazon.com>

Remove unused labels from errors

c7a4f68

Signed-off-by: Essam Eldaly <eeldaly@amazon.com>

fmt

fc66e3d

Signed-off-by: Essam Eldaly <eeldaly@amazon.com>

Merge branch 'master' into master

581310d

Signed-off-by: Ben Ye <benye@amazon.com>

Merge branch 'cortexproject:master' into master

6dca9e9

Add for ring to auto forget unhealthy compactors

0a90a81

Signed-off-by: Essam Eldaly <eeldaly@amazon.com>

pull-request-size bot added the size/L label Jan 28, 2025

dosubot bot added component/compactor component/ring labels Jan 28, 2025

yeya24 reviewed Jan 28, 2025

View reviewed changes

danielblando reviewed Jan 28, 2025

View reviewed changes

eeldaly added 3 commits January 30, 2025 14:42

Changed LifecyclerDelegate implementation to more closely match Basic…

788fcba

…LifecyclerDelegate Signed-off-by: Essam Eldaly <eeldaly@amazon.com>

Changelog fix

8262457

Signed-off-by: Essam Eldaly <eeldaly@amazon.com>

Changelog fix

6a187fd

Signed-off-by: Essam Eldaly <eeldaly@amazon.com>

yeya24 reviewed Jan 30, 2025

View reviewed changes

danielblando reviewed Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add compactor auto-forget from ring on unhealthy #6563

Add compactor auto-forget from ring on unhealthy #6563

eeldaly commented Jan 28, 2025 •

edited

Loading

yeya24 Jan 28, 2025

yeya24 Jan 28, 2025

danielblando Jan 28, 2025

yeya24 Jan 28, 2025

yeya24 Jan 28, 2025

yeya24 Jan 28, 2025

yeya24 left a comment

yeya24 Jan 28, 2025

danielblando Jan 28, 2025

danielblando Jan 28, 2025

yeya24 Jan 30, 2025 •

edited

Loading

yeya24 Jan 30, 2025

danielblando Jan 30, 2025

danielblando left a comment

danielblando Jan 30, 2025

danielblando Jan 30, 2025

Add compactor auto-forget from ring on unhealthy #6563

Are you sure you want to change the base?

Add compactor auto-forget from ring on unhealthy #6563

Conversation

eeldaly commented Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeya24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeya24 Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielblando left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eeldaly commented Jan 28, 2025 •

edited

Loading

yeya24 Jan 30, 2025 •

edited

Loading