Skip to content

update ring with new ip address when instance is lost, rejoins, but heartbeat is disabled #6271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

CharlieTLe
Copy link
Member

@CharlieTLe CharlieTLe commented Oct 16, 2024

What this PR does:
Updates the ring with new ip address when instance is lost, rejoins, but heartbeat is disabled.

An instance can become lost, for example, when it does not announce that it is leaving the ring before exiting. When this happens and a new instance is created with a new ip address but the same name, the instance will rejoin the ring and reclaim the tokens it once had. However, it will not update the ring with the new ip address it can be located at. This can cause problems for services that use the ring to find where it can reach a member of the ring.

Which issue(s) this PR fixes:
Fixes #

Checklist

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

Signed-off-by: Charlie Le <charlie_le@apple.com>
```
pkg/ring/lifecycler_test.go:728:2: ineffectual assignment to l2 (ineffassign)
	l2 = startIngesterAndWaitActive("ing2", "0.0.0.0")
	^
```

Signed-off-by: Charlie Le <charlie_le@apple.com>
Signed-off-by: Charlie Le <charlie_le@apple.com>
@yeya24
Copy link
Contributor

yeya24 commented Oct 16, 2024

@CharlieTLe Is there a specific use case or component you are looking at for this feature?
We can persist tokens to a file for instances to re-join the ring with the same token by reusing the token file (stored in a PVC)

@CharlieTLe
Copy link
Member Author

@yeya24

Yeah, it's to handle the case where an ingester changes its IP address but doesn't go through the typical lifecycle of leaving the ring while doing so. Then there's a discrepancy between the ring description and the actual description of the ingester.

This specific issue isn't about reclaiming the tokens.

Here's what can happen:

  1. An ingester (id=ingester-0, addr=1.1.1.1) has a heartbeat_period=0 and is in the memberlist ring with the state=ACTIVE
  2. The node that the ingester is running on is abruptly terminated
  3. A new node is created for the ingester (id=ingester-0, addr=2.2.2.2) to run on
  4. The ingester (id=ingester-0, addr=2.2.2.2) joins the ring and reclaims its token
  5. The ring description still using the ingester's old address information (id=ingester=0, addr=1.1.1.1)

A symptom of the problem would be on the distributor logging the error that it is unable to reach the ingester because it's using the ingester's old address:

{
  "addr": "1.1.1.1:9095",
  "caller": "pool.go:184",
  "level": "warn",
  "msg": "removing ingester failing healthcheck",
  "reason": "rpc error: code = DeadlineExceeded desc = context deadline exceeded"
}
@yeya24 yeya24 requested a review from alanprot October 25, 2024 05:45
Copy link
Member

@alanprot alanprot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This make sense to me. Just a small comment but overall it make sense.

CharlieTLe and others added 2 commits November 19, 2024 18:54
applying feedback from alanprot

Signed-off-by: Charlie Le <charlie_le@apple.com>
…-new-ip-addr

Signed-off-by: Charlie Le <3375195+CharlieTLe@users.noreply.github.com>
Copy link
Member

@alanprot alanprot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge on green

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 21, 2024
@alanprot alanprot merged commit 371b2af into cortexproject:master Nov 21, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/ring lgtm This PR has been approved by a maintainer size/M
3 participants