grpc: add a raw (Buffer::Instance) interface by jplevyak · Pull Request #6525 · envoyproxy/envoy

jplevyak · 2019-04-09T16:45:12Z

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Add a raw (Buffer::Instance) gRPC interface. The existing interface is layered on top
of a new raw interface which takes the service and method names as absl::string_view and which
passes the data to be sent and data received as Buffer::Instance(s). This is similar to the way
the existing typed interface is layered on the untyped interface. This enables extensions and
plugins to use gRPC without the gRPC service protobufs being compiled into envoy. This can also
support general gRPC proxies.
Risk Level: Medium
Testing: all existing tests pass.
Docs Changes: N/A (no externally visible change)
Release Notes: N/A
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: John Plevyak <jplevyak@gmail.com>

lizan

Thanks for working on this! Let's start from some interface level comments.

/wait

include/envoy/grpc/async_client.h

source/common/buffer/buffer_impl.cc

source/common/grpc/async_client.cc

htuch

Thanks @jplevyak, this is going to be super neat for #4272. Some high level structural comments to get started.
/wait

include/envoy/grpc/async_client.h

Signed-off-by: John Plevyak <jplevyak@gmail.com>

mattklein123 · 2019-04-12T04:37:32Z

Can I ask a high level probably stupid question: how is this actually going to be used? Where are the raw message bytes going to come from to send and what is the caller going to do with the raw bytes returned? I understand this change in theory but the utility is not clear to me.

jplevyak · 2019-04-12T16:07:48Z

Can I ask a high level probably stupid question: how is this actually going to be used? Where are the raw message bytes going to come from to send and what is the caller going to do with the raw bytes returned? I understand this change in theory but the utility is not clear to me.

This is going to be used by the WASM extension system. It can also be used by any generic gRPC proxy system.

jplevyak · 2019-04-12T16:12:36Z

I believe I have a plan to address the issue of the API:

make the base API the raw API
- startRaw -> start where the clients convert Protobuf::MethodDescriptor
move the Untyped and Typed Callback support code to source/common/grpc/grpc_callbacks.*
- clients will use to provide the interface used today
continue having mocks for the Untyped and Typed Callbacks in the test/mocks directory

I'll ping this PR when I am done.

Thanx for the comments.

mattklein123 · 2019-04-12T16:46:10Z

This is going to be used by the WASM extension system. It can also be used by any generic gRPC proxy system.

OK, thanks that makes sense. So basically the WASM code will have the protos compiled in and just use a raw API to make calls. Regarding the latter, Envoy already is a generic gRPC proxy so I'm not sure why this would ever be used in that manner, but agree it seems possible. Since this is targeted towards WASM/plugin can you update the issue description to make that more clear?

Also, more generally, @jplevyak @PiotrSikora @lizan @dio is there a design doc on the proposed WASM <-> API bindings? I've been asking for this for a while and would love to review. Thank you.

htuch · 2019-04-12T18:45:50Z

@jplevyak this seems like a good plan. Please ping me on IM if you want to chat further. The key things to preserve are economy of mechanism and mockability :)

stale · 2019-04-19T19:13:08Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: John Plevyak <jplevyak@gmail.com>

jplevyak · 2019-04-19T23:30:12Z

I have updated this PR as per the comments. PTAL

Signed-off-by: John Plevyak <jplevyak@gmail.com>

lizan

A quick skim, will do more review on Monday.

source/common/grpc/common.h

test/common/grpc/common_test.cc

htuch

Great, I think this is the right direction, a few stuctural comments to start.
/wait

source/common/grpc/typed_async_client.h

source/common/grpc/typed_async_client.cc

source/common/grpc/common.h

Signed-off-by: John Plevyak <jplevyak@gmail.com>

lizan

Thanks! Overall this looks much better other than comments already pointed out by @htuch.

include/envoy/grpc/async_client.h

Signed-off-by: John Plevyak <jplevyak@gmail.com>

htuch

LGTM. Coverage is failing, I know we are having Envoy-wide issues with being right at 97.5% right now, but you might want to see if there is any avoidable regression in this PR.

test/mocks/grpc/mocks.h

jplevyak · 2019-05-23T07:04:44Z

The MacOS and coverage failures seem to be unrelated. lizan do you have any more comments?
I am not authorized to merge so I will continue to monitor this thread. Thanks for the reviews!

htuch · 2019-05-23T16:29:48Z

/retest

repokitteh-read-only · 2019-05-23T16:29:51Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #6525 (comment) was created by @htuch.

see: more, trace.

lizan · 2019-05-23T16:34:42Z

/azp run

azure-pipelines · 2019-05-23T16:35:02Z

Azure Pipelines successfully started running 2 pipeline(s).

htuch · 2019-05-23T23:08:30Z

@jplevyak the coverage is below the threshold, and we're getting TSAN flakes. Can you merge master and try again, plus maybe look to see if you can fill in any trivial coverage stuff missing? This PR is good to go modulo this.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

See envoyproxy#6917 for details. Signed-off-by: John Plevyak <jplevyak@gmail.com>

Signed-off-by: John Plevyak <jplevyak@gmail.com>

jplevyak · 2019-06-05T22:56:49Z

I think the coverage is in pretty good shape. If there is anything which stands out, please tell me.

lizan

looks great, one last comment.

source/common/grpc/common.h

source/common/grpc/google_async_client_impl.cc

htuch · 2019-06-06T17:01:55Z

source/common/grpc/google_grpc_utils.cc

  // lifetime of the Slice(s) exceeds our Buffer::Instance.
  std::vector<grpc::Slice> slices;
-  byte_buffer.Dump(&slices);
+  if (!byte_buffer.Dump(&slices).ok()) {


Is there a way to mock this for testing?

Even the gRPC library provided by Google doesn't test for Dump failure. Here is the only test:

TEST_F(ByteBufferTest, Dump) {
grpc_slice hello = grpc_slice_from_copied_string(kContent1);
grpc_slice world = grpc_slice_from_copied_string(kContent2);
std::vector slices;
slices.push_back(Slice(hello, Slice::STEAL_REF));
slices.push_back(Slice(world, Slice::STEAL_REF));
ByteBuffer buffer(&slices[0], 2);
slices.clear();
(void)buffer.Dump(&slices);
EXPECT_TRUE(SliceEqual(slices[0], hello));
EXPECT_TRUE(SliceEqual(slices[1], world));
}

Yeah, this is fine. I think if this was Envoy code we would want a test, but it's not worth the effort to deal with this in external deps that aren't written with mocking in mind.

htuch · 2019-06-06T17:02:37Z

test/common/grpc/google_grpc_utils_test.cc

  auto byte_buffer = GoogleGrpcUtils::makeByteBuffer(std::move(buffer));
  std::vector<grpc::Slice> slices;
-  byte_buffer.Dump(&slices);
+  RELEASE_ASSERT(byte_buffer.Dump(&slices).ok(), "");


When can Dump fail? We should only use RELEASE_ASSERT in situations where it's effectively OOM and some unrecoverable situation exists. Otherwise, there should be error handling.

See discussion in #6917.

Note: RELEASE_ASSERT is used only in tests, and failure is handled properly in the real code.

This is fine in the tests then. I would still like to see the Dump condition mocked and tested if gRPC has mocks that can support this.

This is a test, so it is a RELEASE__ASSERT. It should never fail. For the production code I am handling the error.

See above regarding testing Dump().

Signed-off-by: John Plevyak <jplevyak@gmail.com>

htuch

Great, thanks!

Add a raw (Buffer::Instance) gRPC interface.

e8d933d

Signed-off-by: John Plevyak <jplevyak@gmail.com>

jplevyak force-pushed the raw-grpc-interface branch from 478fd22 to e8d933d Compare April 9, 2019 16:48

snowp assigned lizan Apr 9, 2019

lizan reviewed Apr 9, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 9, 2019

htuch self-assigned this Apr 10, 2019

htuch suggested changes Apr 11, 2019

View reviewed changes

include/envoy/grpc/async_client.h Outdated Show resolved Hide resolved

include/envoy/grpc/async_client.h Outdated Show resolved Hide resolved

include/envoy/grpc/async_client.h Show resolved Hide resolved

include/envoy/grpc/async_client.h Outdated Show resolved Hide resolved

jplevyak added 2 commits April 11, 2019 21:18

use NOT_REACHED_GCOVR_EXCL_LINE in place of exceptions.

80c5ecf

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Fix formating.

c64fcd0

Signed-off-by: John Plevyak <jplevyak@gmail.com>

repokitteh-read-only bot removed the waiting label Apr 12, 2019

htuch added the waiting label Apr 15, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 19, 2019

Update to the new Buffer-based gRPC API.

3a80415

Signed-off-by: John Plevyak <jplevyak@gmail.com>

repokitteh-read-only bot removed the waiting label Apr 19, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 19, 2019

Add missing file.

2210ddc

Signed-off-by: John Plevyak <jplevyak@gmail.com>

lizan suggested changes Apr 20, 2019

View reviewed changes

source/common/grpc/common.h Outdated Show resolved Hide resolved

test/common/grpc/common_test.cc Outdated Show resolved Hide resolved

htuch suggested changes Apr 22, 2019

View reviewed changes

source/common/grpc/typed_async_client.h Outdated Show resolved Hide resolved

source/common/grpc/typed_async_client.h Show resolved Hide resolved

source/common/grpc/typed_async_client.cc Outdated Show resolved Hide resolved

source/common/grpc/common.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Apr 22, 2019

Change serializeBody -> serializeToGrpcFrame as requested.

45f1e17

Signed-off-by: John Plevyak <jplevyak@gmail.com>

repokitteh-read-only bot removed the waiting label Apr 24, 2019

lizan reviewed Apr 24, 2019

View reviewed changes

include/envoy/grpc/async_client.h Outdated Show resolved Hide resolved

htuch added the waiting label Apr 24, 2019

jplevyak added 2 commits May 22, 2019 06:31

Address comments.

70a1688

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Address comments.

b04ee92

Signed-off-by: John Plevyak <jplevyak@gmail.com>

repokitteh-read-only bot removed the waiting label May 22, 2019

htuch previously approved these changes May 22, 2019

View reviewed changes

test/mocks/grpc/mocks.h Show resolved Hide resolved

htuch added the waiting label May 22, 2019

Merge remote-tracking branch 'upstream/master' into raw-grpc-interface

cf58e00

Signed-off-by: John Plevyak <jplevyak@gmail.com>

repokitteh-read-only bot removed the waiting label May 28, 2019

htuch added the waiting label May 28, 2019

Add test for additional coverage.

c5cbb52

Signed-off-by: John Plevyak <jplevyak@gmail.com>

jplevyak dismissed htuch’s stale review via c5cbb52 June 4, 2019 03:40

repokitteh-read-only bot removed the waiting label Jun 4, 2019

jplevyak added 3 commits June 5, 2019 10:22

Merge remote-tracking branch 'upstream/master' into raw-grpc-interface

bdf29de

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Always check return from grpc::ByteBuffer::Dump().

00116b2

See envoyproxy#6917 for details. Signed-off-by: John Plevyak <jplevyak@gmail.com>

Merge remote-tracking branch 'upstream/master' into raw-grpc-interface

7bd3307

Signed-off-by: John Plevyak <jplevyak@gmail.com>

lizan reviewed Jun 5, 2019

View reviewed changes

source/common/grpc/common.h Outdated Show resolved Hide resolved

htuch suggested changes Jun 6, 2019

View reviewed changes

htuch added the waiting label Jun 6, 2019

Address comments.

dab4d57

Signed-off-by: John Plevyak <jplevyak@gmail.com>

repokitteh-read-only bot removed the waiting label Jun 6, 2019

lizan approved these changes Jun 6, 2019

View reviewed changes

htuch approved these changes Jun 6, 2019

View reviewed changes

htuch merged commit d681137 into envoyproxy:master Jun 6, 2019

rgs1 mentioned this pull request Jun 12, 2019

envoy stuck initializing cluster after we updated to 6ea29c7 #7252

Closed

Conversation

jplevyak commented Apr 9, 2019

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 commented Apr 12, 2019

Uh oh!

jplevyak commented Apr 12, 2019

Uh oh!

jplevyak commented Apr 12, 2019

Uh oh!

mattklein123 commented Apr 12, 2019

Uh oh!

htuch commented Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stale bot commented Apr 19, 2019

Uh oh!

jplevyak commented Apr 19, 2019

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jplevyak commented May 23, 2019

Uh oh!

htuch commented May 23, 2019

Uh oh!

repokitteh-read-only bot commented May 23, 2019

Uh oh!

lizan commented May 23, 2019

Uh oh!

azure-pipelines bot commented May 23, 2019

Uh oh!

htuch commented May 23, 2019

Uh oh!

jplevyak commented Jun 5, 2019

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

htuch commented Apr 12, 2019 •

edited

Loading