Skip to content

healthcheck filter: compute response based on upstream cluster health #2387

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
zuercher merged 8 commits into from
Jan 23, 2018
Merged

healthcheck filter: compute response based on upstream cluster health #2387

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9b7eb01
healthcheck filter: compute response based on upstream cluster health
Jan 16, 2018
76da1ac
Code cleanups and a new test case for empty cluster with min health = 0%
Jan 21, 2018
ed13504
Update release notes
Jan 21, 2018
0985b13
Fix thread-safety and clean up based on code review feedback
Jan 21, 2018
0235233
Small cleanup: remove unneeded initializer
Jan 21, 2018
d665fd8
Add "Const" to shared pointer typedef name and fix comment typos
Jan 22, 2018
ca89315
Merge master
Jan 22, 2018
8a06a78
Remove redundant ".Times(1)" from unit tests
Jan 22, 2018
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 RAW_RELEASE_NOTES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,4 @@ final version.
* Added support for route matching based on URL query string parameters.
:ref:`QueryParameterMatcher<envoy_api_msg_QueryParameterMatcher>`
* Added `/runtime` admin endpoint to read the current runtime values.
* Extended the health check filter to support computation of the health check resopnse based on the percent of healthy servers is upstream clusters.

@mattklein123 mattklein123 Jan 21, 2018

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.

typo "resopnse". Also, nit, please try to wrap around 100 cols

2 changes: 1 addition & 1 deletion bazel/repository_locations.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ REPOSITORY_LOCATIONS = dict(
urls = ["https://github.com/google/protobuf/archive/v3.5.0.tar.gz"],
),
envoy_api = dict(
commit = "f268484d221509b175c46cce6c4cc3593a0ede5e",
commit = "b4a303154703d19f49b63e5529877772ce5da617",
remote = "https://github.com/envoyproxy/data-plane-api",
),
grpc_httpjson_transcoding = dict(
Expand Down
43 changes: 39 additions & 4 deletions source/server/http/health_check.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,19 @@ HealthCheckFilterConfig::createFilter(const envoy::api::v2::filter::http::Health
std::chrono::milliseconds(cache_time_ms)));
}

return [&context, pass_through_mode, cache_manager,
hc_endpoint](Http::FilterChainFactoryCallbacks& callbacks) -> void {
callbacks.addStreamFilter(Http::StreamFilterSharedPtr{
new HealthCheckFilter(context, pass_through_mode, cache_manager, hc_endpoint)});
ClusterMinHealthyPercentagesSharedPtr cluster_min_healthy_percentages;
if (!pass_through_mode && !proto_config.cluster_min_healthy_percentages().empty()) {
auto* cluster_to_percentage = new ClusterMinHealthyPercentages();

@mattklein123 mattklein123 Jan 21, 2018

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.

please avoid naked memory allocations. I would either a) assign to unique_ptr than release/move into the final shared_ptr, or b) assign into a non-const shared_ptr which I think might be convertible to the const one (not sure).

for (const auto& item : proto_config.cluster_min_healthy_percentages()) {
cluster_to_percentage->emplace(std::make_pair(item.first, item.second.value()));
}
cluster_min_healthy_percentages.reset(cluster_to_percentage);
}

return [&context, pass_through_mode, cache_manager, hc_endpoint,
cluster_min_healthy_percentages](Http::FilterChainFactoryCallbacks& callbacks) -> void {
callbacks.addStreamFilter(Http::StreamFilterSharedPtr{new HealthCheckFilter(

@mattklein123 mattklein123 Jan 21, 2018

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: not your code but can you switch to std::make_shared?

context, pass_through_mode, cache_manager, hc_endpoint, cluster_min_healthy_percentages)});
};
}

Expand Down Expand Up @@ -155,6 +164,32 @@ void HealthCheckFilter::onComplete() {
Http::Code final_status = Http::Code::OK;
if (cache_manager_) {
final_status = cache_manager_->getCachedResponseCode();
} else if (cluster_min_healthy_percentages_ != nullptr &&
!cluster_min_healthy_percentages_->empty()) {
const auto clusters(context_.clusterManager().clusters());

@mattklein123 mattklein123 Jan 21, 2018

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.

Unfortunately this is not safe. clusters() right now is only safe to be called from the main thread (not workers) since there is no locking. What you actually want to do is go through your targets clusters and call get() which will give you the thread local version which I think should be sufficient for the computations you need.

for (const auto& item : *cluster_min_healthy_percentages_) {
const std::string& cluster_name = item.first;
const double min_healthy_percentage = item.second;
auto match = clusters.find(cluster_name);
if (match == clusters.end()) {
final_status = Http::Code::ServiceUnavailable;

@mattklein123 mattklein123 Jan 21, 2018

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 add some comments here? I guess I see how the lack of the cluster at all is sufficient ground for failure, though I wonder if there should be a stat. At the very least I would probably add a small comment.

break;
}
const auto& stats = match->second.get().info()->stats();
const uint64_t membership_total = stats.membership_total_.value();
if (membership_total == 0) {
if (min_healthy_percentage == 0.0) {
continue;
} else {
final_status = Http::Code::ServiceUnavailable;
break;
}
}
if (stats.membership_healthy_.value() < membership_total * min_healthy_percentage / 100.0) {
final_status = Http::Code::ServiceUnavailable;
break;
}
}
}

if (!Http::CodeUtility::is2xx(enumToInt(final_status))) {
Expand Down
9 changes: 7 additions & 2 deletions source/server/http/health_check.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,19 @@ class HealthCheckCacheManager {

typedef std::shared_ptr<HealthCheckCacheManager> HealthCheckCacheManagerSharedPtr;

typedef std::map<std::string, double> ClusterMinHealthyPercentages;
typedef std::shared_ptr<const ClusterMinHealthyPercentages> ClusterMinHealthyPercentagesSharedPtr;

@mattklein123 mattklein123 Jan 21, 2018

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.

ClusterMinHealthyPercentagesConstSharedPtr


/**
* Health check responder filter.
*/
class HealthCheckFilter : public Http::StreamFilter {
public:
HealthCheckFilter(Server::Configuration::FactoryContext& context, bool pass_through_mode,
HealthCheckCacheManagerSharedPtr cache_manager, const std::string& endpoint)
HealthCheckCacheManagerSharedPtr cache_manager, const std::string& endpoint,
ClusterMinHealthyPercentagesSharedPtr cluster_min_healthy_percentages)
: context_(context), pass_through_mode_(pass_through_mode), cache_manager_(cache_manager),
endpoint_(endpoint) {}
endpoint_(endpoint), cluster_min_healthy_percentages_(cluster_min_healthy_percentages) {}

// Http::StreamFilterBase
void onDestroy() override {}
Expand Down Expand Up @@ -109,5 +113,6 @@ class HealthCheckFilter : public Http::StreamFilter {
bool pass_through_mode_{};
HealthCheckCacheManagerSharedPtr cache_manager_{};
const std::string endpoint_;
ClusterMinHealthyPercentagesSharedPtr cluster_min_healthy_percentages_{};

@mattklein123 mattklein123 Jan 21, 2018

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: {} not needed

};
} // namespace Envoy
110 changes: 108 additions & 2 deletions test/server/http/health_check_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@
#include <memory>

#include "common/buffer/buffer_impl.h"
#include "common/upstream/upstream_impl.h"

#include "server/http/health_check.h"

#include "test/mocks/server/mocks.h"
#include "test/mocks/upstream/cluster_info.h"
#include "test/test_common/printers.h"
#include "test/test_common/utility.h"

Expand Down Expand Up @@ -36,8 +38,11 @@ class HealthCheckFilterTest : public testing::Test {
prepareFilter(pass_through);
}

void prepareFilter(bool pass_through) {
filter_.reset(new HealthCheckFilter(context_, pass_through, cache_manager_, "/healthcheck"));
void
prepareFilter(bool pass_through,
ClusterMinHealthyPercentagesSharedPtr cluster_min_healthy_percentages = nullptr) {
filter_.reset(new HealthCheckFilter(context_, pass_through, cache_manager_, "/healthcheck",
cluster_min_healthy_percentages));
filter_->setDecoderFilterCallbacks(callbacks_);
}

Expand All @@ -49,6 +54,14 @@ class HealthCheckFilterTest : public testing::Test {
NiceMock<Http::MockStreamDecoderFilterCallbacks> callbacks_;
Http::TestHeaderMapImpl request_headers_;
Http::TestHeaderMapImpl request_headers_no_hc_;

class MockHealthCheckCluster : public NiceMock<Upstream::MockCluster> {
public:
MockHealthCheckCluster(uint64_t membership_total, uint64_t membership_healthy) {
info()->stats().membership_total_.set(membership_total);
info()->stats().membership_healthy_.set(membership_healthy);
}
};
};

class HealthCheckFilterNoPassThroughTest : public HealthCheckFilterTest {
Expand Down Expand Up @@ -84,6 +97,99 @@ TEST_F(HealthCheckFilterNoPassThroughTest, NotHcRequest) {
EXPECT_STREQ("true", service_response.EnvoyImmediateHealthCheckFail()->value().c_str());
}

TEST_F(HealthCheckFilterNoPassThroughTest, ComputedHealth) {
// Test non-pass-through health checks without upstream cluster minimum health specified.
prepareFilter(false);
{
Http::TestHeaderMapImpl health_check_response{{":status", "200"}};
EXPECT_CALL(context_, healthCheckFailed()).WillOnce(Return(false));
EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&health_check_response), true))

@mattklein123 mattklein123 Jan 22, 2018

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: .Times(1) is redundant (it's implied). You can remove. Same below.

.Times(1);
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
filter_->decodeHeaders(request_headers_, true));
}
{
Http::TestHeaderMapImpl health_check_response{{":status", "503"}};
EXPECT_CALL(context_, healthCheckFailed()).WillOnce(Return(true));
EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&health_check_response), true))
.Times(1);
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
filter_->decodeHeaders(request_headers_, true));
}

// Test non-pass-through health checks with upstream cluster minimum health specified.
prepareFilter(false, ClusterMinHealthyPercentagesSharedPtr(
new ClusterMinHealthyPercentages{{"www1", 50.0}, {"www2", 75.0}}));
{
// This should pass, because each upstream cluster has at least the
// minimum percentage of healthy servers.
Http::TestHeaderMapImpl health_check_response{{":status", "200"}};
MockHealthCheckCluster cluster_www1(100, 50);
MockHealthCheckCluster cluster_www2(1000, 800);
Upstream::ClusterManager::ClusterInfoMap cluster_info_map{
{"www1", std::reference_wrapper<const Upstream::Cluster>(cluster_www1)},
{"www2", std::reference_wrapper<const Upstream::Cluster>(cluster_www2)}};
EXPECT_CALL(context_, healthCheckFailed()).WillOnce(Return(false));
EXPECT_CALL(context_, clusterManager()).Times(1);
EXPECT_CALL(context_.cluster_manager_, clusters()).WillOnce(Return(cluster_info_map));
EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&health_check_response), true))
.Times(1);
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
filter_->decodeHeaders(request_headers_, true));
}
{
// This should fail, because one upstream cluster has too few healthy servers.
Http::TestHeaderMapImpl health_check_response{{":status", "503"}};
MockHealthCheckCluster cluster_www1(100, 49);
MockHealthCheckCluster cluster_www2(1000, 800);
Upstream::ClusterManager::ClusterInfoMap cluster_info_map{
{"www1", std::reference_wrapper<const Upstream::Cluster>(cluster_www1)},
{"www2", std::reference_wrapper<const Upstream::Cluster>(cluster_www2)}};
EXPECT_CALL(context_, healthCheckFailed()).WillOnce(Return(false));
EXPECT_CALL(context_, clusterManager()).Times(1);
EXPECT_CALL(context_.cluster_manager_, clusters()).WillOnce(Return(cluster_info_map));
EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&health_check_response), true))
.Times(1);
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
filter_->decodeHeaders(request_headers_, true));
}
{
// This should fail, because one upstream cluster has no servers at all.

@zuercher zuercher Jan 18, 2018

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.

Consider adding a test for the empty cluster but min health == 0% case.

Http::TestHeaderMapImpl health_check_response{{":status", "503"}};
MockHealthCheckCluster cluster_www1(0, 0);
MockHealthCheckCluster cluster_www2(1000, 800);
Upstream::ClusterManager::ClusterInfoMap cluster_info_map{
{"www1", std::reference_wrapper<const Upstream::Cluster>(cluster_www1)},
{"www2", std::reference_wrapper<const Upstream::Cluster>(cluster_www2)}};
EXPECT_CALL(context_, healthCheckFailed()).WillOnce(Return(false));
EXPECT_CALL(context_, clusterManager()).Times(1);
EXPECT_CALL(context_.cluster_manager_, clusters()).WillOnce(Return(cluster_info_map));
EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&health_check_response), true))
.Times(1);
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
filter_->decodeHeaders(request_headers_, true));
}
// Test the cases where an upstream cluster is empty, or has no healthy servers, but
// the minimum required percent healthy is zero. The hhealth check should return a 200.

@zuercher zuercher Jan 22, 2018

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.

typo: hhealth

prepareFilter(false, ClusterMinHealthyPercentagesSharedPtr(
new ClusterMinHealthyPercentages{{"www1", 0.0}, {"www2", 0.0}}));
{
Http::TestHeaderMapImpl health_check_response{{":status", "200"}};
MockHealthCheckCluster cluster_www1(0, 0);
MockHealthCheckCluster cluster_www2(1000, 0);
Upstream::ClusterManager::ClusterInfoMap cluster_info_map{
{"www1", std::reference_wrapper<const Upstream::Cluster>(cluster_www1)},
{"www2", std::reference_wrapper<const Upstream::Cluster>(cluster_www2)}};
EXPECT_CALL(context_, healthCheckFailed()).WillOnce(Return(false));
EXPECT_CALL(context_, clusterManager()).Times(1);
EXPECT_CALL(context_.cluster_manager_, clusters()).WillOnce(Return(cluster_info_map));
EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&health_check_response), true))
.Times(1);
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
filter_->decodeHeaders(request_headers_, true));
}
}

TEST_F(HealthCheckFilterNoPassThroughTest, HealthCheckFailedCallbackCalled) {
EXPECT_CALL(context_, healthCheckFailed()).WillOnce(Return(true));
EXPECT_CALL(callbacks_.request_info_, healthCheck(true));
Expand Down