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

Fixed a bug (envoyproxy#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.

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

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;
 }

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 <deprecated>`

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;

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)};