From e14b66d197cb98f58bd92687cc6f76dd29bf1207 Mon Sep 17 00:00:00 2001 From: Charlie Le Date: Tue, 15 Oct 2024 18:21:22 -0700 Subject: [PATCH 1/4] update ring with new ip address when instance is lost, rejoins, but heartbeat is disabled Signed-off-by: Charlie Le --- pkg/ring/lifecycler.go | 7 +++++++ pkg/ring/lifecycler_test.go | 41 ++++++++++++++++++++++++++++++------- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/pkg/ring/lifecycler.go b/pkg/ring/lifecycler.go index e9b9970e1a..d954680c31 100644 --- a/pkg/ring/lifecycler.go +++ b/pkg/ring/lifecycler.go @@ -696,6 +696,13 @@ func (i *Lifecycler) initRing(ctx context.Context) error { return ringDesc, true, nil } + if i.cfg.HeartbeatPeriod == 0 && i.Addr != instanceDesc.Addr { + // Update the address if it has changed + instanceDesc.Addr = i.Addr + ringDesc.Ingesters[i.ID] = instanceDesc + return ringDesc, true, nil + } + // we haven't modified the ring, don't try to store it. return nil, true, nil }) diff --git a/pkg/ring/lifecycler_test.go b/pkg/ring/lifecycler_test.go index 0b8a6402db..1786d6f7eb 100644 --- a/pkg/ring/lifecycler_test.go +++ b/pkg/ring/lifecycler_test.go @@ -40,6 +40,14 @@ func testLifecyclerConfig(ringConfig Config, id string) LifecyclerConfig { return lifecyclerConfig } +// testLifecyclerConfigWithAddr creates a LifecyclerConfig with the given address. +// This is useful for testing when we want to set the address to a specific value. +func testLifecyclerConfigWithAddr(ringConfig Config, id string, addr string) LifecyclerConfig { + l := testLifecyclerConfig(ringConfig, id) + l.Addr = addr + return l +} + func checkNormalised(d interface{}, id string) bool { desc, ok := d.(*Desc) return ok && @@ -644,8 +652,8 @@ func TestRestartIngester_DisabledHeartbeat_unregister_on_shutdown_false(t *testi } // Starts Ingester and wait it to became active - startIngesterAndWaitActive := func(ingId string) *Lifecycler { - lifecyclerConfig := testLifecyclerConfig(ringConfig, ingId) + startIngesterAndWaitActive := func(ingId string, addr string) *Lifecycler { + lifecyclerConfig := testLifecyclerConfigWithAddr(ringConfig, ingId, addr) // Disabling heartBeat and unregister_on_shutdown lifecyclerConfig.UnregisterOnShutdown = false lifecyclerConfig.HeartbeatPeriod = 0 @@ -662,10 +670,10 @@ func TestRestartIngester_DisabledHeartbeat_unregister_on_shutdown_false(t *testi // test if the ingester 2 became active after: // * Clean Shutdown (LEAVING after shutdown) // * Crashes while in the PENDING or JOINING state - l1 := startIngesterAndWaitActive("ing1") + l1 := startIngesterAndWaitActive("ing1", "0.0.0.0") defer services.StopAndAwaitTerminated(context.Background(), l1) //nolint:errcheck - l2 := startIngesterAndWaitActive("ing2") + l2 := startIngesterAndWaitActive("ing2", "0.0.0.0") ingesters := poll(func(desc *Desc) bool { return len(desc.Ingesters) == 2 && desc.Ingesters["ing1"].State == ACTIVE && desc.Ingesters["ing2"].State == ACTIVE @@ -684,7 +692,7 @@ func TestRestartIngester_DisabledHeartbeat_unregister_on_shutdown_false(t *testi assert.Equal(t, LEAVING, ingesters["ing2"].State) // Start Ingester2 again - Should flip back to ACTIVE in the ring - l2 = startIngesterAndWaitActive("ing2") + l2 = startIngesterAndWaitActive("ing2", "0.0.0.0") require.NoError(t, services.StopAndAwaitTerminated(context.Background(), l2)) // Simulate ingester2 crash on startup and left the ring with JOINING state @@ -698,7 +706,7 @@ func TestRestartIngester_DisabledHeartbeat_unregister_on_shutdown_false(t *testi }) require.NoError(t, err) - l2 = startIngesterAndWaitActive("ing2") + l2 = startIngesterAndWaitActive("ing2", "0.0.0.0") require.NoError(t, services.StopAndAwaitTerminated(context.Background(), l2)) // Simulate ingester2 crash on startup and left the ring with PENDING state @@ -712,7 +720,26 @@ func TestRestartIngester_DisabledHeartbeat_unregister_on_shutdown_false(t *testi }) require.NoError(t, err) - l2 = startIngesterAndWaitActive("ing2") + l2 = startIngesterAndWaitActive("ing2", "0.0.0.0") + require.NoError(t, services.StopAndAwaitTerminated(context.Background(), l2)) + + // Simulate ingester2 crashing and left the ring with ACTIVE state, but when it comes up + // it has a different ip address + l2 = startIngesterAndWaitActive("ing2", "0.0.0.0") + ingesters = poll(func(desc *Desc) bool { + return desc.Ingesters["ing2"].State == ACTIVE && desc.Ingesters["ing2"].Addr == "0.0.0.0:1" + }) + assert.Equal(t, ACTIVE, ingesters["ing2"].State) + assert.Equal(t, "0.0.0.0:1", ingesters["ing2"].Addr) + + l2 = startIngesterAndWaitActive("ing2", "1.1.1.1") + + // The ring should have the new ip address + ingesters = poll(func(desc *Desc) bool { + return desc.Ingesters["ing2"].State == ACTIVE && desc.Ingesters["ing2"].Addr == "1.1.1.1:1" + }) + assert.Equal(t, ACTIVE, ingesters["ing2"].State) + assert.Equal(t, "1.1.1.1:1", ingesters["ing2"].Addr) require.NoError(t, services.StopAndAwaitTerminated(context.Background(), l2)) } From 86d0e5a77c7454ab85b6556e3c8efb2e4ccbd98b Mon Sep 17 00:00:00 2001 From: Charlie Le Date: Wed, 16 Oct 2024 10:45:29 -0700 Subject: [PATCH 2/4] Fix lint error (ineffassign) ``` pkg/ring/lifecycler_test.go:728:2: ineffectual assignment to l2 (ineffassign) l2 = startIngesterAndWaitActive("ing2", "0.0.0.0") ^ ``` Signed-off-by: Charlie Le --- pkg/ring/lifecycler_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/ring/lifecycler_test.go b/pkg/ring/lifecycler_test.go index 1786d6f7eb..b121ef258a 100644 --- a/pkg/ring/lifecycler_test.go +++ b/pkg/ring/lifecycler_test.go @@ -725,7 +725,7 @@ func TestRestartIngester_DisabledHeartbeat_unregister_on_shutdown_false(t *testi // Simulate ingester2 crashing and left the ring with ACTIVE state, but when it comes up // it has a different ip address - l2 = startIngesterAndWaitActive("ing2", "0.0.0.0") + startIngesterAndWaitActive("ing2", "0.0.0.0") ingesters = poll(func(desc *Desc) bool { return desc.Ingesters["ing2"].State == ACTIVE && desc.Ingesters["ing2"].Addr == "0.0.0.0:1" }) From 90a4c677e76601428e056380e5669c46d340ecd0 Mon Sep 17 00:00:00 2001 From: Charlie Le Date: Wed, 16 Oct 2024 10:48:57 -0700 Subject: [PATCH 3/4] Update changelog Signed-off-by: Charlie Le --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03a2ddf904..3a6528a97e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ * [ENHANCEMENT] Ingester/Store Gateway Clients: Introduce an experimental HealthCheck handler to quickly fail requests directed to unhealthy targets. #6225 #6257 * [ENHANCEMENT] Upgrade build image and Go version to 1.23.2. #6261 #6262 * [BUGFIX] Runtime-config: Handle absolute file paths when working directory is not / #6224 +* [BUGFIX] Ring: update ring with new ip address when instance is lost, rejoins, but heartbeat is disabled #6271 ## 1.18.0 2024-09-03 From f1f41ffc224baa7f78c017b1509aee381d3a91b2 Mon Sep 17 00:00:00 2001 From: Charlie Le Date: Tue, 19 Nov 2024 18:54:47 -0800 Subject: [PATCH 4/4] simplify logic for handling address change applying feedback from alanprot Signed-off-by: Charlie Le --- pkg/ring/lifecycler.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/pkg/ring/lifecycler.go b/pkg/ring/lifecycler.go index d954680c31..fedf90a259 100644 --- a/pkg/ring/lifecycler.go +++ b/pkg/ring/lifecycler.go @@ -686,6 +686,9 @@ func (i *Lifecycler) initRing(ctx context.Context) error { level.Info(i.logger).Log("msg", "existing entry found in ring", "state", i.GetState(), "tokens", len(tokens), "ring", i.RingName) + // Update the address if it has changed + instanceDesc.Addr = i.Addr + // Update the ring if the instance has been changed and the heartbeat is disabled. // We dont need to update KV here when heartbeat is enabled as this info will eventually be update on KV // on the next heartbeat @@ -696,13 +699,6 @@ func (i *Lifecycler) initRing(ctx context.Context) error { return ringDesc, true, nil } - if i.cfg.HeartbeatPeriod == 0 && i.Addr != instanceDesc.Addr { - // Update the address if it has changed - instanceDesc.Addr = i.Addr - ringDesc.Ingesters[i.ID] = instanceDesc - return ringDesc, true, nil - } - // we haven't modified the ring, don't try to store it. return nil, true, nil })