Skip to content

fix: only enable full scan when enable_full_scan is set explicitly forleast request lb #30794

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

Merged
lizan merged 1 commit into from
Nov 9, 2023
Merged

fix: only enable full scan when enable_full_scan is set explicitly forleast request lb #30794

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: 2 additions & 3 deletions source/common/upstream/load_balancer_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1299,9 +1299,8 @@ HostConstSharedPtr LeastRequestLoadBalancer::unweightedHostPick(const HostVector
const HostsSource&) {
HostSharedPtr candidate_host = nullptr;

// 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_) {
// 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.
Expand Down
52 changes: 18 additions & 34 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 @@ -2787,20 +2787,20 @@ TEST_P(LeastRequestLoadBalancerTest, SingleHost) {

// Host weight is 1.
{
EXPECT_CALL(random_, random()).WillOnce(Return(0));
EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(2)).WillOnce(Return(3));
EXPECT_EQ(hostSet().healthy_hosts_[0], lb_.chooseHost(nullptr));
}

// Host weight is 100.
{
EXPECT_CALL(random_, random()).WillOnce(Return(0));
EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(2)).WillOnce(Return(3));
EXPECT_EQ(hostSet().healthy_hosts_[0], lb_.chooseHost(nullptr));
}

HostVector empty;
{
hostSet().runCallbacks(empty, empty);
EXPECT_CALL(random_, random()).WillOnce(Return(0));
EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(2)).WillOnce(Return(3));
EXPECT_EQ(hostSet().healthy_hosts_[0], lb_.chooseHost(nullptr));
}

Expand All @@ -2823,44 +2823,37 @@ TEST_P(LeastRequestLoadBalancerTest, Normal) {

hostSet().healthy_hosts_[0]->stats().rq_active_.set(1);
hostSet().healthy_hosts_[1]->stats().rq_active_.set(2);
EXPECT_CALL(random_, random()).WillOnce(Return(0));
EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(2)).WillOnce(Return(3));
EXPECT_EQ(hostSet().healthy_hosts_[0], lb_.chooseHost(nullptr));

hostSet().healthy_hosts_[0]->stats().rq_active_.set(2);
hostSet().healthy_hosts_[1]->stats().rq_active_.set(1);
EXPECT_CALL(random_, random()).WillOnce(Return(0));
EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(2)).WillOnce(Return(3));
EXPECT_EQ(hostSet().healthy_hosts_[1], lb_.chooseHost(nullptr));
}

TEST_P(LeastRequestLoadBalancerTest, PNC) {
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())};
makeTestHost(info_, "tcp://127.0.0.1:83", 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::config::cluster::v3::Cluster::LeastRequestLbConfig lr_lb_config;
lr_lb_config.mutable_choice_count()->set_value(2);
LeastRequestLoadBalancer lb_2{priority_set_, nullptr, stats_, runtime_,
random_, common_config_, lr_lb_config, simTime()};
lr_lb_config.mutable_choice_count()->set_value(3);
LeastRequestLoadBalancer lb_3{priority_set_, nullptr, stats_, runtime_,
random_, common_config_, lr_lb_config, simTime()};
lr_lb_config.mutable_choice_count()->set_value(4);
LeastRequestLoadBalancer lb_4{priority_set_, nullptr, stats_, runtime_,
random_, common_config_, lr_lb_config, simTime()};
lr_lb_config.mutable_choice_count()->set_value(6);
LeastRequestLoadBalancer lb_6{priority_set_, nullptr, stats_, runtime_,
lr_lb_config.mutable_choice_count()->set_value(5);
LeastRequestLoadBalancer lb_5{priority_set_, nullptr, stats_, runtime_,
random_, common_config_, lr_lb_config, simTime()};

// Verify correct number of choices.

// 0 choices configured should default to P2C.
Expand All @@ -2871,29 +2864,20 @@ TEST_P(LeastRequestLoadBalancerTest, PNC) {
EXPECT_CALL(random_, random()).Times(3).WillRepeatedly(Return(0));
EXPECT_EQ(hostSet().healthy_hosts_[0], lb_2.chooseHost(nullptr));

// Verify correct host chosen in P3C scenario.
// 5 choices configured results in P5C.
EXPECT_CALL(random_, random()).Times(6).WillRepeatedly(Return(0));
EXPECT_EQ(hostSet().healthy_hosts_[0], lb_5.chooseHost(nullptr));

// Verify correct host chosen in P5C scenario.
EXPECT_CALL(random_, random())
.Times(4)
.Times(6)
.WillOnce(Return(0))
.WillOnce(Return(3))
.WillOnce(Return(1))
.WillOnce(Return(2));
EXPECT_EQ(hostSet().healthy_hosts_[3], lb_3.chooseHost(nullptr));

// Verify correct host chosen in P4C scenario.
EXPECT_CALL(random_, random())
.Times(5)
.WillOnce(Return(0))
.WillOnce(Return(3))
.WillOnce(Return(1))
.WillOnce(Return(1))
.WillOnce(Return(2));
EXPECT_EQ(hostSet().healthy_hosts_[3], lb_4.chooseHost(nullptr));

// When the number of hosts is smaller or equal to the number of choices we don't call
// 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));
.WillOnce(Return(2))
.WillOnce(Return(1));
EXPECT_EQ(hostSet().healthy_hosts_[3], lb_5.chooseHost(nullptr));
}

TEST_P(LeastRequestLoadBalancerTest, FullScan) {
Expand Down
5 changes: 1 addition & 4 deletions test/integration/http_subset_lb_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,10 +176,7 @@ class HttpSubsetLbIntegrationTest
}
}

// The default number of choices for the LEAST_REQUEST policy is 2 hosts, if the number of hosts
// is equal to the number of choices, a full scan happens instead, this means that the same host
// will be chosen.
if (is_hash_lb_ || (GetParam() == envoy::config::cluster::v3::Cluster::LEAST_REQUEST)) {
if (is_hash_lb_) {
EXPECT_EQ(hosts.size(), 1) << "Expected a single unique host to be selected for "
<< envoy::config::cluster::v3::Cluster::LbPolicy_Name(GetParam());
} else {
Expand Down