-
Notifications
You must be signed in to change notification settings - Fork 5.5k
upstream: allow excluding hosts from lb calculations until initial health check #6794
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
Changes from 15 commits
3293441
fd70113
83f454b
f7b142f
8d0b343
75c8a7c
d94e116
821913e
9de4c56
0bc3d03
1cb40f5
a4a4f10
ed4eaa7
d881b27
ce7d5b4
f017af1
5e688e2
6b962fd
8c33216
ab918c3
7950e0b
510a7a9
c89bb7b
8a9153a
820cd35
b69d22e
b79ef40
1a931fb
0207652
5510ac7
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 |
|---|---|---|
|
|
@@ -526,6 +526,27 @@ message Cluster { | |
| // because merging those updates isn't currently safe. See | ||
| // https://github.com/envoyproxy/envoy/pull/3941. | ||
| google.protobuf.Duration update_merge_window = 4; | ||
|
|
||
| // If set to true, Envoy will not consider new hosts when computing load balancing weights until | ||
| // they have been health checked for the first time. This will have no effect unless | ||
| // active health checking is also configured. | ||
| // | ||
| // Ignoring a host means that for any load balancing calculations that adjust weights based | ||
| // on the ratio of eligible hosts and total hosts (priority spillover, locality weighting, etc.) | ||
| // will exclude these hosts in the denominator. | ||
|
Member
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. Envoy will exclude? |
||
| // | ||
| // For example, with hosts in two priorities P0 and P1, where P0 looks like | ||
| // {healthy, unhealthy (new), unhealthy (new)} | ||
| // and where P1 looks like | ||
| // {healthy, healthy} | ||
| // all traffic will still hit P0, as 1 / (3 - 2) = 1. | ||
| // | ||
| // Enabling this will allow scaling up the number of hosts for a given cluster without entering | ||
| // panic mode or triggering priority spillover, assuming the hosts pass the first health check. | ||
| // | ||
| // If panic mode is triggered, new hosts are still eligible for traffic; they simply do not | ||
| // contribute to the calculation when deciding whether panic mode is enabled or not. | ||
| bool ignore_new_hosts_until_first_hc = 5; | ||
| } | ||
|
|
||
| // Common configuration for all load balancer implementations. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,7 +54,9 @@ class Host : virtual public HostDescription { | |
| m(DEGRADED_EDS_HEALTH, 0x10) \ | ||
| /* The host is pending removal from discovery but is stabilized due to */ \ | ||
| /* active HC. */ \ | ||
| m(PENDING_DYNAMIC_REMOVAL, 0x20) | ||
| m(PENDING_DYNAMIC_REMOVAL, 0x20) \ | ||
| /* The host is pending its initial active health check. */ \ | ||
| m(PENDING_ACTIVE_HC, 0x40) | ||
| // clang-format on | ||
|
|
||
| #define DECLARE_ENUM(name, value) name = value, | ||
|
|
@@ -190,19 +192,23 @@ class Host : virtual public HostDescription { | |
| * @param new_used supplies the new value of host being in use to be stored. | ||
| */ | ||
| virtual void used(bool new_used) PURE; | ||
|
|
||
| virtual bool warmed() const PURE; | ||
|
Member
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. nit: doc comments |
||
| }; | ||
|
|
||
| typedef std::shared_ptr<const Host> HostConstSharedPtr; | ||
|
|
||
| typedef std::vector<HostSharedPtr> HostVector; | ||
| typedef Phantom<HostVector, Healthy> HealthyHostVector; | ||
| typedef Phantom<HostVector, Degraded> DegradedHostVector; | ||
| typedef Phantom<HostVector, Excluded> ExcludedHostVector; | ||
| typedef std::unordered_map<std::string, Upstream::HostSharedPtr> HostMap; | ||
| typedef std::shared_ptr<HostVector> HostVectorSharedPtr; | ||
| typedef std::shared_ptr<const HostVector> HostVectorConstSharedPtr; | ||
|
|
||
| typedef std::shared_ptr<const HealthyHostVector> HealthyHostVectorConstSharedPtr; | ||
| typedef std::shared_ptr<const DegradedHostVector> DegradedHostVectorConstSharedPtr; | ||
| typedef std::shared_ptr<const ExcludedHostVector> ExcludedHostVectorConstSharedPtr; | ||
|
|
||
| typedef std::unique_ptr<HostVector> HostListPtr; | ||
| typedef std::unordered_map<envoy::api::v2::core::Locality, uint32_t, LocalityHash, LocalityEqualTo> | ||
|
|
@@ -285,6 +291,12 @@ class HostSet { | |
| */ | ||
| virtual const HostVector& degradedHosts() const PURE; | ||
|
|
||
| /* | ||
| * @return all excluded hosts contained in the set at the current time. Excluded hosts should be | ||
| * ignored when computing load balancing weights, but may overlap with hosts in hosts(). | ||
| * */ | ||
|
Member
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. nit: extra * |
||
| virtual const HostVector& excludedHosts() const PURE; | ||
|
|
||
| /** | ||
| * @return hosts per locality. | ||
| */ | ||
|
|
@@ -300,6 +312,11 @@ class HostSet { | |
| */ | ||
| virtual const HostsPerLocality& degradedHostsPerLocality() const PURE; | ||
|
|
||
| /** | ||
| * @return same as hostsPerLocality but only contains excluded hosts. | ||
| */ | ||
| virtual const HostsPerLocality& excludedHostsPerLocality() const PURE; | ||
|
|
||
| /** | ||
| * @return weights for each locality in the host set. | ||
| */ | ||
|
|
@@ -378,9 +395,11 @@ class PrioritySet { | |
| HostVectorConstSharedPtr hosts; | ||
| HealthyHostVectorConstSharedPtr healthy_hosts; | ||
| DegradedHostVectorConstSharedPtr degraded_hosts; | ||
| ExcludedHostVectorConstSharedPtr excluded_hosts; | ||
| HostsPerLocalityConstSharedPtr hosts_per_locality; | ||
| HostsPerLocalityConstSharedPtr healthy_hosts_per_locality; | ||
| HostsPerLocalityConstSharedPtr degraded_hosts_per_locality; | ||
| HostsPerLocalityConstSharedPtr excluded_hosts_per_locality; | ||
| }; | ||
|
|
||
| /** | ||
|
|
@@ -775,6 +794,12 @@ class ClusterInfo { | |
| */ | ||
| virtual bool drainConnectionsOnHostRemoval() const PURE; | ||
|
|
||
| /** | ||
| * @return true if this cluster is configured to ignore hosts for the purpose of load balancing | ||
| * computations until they have been health checked for the first time. | ||
| */ | ||
| virtual bool warmHosts() const PURE; | ||
|
|
||
| /** | ||
| * @return eds cluster service_name of the cluster. | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -687,17 +687,22 @@ void ClusterManagerImpl::postThreadLocalClusterUpdate(const Cluster& cluster, ui | |
| host_set->healthyHostsPerLocality().clone(); | ||
| HostsPerLocalityConstSharedPtr degraded_hosts_per_locality_copy = | ||
| host_set->degradedHostsPerLocality().clone(); | ||
| ExcludedHostVectorConstSharedPtr excluded_hosts_copy( | ||
|
Member
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. Worth doing the @htuch TODO up above here soon in a follow up? This is continues to grow scarier and scarier. :) Avoiding the copies here would I think make this function less painful to read and avoid asking for some type of param struct like we have done elsewhere?
Contributor
Author
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. Wouldn't you still need to do pull out a shared ptr for each of the values and pass them along to the lambda to ensure that we're using consistent values? If we just pass along the host set I think it's possible for the TLS updates to happen concurrently with a host update on the main thread and result in potentially inconsistent values? I think this whole thing could be simplified pretty easily by reusing the
Member
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.
Sorry yeah this is roughly what I meant. I think we can avoid the copies vector and also simplify the copying? |
||
| new ExcludedHostVector(host_set->excludedHosts())); | ||
| auto excluded_hosts_per_locality_copy = host_set->excludedHostsPerLocality().clone(); | ||
|
|
||
| tls_->runOnAllThreads([this, name = cluster.info()->name(), priority, hosts_copy, | ||
| healthy_hosts_copy, degraded_hosts_copy, hosts_per_locality_copy, | ||
| healthy_hosts_per_locality_copy, degraded_hosts_per_locality_copy, | ||
| excluded_hosts_copy, excluded_hosts_per_locality_copy, | ||
| locality_weights = host_set->localityWeights(), hosts_added, hosts_removed, | ||
| overprovisioning_factor = host_set->overprovisioningFactor()]() { | ||
| ThreadLocalClusterManagerImpl::updateClusterMembership( | ||
| name, priority, | ||
| HostSetImpl::updateHostsParams(hosts_copy, hosts_per_locality_copy, healthy_hosts_copy, | ||
| healthy_hosts_per_locality_copy, degraded_hosts_copy, | ||
| degraded_hosts_per_locality_copy), | ||
| degraded_hosts_per_locality_copy, excluded_hosts_copy, | ||
| excluded_hosts_per_locality_copy), | ||
| locality_weights, hosts_added, hosts_removed, *tls_, overprovisioning_factor); | ||
| }); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -230,6 +230,7 @@ void HealthCheckerImplBase::ActiveHealthCheckSession::handleSuccess(bool degrade | |
| // it to healthy. This makes startup faster with a small reduction in overall reliability | ||
| // depending on the HC settings. | ||
| if (first_check_ || ++num_healthy_ == parent_.healthy_threshold_) { | ||
| host_->healthFlagClear(Host::HealthFlag::PENDING_ACTIVE_HC); | ||
|
Member
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. If someone uses active HC, but has EDS send health state as healthy, what is the initial state? I'm wondering if there is a case where we don't hit this? Would it be better to do this up above like you do in the failure case to make it more clear that this is clearing after the first response?
Member
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. ping on this
Contributor
Author
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 think the initial state is still going to be unhealthy (since we don't care about the EDS value when we set the FAILED_ACTIVE_HC flag initially), but it might be clearer and less likely to break in the future if I do what you're suggesting so I'll change it |
||
| host_->healthFlagClear(Host::HealthFlag::FAILED_ACTIVE_HC); | ||
| parent_.incHealthy(); | ||
| changed_state = HealthTransition::Changed; | ||
|
|
@@ -291,6 +292,11 @@ HealthTransition HealthCheckerImplBase::ActiveHealthCheckSession::setUnhealthy( | |
| } | ||
| } | ||
|
|
||
| if (host_->healthFlagGet(Host::HealthFlag::PENDING_ACTIVE_HC)) { | ||
| host_->healthFlagClear(Host::HealthFlag::PENDING_ACTIVE_HC); | ||
| changed_state = HealthTransition::Changed; | ||
|
mattklein123 marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| if ((first_check_ || parent_.always_log_health_check_failures_) && parent_.event_logger_) { | ||
| parent_.event_logger_->logUnhealthy(parent_.healthCheckerType(), host_, type, first_check_); | ||
| } | ||
|
|
||
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.
maybe put panic mode in the parenthesis here also?