Deprecating and removing envoy.reloadable_features.edf_lb_host_schedu…

…ler_init_fix

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

changelogs/current.yaml

@@ -30,6 +30,9 @@ removed_config_or_runtime:
 - area: router
   change: |
     Removed runtime guard ``envoy_reloadable_features_send_local_reply_when_no_buffer_and_upstream_request``.
+- area: load balancing
+  change: |
+    Removed runtime guard ``envoy.reloadable_features.edf_lb_host_scheduler_init_fix`` and legacy code paths.
 - area: http
   change: |
     Removed runtime flag ``envoy.reloadable_features.http_route_connect_proxy_by_default`` and legacy code paths.

source/common/runtime/runtime_features.cc

@@ -39,7 +39,6 @@ RUNTIME_GUARD(envoy_reloadable_features_defer_processing_backedup_streams);
 RUNTIME_GUARD(envoy_reloadable_features_disallow_quic_client_udp_mmsg);
 RUNTIME_GUARD(envoy_reloadable_features_dns_details);
 RUNTIME_GUARD(envoy_reloadable_features_dns_nodata_noname_is_success);
-RUNTIME_GUARD(envoy_reloadable_features_edf_lb_host_scheduler_init_fix);
 RUNTIME_GUARD(envoy_reloadable_features_edf_lb_locality_scheduler_init_fix);
 RUNTIME_GUARD(envoy_reloadable_features_enable_compression_bomb_protection);
 RUNTIME_GUARD(envoy_reloadable_features_enable_include_histograms);

source/extensions/load_balancing_policies/common/load_balancer_impl.cc

@@ -922,53 +922,25 @@ void EdfLoadBalancerBase::refresh(uint32_t priority) {
       return;
     }
 
-    if (Runtime::runtimeFeatureEnabled(
-            "envoy.reloadable_features.edf_lb_host_scheduler_init_fix")) {
-      // If there are no hosts or a single one, there is no need for an EDF scheduler
-      // (thus lowering memory and CPU overhead), as the (possibly) single host
-      // will be the one always selected by the scheduler.
-      if (hosts.size() <= 1) {
-        return;
-      }
+    // If there are no hosts or a single one, there is no need for an EDF scheduler
+    // (thus lowering memory and CPU overhead), as the (possibly) single host
+    // will be the one always selected by the scheduler.
+    if (hosts.size() <= 1) {
+      return;
+    }
 
-      // Populate the scheduler with the host list with a randomized starting point.
-      // TODO(mattklein123): We must build the EDF schedule even if all of the hosts are currently
-      // weighted 1. This is because currently we don't refresh host sets if only weights change.
-      // We should probably change this to refresh at all times. See the comment in
-      // BaseDynamicClusterImpl::updateDynamicHostList about this.
-      scheduler.edf_ = std::make_unique<EdfScheduler<Host>>(EdfScheduler<Host>::createWithPicks(
-          hosts,
-          // We use a fixed weight here. While the weight may change without
-          // notification, this will only be stale until this host is next picked,
-          // at which point it is reinserted into the EdfScheduler with its new
-          // weight in chooseHost().
-          [this](const Host& host) { return hostWeight(host); }, seed_));
-    } else {
-      scheduler.edf_ = std::make_unique<EdfScheduler<Host>>();
-
-      // Populate scheduler with host list.
-      // TODO(mattklein123): We must build the EDF schedule even if all of the hosts are currently
-      // weighted 1. This is because currently we don't refresh host sets if only weights change.
-      // We should probably change this to refresh at all times. See the comment in
-      // BaseDynamicClusterImpl::updateDynamicHostList about this.
-      for (const auto& host : hosts) {
+    // Populate the scheduler with the host list with a randomized starting point.
+    // TODO(mattklein123): We must build the EDF schedule even if all of the hosts are currently
+    // weighted 1. This is because currently we don't refresh host sets if only weights change.
+    // We should probably change this to refresh at all times. See the comment in
+    // BaseDynamicClusterImpl::updateDynamicHostList about this.
+    scheduler.edf_ = std::make_unique<EdfScheduler<Host>>(EdfScheduler<Host>::createWithPicks(
+        hosts,
         // We use a fixed weight here. While the weight may change without
         // notification, this will only be stale until this host is next picked,
         // at which point it is reinserted into the EdfScheduler with its new
         // weight in chooseHost().
-        scheduler.edf_->add(hostWeight(*host), host);
-      }
-
-      // Cycle through hosts to achieve the intended offset behavior.
-      // TODO(htuch): Consider how we can avoid biasing towards earlier hosts in the schedule across
-      // refreshes for the weighted case.
-      if (!hosts.empty()) {
-        for (uint32_t i = 0; i < seed_ % hosts.size(); ++i) {
-          auto host =
-              scheduler.edf_->pickAndAdd([this](const Host& host) { return hostWeight(host); });
-        }
-      }
-    }
+        [this](const Host& host) { return hostWeight(host); }, seed_));
   };
   // Populate EdfSchedulers for each valid HostsSource value for the host set at this priority.
   const auto& host_set = priority_set_.hostSetsPerPriority()[priority];

test/extensions/load_balancing_policies/round_robin/round_robin_lb_test.cc

@@ -461,35 +461,9 @@ TEST_P(RoundRobinLoadBalancerTest, WeightedSeed) {
   EXPECT_EQ(hostSet().healthy_hosts_[1], lb_->chooseHost(nullptr));
 }
 
-// Validate that the RNG seed influences pick order when weighted RR without
-// the envoy.reloadable_features.edf_lb_host_scheduler_init_fix.
-// This test should be removed once
-// envoy.reloadable_features.edf_lb_host_scheduler_init_fix is deprecated.
-TEST_P(RoundRobinLoadBalancerTest, WeightedSeedOldInit) {
-  TestScopedRuntime scoped_runtime;
-  scoped_runtime.mergeValues(
-      {{"envoy.reloadable_features.edf_lb_host_scheduler_init_fix", "false"}});
-  hostSet().healthy_hosts_ = {makeTestHost(info_, "tcp://127.0.0.1:80", simTime(), 1),
-                              makeTestHost(info_, "tcp://127.0.0.1:81", simTime(), 2)};
-  hostSet().hosts_ = hostSet().healthy_hosts_;
-  EXPECT_CALL(random_, random()).WillRepeatedly(Return(1));
-  init(false);
-  // Initial weights respected.
-  EXPECT_EQ(hostSet().healthy_hosts_[0], lb_->chooseHost(nullptr));
-  EXPECT_EQ(hostSet().healthy_hosts_[1], lb_->chooseHost(nullptr));
-  EXPECT_EQ(hostSet().healthy_hosts_[1], lb_->chooseHost(nullptr));
-  EXPECT_EQ(hostSet().healthy_hosts_[0], lb_->chooseHost(nullptr));
-  EXPECT_EQ(hostSet().healthy_hosts_[1], lb_->chooseHost(nullptr));
-  EXPECT_EQ(hostSet().healthy_hosts_[1], lb_->chooseHost(nullptr));
-}
-
 // Validate that low weighted hosts will be chosen when the LB is created.
 TEST_P(RoundRobinLoadBalancerTest, WeightedInitializationPicksAllHosts) {
   TestScopedRuntime scoped_runtime;
-  // This test should be kept just the runtime override removed once the
-  // feature-flag is deprecated.
-  scoped_runtime.mergeValues(
-      {{"envoy.reloadable_features.edf_lb_host_scheduler_init_fix", "true"}});
   // Add 3 hosts with weights {6, 3, 1}. Out of 10 refreshes with consecutive
   // random value, 6 times the first host will be chosen, 3 times the second
   // host will be chosen, and 1 time the third host will be chosen.