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

maglev: fix maglev stability by sorting host_weights beforehand #28055

Merged
merged 4 commits into from Jun 26, 2023

Conversation

y1r
Copy link
Contributor

@y1r y1r commented Jun 21, 2023

Commit Message:

This PR fixes MaglevLoadBalancer's stability problem reported in #20703. Maglev algorithm prepares hash table by visiting permutations (assignment preference list of each backend) one-by-one, then fill hash table by choosing key from the top of the list. The order of permutation visiting should be stable because this order sightly affects the hash table. In order to make stable the order, I sorted host_weights by key_to_hash value then constructs the hash table.

In order to check the problem is solved or not, I appended BasicStability test to maglev_lb_test.cc. Without changes to maglev_lb.cc, this test will fail.

Additional Description:
Risk Level: Low
Testing: Unit Test
Docs Changes:
Release Notes: loadbalancer: maglev now returns stable key assignment by sorting the backend beforehand
Platform Specific Features:
Fixes #20703

Signed-off-by: Yuichiro Ueno <ueno@preferred.jp>
@y1r y1r requested a review from wbpcode as a code owner June 21, 2023 02:18
@repokitteh-read-only
Copy link

Hi @y1r, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #28055 was opened by y1r.

see: more, trace.

Copy link
Contributor

@KBaichoo KBaichoo left a comment

Choose a reason for hiding this comment

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

Besides small nit this looks great! Thanks for working on this.

@KBaichoo
Copy link
Contributor

/wait

y1r added 2 commits June 23, 2023 07:23
Signed-off-by: Yuichiro Ueno <ueno@preferred.jp>
Signed-off-by: Yuichiro Ueno <ueno@preferred.jp>
@y1r
Copy link
Contributor Author

y1r commented Jun 22, 2023

@KBaichoo Thank you for your review. I also updated the changelog 26cf9fb. Could you take a look at this PR?

@y1r y1r requested a review from KBaichoo June 22, 2023 22:44
@KBaichoo
Copy link
Contributor

/retest

KBaichoo
KBaichoo previously approved these changes Jun 23, 2023
Copy link
Contributor

@KBaichoo KBaichoo left a comment

Choose a reason for hiding this comment

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

I think this is ok without a runtime guard as this would be a very small behavioral change. cc @envoyproxy/runtime-guard-changes

Will merge fix if no objections.

@wbpcode
Copy link
Member

wbpcode commented Jun 25, 2023

need merge main and resolve the conflict. Thanks.

/wait

Signed-off-by: Yuichiro Ueno <ueno@preferred.jp>
@y1r
Copy link
Contributor Author

y1r commented Jun 25, 2023

@wbpcode Thank you for your review. I merged main and resolved the conflict. ef45f35

@KBaichoo KBaichoo enabled auto-merge (squash) June 26, 2023 17:48
@KBaichoo KBaichoo merged commit 08cf7e4 into envoyproxy:main Jun 26, 2023
90 checks passed
@y1r y1r deleted the fix-maglev-stability branch June 27, 2023 00:50
asheryerm pushed a commit to asheryerm/envoy that referenced this pull request Jul 5, 2023
…yproxy#28055)

* maglev: fix maglev stability by sorting host_weights beforehand

Signed-off-by: Yuichiro Ueno <ueno@preferred.jp>
Signed-off-by: asheryer <asheryer@amazon.com>
reskin89 pushed a commit to reskin89/envoy that referenced this pull request Jul 11, 2023
…yproxy#28055)

* maglev: fix maglev stability by sorting host_weights beforehand

Signed-off-by: Yuichiro Ueno <ueno@preferred.jp>
Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MaglevLoadBalancer consistent hashing implementation is NOT stable
3 participants