diff --git a/api/envoy/extensions/load_balancing_policies/least_request/v3/least_request.proto b/api/envoy/extensions/load_balancing_policies/least_request/v3/least_request.proto index 7be284a4c6090..12107dd9cc1e0 100644 --- a/api/envoy/extensions/load_balancing_policies/least_request/v3/least_request.proto +++ b/api/envoy/extensions/load_balancing_policies/least_request/v3/least_request.proto @@ -63,5 +63,7 @@ message LeastRequest { // Configuration for performing full scan on the list of hosts. // If this configuration is set, when selecting the host a full scan on the list hosts will be // used to select the one with least requests instead of using random choices. + // It uses a random index to start scan to avoid selecting the same host when the number of + // requests are the same. google.protobuf.BoolValue enable_full_scan = 5; } diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 571fc8cf3b48a..2fab593d29966 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -101,6 +101,11 @@ bug_fixes: used for retrieval. This caused cache misses on initial use, even though the host DNS entry was pre-resolved. The fix is guarded by runtime guard ``envoy.reloadable_features.normalize_host_for_preresolve_dfp_dns``, which defaults to true. +- area: upstream + change: | + Fixed a bug (https://github.com/envoyproxy/envoy/pull/11006) that caused the Least Request load balancer policy to choose + the first host of the list when the number of requests are the same during a full scan. Start the selection from a random + index instead of 0. removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/source/common/upstream/load_balancer_impl.cc b/source/common/upstream/load_balancer_impl.cc index bfcc451981d96..2dedc7eee9fd1 100644 --- a/source/common/upstream/load_balancer_impl.cc +++ b/source/common/upstream/load_balancer_impl.cc @@ -1301,7 +1301,11 @@ HostConstSharedPtr LeastRequestLoadBalancer::unweightedHostPick(const HostVector // Do full scan if it's required explicitly. if (enable_full_scan_) { - for (const auto& sampled_host : hosts_to_use) { + // Choose a random index to start from preventing always picking the first host in the list. + const int rand_idx = random_.random() % hosts_to_use.size(); + for (unsigned long i = 0; i < hosts_to_use.size(); i++) { + const HostSharedPtr& sampled_host = hosts_to_use[(rand_idx + i) % hosts_to_use.size()]; + if (candidate_host == nullptr) { // Make a first choice to start the comparisons. candidate_host = sampled_host; diff --git a/test/common/upstream/load_balancer_impl_test.cc b/test/common/upstream/load_balancer_impl_test.cc index 379f6b082b189..0499e76f88c36 100644 --- a/test/common/upstream/load_balancer_impl_test.cc +++ b/test/common/upstream/load_balancer_impl_test.cc @@ -2898,7 +2898,7 @@ TEST_P(LeastRequestLoadBalancerTest, FullScan) { // Creating various load balancer objects with different choice configs. envoy::extensions::load_balancing_policies::least_request::v3::LeastRequest lr_lb_config; lr_lb_config.mutable_choice_count()->set_value(2); - // Enable full table scan on hosts + // Enable full table scan on hosts. lr_lb_config.mutable_enable_full_scan()->set_value(true); common_config_.mutable_healthy_panic_threshold()->set_value(0); @@ -2914,21 +2914,61 @@ TEST_P(LeastRequestLoadBalancerTest, FullScan) { LeastRequestLoadBalancer lb_6{priority_set_, nullptr, stats_, runtime_, random_, 1, lr_lb_config, simTime()}; - // random is called only once every time and is not to select the host. - - EXPECT_CALL(random_, random()).WillOnce(Return(9999)); + // With full scan we will always choose the host with least number of active requests. EXPECT_EQ(hostSet().healthy_hosts_[3], lb_2.chooseHost(nullptr)); - - EXPECT_CALL(random_, random()).WillOnce(Return(9999)); EXPECT_EQ(hostSet().healthy_hosts_[3], lb_3.chooseHost(nullptr)); - - EXPECT_CALL(random_, random()).WillOnce(Return(9999)); EXPECT_EQ(hostSet().healthy_hosts_[3], lb_4.chooseHost(nullptr)); - - EXPECT_CALL(random_, random()).WillOnce(Return(9999)); EXPECT_EQ(hostSet().healthy_hosts_[3], lb_6.chooseHost(nullptr)); } +TEST_P(LeastRequestLoadBalancerTest, FullScanReturnDifferentHostWhenRequestsAreEqual) { + hostSet().healthy_hosts_ = {makeTestHost(info_, "tcp://127.0.0.1:80", simTime()), + makeTestHost(info_, "tcp://127.0.0.1:81", simTime()), + makeTestHost(info_, "tcp://127.0.0.1:82", simTime()), + makeTestHost(info_, "tcp://127.0.0.1:83", simTime()), + makeTestHost(info_, "tcp://127.0.0.1:84", simTime())}; + hostSet().hosts_ = hostSet().healthy_hosts_; + hostSet().runCallbacks({}, {}); // Trigger callbacks. The added/removed lists are not relevant. + + hostSet().healthy_hosts_[0]->stats().rq_active_.set(3); + hostSet().healthy_hosts_[1]->stats().rq_active_.set(3); + hostSet().healthy_hosts_[2]->stats().rq_active_.set(3); + hostSet().healthy_hosts_[3]->stats().rq_active_.set(3); + hostSet().healthy_hosts_[4]->stats().rq_active_.set(3); + + // Creating various load balancer objects with different choice configs. + envoy::extensions::load_balancing_policies::least_request::v3::LeastRequest lr_lb_config; + lr_lb_config.mutable_choice_count()->set_value(2); + // Enable full table scan on hosts. + lr_lb_config.mutable_enable_full_scan()->set_value(true); + common_config_.mutable_healthy_panic_threshold()->set_value(0); + + LeastRequestLoadBalancer lb{priority_set_, nullptr, stats_, runtime_, + random_, 1, lr_lb_config, simTime()}; + + // random is called every time to choose the first host. + EXPECT_CALL(random_, random()) + .WillOnce(Return(0)) + .WillOnce(Return(0)) + .WillOnce(Return(1)) + .WillOnce(Return(1)) + .WillOnce(Return(2)) + .WillOnce(Return(2)) + .WillOnce(Return(3)) + .WillOnce(Return(3)) + .WillOnce(Return(4)) + .WillOnce(Return(4)) + .WillOnce(Return(5)) + .WillOnce(Return(5)); + + EXPECT_EQ(hostSet().healthy_hosts_[0], lb.chooseHost(nullptr)); + EXPECT_EQ(hostSet().healthy_hosts_[1], lb.chooseHost(nullptr)); + EXPECT_EQ(hostSet().healthy_hosts_[2], lb.chooseHost(nullptr)); + EXPECT_EQ(hostSet().healthy_hosts_[3], lb.chooseHost(nullptr)); + EXPECT_EQ(hostSet().healthy_hosts_[4], lb.chooseHost(nullptr)); + EXPECT_EQ(hostSet().healthy_hosts_[0], lb.chooseHost(nullptr)); +} + TEST_P(LeastRequestLoadBalancerTest, WeightImbalance) { hostSet().healthy_hosts_ = {makeTestHost(info_, "tcp://127.0.0.1:80", simTime(), 1), makeTestHost(info_, "tcp://127.0.0.1:81", simTime(), 2)};