Fix Ingesters unable to re-join the cluster then `unregister_on_shutdown=false` + `-ingester.heartbeat-period=0` #4366

alanprot · 2021-07-15T22:37:31Z

What this PR does:

Address problem state on #4365

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

Checklist

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

CHANGELOG.md

pkg/ring/lifecycler.go

pracucci · 2021-07-16T07:24:13Z

pkg/ring/lifecycler.go

@@ -588,6 +589,13 @@ func (i *Lifecycler) initRing(ctx context.Context) error {
 		i.setTokens(tokens)

 		level.Info(log.Logger).Log("msg", "existing entry found in ring", "state", i.GetState(), "tokens", len(tokens), "ring", i.RingName)
+
+		// If we flip the instance from LEAVING to ACTIVE and the heartbeat is disabled, we need to update KV here
+		if i.cfg.HeartbeatPeriod == 0 && ringDesc.Ingesters[i.ID].State == LEAVING && instanceDesc.State == ACTIVE {


Question: are we going to hit the issue #4365 in the case the ingester crashes while in the PENDING or JOINING state in the ring and then restarts? Could you add unit tests to check that too, please?

Sure..

But i think we are fine there as..

if instanceDesc.State == JOINING { level.Warn(log.Logger).Log("msg", "instance found in ring as JOINING, setting to PENDING", "ring", i.RingName) instanceDesc.State = PENDING return ringDesc, true, nil }

We will still hit the issue if the number of tokens change though... but this is already the case now

alanprot · 2021-07-20T16:06:12Z

Hi @pracucci

Did u had a change to see the updated change?

bboreham

The change looks reasonable, but I got lost in the test.
I made a few comments, but I think I'm not getting to the crux of what it is doing.
E.g. how many ingesters are there supposed to be?

pkg/ring/lifecycler.go

bboreham · 2021-07-29T08:49:31Z

pkg/ring/lifecycler_test.go

+	require.NoError(t, err)
+	require.NoError(t, services.StartAndAwaitRunning(context.Background(), l2))
+
+	pool := func(condition func(*Desc) bool) map[string]InstanceDesc {


I didn't get what "pool" means here. Can you pick a more descriptive name, or add a comment saying what this function does?

It should be poll instead of pool :P
This function is basically waiting for a condition and returning actual state of the ingesters after the condition succeed.

pkg/ring/lifecycler_test.go

bboreham · 2021-07-29T08:53:02Z

pkg/ring/lifecycler_test.go

+		return ingesters
+	}
+
+	startIngesterAndWaitActive := func(lcConfig LifecyclerConfig) *Lifecycler {


This function starts one ingester then also waits for two more to be active?
Since starting one ingester is repeated many times, could that be a separate function from the one that waits for two to be active?

This function does not need to check the ingester1. I did that just in case... removing it.

But basically this function does what you stated: "starts ingester2 and wait it to became active."

pkg/ring/lifecycler_test.go

pkg/ring/lifecycler.go

…tdown=false Signed-off-by: Alan Protasio <approtas@amazon.com>

Co-authored-by: Marco Pracucci <marco@pracucci.com> Signed-off-by: Alan Protasio <approtas@amazon.com>

Co-authored-by: Bryan Boreham <bjboreham@gmail.com> Signed-off-by: Alan Protasio <approtas@amazon.com>

Signed-off-by: Alan Protasio <approtas@amazon.com>

bboreham

OK I think I get it now, but a bit more consistency in naming and usage would improve clarity in the test.

pkg/ring/lifecycler_test.go

bboreham · 2021-08-02T08:19:08Z

pkg/ring/lifecycler_test.go

+
+	// Starts Ingester2 and wait it to became active
+	startIngester2AndWaitActive := func(lcConfig LifecyclerConfig) *Lifecycler {
+		ingester, err := NewLifecycler(lcConfig, &noopFlushTransferer{}, "ingester", IngesterRingKey, true, nil)


Why do we create a new Lifecycler here? What happens to the old one in l2?

And why is it named ingester ? Nearby we have a variable ingesters which is a map[string]InstanceDesc.

Why do we create a new Lifecycler here? What happens to the old one in l2?

I think i tried to reuse it and it did not work.. but besides that, In order to simulated a brand new ingester i would assume that creating a new instance is the safest thing to do. make sense?

pkg/ring/lifecycler_test.go

bboreham · 2021-08-02T08:26:39Z

pkg/ring/lifecycler_test.go

+	}
+
+	// Starts Ingester2 and wait it to became active
+	startIngester2AndWaitActive := func(lcConfig LifecyclerConfig) *Lifecycler {


There's nothing specific to Ingester 2 in this function, is there?
That is a consequence of passing LifecyclerConfig with ID set to "ing2", I think.

Signed-off-by: Alan Protasio <approtas@amazon.com>

bboreham

LGTM - sorry it took a while but I think the flow of the test is much clearer now.
Thanks!

pracucci

Good job, LGTM!

alanprot · 2021-08-03T23:51:46Z

Thanks guys! 😄

…own=false` + `-ingester.heartbeat-period=0` (cortexproject#4366) * Fix problem when ingester heartbeat is disabled and unregister_on_shutdown=false Signed-off-by: Alan Protasio <approtas@amazon.com> * Update CHANGELOG.md Co-authored-by: Marco Pracucci <marco@pracucci.com> Signed-off-by: Alan Protasio <approtas@amazon.com> * Update pkg/ring/lifecycler.go Co-authored-by: Bryan Boreham <bjboreham@gmail.com> Signed-off-by: Alan Protasio <approtas@amazon.com> * Update pkg/ring/lifecycler_test.go Co-authored-by: Bryan Boreham <bjboreham@gmail.com> Signed-off-by: Alan Protasio <approtas@amazon.com> * Address comments Signed-off-by: Alan Protasio <approtas@amazon.com> * Addressing comments on test Signed-off-by: Alan Protasio <approtas@amazon.com> Co-authored-by: Marco Pracucci <marco@pracucci.com> Co-authored-by: Bryan Boreham <bjboreham@gmail.com> Signed-off-by: Alvin Lin <alvinlin@amazon.com>

alanprot force-pushed the 4365 branch from f12c9d8 to 0bd04aa Compare July 15, 2021 22:38

alanprot changed the title ~~unregister_on_shutdown=false + -ingester.heartbeat-period=0 seems to not be working~~ Fix Ingesters unable to re-join the cluster then unregister_on_shutdown=false + -ingester.heartbeat-period=0 Jul 16, 2021

pracucci reviewed Jul 16, 2021

View reviewed changes

alanprot force-pushed the 4365 branch 7 times, most recently from 6f69d75 to e27561c Compare July 16, 2021 19:34

bboreham reviewed Jul 30, 2021

View reviewed changes

pull-request-size bot added the size/L label Jul 30, 2021

alanprot force-pushed the 4365 branch 2 times, most recently from 13aa6bf to 4587b67 Compare July 30, 2021 17:07

alanprot and others added 4 commits July 30, 2021 10:10

Fix problem when ingester heartbeat is disabled and unregister_on_shu…

ccdde4e

…tdown=false Signed-off-by: Alan Protasio <approtas@amazon.com>

Update CHANGELOG.md

8eeaae2

Co-authored-by: Marco Pracucci <marco@pracucci.com> Signed-off-by: Alan Protasio <approtas@amazon.com>

Update pkg/ring/lifecycler.go

64cf46a

Co-authored-by: Bryan Boreham <bjboreham@gmail.com> Signed-off-by: Alan Protasio <approtas@amazon.com>

Update pkg/ring/lifecycler_test.go

50402e0

Co-authored-by: Bryan Boreham <bjboreham@gmail.com> Signed-off-by: Alan Protasio <approtas@amazon.com>

alanprot force-pushed the 4365 branch 2 times, most recently from 88353cb to 8111704 Compare July 30, 2021 17:11

Address comments

6092356

Signed-off-by: Alan Protasio <approtas@amazon.com>

alanprot force-pushed the 4365 branch from 8111704 to 6092356 Compare July 30, 2021 17:14

bboreham reviewed Aug 2, 2021

View reviewed changes

Addressing comments on test

f8b484d

Signed-off-by: Alan Protasio <approtas@amazon.com>

alanprot force-pushed the 4365 branch from 06b7b64 to f8b484d Compare August 2, 2021 18:18

bboreham approved these changes Aug 3, 2021

View reviewed changes

pracucci approved these changes Aug 3, 2021

View reviewed changes

pracucci merged commit 4405f9c into cortexproject:master Aug 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Ingesters unable to re-join the cluster then `unregister_on_shutdown=false` + `-ingester.heartbeat-period=0` #4366

Fix Ingesters unable to re-join the cluster then `unregister_on_shutdown=false` + `-ingester.heartbeat-period=0` #4366

alanprot commented Jul 15, 2021

pracucci Jul 16, 2021

alanprot Jul 16, 2021 •

edited

Loading

alanprot commented Jul 20, 2021

bboreham left a comment

bboreham Jul 29, 2021

alanprot Jul 30, 2021

bboreham Jul 29, 2021

alanprot Jul 30, 2021

bboreham left a comment

bboreham Aug 2, 2021

bboreham Aug 2, 2021

alanprot Aug 2, 2021

bboreham Aug 2, 2021

alanprot Aug 2, 2021

bboreham left a comment

pracucci left a comment

alanprot commented Aug 3, 2021

Fix Ingesters unable to re-join the cluster then unregister_on_shutdown=false + -ingester.heartbeat-period=0 #4366

Fix Ingesters unable to re-join the cluster then unregister_on_shutdown=false + -ingester.heartbeat-period=0 #4366

Conversation

alanprot commented Jul 15, 2021

Choose a reason for hiding this comment

alanprot Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

alanprot commented Jul 20, 2021

bboreham 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

Choose a reason for hiding this comment

bboreham 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboreham left a comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

alanprot commented Aug 3, 2021

Fix Ingesters unable to re-join the cluster then `unregister_on_shutdown=false` + `-ingester.heartbeat-period=0` #4366

Fix Ingesters unable to re-join the cluster then `unregister_on_shutdown=false` + `-ingester.heartbeat-period=0` #4366

alanprot Jul 16, 2021 •

edited

Loading