Revert enable_full_scan (#30812)

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:

Author: kyessenov

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

@@ -60,6 +60,7 @@ message LeastRequest {
   // Configuration for local zone aware load balancing or locality weighted load balancing.
   common.v3.LocalityLbConfig locality_lb_config = 4;
 
+  // [#not-implemented-hide:]
   // 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.

changelogs/current.yaml

@@ -43,12 +43,6 @@ minor_behavior_changes:
     Added support to use http async client to fetch the credentials from EC2 instance metadata and ECS task metadata providers
     instead of libcurl which is deprecated. By default this behavior is disabled. To enable set
     ``envoy.reloadable_features.use_http_client_to_fetch_aws_credentials`` to true.
-- area: upstream
-  change: |
-    Fixed a reported issue (https://github.com/envoyproxy/envoy/issues/11004) that causes the Least
-    Request load balancer policy to be unfair when the number of hosts are very small, when the number
-    of hosts is smaller than the choice_count, instead of randomly selection hosts from the list, we
-    perform a full scan on it to choose the host with least requests.
 - area: local_rate_limit
   change: |
     Added new configuration field :ref:`rate_limited_as_resource_exhausted
@@ -165,13 +159,6 @@ new_features:
   change: |
     Added :ref:`the Basic Auth filter <envoy_v3_api_msg_extensions.filters.http.basic_auth.v3.BasicAuth>`, which can be used to
     authenticate user credentials in the HTTP Authentication heaer defined in `RFC7617 <https://tools.ietf.org/html/rfc7617>`_.
-- area: upstream
-  change: |
-    Added :ref:`enable_full_scan <envoy_v3_api_msg_extensions.load_balancing_policies.least_request.v3.LeastRequest>`
-    option to the least requested load balancer. If set to true, Envoy will perform a full scan on the list of hosts
-    instead of using :ref:`choice_count
-    <envoy_v3_api_msg_extensions.load_balancing_policies.least_request.v3.LeastRequest>`
-    to select the hosts.
 - area: upstream
   change: |
     Implmented API :ref:`drop_overloads<envoy_v3_api_field_config.endpoint.v3.ClusterLoadAssignment.Policy.drop_overloads>`

docs/root/intro/arch_overview/upstream/load_balancing/load_balancers.rst

@@ -38,9 +38,7 @@ same or different weights.
   approach is nearly as good as an O(N) full scan). This is also known as P2C (power of two
   choices). The P2C load balancer has the property that a host with the highest number of active
   requests in the cluster will never receive new requests. It will be allowed to drain until it is
-  less than or equal to all of the other hosts. The number of hosts chosen can be changed by setting
-  ``choice_count``.
-
+  less than or equal to all of the other hosts.
 * *all weights not equal*:  If two or more hosts in the cluster have different load balancing
   weights, the load balancer shifts into a mode where it uses a weighted round robin schedule in
   which weights are dynamically adjusted based on the host's request load at the time of selection.

source/common/upstream/load_balancer_impl.cc

@@ -1299,24 +1299,6 @@ HostConstSharedPtr LeastRequestLoadBalancer::unweightedHostPick(const HostVector
                                                                 const HostsSource&) {
   HostSharedPtr candidate_host = nullptr;
 
-  // Do full scan if it's required explicitly.
-  if (enable_full_scan_) {
-    for (const auto& sampled_host : hosts_to_use) {
-      if (candidate_host == nullptr) {
-        // Make a first choice to start the comparisons.
-        candidate_host = sampled_host;
-        continue;
-      }
-
-      const auto candidate_active_rq = candidate_host->stats().rq_active_.value();
-      const auto sampled_active_rq = sampled_host->stats().rq_active_.value();
-      if (sampled_active_rq < candidate_active_rq) {
-        candidate_host = sampled_host;
-      }
-    }
-    return candidate_host;
-  }
-
   for (uint32_t choice_idx = 0; choice_idx < choice_count_; ++choice_idx) {
     const int rand_idx = random_.random() % hosts_to_use.size();
     const HostSharedPtr& sampled_host = hosts_to_use[rand_idx];

source/common/upstream/load_balancer_impl.h

@@ -710,9 +710,7 @@ class LeastRequestLoadBalancer : public EdfLoadBalancerBase {
             least_request_config.has_active_request_bias()
                 ? absl::optional<Runtime::Double>(
                       {least_request_config.active_request_bias(), runtime})
-                : absl::nullopt),
-        enable_full_scan_(
-            PROTOBUF_GET_WRAPPED_OR_DEFAULT(least_request_config, enable_full_scan, false)) {
+                : absl::nullopt) {
     initialize();
   }
 
@@ -748,7 +746,6 @@ class LeastRequestLoadBalancer : public EdfLoadBalancerBase {
   double active_request_bias_{};
 
   const absl::optional<Runtime::Double> active_request_bias_runtime_;
-  const bool enable_full_scan_{};
 };
 
 /**

test/common/upstream/load_balancer_impl_test.cc

@@ -2880,55 +2880,6 @@ TEST_P(LeastRequestLoadBalancerTest, PNC) {
   EXPECT_EQ(hostSet().healthy_hosts_[3], lb_5.chooseHost(nullptr));
 }
 
-TEST_P(LeastRequestLoadBalancerTest, FullScan) {
-  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(4);
-  hostSet().healthy_hosts_[1]->stats().rq_active_.set(3);
-  hostSet().healthy_hosts_[2]->stats().rq_active_.set(2);
-  hostSet().healthy_hosts_[3]->stats().rq_active_.set(1);
-  hostSet().healthy_hosts_[4]->stats().rq_active_.set(5);
-
-  // 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_2{priority_set_, nullptr, stats_,       runtime_,
-                                random_,       1,       lr_lb_config, simTime()};
-  lr_lb_config.mutable_choice_count()->set_value(3);
-  LeastRequestLoadBalancer lb_3{priority_set_, nullptr, stats_,       runtime_,
-                                random_,       1,       lr_lb_config, simTime()};
-  lr_lb_config.mutable_choice_count()->set_value(4);
-  LeastRequestLoadBalancer lb_4{priority_set_, nullptr, stats_,       runtime_,
-                                random_,       1,       lr_lb_config, simTime()};
-  lr_lb_config.mutable_choice_count()->set_value(6);
-  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));
-  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, WeightImbalance) {
   hostSet().healthy_hosts_ = {makeTestHost(info_, "tcp://127.0.0.1:80", simTime(), 1),
                               makeTestHost(info_, "tcp://127.0.0.1:81", simTime(), 2)};