Fix least request lb not fair by barroca · Pull Request #29873 · envoyproxy/envoy

barroca · 2023-09-29T14:19:07Z

Commit Message: Fix Least requests LB when doing a random pick so it removes already chosen hosts from the random function to remove the chance of selecting the same host again when dealing with a small amount of hosts. Also, when the number of choices is smaller or equal the number of hosts, use full scan of least used instead.

Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
This Release changes the default behaviour of Least Request Load Balancer doing a full scan when the number of choices is more than equal the size of hosts and also adds a new option on the envoy::extensions::load_balancing_policies::least_request::v3::LeastRequest configuration to always do a full scan. Allowing a full scan instead of random making choices reduces the chance of selecting a host that doesn't have least requests when the number of hosts is smaller.

Platform Specific Features:
[Optional Runtime guard:]
Fixes #11004
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Leonardo da Mata <ldamata@spotify.com>

repokitteh-read-only · 2023-09-29T14:19:12Z

Hi @barroca, 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: #29873 was opened by barroca.

see: more, trace.

repokitteh-read-only · 2023-09-29T14:19:19Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #29873 was opened by barroca.

see: more, trace.

Signed-off-by: Leonardo da Mata <ldamata@spotify.com>

wbpcode · 2023-09-30T02:28:10Z

/assign

wbpcode

Thanks for your contribution. And some comments are added.

And at least a release note is necessary to tell what this PR changed or fixed. You can add a new change log entry in this file. https://github.com/envoyproxy/envoy/blob/main/changelogs/current.yaml

api/envoy/config/cluster/v3/cluster.proto

source/common/upstream/load_balancer_impl.cc

source/common/upstream/load_balancer_impl.h

wbpcode · 2023-10-03T09:04:31Z

test/common/upstream/load_balancer_impl_test.cc

-    EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(2)).WillOnce(Return(3));
+    EXPECT_CALL(random_, random()).WillOnce(Return(9999));
    EXPECT_EQ(hostSet().healthy_hosts_[0], lb_.chooseHost(nullptr));
  }

  // Host weight is 100.
  {
-    EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(2)).WillOnce(Return(3));
+    EXPECT_CALL(random_, random()).WillOnce(Return(9999));
    EXPECT_EQ(hostSet().healthy_hosts_[0], lb_.chooseHost(nullptr));
  }

  HostVector empty;
  {
    hostSet().runCallbacks(empty, empty);
-    EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(2)).WillOnce(Return(3));
+    EXPECT_CALL(random_, random()).WillOnce(Return(9999));


I didn't get why these change is necessary 🤔

Doesn't make sense to change to 9999, but the number of calls for random have changed since we will call full scan instead.

api/envoy/extensions/load_balancing_policies/least_request/v3/least_request.proto

wbpcode · 2023-10-03T09:12:50Z

api/envoy/extensions/load_balancing_policies/least_request/v3/least_request.proto

  // The number of random healthy hosts from which the host with the fewest active requests will
  // be chosen. Defaults to 2 so that we perform two-choice selection if the field is not set.


This PR change the behavior of the LB. It's necessary to add some more comment to tell that if the choice_count is larger than or equal to the hosts size, a full scan will be used.

This behavior change should be harmless to end users and maybe make this LB's behavior closer to it's name least request.
So, I think a change log should enough rather than a runtime guard.

But still need a check from @envoyproxy/runtime-guard-changes

wbpcode · 2023-10-12T08:37:55Z

/wait

…number of choices is larger than the size. Signed-off-by: Leonardo da Mata <ldamata@spotify.com>

lizan · 2023-10-17T18:47:33Z

/lgtm api

defer to @wbpcode

wbpcode · 2023-10-19T08:56:04Z

source/common/upstream/load_balancer_impl.h

-                : absl::nullopt) {
+                : absl::nullopt),
+        enable_full_scan_(
+            PROTOBUF_GET_WRAPPED_OR_DEFAULT(least_request_config.ref(), enable_full_scan, false)) {


I'm not 100% sure how to do this on the code, when removing the field from here I was having problems compiling the load_balancer_impl.* files. I would appreciate help on this.

Just keep this enable_full_scan_ always be false if legacy API is used. Then I think this will resolve the compiling problem.

/wait

Signed-off-by: Leonardo da Mata <ldamata@spotify.com>

wbpcode

This LGTM overall. Please check the CI and add a release note, thanks.

wbpcode · 2023-10-20T02:50:43Z

source/common/upstream/load_balancer_impl.h

  double active_request_bias_{};

  const absl::optional<Runtime::Double> active_request_bias_runtime_;
+  const bool enable_full_scan_;


nit: const bool enable_full_scan_{};

wbpcode · 2023-10-20T02:52:17Z

/wait

Signed-off-by: Leonardo da Mata <ldamata@spotify.com>

wbpcode

LGTM. Thanks.

Signed-off-by: Leonardo da Mata <barroca@gmail.com>

barroca · 2023-10-25T17:15:06Z

Not sure If I need someone else to approve since "envoyproxy/api-shepherds must approve for any API change" is pending.

Signed-off-by: Leonardo da Mata <barroca@gmail.com>

barroca · 2023-10-26T12:56:20Z

perhaps @lizan needs to take a look again?

wbpcode

LGTM. Thanks.

…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 <kuat@google.com> fix merge Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2023-11-09T21:13:52Z

We are reverting this change since we noticed that there is a significant impact on clusters with a small number of hosts caused by the deterministic loop which makes every client pick the same hosts. I think we need a better understanding of the impact with a load simulation for this change to be re-applied. Please consider adding some independent sampling to the algorithm and we can do a more thorough review again.

barroca · 2023-11-10T13:53:38Z

I understand that changing the behaviour would have impacted people, but It is not clear to me that adding an option to enable full scan would have a bad impact since it is explicitly selecting the host with least requests. Wondering if we can add it back?

Thanks @kyessenov and @wbpcode for fixing. I wasn't aware that I had breaking issues . :)

wbpcode · 2023-11-10T14:17:16Z

The API will be kept and new implementation is still welcome. And @tonya11en is working a simulated system to ensure we can get a more reasonable implementation in the future.

Thanks for your contribution and so sorry for that we need to revert it. We know it taken you lots of time. :(

Hope we can bring it back soon.

tonya11en · 2023-11-10T17:51:21Z

If the patch is limited to only the “full scan” flag, it should do what you want without affecting the existing selection behavior.

…

On Fri, Nov 10, 2023, at 6:17 AM, code wrote: The API will be kept and new implementation is still welcome. And @tonya11en <https://github.com/tonya11en> is working a simulated system to ensure we can get a more reasonable implementation in the future. Thanks for your contribution and so sorry for that we need to revert it. We know it taken you lots of time. :( Hope we can bring it back soon. — Reply to this email directly, view it on GitHub <#29873 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIOZ7QPECF7Z7BNKBKEMJTYDYZPRAVCNFSM6AAAAAA5MROLXKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBVHAZDEMJYGI>. You are receiving this because you were mentioned.Message ID: ***@***.***>

jkirschner-hashicorp · 2023-11-30T21:31:04Z

I noticed the "revert PR" hasn't been merged yet.

Instead of reverting, is it sufficient to instead add a patch that starts the full scan at a random index (as suggested by @ggreenway and @tomwans here)? Or are there additional concerns that need to be addressed regarding full scan mode?

barroca · 2023-12-02T16:04:54Z

Hello, I have this PR open that changes the behaviour for a random index: #31146

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:

* Add new idea for selecting hosts among those not selected yet. Signed-off-by: Leonardo da Mata <ldamata@spotify.com> * Change how we choose full table scan Signed-off-by: Leonardo da Mata <ldamata@spotify.com> * Remove cout Signed-off-by: Leonardo da Mata <ldamata@spotify.com> * Fix Tests for load_balancer_impl_test Signed-off-by: Leonardo da Mata <ldamata@spotify.com> * Fix format and make sure full scan happens only when selected or the number of choices is larger than the size. Signed-off-by: Leonardo da Mata <ldamata@spotify.com> * Enable new option on extesions api only Signed-off-by: Leonardo da Mata <ldamata@spotify.com> * Fix Integration tests. Signed-off-by: Leonardo da Mata <ldamata@spotify.com> * Add release notes for full scan in least request LB. Signed-off-by: Leonardo da Mata <ldamata@spotify.com> * Fix ref for release note. Signed-off-by: Leonardo da Mata <ldamata@spotify.com> * Fix release notes Signed-off-by: Leonardo da Mata <ldamata@spotify.com> * Update release note Signed-off-by: Leonardo da Mata <ldamata@spotify.com> --------- Signed-off-by: Leonardo da Mata <ldamata@spotify.com> Signed-off-by: Leonardo da Mata <barroca@gmail.com> Co-authored-by: Leonardo da Mata <ldamata@spotify.com>

barroca and others added 3 commits September 29, 2023 14:11

Add new idea for selecting hosts among those not selected yet.

bf274e2

Signed-off-by: Leonardo da Mata <ldamata@spotify.com>

Change how we choose full table scan

e5237d5

Signed-off-by: Leonardo da Mata <ldamata@spotify.com>

Remove cout

d9f6c1a

Signed-off-by: Leonardo da Mata <ldamata@spotify.com>

repokitteh-read-only bot added the api label Sep 29, 2023

repokitteh-read-only bot assigned lizan Sep 29, 2023

Fix Tests for load_balancer_impl_test

7711181

Signed-off-by: Leonardo da Mata <ldamata@spotify.com>

barroca force-pushed the ldamata/fix-least-request-LB-not-fair branch from 0c4f25f to 7711181 Compare September 29, 2023 20:44

repokitteh-read-only bot assigned wbpcode Sep 30, 2023

wbpcode requested changes Oct 3, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 12, 2023

Fix format and make sure full scan happens only when selected or the …

032d925

…number of choices is larger than the size. Signed-off-by: Leonardo da Mata <ldamata@spotify.com>

repokitteh-read-only bot removed the waiting label Oct 13, 2023

barroca requested a review from wbpcode October 13, 2023 23:06

repokitteh-read-only bot removed the api label Oct 17, 2023

lizan removed their assignment Oct 18, 2023

wbpcode reviewed Oct 19, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 19, 2023

Enable new option on extesions api only

99863c0

Signed-off-by: Leonardo da Mata <ldamata@spotify.com>

repokitteh-read-only bot removed the waiting label Oct 19, 2023

wbpcode reviewed Oct 20, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 20, 2023

Fix Integration tests.

e58b34f

Signed-off-by: Leonardo da Mata <ldamata@spotify.com>

repokitteh-read-only bot removed the waiting label Oct 21, 2023

barroca requested a review from wbpcode October 21, 2023 17:15

repokitteh-read-only bot removed the waiting label Oct 24, 2023

barroca requested a review from wbpcode October 24, 2023 13:14

wbpcode previously approved these changes Oct 24, 2023

View reviewed changes

repokitteh-read-only bot removed the api label Oct 24, 2023

Merge branch 'main' into ldamata/fix-least-request-LB-not-fair

35e87e5

Signed-off-by: Leonardo da Mata <barroca@gmail.com>

barroca dismissed wbpcode’s stale review via 35e87e5 October 25, 2023 09:16

repokitteh-read-only bot added the api label Oct 25, 2023

Merge branch 'main' into ldamata/fix-least-request-LB-not-fair

1bf4ac3

Signed-off-by: Leonardo da Mata <barroca@gmail.com>

wbpcode enabled auto-merge (squash) October 26, 2023 13:05

wbpcode approved these changes Oct 26, 2023

View reviewed changes

repokitteh-read-only bot removed the api label Oct 26, 2023

wbpcode merged commit 3ea2bc4 into envoyproxy:main Oct 26, 2023

barroca deleted the ldamata/fix-least-request-LB-not-fair branch October 26, 2023 14:01

kyessenov mentioned this pull request Nov 8, 2023

runtime: add new runtime guard for previous least request lb patch #30768

Closed

wbpcode mentioned this pull request Nov 9, 2023

fix: only enable full scan when enable_full_scan is set explicitly forleast request lb #30794

Merged

kyessenov mentioned this pull request Nov 9, 2023

Revert enable_full_scan #30812

Merged

jkirschner-hashicorp mentioned this pull request Dec 20, 2023

Start Full Scan from a random index for Least Request LB. #31146

Closed

wbpcode mentioned this pull request Jan 3, 2024

Add FULL_SCAN selection mode to least request LB #31507

Merged

		// The number of random healthy hosts from which the host with the fewest active requests will
		// be chosen. Defaults to 2 so that we perform two-choice selection if the field is not set.

Conversation

barroca commented Sep 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Sep 29, 2023

Uh oh!

repokitteh-read-only bot commented Sep 29, 2023

Uh oh!

wbpcode commented Sep 30, 2023

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wbpcode Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

barroca Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wbpcode Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

wbpcode Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Oct 12, 2023

Uh oh!

lizan commented Oct 17, 2023

Uh oh!

wbpcode Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Oct 20, 2023

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

barroca commented Oct 25, 2023

Uh oh!

barroca commented Oct 26, 2023

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

barroca commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbpcode commented Nov 10, 2023

Uh oh!

tonya11en commented Nov 10, 2023 via email

Uh oh!

jkirschner-hashicorp commented Nov 30, 2023

Uh oh!

barroca commented Dec 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

barroca commented Sep 29, 2023 •

edited

Loading

wbpcode Oct 3, 2023 •

edited

Loading

kyessenov commented Nov 9, 2023 •

edited

Loading

barroca commented Nov 10, 2023 •

edited

Loading