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

jmuia · 2020-05-26T18:55:08Z

Commit Message:
Gracefully handle GOAWAY in gRPC health checker, allowing in-progress healthchecks to complete before closing the connection.

Additional Description:
We've observed failures in our production and integration testing environments due to the gRPC health checker interpreting GOAWAYs as a health check failure. As an example, if all endpoints in an upstream cluster drain their incoming connections at the same time (eg. listener config change) a grpc healthcheck client with unhealthy_threshold=1 will see the entire cluster become unhealthy, which is undesirable.

This is my first contribution to Envoy, and would appreciate a close eye for edge cases I may have missed!

Risk Level: low/medium
Testing: added unit tests, ran (failing) grpc hc internal integration test with this patch which passed
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Joey Muia [email protected]

murray-stripe · 2020-05-26T20:14:30Z

/retest

repokitteh-read-only · 2020-05-26T20:14:33Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #11324 (comment) was created by @murray-stripe.

see: more, trace.

murray-stripe · 2020-05-28T13:23:25Z

/azp run envoy-presubmit

azure-pipelines · 2020-05-28T13:23:30Z

Commenter does not have sufficient privileges for PR 11324 in repo envoyproxy/envoy

murray-stripe · 2020-05-28T15:08:08Z

@lizan These tests seem to be a flake. Or at least I can't seem to reproduce the failure locally with either

bazel test //test/common/config:subscription_factory_impl_test
# or
./ci/do_ci.sh 'bazel.release' //test/common/config:subscription_factory_impl_test

Would you mind taking a look, or kicking off the azure-pipeline tests again?

Signed-off-by: Joey Muia <[email protected]>

snowp

Thanks for working on this! A few small comments and an open question, otherwise this looks good. I would probably want a release note for this, especially if we end up making this configurable.

snowp · 2020-06-02T16:23:25Z

source/common/upstream/health_checker_impl.cc

-    handleFailure(envoy::data::core::v3::NETWORK);
-    expect_reset_ = true;
-    request_encoder_->getStream().resetStream(Http::StreamResetReason::LocalReset);
+    received_goaway_ = true;


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?

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

snowp · 2020-06-02T16:26:00Z

source/common/upstream/health_checker_impl.cc

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

+  const bool goaway = received_goaway_;


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

source/common/upstream/health_checker_impl.cc

snowp · 2020-06-02T16:52:07Z

test/common/upstream/health_checker_impl_test.cc

@@ -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.


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

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.

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.

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.

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;

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?

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

Signed-off-by: Joey Muia <[email protected]>

repokitteh-read-only · 2020-06-10T04:21:06Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #11324 was synchronize by jmuia.

see: more, trace.

This reverts commit 00a1fea. Signed-off-by: John Murray <[email protected]>

Revert "fix format with 'tools/code_format/check_format.py fix'"

Signed-off-by: Joey Muia <[email protected]>

jmuia

@snowp this is ready for another review.

(sorry for pinging everyone with the formatting change! 😬)

jmuia · 2020-06-12T02:39:25Z

source/common/http/http2/codec_impl.cc

-  // notifications are the same as a normal GOAWAY.
-  if (frame->hd.type == NGHTTP2_GOAWAY && !raised_goaway_) {
+  // Shutdown notifications are the same as a normal GOAWAY.
+  if (frame->hd.type == NGHTTP2_GOAWAY) {


I'm not sure if this is fine or if we want to track which goaway error codes we've already seen, and run the callbacks once per error code.

Is it expected to get multiple GOAWAY frames with different error codes? If yes, it seems bad only look at the first one, if no then we can probably keep the old logic.

It is possible to receive GOAWAY frames with different error codes:

An endpoint MAY send multiple GOAWAY frames if circumstances change. For instance, an endpoint that sends GOAWAY with NO_ERROR during graceful shutdown could subsequently encounter a condition that requires immediate termination of the connection.

(from https://http2.github.io/http2-spec/#GOAWAY)

Hmm in that case should the logic for failing the HC be that we see a NO_ERROR + no error GOAWAYs?

Also I'm not sure what the implications are of making onGoAway trigger multiple times per stream, maybe @mattklein123 or @alyssawilk has an idea? I'd be worried about invariants elsewhere relying on this being called at most once.

Hmm in that case should the logic for failing the HC be that we see a NO_ERROR + no error GOAWAYs?

Hm, I don't quite follow, could you rephrase?

I'd be worried about invariants elsewhere relying on this being called at most once.

Agreed. It seems it was added in #103. I'm not sure if that was an optimization back then. I'm also not sure if it has become an expected behaviour (intentionally or not) since then.

if (request_encoder_ && error_code == Http::GoAwayErrorCode::NoError) { received_no_error_goaway_ = true; return; }

if a GOAWAY error follows a NO_ERROR then received_no_error_goaway_ would still be true. I was thinking that the logic should reset this flag if it saw an error frame.

Ah, I see. I don't expect that to change the behaviour as it stands:

If there isn't an active probe, the first (NO_ERROR) GOAWAY would've caused the connection to be closed (and we only set received_no_error_goaway_ = true; when there's an active probe).

If there is an active probe, the stream reset will end up resetting state (resetState() will be called by onResetStream()).

In general, I believe a connection close will end up resetting state because the onResetStream() callbacks will run (from here).

I admit this isn't obvious / does feel a bit brittle -- happy to add a comment and/or reset the flag anyways.

Friendly ping @mattklein123 @alyssawilk

Sorry for the delay. My preference would be to not raise multiple goaways from the codec as I'm guessing there will be code that won't expect this and will break.

Can we just keep the previous behavior and not handle the case where multiple go away frames are received? We can potentially deal with this in a follow up if someone requests it?

Thanks for the feedback - that seems reasonable to me. I've reverted that specific commit and added a TODO instead.

jmuia · 2020-06-12T02:47:01Z

test/common/http/http2/codec_impl_test.cc

@@ -1456,7 +1457,7 @@ TEST_P(Http2CodecImplTest, LargeRequestHeadersExceedPerHeaderLimit) {
  request_headers.addCopy("big", long_string);

  EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)).Times(0);
-  EXPECT_CALL(client_callbacks_, onGoAway());
+  EXPECT_CALL(client_callbacks_, onGoAway(_));


I don't completely follow why this test doesn't trigger onGoAway(_) to be called twice like the other one.

snowp · 2020-06-15T17:58:37Z

include/envoy/http/codec.h

@@ -36,6 +36,11 @@ const char MaxResponseHeadersCountOverrideKey[] =

 class Stream;

+enum class ErrorCode {


maybe call this GoAwayErrorCode? to make it clearer what kind of error codes are being modeled here

👍 will update. Initially this enumerated all the error codes here, which are shared with RST_STREAM. But we've since undone that and there's already a separate Envoy-internal enum for stream resets.

snowp · 2020-06-15T19:38:21Z

source/common/http/http2/codec_impl.cc

-  // notifications are the same as a normal GOAWAY.
-  if (frame->hd.type == NGHTTP2_GOAWAY && !raised_goaway_) {
+  // Shutdown notifications are the same as a normal GOAWAY.
+  if (frame->hd.type == NGHTTP2_GOAWAY) {


Is it expected to get multiple GOAWAY frames with different error codes? If yes, it seems bad only look at the first one, if no then we can probably keep the old logic.

snowp · 2020-06-15T19:38:55Z

source/common/upstream/health_checker_impl.cc

+  // The connection will be closed when the active check completes or another
+  // terminal condition occurs, such as a timeout or stream reset.
+  if (request_encoder_ && error_code == Http::ErrorCode::NoError) {
+    received_goaway_ = true;


maybe rename this to make it clear that we received a non-error GOAWAY?

Signed-off-by: Joey Muia <[email protected]>

snowp · 2020-06-16T00:08:35Z

include/envoy/http/codec.h

@@ -36,6 +36,11 @@ const char MaxResponseHeadersCountOverrideKey[] =

 class Stream;

+enum class GoAwayErrorCode {


sorry should have mention this but this should have a doc comment as its in include/

snowp · 2020-06-16T00:24:32Z

source/common/http/http2/codec_impl.cc

-  // notifications are the same as a normal GOAWAY.
-  if (frame->hd.type == NGHTTP2_GOAWAY && !raised_goaway_) {
+  // Shutdown notifications are the same as a normal GOAWAY.
+  if (frame->hd.type == NGHTTP2_GOAWAY) {


Hmm in that case should the logic for failing the HC be that we see a NO_ERROR + no error GOAWAYs?

Also I'm not sure what the implications are of making onGoAway trigger multiple times per stream, maybe @mattklein123 or @alyssawilk has an idea? I'd be worried about invariants elsewhere relying on this being called at most once.

Signed-off-by: Joey Muia <[email protected]>

jmuia · 2020-06-17T23:58:47Z

@snowp the CI failures look like flakes. Can you re-run them?

This reverts commit 73e7308. Signed-off-by: Joey Muia <[email protected]>

Signed-off-by: Joey Muia <[email protected]>

jmuia · 2020-06-24T15:45:46Z

/retest

repokitteh-read-only · 2020-06-24T15:45:49Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #11324 (comment) was created by @jmuia.

see: more, trace.

jmuia · 2020-06-24T15:47:31Z

/azp run envoy-presubmit

azure-pipelines · 2020-06-24T15:47:36Z

Commenter does not have sufficient privileges for PR 11324 in repo envoyproxy/envoy

Signed-off-by: Joey Muia <[email protected]>

jmuia · 2020-06-24T19:52:31Z

@snowp ready for another review. I reverted the commit that allowed multiple GOAWAYs and added a TODO instead.

mattklein123

Thanks, looks great. Will defer to @snowp for final review/merge.

snowp

LGTM, thanks for iterating on this!

…roxy#11324) Gracefully handle GOAWAY in gRPC health checker, allowing in-progress health checks to complete before closing the connection. Signed-off-by: Joey Muia <[email protected]> Co-authored-by: John Murray <[email protected]>

…roxy#11324) Gracefully handle GOAWAY in gRPC health checker, allowing in-progress health checks to complete before closing the connection. Signed-off-by: Joey Muia <[email protected]> Co-authored-by: John Murray <[email protected]> Signed-off-by: yashwant121 <[email protected]>

jmuia mentioned this pull request May 26, 2020

health check: close conn after timeout/stream rst when grpc hc has conn reuse disabled #11325

Merged

jmuia force-pushed the jmuia.grpc-hc-goaway branch from dae6ee5 to 518fe68 Compare May 27, 2020 19:07

jmuia added 2 commits May 29, 2020 10:33

health check: gracefully handle GOAWAY in grpc health checker

e4ddb8e

Signed-off-by: Joey Muia <[email protected]>

Close connection after logging and add unit test for remote close

70263a6

Signed-off-by: Joey Muia <[email protected]>

jmuia force-pushed the jmuia.grpc-hc-goaway branch from 518fe68 to 70263a6 Compare May 29, 2020 17:40

mattklein123 assigned snowp May 30, 2020

snowp suggested changes Jun 2, 2020

View reviewed changes

murray-stripe and others added 2 commits June 9, 2020 11:07

wip

a069698

Signed-off-by: Joey Muia <[email protected]>

wip - plumb through

725caf8

Signed-off-by: Joey Muia <[email protected]>

jmuia requested a review from alyssawilk as a code owner June 9, 2020 19:12

jmuia added 2 commits June 9, 2020 20:31

Merge branch 'envoyproxy/master' into jmuia.grpc-hc-goaway

58d2143

Signed-off-by: Joey Muia <[email protected]>

fix format with 'tools/code_format/check_format.py fix'

00a1fea

Signed-off-by: Joey Muia <[email protected]>

jmuia requested review from dio, jmarantz, junr03, lizan, PiotrSikora and zuercher as code owners June 10, 2020 04:20

repokitteh-read-only bot added the api label Jun 10, 2020

murray-stripe and others added 5 commits June 10, 2020 08:20

Revert "fix format with 'tools/code_format/check_format.py fix'"

1828a2b

This reverts commit 00a1fea. Signed-off-by: John Murray <[email protected]>

Merge pull request #1 from jmuia/wip/grpc-hc-goaway

64c6aef

Revert "fix format with 'tools/code_format/check_format.py fix'"

only handle GOAWAY NO_ERROR gracefully

6ba45a4

Signed-off-by: Joey Muia <[email protected]>

add inline comments

c5958dc

Signed-off-by: Joey Muia <[email protected]>

make quic error mapper consistent with http2

b474edd

Signed-off-by: Joey Muia <[email protected]>

jmuia commented Jun 12, 2020

View reviewed changes

snowp suggested changes Jun 15, 2020

View reviewed changes

jmuia added 3 commits June 15, 2020 16:42

rename ErrorCode to GoAwayErrorCode

dc5effd

Signed-off-by: Joey Muia <[email protected]>

rename received_goaway_ to received_no_error_goaway_

7c7d930

Signed-off-by: Joey Muia <[email protected]>

fix format

41ea1e1

Signed-off-by: Joey Muia <[email protected]>

snowp suggested changes Jun 16, 2020

View reviewed changes

add comment to GoAwayErrorCode enum

a66c176

Signed-off-by: Joey Muia <[email protected]>

mattklein123 self-assigned this Jun 20, 2020

mattklein123 added the waiting label Jun 22, 2020

jmuia added 2 commits June 23, 2020 17:18

Revert "run onGoAway callbacks for each GOAWAY received"

bf9c59c

This reverts commit 73e7308. Signed-off-by: Joey Muia <[email protected]>

Add TODO for handling multiple GOAWAY frames

878bd4a

Signed-off-by: Joey Muia <[email protected]>

repokitteh-read-only bot removed the waiting label Jun 24, 2020

kick ci

541eb54

Signed-off-by: Joey Muia <[email protected]>

mattklein123 approved these changes Jun 24, 2020

View reviewed changes

snowp approved these changes Jun 24, 2020

View reviewed changes

snowp merged commit 4c4fc05 into envoyproxy:master Jun 24, 2020

mpuncel mentioned this pull request Oct 2, 2020

Don't count NO_ERROR GOAWAY as a failure in HTTP health checker #13379

Closed

		@@ -36,6 +36,11 @@ const char MaxResponseHeadersCountOverrideKey[] =

		class Stream;

		enum class ErrorCode {

		@@ -36,6 +36,11 @@ const char MaxResponseHeadersCountOverrideKey[] =

		class Stream;

		enum class GoAwayErrorCode {

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

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

Conversation

jmuia commented May 26, 2020

murray-stripe commented May 26, 2020

repokitteh-read-only bot commented May 26, 2020

murray-stripe commented May 28, 2020

azure-pipelines bot commented May 28, 2020

murray-stripe commented May 28, 2020 • edited Loading

snowp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented Jun 10, 2020

jmuia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmuia commented Jun 17, 2020

jmuia commented Jun 24, 2020

repokitteh-read-only bot commented Jun 24, 2020

jmuia commented Jun 24, 2020

azure-pipelines bot commented Jun 24, 2020

jmuia commented Jun 24, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

snowp left a comment

Choose a reason for hiding this comment

murray-stripe commented May 28, 2020 •

edited

Loading