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: only enable full scan when enable_full_scan is set explicitly forleast request lb #30794

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

wbpcode
Copy link
Member

@wbpcode wbpcode commented Nov 9, 2023

Commit Message: fix: only enable full scan when enable_full_scan is set explicitly forleast request lb
Additional Description:

#29873 introduced a minor breaking change. The full scan will be enabled automatically when host num is less than choice count.
Although from technical point, I think this change basically harmless and could make a better load balancing result, a new runtime guard should be enough.

But in practice, it effects our downstream users in a unexpected way. See #30768 (comment)

No change log is updated or added because our change log or doc doesn't mention that full scan will be enabled automatically when host num less than choice count.

Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@wbpcode
Copy link
Member Author

wbpcode commented Nov 9, 2023

cc @lizan could you take a look as a senior maintainer when you have free time? Thanks.

@wbpcode
Copy link
Member Author

wbpcode commented Nov 9, 2023

cc @barroca

@lizan lizan enabled auto-merge (squash) November 9, 2023 05:29
@lizan lizan merged commit e93e556 into envoyproxy:main Nov 9, 2023
102 of 104 checks passed
@wbpcode wbpcode deleted the dev-least-request-minor-revert branch November 9, 2023 06:07
@ggreenway
Copy link
Contributor

@barroca part of your change had to be reverted due to a regression. I think this can be fixed by, during the full scan, picking a random host from those that have the lowest rq count, instead of the first host with that count. Would you be willing to make that fix?

kyessenov added a commit to kyessenov/envoy that referenced this pull request Nov 9, 2023
…citly forleast request lb (envoyproxy#30794)"

This reverts commit e93e556.

Revert "Fix least request lb not fair (envoyproxy#29873)"

This reverts commit 3ea2bc4.

restore api

Signed-off-by: Kuat Yessenov <[email protected]>

fix merge

Signed-off-by: Kuat Yessenov <[email protected]>
@tomwans
Copy link
Contributor

tomwans commented Nov 10, 2023

@barroca part of your change had to be reverted due to a regression. I think this can be fixed by, during the full scan, picking a random host from those that have the lowest rq count, instead of the first host with that count. Would you be willing to make that fix?

To be explicit here: if active_rq is equal across all hosts, and the host list is in the same order for all callers, you can end up picking the same host in the list every time if active_rq is the same for all of them. This can happen if you have a Envoy cluster that is rarely called but responds with low latency, so active_rq tends to be zero across all hosts. Maybe we start the for loop from a random index when doing the full scan, and that should alleviate this issue as well?

kyessenov added a commit that referenced this pull request Dec 9, 2023
Commit Message: Reverts #29873 and #30794

Multiple concerns about the effect of a full scan on LEAST_REQUEST have been raised.
See previous discussions in #11004 and #11006.

Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
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.

5 participants