-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.
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
Make over provisioning factor configurable #4003
Changes from 5 commits
a95d1c7
8d0c010
12dd1d6
8631fca
aaf0a9b
aba9552
dafea16
cdaf91b
e54762c
4186634
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ import "google/api/annotations.proto"; | |
|
||
import "validate/validate.proto"; | ||
import "gogoproto/gogo.proto"; | ||
import "google/protobuf/wrappers.proto"; | ||
|
||
option (gogoproto.equal_all) = true; | ||
|
||
|
@@ -80,6 +81,17 @@ message ClusterLoadAssignment { | |
// "lb"_drop = 20% // 50% of the remaining 'actual' load, which is 40%. | ||
// actual_outgoing_load = 20% // remaining after applying all categories. | ||
repeated DropOverload drop_overloads = 2; | ||
|
||
// Priority levels and localities are considered overprovisioned with this | ||
// factor (in percentage). This means that we don't consider a priority | ||
// level or locality unhealthy until the percentage of healthy hosts | ||
// multiplied by the overprovisioning factor drops below 100. | ||
// With the default value 140(1.4), Envoy doesn't consider a priority level | ||
// or a locality unhealthy until their percentage of healthy hosts drops | ||
// below 72%. | ||
// Read more at :ref:`priority levels <arch_overview_load_balancing_priority_levels>` and | ||
// :ref:`localities <arch_overview_load_balancing_locality_weighted_lb>`. | ||
google.protobuf.UInt32Value overprovisioning_factor = 3 [(validate.rules).uint32.gt = 0]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to allow values less than 100? It seems like in a couple of places this will trigger load shifting even when all the hosts in a given priority are healthy, which seems counterintuitive to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd imagine if the user wants say servers with less cores to have less requests routed to them, they can tune this parameter to achieve that. |
||
} | ||
|
||
// Load balancing policy settings. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
#include "common/network/resolver_impl.h" | ||
#include "common/network/utility.h" | ||
#include "common/protobuf/utility.h" | ||
#include "common/upstream/load_balancer_impl.h" | ||
#include "common/upstream/sds_subscription.h" | ||
|
||
namespace Envoy { | ||
|
@@ -67,6 +68,7 @@ void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources, const std:: | |
PriorityStateManager priority_state_manager(*this, local_info_); | ||
for (const auto& locality_lb_endpoint : cluster_load_assignment.endpoints()) { | ||
const uint32_t priority = locality_lb_endpoint.priority(); | ||
|
||
if (priority > 0 && !cluster_name_.empty() && cluster_name_ == cm_.localClusterName()) { | ||
throw EnvoyException( | ||
fmt::format("Unexpected non-zero priority for local cluster '{}'.", cluster_name_)); | ||
|
@@ -83,6 +85,9 @@ void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources, const std:: | |
// Track whether we rebuilt any LB structures. | ||
bool cluster_rebuilt = false; | ||
|
||
const uint32_t overprovisioning_factor = PROTOBUF_GET_WRAPPED_OR_DEFAULT( | ||
cluster_load_assignment.policy(), overprovisioning_factor, kDefaultOverProvisioningFactor); | ||
|
||
// Loop over existing priorities not present in the config. This will empty out any priorities | ||
// the config update did not refer to | ||
auto& priority_state = priority_state_manager.priorityState(); | ||
|
@@ -91,9 +96,9 @@ void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources, const std:: | |
if (locality_weights_map_.size() <= i) { | ||
locality_weights_map_.resize(i + 1); | ||
} | ||
cluster_rebuilt |= | ||
updateHostsPerLocality(i, *priority_state[i].first, locality_weights_map_[i], | ||
priority_state[i].second, priority_state_manager); | ||
cluster_rebuilt |= updateHostsPerLocality(i, overprovisioning_factor, | ||
*priority_state[i].first, locality_weights_map_[i], | ||
priority_state[i].second, priority_state_manager); | ||
} | ||
} | ||
|
||
|
@@ -106,8 +111,9 @@ void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources, const std:: | |
if (locality_weights_map_.size() <= i) { | ||
locality_weights_map_.resize(i + 1); | ||
} | ||
cluster_rebuilt |= updateHostsPerLocality(i, empty_hosts, locality_weights_map_[i], | ||
empty_locality_map, priority_state_manager); | ||
cluster_rebuilt |= | ||
updateHostsPerLocality(i, overprovisioning_factor, empty_hosts, locality_weights_map_[i], | ||
empty_locality_map, priority_state_manager); | ||
} | ||
|
||
if (!cluster_rebuilt) { | ||
|
@@ -119,11 +125,13 @@ void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources, const std:: | |
onPreInitComplete(); | ||
} | ||
|
||
bool EdsClusterImpl::updateHostsPerLocality(const uint32_t priority, const HostVector& new_hosts, | ||
bool EdsClusterImpl::updateHostsPerLocality(const uint32_t priority, | ||
const uint32_t overprovisioning_factor, | ||
const HostVector& new_hosts, | ||
LocalityWeightsMap& locality_weights_map, | ||
LocalityWeightsMap& new_locality_weights_map, | ||
PriorityStateManager& priority_state_manager) { | ||
const auto& host_set = priority_set_.getOrCreateHostSet(priority); | ||
const auto& host_set = priority_set_.getOrCreateHostSet(priority, overprovisioning_factor); | ||
HostVectorSharedPtr current_hosts_copy(new HostVector(host_set.hosts())); | ||
|
||
HostVector hosts_added; | ||
|
@@ -136,14 +144,16 @@ bool EdsClusterImpl::updateHostsPerLocality(const uint32_t priority, const HostV | |
// out of the locality scheduler, we discover their new weights. We don't currently have a shared | ||
// object for locality weights that we can update here, we should add something like this to | ||
// improve performance and scalability of locality weight updates. | ||
if (updateDynamicHostList(new_hosts, *current_hosts_copy, hosts_added, hosts_removed) || | ||
if (host_set.overprovisioning_factor() != overprovisioning_factor || | ||
updateDynamicHostList(new_hosts, *current_hosts_copy, hosts_added, hosts_removed) || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, it looks like we call this function with the overprovisioning factor if it's present in the config. Can we avoid doing an update of the old and new overprovisioning factors are the same? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we cannot? if the overprovisioning factor is changed, we have to update the host_set. |
||
locality_weights_map != new_locality_weights_map) { | ||
locality_weights_map = new_locality_weights_map; | ||
ENVOY_LOG(debug, "EDS hosts or locality weights changed for cluster: {} ({}) priority {}", | ||
info_->name(), host_set.hosts().size(), host_set.priority()); | ||
|
||
priority_state_manager.updateClusterPrioritySet(priority, std::move(current_hosts_copy), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given we call updateHostsPerLocality with the overprovisioning factor if it's present in config, can we make sure that if there's no change in hosts and there's no change in overprovisioning factor (it's present but goes from 150->150 or something) that we don't update the cluster priority set? I think as the code is written we don't check to see if it has changed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
hosts_added, hosts_removed, absl::nullopt); | ||
hosts_added, hosts_removed, absl::nullopt, | ||
overprovisioning_factor); | ||
return true; | ||
} | ||
return false; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -544,10 +544,16 @@ void SubsetLoadBalancer::HostSubsetImpl::update(const HostVector& hosts_added, | |
} | ||
} | ||
|
||
HostSetImplPtr SubsetLoadBalancer::PrioritySubsetImpl::createHostSet(uint32_t priority) { | ||
HostSetImplPtr SubsetLoadBalancer::PrioritySubsetImpl::createHostSet( | ||
uint32_t priority, absl::optional<uint32_t> overprovisioning_factor) { | ||
// Use original hostset's overprovisioning_factor. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we consider it an error if they're not the same? ASSERT-validate maybe? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may still need UNREFERENCED_PARAMETER for the opt build to compile, since the ASSERT compiles out there. |
||
RELEASE_ASSERT(priority < original_priority_set_.hostSetsPerPriority().size(), ""); | ||
return HostSetImplPtr{new HostSubsetImpl(*original_priority_set_.hostSetsPerPriority()[priority], | ||
locality_weight_aware_)}; | ||
|
||
const HostSetPtr& host_set = original_priority_set_.hostSetsPerPriority()[priority]; | ||
|
||
ASSERT(!overprovisioning_factor.has_value() || | ||
overprovisioning_factor.value() == host_set->overprovisioning_factor()); | ||
return HostSetImplPtr{new HostSubsetImpl(*host_set, locality_weight_aware_)}; | ||
} | ||
|
||
void SubsetLoadBalancer::PrioritySubsetImpl::update(uint32_t priority, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh and sorry I missed this last time but we should note the default in our docs here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done