Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
a1b52fd
Add support for making Least Requests LB behave like Round Robin in w…
gkleiman May 19, 2020
762e61b
Address feedback
gkleiman May 19, 2020
4cf5f19
Perf/logging improvements
gkleiman May 20, 2020
0a2b1fc
Address feedback and cleanup BUILD file
gkleiman May 20, 2020
6f9b89c
Merge branch 'master' into alternative-lr-weights
gkleiman Jun 16, 2020
6864b50
Make active requests exponent configurable via CDS/runtime
gkleiman Jun 11, 2020
25f7c98
Address feedback
gkleiman Jun 22, 2020
f4b2da3
Validate log message
gkleiman Jun 23, 2020
e96d52d
Update cluster memory test golden values
gkleiman Jun 23, 2020
a5d7782
Merge remote-tracking branch 'origin/master' into alternative-lr-weights
gkleiman Jun 24, 2020
cfca8f7
Fix method name
gkleiman Jun 26, 2020
ce291d6
Merge remote-tracking branch 'origin/master' into alternative-lr-weights
gkleiman Jun 26, 2020
b487a5e
Explicitly initialize active_request_bias_
gkleiman Jun 29, 2020
d7cf64c
Merge remote-tracking branch 'origin/master' into alternative-lr-weights
gkleiman Jun 29, 2020
3fc99ea
Try to make clang-tidy happy
gkleiman Jun 29, 2020
9be22f0
Use unique_ptr instead of optional
gkleiman Jun 30, 2020
f2d8924
Merge remote-tracking branch 'origin/master' into alternative-lr-weights
gkleiman Jul 1, 2020
71fcad8
Update stats integration test
gkleiman Jul 1, 2020
2690778
Check whether memory footprint is reduced without LB changes
gkleiman Jul 1, 2020
be7c000
Use plain double for active request bias
gkleiman Jul 6, 2020
a4e7b39
Revert back to approved implementation using RuntimeDouble
gkleiman Jul 7, 2020
1010883
Merge remote-tracking branch 'origin/master' into alternative-lr-weights
gkleiman Jul 7, 2020
a6a285d
Add extra fields to CDS cluster proto to check memory usage
gkleiman Jul 8, 2020
2676928
Revert "Add extra fields to CDS cluster proto to check memory usage"
gkleiman Jul 13, 2020
100c3db
Merge remote-tracking branch 'origin/master' into alternative-lr-weights
gkleiman Jul 13, 2020
cf06a76
Merge remote-tracking branch 'origin/master' into alternative-lr-weights
gkleiman Jul 14, 2020
eb1b857
Add changelog entry
gkleiman Jul 15, 2020
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
1 change: 1 addition & 0 deletions source/common/runtime/runtime_features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ constexpr const char* runtime_features[] = {
constexpr const char* disabled_runtime_features[] = {
// Sentinel and test flag.
"envoy.reloadable_features.test_feature_false",
"envoy.reloadable_features.alternative_least_request_weights",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we end up doing a real PR for this, it should be a real config option vs. one of these. I would just use a RuntimeFeatureFlag inside the LB config proto.

};

RuntimeFeatures::RuntimeFeatures() {
Expand Down
1 change: 1 addition & 0 deletions source/common/upstream/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ envoy_cc_library(
"//include/envoy/upstream:upstream_interface",
"//source/common/common:assert_lib",
"//source/common/protobuf:utility_lib",
"//source/common/runtime:runtime_features_lib",
"@envoy_api//envoy/config/cluster/v3:pkg_cc_proto",
],
)
Expand Down
22 changes: 21 additions & 1 deletion source/common/upstream/load_balancer_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "envoy/upstream/upstream.h"

#include "common/protobuf/utility.h"
#include "common/runtime/runtime_features.h"
#include "common/upstream/edf_scheduler.h"

namespace Envoy {
Expand Down Expand Up @@ -367,6 +368,8 @@ class EdfLoadBalancerBase : public ZoneAwareLoadBalancerBase {

void initialize();

virtual void refresh(uint32_t priority);

// Seed to allow us to desynchronize load balancers across a fleet. If we don't
// do this, multiple Envoys that receive an update at the same time (or even
// multiple load balancers on the same host) will send requests to
Expand All @@ -375,7 +378,6 @@ class EdfLoadBalancerBase : public ZoneAwareLoadBalancerBase {
const uint64_t seed_;

private:
void refresh(uint32_t priority);
virtual void refreshHostSource(const HostsSource& source) PURE;
virtual double hostWeight(const Host& host) PURE;
virtual HostConstSharedPtr unweightedHostPick(const HostVector& hosts_to_use,
Expand Down Expand Up @@ -454,9 +456,25 @@ class LeastRequestLoadBalancer : public EdfLoadBalancerBase {
initialize();
}

HostConstSharedPtr chooseHost(LoadBalancerContext* context) override {
alternativeWeights_ = Runtime::runtimeFeatureEnabled(
"envoy.reloadable_features.alternative_least_request_weights");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For perf reasons I would not re-snap this on every choice. I would just do it in refresh below. I think this is fine.

return EdfLoadBalancerBase::chooseHost(context);
}

void refresh(uint32_t priority) override {
alternativeWeights_ = Runtime::runtimeFeatureEnabled(
"envoy.reloadable_features.alternative_least_request_weights");
EdfLoadBalancerBase::refresh(priority);
}

private:
void refreshHostSource(const HostsSource&) override {}
double hostWeight(const Host& host) override {
if (alternativeWeights_) {
return host.weight();
}

// Here we scale host weight by the number of active requests at the time we do the pick. We
// always add 1 to avoid division by 0. It might be possible to do better by picking two hosts
// off of the schedule, and selecting the one with fewer active requests at the time of
Expand All @@ -469,7 +487,9 @@ class LeastRequestLoadBalancer : public EdfLoadBalancerBase {
}
HostConstSharedPtr unweightedHostPick(const HostVector& hosts_to_use,
const HostsSource& source) override;

const uint32_t choice_count_;
bool alternativeWeights_;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

alternative_weights_

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also explain what this means exactly in a comment. It's non-obvious from the name.

};

/**
Expand Down
1 change: 1 addition & 0 deletions test/common/upstream/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ envoy_cc_test(
"//source/common/upstream:upstream_lib",
"//test/mocks/runtime:runtime_mocks",
"//test/mocks/upstream:upstream_mocks",
"//test/test_common:test_runtime_lib",
"@envoy_api//envoy/config/cluster/v3:pkg_cc_proto",
],
)
Expand Down
25 changes: 25 additions & 0 deletions test/common/upstream/load_balancer_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "test/common/upstream/utility.h"
#include "test/mocks/runtime/mocks.h"
#include "test/mocks/upstream/mocks.h"
#include "test/test_common/test_runtime.h"

#include "gmock/gmock.h"
#include "gtest/gtest.h"
Expand Down Expand Up @@ -1532,6 +1533,30 @@ TEST_P(LeastRequestLoadBalancerTest, WeightImbalance) {
EXPECT_EQ(hostSet().healthy_hosts_[0], lb_.chooseHost(nullptr));
}

TEST_P(LeastRequestLoadBalancerTest, WeightImbalanceWithAlternativeWeights) {
auto scoped_runtime = std::make_unique<TestScopedRuntime>();
Runtime::LoaderSingleton::getExisting()->mergeValues(
{{"envoy.reloadable_features.alternative_least_request_weights", "true"}});

hostSet().healthy_hosts_ = {makeTestHost(info_, "tcp://127.0.0.1:80", 1),
makeTestHost(info_, "tcp://127.0.0.1:81", 2)};
stats_.max_host_weight_.set(2UL);

hostSet().hosts_ = hostSet().healthy_hosts_;
hostSet().runCallbacks({}, {}); // Trigger callbacks. The added/removed lists are not relevant.

EXPECT_CALL(random_, random()).WillRepeatedly(Return(0));

// We should see 2:1 ratio for hosts[1] to hosts[0], regardless of the active request count.
hostSet().healthy_hosts_[1]->stats().rq_active_.set(1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm surprised this works. Are the stats affecting the outstanding requests perceived by the LB when the exponent is non-zero?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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));
EXPECT_EQ(hostSet().healthy_hosts_[0], lb_.chooseHost(nullptr));
EXPECT_EQ(hostSet().healthy_hosts_[1], lb_.chooseHost(nullptr));
}

TEST_P(LeastRequestLoadBalancerTest, WeightImbalanceCallbacks) {
hostSet().healthy_hosts_ = {makeTestHost(info_, "tcp://127.0.0.1:80", 1),
makeTestHost(info_, "tcp://127.0.0.1:81", 2)};
Expand Down