Native gRPC health checker implementation by baranov1ch · Pull Request #2316 · envoyproxy/envoy

baranov1ch · 2018-01-05T12:04:04Z

Signed-off-by: Alexey Baranov me@kotiki.cc

title: gRPC health checker

Description:
This PR introduces gRPC-based health checking cluster hosts that implement grpc.health.v1.Health. New grpc_health_check option is added to health_checks section of CDS v2.Cluster config. User can also set service_name which will be passed in HealthCheckRequest.service in Check RPC request payload. Cluster MUST be configured to support HTTP/2 to use gRPC health check, otherwise exception is thrown during configuration.

Risk Level: Medium

Testing:
Rather comprehensive (imo :)) unit testing for configuration and operation. Also it was manually tested against grpc-go based server to catch a lot of graceful shutdown edge cases (see long comments in GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession).

Docs Changes:
envoyproxy/data-plane-api#383

Release Notes:
Added gRPC healthcheck based on grpc.health.v1.Health service.

Fixes #369

API Changes:
envoyproxy/data-plane-api#383

baranov1ch · 2018-01-05T14:15:23Z

I'm not sure how timed out //test/integration:access_log_integration_test relates to my changes. Plus IWOMM

htuch · 2018-01-05T15:19:40Z

source/common/upstream/health.proto

I think this should be sourced from the gRPC repo as an external dependency. I'm going to be looking into adding the gRPC repos as an external dep for Envoy today, so stay tuned.

See #2318. When this merges, you should be able to do something like @grpc//src/proto/grpc/health/v1:health.proto to reference the proto there.

trying like this:

--- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -410,3 +410,18 @@ def _com_github_grpc_grpc(): name = "grpc", actual = "@com_github_grpc_grpc//:grpc++" ) + // For grpc proto compilation to work. + native.bind( + name = "protocol_compiler", + actual = "@com_google_protobuf_cc//:protoc", + ) + + // For grpc proto compilation to work. + native.bind( + name = "protobuf_clib", + actual = "@com_google_protobuf_cc//:protobuf", + ) + + native.bind( + name = "grpc_health_proto", + actual = "@com_github_grpc_grpc//src/proto/grpc/health/v1:health_proto", + ) --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -89,9 +89,8 @@ envoy_cc_library( name = "health_checker_lib", srcs = ["health_checker_impl.cc"], hdrs = ["health_checker_impl.h"], - external_deps = ["envoy_health_check"], + external_deps = ["envoy_health_check", "grpc_health_proto"],

does not work with some strange errors:

bazel build -c dbg --verbose_failures //source/common/upstream:health_checker_lib <...> INFO: Analysed target //source/common/upstream:health_checker_lib (0 packages loaded). INFO: Found 1 target... ERROR: /private/var/tmp/_bazel_baranovich/b62ff95190308ee02bfc031330094506/external/com_github_grpc_grpc/src/proto/grpc/health/v1/BUILD:21:1: output 'external/com_github_grpc_grpc/src/proto/grpc/health/v1/grpc/src/proto/grpc/health/v1/health.pb.h' was not created ERROR: /private/var/tmp/_bazel_baranovich/b62ff95190308ee02bfc031330094506/external/com_github_grpc_grpc/src/proto/grpc/health/v1/BUILD:21:1: output 'external/com_github_grpc_grpc/src/proto/grpc/health/v1/grpc/src/proto/grpc/health/v1/health.pb.cc' was not created ERROR: /private/var/tmp/_bazel_baranovich/b62ff95190308ee02bfc031330094506/external/com_github_grpc_grpc/src/proto/grpc/health/v1/BUILD:21:1: not all outputs were created or valid Target //source/common/upstream:health_checker_lib failed to build INFO: Elapsed time: 0,506s, Critical Path: 0,30s FAILED: Build did NOT complete successfully

Seems like grpc proto build rules does smth strange with proto out paths...

I wasn't able to build any of external grpc targets building protobufs with the same kind of errors(

It's hard to know what's going on with just that error message. The diff you provide looks correct. Can you do bazel build -s and attach the protoc invocation on health.proto? This might give more of a clue about what is going on.

SUBCOMMAND: # @com_github_grpc_grpc//src/proto/grpc/health/v1:_health_proto_codegen [action 'SkylarkAction external/com_github_grpc_grpc/src/proto/grpc/health/v1/grpc/src/proto/grpc/health/v1/health.pb.h'] (cd /private/var/tmp/_bazel_baranovich/b62ff95190308ee02bfc031330094506/execroot/envoy && \ exec env - \ bazel-out/host/bin/external/com_google_protobuf_cc/protoc '--cpp_out=:bazel-out/darwin-dbg/genfiles' '-Iexternal/com_github_grpc_grpc/src/proto/grpc/health/v1/health.proto=external/com_github_grpc_grpc/src/proto/grpc/health/v1/health.proto' external/com_github_grpc_grpc/src/proto/grpc/health/v1/health.proto)

htuch · 2018-01-05T15:22:26Z

source/common/upstream/health_checker_impl.h

Before going into the review, I'd like to understand some design stuff. How come we're doing all the low level gRPC codec and connection management in this HC rather than leveraging the existing gRPC client support in Envoy (https://github.com/envoyproxy/envoy/blob/master/include/envoy/grpc/async_client.h, https://github.com/envoyproxy/envoy/blob/master/source/common/grpc/async_client_impl.h)?

Grpc::AsyncClientImpl was my first thought. Unfortunately async client is tied to cluster manager, router, retries, AsyncHttpClient, etc. We don't have (and don't need) all those abstractions in healthchecker code - we need to connect directly to a specific endpoint.

Other option is to refactor out connection establishment part from current Grpc::AsyncStreamImpl, leaving it as a dumb protocol decoder, but that requires careful thinking and greatly widens scope of this pretty germetic PR, which I would like to avoid and work on tech debt later. Imo, low-level protocol stuff here is pretty simple.

Got it, I think this is fine then, will take a deeper look in a bit.

FWIW the issue of doing it this way vs. AsyncClient came up when @craffert0 took a quick stab at implementing this a couple of months ago. I think it's definitely possible to refactor AsyncClient so it doesn't depend on cluster manager, but it's non-trivial. I'm in favor of doing it this way and adding a TODO to come back and revisit at a later time.

htuch

Thanks for the contribution, here's my first pass of the code (not tests yet).

htuch · 2018-01-08T18:09:09Z

source/common/upstream/health_checker_impl.cc

Nit: slight preference for "HTTP/2" over "http2" here.

htuch · 2018-01-08T18:27:27Z

source/common/upstream/health_checker_impl.cc

Nit: const auto

htuch · 2018-01-08T18:30:10Z

source/common/upstream/health_checker_impl.cc

Isn't gRPC always 200? If it's non-200, it seems like we shouldn't be expecting grpc-status at all.

this code was taken from grpc_async_client_impl.h. I haven't managed to receive such a thing from grpc-go server, but maybe it can exist

This is copied from Grpc::AsyncClentImpl. I wasn't able to get to this codepath from grpc-go server, but maybe it can happen with more intermediaries en route.

Ah, fair enough. I actually wrote that code, and suspect that it's gratuitous, but let's leave it as is for consistency.

htuch · 2018-01-08T18:31:08Z

source/common/upstream/health_checker_impl.cc

This is a gRPC protocol violation, so maybe be a bit more explicit about that.

Not sure what to do here then... there is no more suitable grpc status. I can add "protocol violation: " to error message.

Yeah, just add to the error message.

htuch · 2018-01-08T18:36:46Z

source/common/upstream/health_checker_impl.cc

Nit: const bool

Actually, would also rename to expected_reset when snapshotting.

htuch · 2018-01-08T18:36:59Z

source/common/upstream/health_checker_impl.cc

Nit: use braces even for single line if.

htuch · 2018-01-08T18:38:48Z

source/common/upstream/health_checker_impl.cc

How does any of this differ from other cases where we need to handle GOAWAYs and connection closes on Envoy's data or control planes?

Other HTTP/2 cases all go through cluster manager and Http2::ConnPool which handles remote goaways implementing Http::ConnectionCallbacks by closing the client. I'd better use it here too, thanks for pointing out.

It got much cleaner after using onGoAway callback

htuch · 2018-01-08T18:39:09Z

source/common/upstream/health_checker_impl.cc

Use braces on single line if

htuch · 2018-01-08T18:40:37Z

source/common/upstream/health_checker_impl.cc

This logic is a bit confusing to me when comparing with Grpc::AsyncClient. Can we use the same close local/remote as done there?

here I decided not to use local/remote close, logic is simpler:

'local' is always closed, RPC is unary, everything is sent by onInterval()

end_stream argument is passed to onRpcComplete instead of using a field indicating 'remote close'.
Here local/remote close logic is not necessary, since there are no clients trying to send/receive streams of grpc messages.

besides local/remote close, there is http_reset_ in Grpc::AsyncClient which is used to determine if Http::AsyncClient::Stream::reset() should be called to recycle http stream, or stream was already reset by http stack. expect_reset_ has nearly the same semantics as http_reset_. But unlike Grpc::AsyncClient, which maps stream reset just to internal error, health checker distinguishes between active healthcheck errors (when service is in really bad mood) and network-ish healthcheck errors produced by stream reset.

I'm not sure if introducing all Grpc::AsyncClient state would make code more readable, but I will gladly add more comments about this flag:)

Fair enough. I think it would be helpful to comment here on why we might have seen an RPC completion, without hitting the end-of-stream, and why this might lead to a following onResetStream() callback. Thanks.

added some comments. PTAL if they are good enough

Yeah, these are good, thanks. Maybe also explain how we ever get into the situation where we are in this branch, i.e. who called onRpcComplete with !end_stream as well.

htuch · 2018-01-08T18:42:17Z

source/common/upstream/health_checker_impl.cc

We're doing a bunch of work above that isn't strictly needed when log level is above debug. Is it possible to condition on log level?

I haven't found how to do that, but I'm quite new to the codebase. Would appreciate any help :)

Just checked with @mrice32, this doesn't exist today unfortunately. I've opened #2330 to track.

Signed-off-by: Alexey Baranov <me@kotiki.cc>

Catching GOAWAYs this way allows not writing a lot of nullptr checks Signed-off-by: Alexey Baranov <me@kotiki.cc>

baranov1ch · 2018-01-09T20:14:52Z

oops, did rebase( Promise to to do it anymore)

Signed-off-by: Alexey Baranov <me@kotiki.cc>

mattklein123

Thanks, in general this is extremely well done. Great work.

A few small comments and questions about coverage.

mattklein123 · 2018-01-10T16:32:39Z

source/common/upstream/health_checker_impl.cc

                                                    Redis::ConnPool::ClientFactoryImpl::instance_);
+  case envoy::api::v2::HealthCheck::HealthCheckerCase::kGrpcHealthCheck:
+    if (!(cluster.info()->features() & Upstream::ClusterInfo::Features::HTTP2)) {
+      throw EnvoyException(fmt::format("{} cluster must support HTTP/2 for gRPC healthchecking",


Please have a test that does EXPECT_THROW_WITH_MESSAGE to catch this case.

mattklein123 · 2018-01-10T16:35:03Z

source/common/upstream/health_checker_impl.h

+  struct GrpcActiveHealthCheckSession : public ActiveHealthCheckSession,
+                                        public Http::StreamDecoder,
+                                        public Http::StreamCallbacks {
+    GrpcActiveHealthCheckSession(GrpcHealthCheckerImpl& parent, HostSharedPtr host);


nit: const HostSharedPtr& host (Please fix elsewhere in this file in old code if similar).

mattklein123 · 2018-01-10T16:37:20Z

source/common/upstream/health_checker_impl.h

+
+  // HealthCheckerImplBase
+  ActiveHealthCheckSessionPtr makeSession(HostSharedPtr host) override {
+    return ActiveHealthCheckSessionPtr{new GrpcActiveHealthCheckSession(*this, host)};


nit: std::make_unique (please fix elsewhere in this file for similar code, since we are using C++14 now).

mattklein123 · 2018-01-10T16:44:58Z

source/common/upstream/health_checker_impl.cc

+  } else {
+    grpc_status_message = fmt::format("{}", grpc_status);
+  }
+  ENVOY_CONN_LOG(error, "hc grpc_status={} service_status={} health_flags={}", *client_,


I think this should actually be debug level. (Since upstream can error and we don't want to spew logs by default).

fixed. Just forgot to return back after testing

mattklein123 · 2018-01-10T16:47:03Z

source/common/upstream/health_checker_impl.cc

+  if (!decoder_.decode(data, decoded_frames)) {
+    onRpcComplete(Grpc::Status::GrpcStatus::Internal, "gRPC wire protocol decode error", false);
+  }
+  for (auto& frame : decoded_frames) {


Do you have a test with the response split across two decodeData calls?

added the test

mattklein123 · 2018-01-10T16:47:46Z

source/common/upstream/health_checker_impl.cc

+  }
+}
+
+void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::decodeHeaders(


I haven't looked at the coverage results yet in CI, but please check and make sure you have coverage on all error branches in the code you added.

checked, added more coverage

Signed-off-by: Alexey Baranov <me@kotiki.cc>

mattklein123

Thanks this LGTM other than sorting out proto build issues. @htuch do you have time to check out the branch and see what might be wrong? If not @jmillikin-stripe do you have a little time?

htuch

LGTM as well with some small suggestions on how to improve test readability (which are amazing BTW, that's some thorough testing!).

I'd like to avoid duplicating health.proto from the gRPC dep, so we should figure out why this isn't building. I'd have to poke around at the Bazel build dir and better understand the grpc_proto_library rule to do this. I'm fairly busy this week, but could look EOW if someone doesn't beat me to it.

htuch · 2018-01-11T03:08:05Z

test/common/upstream/health_checker_impl_test.cc

+  }
+
+  void respondResponseSpec(size_t index, ResponseSpec&& spec) {
+    bool trailers_empty = spec.trailers.size() == 0U;


Nit: can you constify the local tmps where they are immutable everywhere in the tests? Thanks.

htuch · 2018-01-11T03:09:35Z

test/common/upstream/health_checker_impl_test.cc

+  EXPECT_TRUE(cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->healthy());
+}
+
+TEST_F(GrpcHealthCheckerImplTest, SuccessResponseSplitBetweenChunks) {


Generally prefer a single line // comment above each test explaining in plain language what the test is trying to accomplish. E.g. // Validate that a regular response split across chunks is handled.

htuch · 2018-01-11T03:13:14Z

test/common/upstream/health_checker_impl_test.cc

+  EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_));
+  health_checker_->start();
+
+  EXPECT_CALL(*test_sessions_[0]->interval_timer_, enableTimer(_));


Can this be factored out into some expect function for healthy()? Seems very verbose to have this enable/disable timer stuff everywhere, which makes it a bit harder to read.

htuch · 2018-01-11T03:13:52Z

test/common/upstream/health_checker_impl_test.cc

+  expectSessionCreate();
+  expectStreamCreate(0);
+  EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_));
+  health_checker_->start();


I think there is also some common setup code that could be factored out.

- Factor out some helpers to make test bodies more readable. - Small explanation comments for each test Signed-off-by: Alexey Baranov <me@kotiki.cc>

htuch · 2018-01-12T01:21:49Z

Thanks for the test cleanups, this is rad. If we can get the health.proto build sorted this LGTM.

htuch · 2018-01-15T16:07:13Z

@baranov1ch I looked into the health.proto build issue, fix is at https://gist.github.com/htuch/ff95a662d3c5684eb98d005f6bbbdb8e.

Signed-off-by: Alexey Baranov <me@kotiki.cc>

baranov1ch · 2018-01-16T09:48:27Z

@htuch thanks, it worked. I wish I could understand why :)

* Add id to header exchange * remove extra include * review comments * review comments2

Fixes envoyproxy/envoy-mobile#2301 Discussion: https://envoyproxy.slack.com/archives/C02F93EEJCE/p1653485522681549 Signed-off-by: JP Simard <jp@jpsim.com>

htuch reviewed Jan 5, 2018

View reviewed changes

htuch reviewed Jan 8, 2018

View reviewed changes

baranov1ch added 3 commits January 9, 2018 17:57

Native gRPC health checker implementation

2557199

Signed-off-by: Alexey Baranov <me@kotiki.cc>

fix: newer clang-format warnings

7d6749d

Signed-off-by: Alexey Baranov <me@kotiki.cc>

gRPC healthcheck: use Http::ConnectionCallbacks::onGoAway

2267186

Catching GOAWAYs this way allows not writing a lot of nullptr checks Signed-off-by: Alexey Baranov <me@kotiki.cc>

baranov1ch force-pushed the grpc-health-checker branch from 650436c to 6edcca9 Compare January 9, 2018 20:14

Fix nits and build after rebase

b40cce6

Signed-off-by: Alexey Baranov <me@kotiki.cc>

baranov1ch force-pushed the grpc-health-checker branch from 6edcca9 to b40cce6 Compare January 9, 2018 20:28

mattklein123 reviewed Jan 10, 2018

View reviewed changes

gRPC healthcheck: more test coverage and review nits

e594e63

Signed-off-by: Alexey Baranov <me@kotiki.cc>

mattklein123 reviewed Jan 11, 2018

View reviewed changes

htuch reviewed Jan 11, 2018

View reviewed changes

gRPC-healthcheck tests: add readbility

490c277

- Factor out some helpers to make test bodies more readable. - Small explanation comments for each test Signed-off-by: Alexey Baranov <me@kotiki.cc>

mattklein123 assigned htuch Jan 12, 2018

Merge branch 'master' into grpc-health-checker

0bda952

gRPC healthchecker: use health.proto from vendored grpc

015dbb2

Signed-off-by: Alexey Baranov <me@kotiki.cc>

mattklein123 approved these changes Jan 16, 2018

View reviewed changes

htuch approved these changes Jan 16, 2018

View reviewed changes

mattklein123 merged commit bc67c6e into envoyproxy:master Jan 16, 2018

taiki45 mentioned this pull request Feb 11, 2018

Native gRPC health checker must send a scheme header #2583

Closed

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020

Update metadata exchange filter (envoyproxy#2316)

aeb8abe

* Add id to header exchange * remove extra include * review comments * review comments2

jpsim added a commit that referenced this pull request Nov 28, 2022

kotlin: fix flaky grpc test (#2316)

292cfe5

Fixes envoyproxy/envoy-mobile#2301 Discussion: https://envoyproxy.slack.com/archives/C02F93EEJCE/p1653485522681549 Signed-off-by: JP Simard <jp@jpsim.com>

jpsim added a commit that referenced this pull request Nov 29, 2022

kotlin: fix flaky grpc test (#2316)

0108e50

Fixes envoyproxy/envoy-mobile#2301 Discussion: https://envoyproxy.slack.com/archives/C02F93EEJCE/p1653485522681549 Signed-off-by: JP Simard <jp@jpsim.com>

Conversation

baranov1ch commented Jan 5, 2018

Uh oh!

baranov1ch commented Jan 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baranov1ch Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baranov1ch Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baranov1ch Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baranov1ch Jan 9, 2018 •

edited

Loading

baranov1ch Jan 5, 2018 •

edited

Loading

baranov1ch Jan 9, 2018 •

edited

Loading

baranov1ch commented Jan 9, 2018 •

edited

Loading