Skip to content
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

Fix ingesters with less tokens stuck in LEAVING #5061

Merged
merged 4 commits into from
Jan 9, 2023

Conversation

marianafranco
Copy link
Contributor

@marianafranco marianafranco commented Dec 23, 2022

Signed-off-by: Mariana Franco marfram@amazon.com

What this PR does:

It's possible to have ingesters in the ring with less tokens than configured because of conflict resolution:

cortex/pkg/ring/model.go

Lines 354 to 361 in f930c7a

// This function resolves token conflicts, if there are any.
//
// We deal with two possibilities:
// 1) if one node is LEAVING or LEFT and the other node is not, LEVING/LEFT one loses the token
// 2) otherwise node names are compared, and node with "lower" name wins the token
//
// Modifies ingesters map with updated tokens.
func resolveConflicts(normalizedIngesters map[string]InstanceDesc) {

In this case, the ingester will get stuck in LEAVING when they are evicted and rescheduled.

Looking at previous changes, seems that there was not a good reason to check for NumTokens: #1921 (comment)

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]

Signed-off-by: Mariana Franco <marfram@amazon.com>
Signed-off-by: Mariana Franco <marfram@amazon.com>
Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.

I also notice that function is not completely tested with unit tests

Screen Shot 2022-12-27 at 11 30 10 AM

pkg/ring/lifecycler_test.go Show resolved Hide resolved
@marianafranco
Copy link
Contributor Author

I also notice that function is not completely tested with unit tests

Sorry, I don't have time to improve these unit tests now. I can create an issue if you want. Hopefully other contributors can help to improve the coverage.

Signed-off-by: Mariana Franco <marfram@amazon.com>
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jan 3, 2023
Signed-off-by: Friedrich Gonzalez <friedrichg@gmail.com>
Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

Thanks!

@marianafranco
Copy link
Contributor Author

@alanprot Could you take a look?

@alanprot alanprot merged commit 0d4cd74 into cortexproject:master Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants