Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wbpcode wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions changelogs/current.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ minor_behavior_changes:
Flip the runtime guard ``envoy.reloadable_features.defer_processing_backedup_streams`` to be on by default.
This feature improves flow control within the proxy by deferring work on the receiving end if the other
end is backed up.
- area: upstream
change: |
Do full scan on the list of hosts for the least requested load balancer if the number of hosts is smaller
than the choice_count by default. This behavior can be reverted temporarily by setting the runtime flag
``envoy.reloadable_features.full_scan_if_host_num_less_than_choice_num`` to false.

bug_fixes:
# *Changes expected to improve the state of the world and are unlikely to have negative effects*
Expand Down
1 change: 1 addition & 0 deletions source/common/runtime/runtime_features.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ RUNTIME_GUARD(envoy_reloadable_features_enable_connect_udp_support);
RUNTIME_GUARD(envoy_reloadable_features_enable_intermediate_ca);
RUNTIME_GUARD(envoy_reloadable_features_enable_zone_routing_different_zone_counts);
RUNTIME_GUARD(envoy_reloadable_features_ext_authz_http_send_original_xff);
RUNTIME_GUARD(envoy_reloadable_features_full_scan_if_host_num_less_than_choice_num);
RUNTIME_GUARD(envoy_reloadable_features_handle_uppercase_scheme);
RUNTIME_GUARD(envoy_reloadable_features_hmac_base64_encoding_only);
RUNTIME_GUARD(envoy_reloadable_features_http1_allow_codec_error_response_after_1xx_headers);
Expand Down
3 changes: 2 additions & 1 deletion source/common/upstream/load_balancer_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1301,7 +1301,8 @@ HostConstSharedPtr LeastRequestLoadBalancer::unweightedHostPick(const HostVector

// Do full scan if it's required explicitly or the number of choices is equal to or larger than
// the hosts size.
if ((hosts_to_use.size() <= choice_count_) || enable_full_scan_) {
if ((full_scan_if_host_num_less_than_choice_num_ && hosts_to_use.size() <= choice_count_) ||
Copy link
Copy Markdown
Member

@botengyao botengyao Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reported issue for this, and what is the reason? Thanks!

Copy link
Copy Markdown
Contributor

@kyessenov kyessenov Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue for public reference: Istio disables cluster stats so it causes this PR to change load balancing to always pick the first host. Due to the DNS traffic concentration, there's some unexpected DNS failure as a result. We were not able to get Istio CI to deploy #29873, see istio/istio#47694.

Copy link
Copy Markdown
Member Author

@wbpcode wbpcode Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can revert that PR temporarily until we figure out a new API for that. (Today is the last day that could do a break change to the API)
cc @kyessenov

Copy link
Copy Markdown
Contributor

@kyessenov kyessenov Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeap, let's do that, and then open it again with a safe default (e.g. by putting && instead of || when the flag is enabled).

enable_full_scan_) {
for (const auto& sampled_host : hosts_to_use) {
if (candidate_host == nullptr) {
// Make a first choice to start the comparisons.
Expand Down
10 changes: 8 additions & 2 deletions source/common/upstream/load_balancer_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "envoy/upstream/upstream.h"

#include "source/common/protobuf/utility.h"
#include "source/common/runtime/runtime_features.h"
#include "source/common/runtime/runtime_protos.h"
#include "source/common/upstream/edf_scheduler.h"
#include "source/common/upstream/subset_lb_config.h"
Expand Down Expand Up @@ -691,7 +692,9 @@ class LeastRequestLoadBalancer : public EdfLoadBalancerBase {
least_request_config.has_value() && least_request_config->has_active_request_bias()
? absl::optional<Runtime::Double>(
{least_request_config->active_request_bias(), runtime})
: absl::nullopt) {
: absl::nullopt),
full_scan_if_host_num_less_than_choice_num_(Runtime::runtimeFeatureEnabled(
"envoy.reloadable_features.full_scan_if_host_num_less_than_choice_num")) {
initialize();
}

Expand All @@ -712,7 +715,9 @@ class LeastRequestLoadBalancer : public EdfLoadBalancerBase {
{least_request_config.active_request_bias(), runtime})
: absl::nullopt),
enable_full_scan_(
PROTOBUF_GET_WRAPPED_OR_DEFAULT(least_request_config, enable_full_scan, false)) {
PROTOBUF_GET_WRAPPED_OR_DEFAULT(least_request_config, enable_full_scan, false)),
full_scan_if_host_num_less_than_choice_num_(Runtime::runtimeFeatureEnabled(
"envoy.reloadable_features.full_scan_if_host_num_less_than_choice_num")) {
initialize();
}

Expand Down Expand Up @@ -749,6 +754,7 @@ class LeastRequestLoadBalancer : public EdfLoadBalancerBase {

const absl::optional<Runtime::Double> active_request_bias_runtime_;
const bool enable_full_scan_{};
const bool full_scan_if_host_num_less_than_choice_num_{};
};

/**
Expand Down
21 changes: 21 additions & 0 deletions test/common/upstream/load_balancer_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2894,6 +2894,27 @@ TEST_P(LeastRequestLoadBalancerTest, PNC) {
// random() since we do a full table scan.
EXPECT_CALL(random_, random()).WillOnce(Return(9999));
EXPECT_EQ(hostSet().healthy_hosts_[3], lb_6.chooseHost(nullptr));

TestScopedRuntime scoped_runtime;
scoped_runtime.mergeValues(
{{"envoy.reloadable_features.full_scan_if_host_num_less_than_choice_num", "false"}});
lr_lb_config.mutable_choice_count()->set_value(7);
LeastRequestLoadBalancer lb_7{priority_set_, nullptr, stats_, runtime_,
random_, common_config_, lr_lb_config, simTime()};

// Host number is smaller than choice number but full scan is disabled, we should still call
// random().
EXPECT_CALL(random_, random())
.Times(8) // One more time than the number of choices.
.WillOnce(Return(0))
.WillOnce(Return(1))
.WillOnce(Return(2))
.WillOnce(Return(0))
.WillOnce(Return(1))
.WillOnce(Return(2))
.WillOnce(Return(0))
.WillOnce(Return(1));
EXPECT_EQ(hostSet().healthy_hosts_[2], lb_7.chooseHost(nullptr));
}

TEST_P(LeastRequestLoadBalancerTest, FullScan) {
Expand Down