Skip to content

Commit

Permalink
fix: only enable full scan when enable_full_scan is set explicitly fo…
Browse files Browse the repository at this point in the history 
…rleast request lb (envoyproxy#30794)

Signed-off-by: wbpcode <[email protected]>
  • Loading branch information
code authored Nov 9, 2023
1 parent 75791d7 commit e93e556
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 41 deletions.
5 changes: 2 additions & 3 deletions source/common/upstream/load_balancer_impl.cc
View file
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
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
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

0 comments on commit e93e556

Please sign in to comment.