Skip to content

upstream: add support for setting degraded through LoadAssignment #5649

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
htuch merged 10 commits into from
Feb 8, 2019
Merged

upstream: add support for setting degraded through LoadAssignment #5649

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d8d4aa3
upstream: add support for setting degraded through EDS
Jan 18, 2019
5f6fe9a
add admin output for new flag
Jan 18, 2019
c980745
PR feedback naming changes, remove std::cerrs
Jan 22, 2019
8f413f5
Merge remote-tracking branch 'origin/master' into degraded-eds
Jan 29, 2019
0071ab8
clean up merge
Jan 30, 2019
aa99b0e
specify FALLTHRU and add integration test coverage
Feb 4, 2019
5cb9477
Merge remote-tracking branch 'origin/master' into degraded-eds
Feb 4, 2019
198eb6e
fix bad merge
Feb 5, 2019
3626ea7
move setEdsFlag to HostImpl ctor, add test description
Feb 5, 2019
44015a1
fix load balancer simulation
Feb 6, 2019
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
3 changes: 3 additions & 0 deletions api/envoy/api/v2/core/health_check.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,4 +251,7 @@ enum HealthStatus {
// Health check timed out. This is part of HDS and is interpreted by Envoy as
// *UNHEALTHY*.
TIMEOUT = 4;

// Degraded.
DEGRADED = 5;
}
4 changes: 3 additions & 1 deletion include/envoy/upstream/upstream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ class Host : virtual public HostDescription {
/* The host is currently marked as unhealthy by EDS. */ \
m(FAILED_EDS_HEALTH, 0x04) \
/* The host is currently marked as degraded through active health checking. */ \
m(DEGRADED_ACTIVE_HC, 0x08)
m(DEGRADED_ACTIVE_HC, 0x08) \
/* The host is currently marked as degraded by EDS. */ \
m(DEGRADED_EDS_HEALTH, 0x10)

@htuch htuch Jan 31, 2019

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.

Can you remind me why we can't just use FAILED_EDS_HEALTH for this use case? AFAICT it fits the bill, and looking at the underlying issue being fixed, it seemed we just needed some plumbing around that.

@snowp snowp Feb 1, 2019

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.

Because this is for degrading an endpoint, not marking it as unhealthy. The new value is necessary to differentiate it here https://github.com/envoyproxy/envoy/pull/5649/files#diff-583237ddb4e16f38ccda2e9affdb0ad8R210 from the host being marked as unhealthy.

Degraded docs if you're not familiar: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/load_balancing/degraded

@snowp snowp Feb 1, 2019

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.

Maybe it wasn't super clear, but this PR adds support for marking endpoints as degraded, and while I was in here I also made sure to fix #5637

@htuch htuch Feb 1, 2019

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.

Thanks, that clarifies.

// clang-format on

#define DECLARE_ENUM(name, value) name = value,
Expand Down
2 changes: 1 addition & 1 deletion source/common/upstream/eds.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources, const std::
for (const auto& lb_endpoint : locality_lb_endpoint.lb_endpoints()) {
priority_state_manager.registerHostForPriority(
"", resolveProtoAddress(lb_endpoint.endpoint().address()), locality_lb_endpoint,
lb_endpoint, Host::HealthFlag::FAILED_EDS_HEALTH);
lb_endpoint);
}
}

Expand Down
11 changes: 6 additions & 5 deletions source/common/upstream/health_discovery_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,11 +204,12 @@ HdsCluster::HdsCluster(Server::Admin& admin, Runtime::Loader& runtime,
dispatcher, random, singleton_manager, tls, api});

for (const auto& host : cluster.hosts()) {
initial_hosts_->emplace_back(new HostImpl(
info_, "", Network::Address::resolveProtoAddress(host),
envoy::api::v2::core::Metadata::default_instance(), 1,
envoy::api::v2::core::Locality().default_instance(),
envoy::api::v2::endpoint::Endpoint::HealthCheckConfig().default_instance(), 0));
initial_hosts_->emplace_back(
new HostImpl(info_, "", Network::Address::resolveProtoAddress(host),
envoy::api::v2::core::Metadata::default_instance(), 1,
envoy::api::v2::core::Locality().default_instance(),
envoy::api::v2::endpoint::Endpoint::HealthCheckConfig().default_instance(), 0,
envoy::api::v2::core::HealthStatus::UNKNOWN));
}
initialize([] {});
}
Expand Down
3 changes: 1 addition & 2 deletions source/common/upstream/logical_dns_cluster.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,7 @@ void LogicalDnsCluster::startResolve() {
const auto& locality_lb_endpoint = localityLbEndpoint();
PriorityStateManager priority_state_manager(*this, local_info_);
priority_state_manager.initializePriorityFor(locality_lb_endpoint);
priority_state_manager.registerHostForPriority(logical_host_, locality_lb_endpoint,
lbEndpoint(), absl::nullopt);
priority_state_manager.registerHostForPriority(logical_host_, locality_lb_endpoint);

const uint32_t priority = locality_lb_endpoint.priority();
priority_state_manager.updateClusterPrioritySet(
Expand Down
2 changes: 1 addition & 1 deletion source/common/upstream/logical_dns_cluster.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class LogicalDnsCluster : public ClusterImplBase {
parent.lbEndpoint().load_balancing_weight().value(),
parent.localityLbEndpoint().locality(),
parent.lbEndpoint().endpoint().health_check_config(),
parent.localityLbEndpoint().priority()),
parent.localityLbEndpoint().priority(), parent.lbEndpoint().health_status()),
parent_(parent) {}

// Upstream::Host
Expand Down
11 changes: 6 additions & 5 deletions source/common/upstream/original_dst_cluster.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,12 @@ HostConstSharedPtr OriginalDstCluster::LoadBalancer::chooseHost(LoadBalancerCont
Network::Address::InstanceConstSharedPtr host_ip_port(
Network::Utility::copyInternetAddressAndPort(*dst_ip));
// Create a host we can use immediately.
host.reset(new HostImpl(
info_, info_->name() + dst_addr.asString(), std::move(host_ip_port),
envoy::api::v2::core::Metadata::default_instance(), 1,
envoy::api::v2::core::Locality().default_instance(),
envoy::api::v2::endpoint::Endpoint::HealthCheckConfig().default_instance(), 0));
host.reset(
new HostImpl(info_, info_->name() + dst_addr.asString(), std::move(host_ip_port),
envoy::api::v2::core::Metadata::default_instance(), 1,
envoy::api::v2::core::Locality().default_instance(),
envoy::api::v2::endpoint::Endpoint::HealthCheckConfig().default_instance(),
0, envoy::api::v2::core::HealthStatus::UNKNOWN));

ENVOY_LOG(debug, "Created host {}.", host->address()->asString());
// Add the new host to the map. We just failed to find it in
Expand Down
93 changes: 55 additions & 38 deletions source/common/upstream/upstream_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,30 @@ parseExtensionProtocolOptions(const envoy::api::v2::Cluster& config) {
return options;
}

// Updates the health flags for an existing host to match the new host.
// @param updated_host the new host to read health flag values from.
// @param existing_host the host to update.
// @param flag the health flag to update.
// @return bool whether the flag update caused the host health to change.
bool updateHealthFlag(const Host& updated_host, Host& existing_host, Host::HealthFlag flag) {
// Check if the health flag has changed.
if (existing_host.healthFlagGet(flag) != updated_host.healthFlagGet(flag)) {
// Keep track of the previous health value of the host.
const auto previous_health = existing_host.health();

if (updated_host.healthFlagGet(flag)) {
existing_host.healthFlagSet(flag);
} else {
existing_host.healthFlagClear(flag);
}

// Rebuild if changing the flag affected the host health.
return previous_health != existing_host.health();
}

return false;
}

} // namespace

Host::CreateConnectionData HostImpl::createConnection(
Expand All @@ -179,6 +203,24 @@ Host::CreateConnectionData HostImpl::createConnection(
shared_from_this()};
}

void HostImpl::setEdsHealthFlag(envoy::api::v2::core::HealthStatus health_status) {
switch (health_status) {
case envoy::api::v2::core::HealthStatus::UNHEALTHY:

@htuch htuch Feb 1, 2019

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.

Nit: Maybe add fall-thru annotations or comments here.

FALLTHRU;
case envoy::api::v2::core::HealthStatus::DRAINING:
FALLTHRU;
case envoy::api::v2::core::HealthStatus::TIMEOUT:
healthFlagSet(Host::HealthFlag::FAILED_EDS_HEALTH);
break;
case envoy::api::v2::core::HealthStatus::DEGRADED:
healthFlagSet(Host::HealthFlag::DEGRADED_EDS_HEALTH);
break;
default:;
break;
// No health flags should be set.
}
}

Host::CreateConnectionData
HostImpl::createHealthCheckConnection(Event::Dispatcher& dispatcher) const {
return {createConnection(dispatcher, *cluster_, healthCheckAddress(), nullptr, nullptr),
Expand Down Expand Up @@ -863,32 +905,22 @@ void PriorityStateManager::initializePriorityFor(
void PriorityStateManager::registerHostForPriority(
const std::string& hostname, Network::Address::InstanceConstSharedPtr address,
const envoy::api::v2::endpoint::LocalityLbEndpoints& locality_lb_endpoint,
const envoy::api::v2::endpoint::LbEndpoint& lb_endpoint,
const absl::optional<Upstream::Host::HealthFlag> health_checker_flag) {
const envoy::api::v2::endpoint::LbEndpoint& lb_endpoint) {
const HostSharedPtr host(
new HostImpl(parent_.info(), hostname, address, lb_endpoint.metadata(),
lb_endpoint.load_balancing_weight().value(), locality_lb_endpoint.locality(),
lb_endpoint.endpoint().health_check_config(), locality_lb_endpoint.priority()));
registerHostForPriority(host, locality_lb_endpoint, lb_endpoint, health_checker_flag);
lb_endpoint.endpoint().health_check_config(), locality_lb_endpoint.priority(),
lb_endpoint.health_status()));
registerHostForPriority(host, locality_lb_endpoint);
}

void PriorityStateManager::registerHostForPriority(
const HostSharedPtr& host,
const envoy::api::v2::endpoint::LocalityLbEndpoints& locality_lb_endpoint,
const envoy::api::v2::endpoint::LbEndpoint& lb_endpoint,
const absl::optional<Upstream::Host::HealthFlag> health_checker_flag) {
const envoy::api::v2::endpoint::LocalityLbEndpoints& locality_lb_endpoint) {
const uint32_t priority = locality_lb_endpoint.priority();
// Should be called after initializePriorityFor.
ASSERT(priority_state_[priority].first);
priority_state_[priority].first->emplace_back(host);
if (health_checker_flag.has_value()) {
const auto& health_status = lb_endpoint.health_status();
if (health_status == envoy::api::v2::core::HealthStatus::UNHEALTHY ||
health_status == envoy::api::v2::core::HealthStatus::DRAINING ||
health_status == envoy::api::v2::core::HealthStatus::TIMEOUT) {
priority_state_[priority].first->back()->healthFlagSet(health_checker_flag.value());
}
}
}

void PriorityStateManager::updateClusterPrioritySet(
Expand Down Expand Up @@ -982,7 +1014,7 @@ StaticClusterImpl::StaticClusterImpl(
for (const auto& lb_endpoint : locality_lb_endpoint.lb_endpoints()) {
priority_state_manager_->registerHostForPriority(
"", resolveProtoAddress(lb_endpoint.endpoint().address()), locality_lb_endpoint,
lb_endpoint, absl::nullopt);
lb_endpoint);
}
}
}
Expand Down Expand Up @@ -1069,24 +1101,10 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts,
max_host_weight = host->weight();
}

if (existing_host->second->healthFlagGet(Host::HealthFlag::FAILED_EDS_HEALTH) !=
host->healthFlagGet(Host::HealthFlag::FAILED_EDS_HEALTH)) {
// TODO(snowp): To accommodate degraded, this bit should be checking for any changes
// to the health flag, not just healthy vs not healthy.
const bool previously_healthy = existing_host->second->health() == Host::Health::Healthy;
if (host->healthFlagGet(Host::HealthFlag::FAILED_EDS_HEALTH)) {
existing_host->second->healthFlagSet(Host::HealthFlag::FAILED_EDS_HEALTH);
// If the host was previously healthy and we're now unhealthy, we need to
// rebuild.
hosts_changed |= previously_healthy;
} else {
existing_host->second->healthFlagClear(Host::HealthFlag::FAILED_EDS_HEALTH);
// If the host was previously unhealthy and now healthy, we need to
// rebuild.
hosts_changed |=
!previously_healthy && existing_host->second->health() == Host::Health::Healthy;
}
}
hosts_changed |=
updateHealthFlag(*host, *existing_host->second, Host::HealthFlag::FAILED_EDS_HEALTH);
hosts_changed |=
updateHealthFlag(*host, *existing_host->second, Host::HealthFlag::DEGRADED_EDS_HEALTH);

// Did metadata change?
const bool metadata_changed = !Protobuf::util::MessageDifferencer::Equivalent(
Expand Down Expand Up @@ -1259,8 +1277,7 @@ void StrictDnsClusterImpl::updateAllHosts(const HostVector& hosts_added,
priority_state_manager.initializePriorityFor(target->locality_lb_endpoint_);
for (const HostSharedPtr& host : target->hosts_) {
if (target->locality_lb_endpoint_.priority() == current_priority) {
priority_state_manager.registerHostForPriority(host, target->locality_lb_endpoint_,
target->lb_endpoint_, absl::nullopt);
priority_state_manager.registerHostForPriority(host, target->locality_lb_endpoint_);
}
}
}
Expand Down Expand Up @@ -1309,7 +1326,7 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() {
parent_.info_, dns_address_, Network::Utility::getAddressWithPort(*address, port_),
lb_endpoint_.metadata(), lb_endpoint_.load_balancing_weight().value(),
locality_lb_endpoint_.locality(), lb_endpoint_.endpoint().health_check_config(),
locality_lb_endpoint_.priority()));
locality_lb_endpoint_.priority(), lb_endpoint_.health_status()));
}

HostVector hosts_added;
Expand Down Expand Up @@ -1337,4 +1354,4 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() {
}

} // namespace Upstream
} // namespace Envoy
} // namespace Envoy
19 changes: 10 additions & 9 deletions source/common/upstream/upstream_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,11 @@ class HostImpl : public HostDescriptionImpl,
const envoy::api::v2::core::Metadata& metadata, uint32_t initial_weight,
const envoy::api::v2::core::Locality& locality,
const envoy::api::v2::endpoint::Endpoint::HealthCheckConfig& health_check_config,
uint32_t priority)
uint32_t priority, const envoy::api::v2::core::HealthStatus health_status)
: HostDescriptionImpl(cluster, hostname, address, metadata, locality, health_check_config,
priority),
used_(true) {
setEdsHealthFlag(health_status);
weight(initial_weight);
}

Expand Down Expand Up @@ -206,7 +207,8 @@ class HostImpl : public HostDescriptionImpl,
}

// Only possible option at this point is that the host is degraded.
ASSERT(health_flags_ == static_cast<uint32_t>(HealthFlag::DEGRADED_ACTIVE_HC));
ASSERT(healthFlagGet(HealthFlag::DEGRADED_ACTIVE_HC) ||
healthFlagGet(HealthFlag::DEGRADED_EDS_HEALTH));
return Host::Health::Degraded;
}

Expand All @@ -223,6 +225,8 @@ class HostImpl : public HostDescriptionImpl,
Network::TransportSocketOptionsSharedPtr transport_socket_options);

private:
void setEdsHealthFlag(envoy::api::v2::core::HealthStatus health_status);

std::atomic<uint64_t> health_flags_{};
ActiveHealthFailureType active_health_failure_type_{};
std::atomic<uint32_t> weight_;
Expand Down Expand Up @@ -666,14 +670,11 @@ class PriorityStateManager : protected Logger::Loggable<Logger::Id::upstream> {
registerHostForPriority(const std::string& hostname,
Network::Address::InstanceConstSharedPtr address,
const envoy::api::v2::endpoint::LocalityLbEndpoints& locality_lb_endpoint,
const envoy::api::v2::endpoint::LbEndpoint& lb_endpoint,
const absl::optional<Upstream::Host::HealthFlag> health_checker_flag);
const envoy::api::v2::endpoint::LbEndpoint& lb_endpoint);

void
registerHostForPriority(const HostSharedPtr& host,
const envoy::api::v2::endpoint::LocalityLbEndpoints& locality_lb_endpoint,
const envoy::api::v2::endpoint::LbEndpoint& lb_endpoint,
const absl::optional<Upstream::Host::HealthFlag> health_checker_flag);
void registerHostForPriority(
const HostSharedPtr& host,
const envoy::api::v2::endpoint::LocalityLbEndpoints& locality_lb_endpoint);

void
updateClusterPrioritySet(const uint32_t priority, HostVectorSharedPtr&& current_hosts,
Expand Down
12 changes: 8 additions & 4 deletions source/server/http/admin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,10 +169,14 @@ void setHealthFlag(Upstream::Host::HealthFlag flag, const Upstream::Host& host,
host.healthFlagGet(Upstream::Host::HealthFlag::FAILED_OUTLIER_CHECK));
break;
case Upstream::Host::HealthFlag::FAILED_EDS_HEALTH:
health_status.set_eds_health_status(
host.healthFlagGet(Upstream::Host::HealthFlag::FAILED_EDS_HEALTH)
? envoy::api::v2::core::HealthStatus::UNHEALTHY
: envoy::api::v2::core::HealthStatus::HEALTHY);
case Upstream::Host::HealthFlag::DEGRADED_EDS_HEALTH:
if (host.healthFlagGet(Upstream::Host::HealthFlag::FAILED_EDS_HEALTH)) {
health_status.set_eds_health_status(envoy::api::v2::core::HealthStatus::UNHEALTHY);
} else if (host.healthFlagGet(Upstream::Host::HealthFlag::DEGRADED_EDS_HEALTH)) {
health_status.set_eds_health_status(envoy::api::v2::core::HealthStatus::DEGRADED);
} else {
health_status.set_eds_health_status(envoy::api::v2::core::HealthStatus::HEALTHY);
}
break;
case Upstream::Host::HealthFlag::DEGRADED_ACTIVE_HC:
health_status.set_failed_active_degraded_check(
Expand Down
30 changes: 30 additions & 0 deletions test/common/upstream/eds_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ TEST_F(EdsTest, EndpointHealthStatus) {
{envoy::api::v2::core::HealthStatus::UNHEALTHY, Host::Health::Unhealthy},
{envoy::api::v2::core::HealthStatus::DRAINING, Host::Health::Unhealthy},
{envoy::api::v2::core::HealthStatus::TIMEOUT, Host::Health::Unhealthy},
{envoy::api::v2::core::HealthStatus::DEGRADED, Host::Health::Degraded},
};

int port = 80;
Expand Down Expand Up @@ -423,6 +424,35 @@ TEST_F(EdsTest, EndpointHealthStatus) {
hosts[0]->healthFlagClear(Host::HealthFlag::FAILED_ACTIVE_HC);
EXPECT_EQ(Host::Health::Healthy, hosts[0]->health());
}

const auto rebuild_container = stats_.counter("cluster.name.update_no_rebuild").value();
// Now mark host 0 degraded via EDS, it should be degraded.
endpoints->mutable_lb_endpoints(0)->set_health_status(
envoy::api::v2::core::HealthStatus::DEGRADED);
VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, ""));
{
auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts();
EXPECT_EQ(Host::Health::Degraded, hosts[0]->health());
}

// We should rebuild the cluster since we went from healthy -> degraded.
EXPECT_EQ(rebuild_container, stats_.counter("cluster.name.update_no_rebuild").value());

// Now mark the host as having been degraded through active hc.
cluster_->prioritySet().hostSetsPerPriority()[0]->hosts()[0]->healthFlagSet(
Host::HealthFlag::DEGRADED_ACTIVE_HC);

// Now mark host 0 healthy via EDS, it should still be degraded.
endpoints->mutable_lb_endpoints(0)->set_health_status(
envoy::api::v2::core::HealthStatus::HEALTHY);
VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, ""));
{
auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts();
EXPECT_EQ(Host::Health::Degraded, hosts[0]->health());
}

// Since the host health didn't change, expect no rebuild.
EXPECT_EQ(rebuild_container + 1, stats_.counter("cluster.name.update_no_rebuild").value());
}

// Validate that onConfigUpdate() removes endpoints that are marked as healthy
Expand Down
Loading