Skip to content

health check: gracefully handle GOAWAY in grpc health checker #11324

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
snowp merged 20 commits into from
Jun 24, 2020
Merged

health check: gracefully handle GOAWAY in grpc health checker #11324

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
e4ddb8e
health check: gracefully handle GOAWAY in grpc health checker
jmuia May 26, 2020
70263a6
Close connection after logging and add unit test for remote close
jmuia May 27, 2020
a069698
wip
murray-stripe Jun 5, 2020
725caf8
wip - plumb through
jmuia Jun 9, 2020
58d2143
Merge branch 'envoyproxy/master' into jmuia.grpc-hc-goaway
jmuia Jun 10, 2020
00a1fea
fix format with 'tools/code_format/check_format.py fix'
jmuia Jun 10, 2020
1828a2b
Revert "fix format with 'tools/code_format/check_format.py fix'"
murray-stripe Jun 10, 2020
64c6aef
Merge pull request #1 from jmuia/wip/grpc-hc-goaway
murray-stripe Jun 10, 2020
6ba45a4
only handle GOAWAY NO_ERROR gracefully
jmuia Jun 10, 2020
c5958dc
add inline comments
jmuia Jun 10, 2020
b474edd
make quic error mapper consistent with http2
jmuia Jun 10, 2020
9171180
fix up declaration
jmuia Jun 11, 2020
73e7308
run onGoAway callbacks for each GOAWAY received
jmuia Jun 11, 2020
dc5effd
rename ErrorCode to GoAwayErrorCode
jmuia Jun 15, 2020
7c7d930
rename received_goaway_ to received_no_error_goaway_
jmuia Jun 15, 2020
41ea1e1
fix format
jmuia Jun 16, 2020
a66c176
add comment to GoAwayErrorCode enum
jmuia Jun 16, 2020
bf9c59c
Revert "run onGoAway callbacks for each GOAWAY received"
jmuia Jun 24, 2020
878bd4a
Add TODO for handling multiple GOAWAY frames
jmuia Jun 24, 2020
541eb54
kick ci
jmuia Jun 24, 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
29 changes: 21 additions & 8 deletions source/common/upstream/health_checker_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,17 +703,24 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onInterval() {
void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onResetStream(Http::StreamResetReason,
absl::string_view) {
const bool expected_reset = expect_reset_;
const bool goaway = received_goaway_;
resetState();

if (expected_reset) {
// Stream reset was initiated by us (bogus gRPC response, timeout or cluster host is going
// away). In these cases health check failure has already been reported, so just return.
// away). In these cases health check failure has already been reported and a GOAWAY (if any)
// has already been handled, so just return.
return;
}

ENVOY_CONN_LOG(debug, "connection/stream error health_flags={}", *client_,
HostUtility::healthFlagsToString(*host_));

if (goaway) {
// Stream reset was unexpected, so GOAWAY hasn't been handled yet.
client_->close();
}

// TODO(baranov1ch): according to all HTTP standards, we should check if reason is one of
// Http::StreamResetReason::RemoteRefusedStreamReset (which may mean GOAWAY),
// Http::StreamResetReason::RemoteReset or Http::StreamResetReason::ConnectionTermination (both
Expand All @@ -725,13 +732,12 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onResetStream(Http::St
void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onGoAway() {
ENVOY_CONN_LOG(debug, "connection going away health_flags={}", *client_,
HostUtility::healthFlagsToString(*host_));
// Even if we have active health check probe, fail it on GOAWAY and schedule new one.
// If we have an active health check probe, allow it to complete before closing the connection.
if (request_encoder_) {
handleFailure(envoy::data::core::v3::NETWORK);
expect_reset_ = true;
request_encoder_->getStream().resetStream(Http::StreamResetReason::LocalReset);
received_goaway_ = true;

@snowp snowp Jun 2, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A one liner comment here about when this connection would eventually get closed would be nice. IIUC it's either on the next timeout or when the remote closes it?

@jmuia jmuia Jun 3, 2020

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.

I'll add a comment. It would be closed when the active check completes (onRpcComplete) or other conditions like onResetStream and onTimeout.

} else {
client_->close();
}
client_->close();
}

bool GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::isHealthCheckSucceeded(
Expand All @@ -757,6 +763,8 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onRpcComplete(
handleFailure(envoy::data::core::v3::ACTIVE);
}

const bool goaway = received_goaway_;

@snowp snowp Jun 2, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mind adding a comment here explaining why we have to read the value here? Its a bit less clear than on L705 since we're not unconditionally calling resetState


// |end_stream| will be false if we decided to stop healthcheck before HTTP stream has ended -
// invalid gRPC payload, unexpected message stream or wrong content-type.
if (end_stream) {
Expand All @@ -767,7 +775,7 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onRpcComplete(
request_encoder_->getStream().resetStream(Http::StreamResetReason::LocalReset);
}

if (!parent_.reuse_connection_) {
if (!parent_.reuse_connection_ || goaway) {
client_->close();
}
}
Expand All @@ -777,13 +785,18 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::resetState() {
request_encoder_ = nullptr;
decoder_ = Grpc::Decoder();
health_check_response_.reset();
received_goaway_ = false;
}

void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onTimeout() {
ENVOY_CONN_LOG(debug, "connection/stream timeout health_flags={}", *client_,
HostUtility::healthFlagsToString(*host_));
expect_reset_ = true;
Comment thread
snowp marked this conversation as resolved.
request_encoder_->getStream().resetStream(Http::StreamResetReason::LocalReset);
if (received_goaway_) {
client_->close();
} else {
request_encoder_->getStream().resetStream(Http::StreamResetReason::LocalReset);
}
}

void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::logHealthCheckStatus(
Expand Down
1 change: 1 addition & 0 deletions source/common/upstream/health_checker_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ class GrpcHealthCheckerImpl : public HealthCheckerImplBase {
// e.g. remote reset. In this case healthcheck status has already been reported, only state
// cleanup is required.
bool expect_reset_ = false;
bool received_goaway_ = false;
};

virtual Http::CodecClientPtr createCodecClient(Upstream::Host::CreateConnectionData& data) PURE;
Expand Down
173 changes: 164 additions & 9 deletions test/common/upstream/health_checker_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4529,21 +4529,176 @@ TEST_F(GrpcHealthCheckerImplTest, GrpcFailUnknownHealthStatus) {
cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->health());
}

// Test receiving GOAWAY is interpreted as connection close event.
// Test receiving GOAWAY is handled gracefully while a check is in progress.

@snowp snowp Jun 2, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This tells me that the code intentionally treated GOAWAY as failures, which makes me wonder if this is the desired behavior for some? Does this mean that this should be configurable?

@baranov1ch @htuch @mattklein123 WDYT

@htuch htuch Jun 2, 2020

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.

This might depend on the reason for the GOAWAY? I.e. a NO_ERROR mid check might be fine as it's telling to drain, but not if it was an error.

@jmuia jmuia Jun 3, 2020

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.

I see, that makes sense.

It looks like the GOAWAY error code isn't provided in the callback at the moment, but I think it would be reasonable to do that?

It seems that the onGoAway() callbacks are only fired once (see here), and - with error codes - we'd want to call them for each GOAWAY frame received. I'm not sure if that would be problematic. For reference it looks the "fire once" behaviour was added in #103.

It may also be worth mentioning that the http2 connection pool implementation doesn't change behaviour based on the GOAWAY error code at the moment (see here). It transitions the active client to the draining state if it has active requests.

@mattklein123 mattklein123 Jun 3, 2020

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.

Right, if the GOAWAY error code is NO_ERROR we should just treat it as "don't reuse connection", not an error. We may have to add some additional error code info to the callback. Note that @antoniovicente just opened #11420, though IIRC nghttp2 does provide the reason code inside the frame here:

envoy/source/common/http/http2/codec_impl.cc

Line 582 in 5d8f7d7

if (frame->hd.type == NGHTTP2_GOAWAY && !raised_goaway_) {
, we just need to store it.

@jmuia jmuia Jun 4, 2020

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.

IRC nghttp2 does provide the reason code inside the frame here:

Yep - it looks like it's provided in frame->goaway.error_code and the debug data in frame->goaway.opaque_data

I'm happy to update the onGoAway() interface as part of this PR if @antoniovicente doesn't already have something started.

Does something along these lines look right?

  # Maybe pass an Envoy-internal enum rather than the nghttp2 enum?
  virtual void onGoAway(nghttp2_error_code error_code, absl::Span<uint8_t> debug_data) PURE;

@mattklein123 mattklein123 Jun 4, 2020

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.

We should not leak nghttp2 specific structs to the public interface, so at least for the error code we should do a mapping. For your use case maybe just have an enum with {NO_ERROR, OTHER} and @antoniovicente can fill out more stuff later as needed?

@antoniovicente antoniovicente Jun 4, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

enum with {NO_ERROR, OTHER} sounds like a good start

TEST_F(GrpcHealthCheckerImplTest, GoAwayProbeInProgress) {
// FailureType::Network will be issued, it will render host unhealthy only if unhealthy_threshold
// is reached.
setupHCWithUnhealthyThreshold(1);
expectSingleHealthcheck(HealthTransition::Changed);
setupHCWithUnhealthyThreshold(/*threshold=*/1);
cluster_->prioritySet().getMockHostSet(0)->hosts_ = {
makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")};

expectSessionCreate();
expectHealthcheckStart(0);
health_checker_->start();

expectHealthcheckStop(0);
EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Unchanged));

// GOAWAY during check should be handle gracefully.
test_sessions_[0]->codec_client_->raiseGoAway();
respondServiceStatus(0, grpc::health::v1::HealthCheckResponse::SERVING);
expectHostHealthy(true);

// GOAWAY should cause a new connection to be created.
expectClientCreate(0);
expectHealthcheckStart(0);
test_sessions_[0]->interval_timer_->invokeCallback();

expectHealthcheckStop(0);
// Test host state haven't changed.
EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Unchanged));
respondServiceStatus(0, grpc::health::v1::HealthCheckResponse::SERVING);
expectHostHealthy(true);
}

// Test receiving GOAWAY closes connection after an in progress probe times outs.
TEST_F(GrpcHealthCheckerImplTest, GoAwayProbeInProgressTimeout) {
setupHCWithUnhealthyThreshold(/*threshold=*/1);
cluster_->prioritySet().getMockHostSet(0)->hosts_ = {
makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")};

expectSessionCreate();
expectHealthcheckStart(0);
EXPECT_CALL(event_logger_, logUnhealthy(_, _, _, true));
health_checker_->start();

expectHealthcheckStop(0);
// Unhealthy threshold is 1 so first timeout causes unhealthy
EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Changed));
EXPECT_CALL(event_logger_, logEjectUnhealthy(_, _, _));

// GOAWAY during check should be handled gracefully.
test_sessions_[0]->codec_client_->raiseGoAway();
expectHostHealthy(true);

test_sessions_[0]->timeout_timer_->invokeCallback();
expectHostHealthy(false);

// GOAWAY should cause a new connection to be created.
expectClientCreate(0);
expectHealthcheckStart(0);
test_sessions_[0]->interval_timer_->invokeCallback();

expectHealthcheckStop(0);
// Healthy threshold is 2, so the we'ere pending a state change.
EXPECT_CALL(*this, onHostStatus(_, HealthTransition::ChangePending));
respondServiceStatus(0, grpc::health::v1::HealthCheckResponse::SERVING);
expectHostHealthy(false);
}

// Test receiving GOAWAY closes connection after an unexpected stream reset.
TEST_F(GrpcHealthCheckerImplTest, GoAwayProbeInProgressStreamReset) {
setupHCWithUnhealthyThreshold(/*threshold=*/1);
cluster_->prioritySet().getMockHostSet(0)->hosts_ = {
makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")};

expectSessionCreate();
expectHealthcheckStart(0);
EXPECT_CALL(event_logger_, logUnhealthy(_, _, _, true));
health_checker_->start();

expectHealthcheckStop(0);
// Unhealthy threshold is 1 so first stream reset causes unhealthy
EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Changed));
EXPECT_CALL(event_logger_, logEjectUnhealthy(_, _, _));

// GOAWAY during check should be handled gracefully.
test_sessions_[0]->codec_client_->raiseGoAway();
expectHostHealthy(true);

EXPECT_TRUE(cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->healthFlagGet(
Host::HealthFlag::FAILED_ACTIVE_HC));
EXPECT_EQ(Host::Health::Unhealthy,
cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->health());
test_sessions_[0]->request_encoder_.stream_.resetStream(Http::StreamResetReason::RemoteReset);
expectHostHealthy(false);

// GOAWAY should cause a new connection to be created.
expectClientCreate(0);
expectHealthcheckStart(0);
test_sessions_[0]->interval_timer_->invokeCallback();

expectHealthcheckStop(0);
// Healthy threshold is 2, so the we'ere pending a state change.
EXPECT_CALL(*this, onHostStatus(_, HealthTransition::ChangePending));
respondServiceStatus(0, grpc::health::v1::HealthCheckResponse::SERVING);
expectHostHealthy(false);
}

// Test receiving GOAWAY closes connection after a bad response.
TEST_F(GrpcHealthCheckerImplTest, GoAwayProbeInProgressBadResponse) {
setupHCWithUnhealthyThreshold(/*threshold=*/1);
cluster_->prioritySet().getMockHostSet(0)->hosts_ = {
makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")};

expectSessionCreate();
expectHealthcheckStart(0);
EXPECT_CALL(event_logger_, logUnhealthy(_, _, _, true));
health_checker_->start();

expectHealthcheckStop(0);
// Unhealthy threshold is 1 so first bad response causes unhealthy
EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Changed));
EXPECT_CALL(event_logger_, logEjectUnhealthy(_, _, _));

// GOAWAY during check should be handled gracefully.
test_sessions_[0]->codec_client_->raiseGoAway();
expectHostHealthy(true);

respondResponseSpec(0, ResponseSpec{{{":status", "200"}, {"content-type", "application/grpc"}},
{ResponseSpec::invalidChunk()},
{}});
expectHostHealthy(false);

// GOAWAY should cause a new connection to be created.
expectClientCreate(0);
expectHealthcheckStart(0);
test_sessions_[0]->interval_timer_->invokeCallback();

expectHealthcheckStop(0);
// Healthy threshold is 2, so the we'ere pending a state change.
EXPECT_CALL(*this, onHostStatus(_, HealthTransition::ChangePending));
respondServiceStatus(0, grpc::health::v1::HealthCheckResponse::SERVING);
expectHostHealthy(false);
}

// Test receiving GOAWAY and a connection close.
TEST_F(GrpcHealthCheckerImplTest, GoAwayProbeInProgressConnectionClose) {
setupHCWithUnhealthyThreshold(/*threshold=*/1);
cluster_->prioritySet().getMockHostSet(0)->hosts_ = {
makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")};

expectSessionCreate();
expectHealthcheckStart(0);
EXPECT_CALL(event_logger_, logUnhealthy(_, _, _, true));
health_checker_->start();

expectHealthcheckStop(0);
// Unhealthy threshold is 1 so first bad response causes unhealthy
EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Changed));
EXPECT_CALL(event_logger_, logEjectUnhealthy(_, _, _));

// GOAWAY during check should be handled gracefully.
test_sessions_[0]->codec_client_->raiseGoAway();
expectHostHealthy(true);

test_sessions_[0]->client_connection_->raiseEvent(Network::ConnectionEvent::RemoteClose);
expectHostHealthy(false);

// GOAWAY should cause a new connection to be created.
expectClientCreate(0);
expectHealthcheckStart(0);
test_sessions_[0]->interval_timer_->invokeCallback();

expectHealthcheckStop(0);
// Healthy threshold is 2, so the we'ere pending a state change.
EXPECT_CALL(*this, onHostStatus(_, HealthTransition::ChangePending));
respondServiceStatus(0, grpc::health::v1::HealthCheckResponse::SERVING);
expectHostHealthy(false);
}

// Test receiving GOAWAY between checks affects nothing.
Expand Down