Upstream ABI Part 1 (logging + stats) from envoy-wasm . by jplevyak · Pull Request #9217 · envoyproxy/envoy

jplevyak · 2019-12-04T01:10:32Z

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

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

Description: Upstream from envoyproxy/envoy-wasm the Part1 of the core ABI: logging and stats.
Risk Level: Low
Testing: Unit tests
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

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

repokitteh-read-only · 2019-12-04T01:10:37Z

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

🐱

Caused by: #9217 was opened by jplevyak.

see: more, trace.

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

htuch

Thanks, this is epic. I think we need to ensure that this review is done with the appropriate context. The ABI being defined here is going to be essentially the future of Envoy's filter ABI, even for C++, when folks are looking for a stable ABI. I think with NullVM, we basically have a solution for #3390.

With that in mind, the ABI/API here needs to be scrutinized with this as a the goal, rather than being a limited ABI/API "just for WASM".

Is it possible to reduce the size of this PR to make it more tractable BTW? I think 3k is really pushing the limit on what can be reasonable reviewed.

htuch · 2019-12-04T01:52:26Z

+//
+
+// Non-stream calls.
+extern "C" EMSCRIPTEN_KEEPALIVE uint32_t proxy_on_start(uint32_t root_context_id,


Why aren't we following Envoy style conventions in our SDK APIs?

This is the ABI and it is following the WASI style since that is the standardization body which we are working with for the ABI.

As per @mattklein123 comment, this is the first time this has come up, it would be good to understand more details. Given the key role this ABI is going to play in Envoy going forward beyond just WASM, it would be good to align on strategy for how this ABI is going to be standardized, governed and evolved.

htuch · 2019-12-04T01:52:56Z

@@ -0,0 +1,13 @@
+/*


Should this interface tree be versioned? I.e. the C++ ABI might have a v2 in the future, it should have a directory structure and tree that reflects this.

We consider this to be v0 since we we haven't gotten it all in and gotten buy-in from the community. I'll move this to api/wasm/cpp/v0/.... and we can snap v1 when we have upstreamed and addressed any pending issues (e.g. renaming).

Do you plan to use semantic version, or a scheme similar to our APIs, e.g. v1alpha, v2, v2alpha, etc?

Semantic versioning is probably overkill at this point, particularly since WASI hasn't (AFAICT) decided on how they will do versioning in the long run.

I really think we should mostly ignore WASI right now. The WASM ecosystem is too immature to be focused on standardization in this first pass. Can we just go ahead and do a v0 for now? I agree that full semantic versioning might be a bit premature.

lizan · 2019-12-04T02:42:21Z

Is it possible to split this PR to parts:

the C++ part binding WASM into Envoy (WasmVm with api/**.proto)
the C ABI part
build container (some of them should go to https://github.com/envoyproxy/envoy-build-tools, to publish wasm build docker image, maybe it can be consolidated to one)
test extension with tests.

mattklein123 · 2019-12-04T03:49:58Z

+1 on splitting this PR into multiple smaller PRs. In the beginning, let's shoot for just a few APIs so we can nail high level concerns. Hopefully once we agree on the basics it will go faster.

yskopets · 2019-12-04T16:58:49Z

+  string configuration = 4;
+}
+
+// WasmService is configured as a built-in *envoy.wasm_service* :ref:`ServiceConfig


Is envoy.wasm_service a reserved constant ?
In which context should it be used ?

A WASM service is a WASM module which is not configured as part of a filter. It is intended to be used for background activity, e.g. for log or stat collation.

Naming suggestion: Wasm worker, Wasm daemon. I find the term "service" to overloaded as-is.

Worker is overused and has a specific meaning in Envoy. This is also not a daemon in the normal Unix sense. I am open to other suggestions. In general I find that naming is controversial and orthogonal. Let's create a separate issue for each renaming and then we can discuss it there and do the rename in a separate PR without distracting from the main design review.

yskopets · 2019-12-04T17:00:01Z

+}
+
+// WasmService is configured as a built-in *envoy.wasm_service* :ref:`ServiceConfig
+// <envoy_api_msg_config.wasm.v2.ServiceConfig>`. This opaque configuration will be used to


What is wasm.v2.ServiceConfig ?

It's not present in this PR (neither in envoy-wasm repo)

Changed to WasmService.

yskopets · 2019-12-04T17:10:03Z

@@ -0,0 +1,11 @@
+mergeInto(LibraryManager.library, {


Can we add a comment about the purpose of this file ?

Effectively, it duplicates definitions from proxy_wasm_externs.h.

Is it a mandatory file for Emscripten ? Should we maintain it manually ?

This is required by Emscripten and it needs to contain all the imported functions from the WASM host.

yskopets · 2019-12-04T17:13:21Z

+cp src/.libs/libprotobuf.a ${CPP_API}/libprotobuf.a
+```
+
+### WAVM binaries


Why do we need WAVM binaries ?

To precompile *.wasm file ?

We don't. However WAVM does provide some useful tools. I can remove those.

Done.

yskopets · 2019-12-04T17:24:53Z

+
+#include "stddef.h"
+
+//


Can we document in here the implicit assumptions of ABI ?

E.g.,

a WebAssembly module MUST export _malloc and _free functions

Envoy uses _malloc to pass strings into WebAssembly module

Envoy always passes ownership over memory allocated with _malloc to a WebAssembly module

Does WebAssembly module ever pass ownership over memory to Envoy ? E.g., as part of proxy_log and proxy_define_metric

There is an ABI document which describes these things. I will talk with @PiotrSikora about adding this document in a different PR and referencing it here.

yskopets · 2019-12-04T19:19:13Z

+  return wasmResultToWord(WasmResult::Ok);
+}
+
+Word set_effective_context(void* raw_context, Word context_id) {


What is the use case for set_effective_context() ?

Calls out of the VM are done w.r.t a Context, e.g. stream or root context. gRPC calls are passed to a RootContext since they can outlive a give stream (e.g. reporting events to an external system). If the gRPC is (for example) filling a cache entry where there are multiple streams waiting on that cache entry then we need to be able to set the effective context to each of those streams so that they can be restarted and handled w.r.t. the new cached data.

However, I will pull this out and bring it back in a future PR which includes gRPC support.

yskopets · 2019-12-04T19:44:06Z

+  void setContext(Context* context) { contexts_[context->id()] = context; }
+  void startForTesting(std::unique_ptr<Context> root_context, PluginSharedPtr plugin);
+
+  bool getEmscriptenVersion(uint32_t* emscripten_metadata_major_version,


Where do we need Emscripten-specific logic ?

This can be used to support multiple emscripten versions. Currently we are not doing that because we have not upstreamed and so we don't have lots of existing WASM plugins. Once we upstream and have production users we will want to support previous versions of emscripten subject to a policy which we should develop.

yskopets · 2019-12-04T20:01:07Z

+  // Calls with the "proxy_" prefix.
+#define _REGISTER_PROXY(_fn)                                                                       \
+  wasm_vm_->registerCallback(                                                                      \
+      "env", "proxy_" #_fn, &Exports::_fn,                                                         \


Why do we register Envoy's ABI as part of env module instead of using a dedicated module name like envoy ?

We originally had it like that, but ultimately decided against it. As part of standardization we want to be WASI naming compatible, and using an "envoy" module would not fit with that.

Can you link to more information whatever standardization process you are referring to? What is being standardized here? At a high level this sounds nice but I also want to make sure we are doing the right things here independent of whatever a standardizing body wants (not to mention I'm not sure we want to slow down to wait for a standardizing body).

We discussed Proxy-WASM ABI design and standardization at the WebAssembly / WASI meeting on 10/15. There was general interest in having a Proxy-WASM WASI standard and another similar proposal from Fastly. The general feeling was that we should work together with the goal of standardization which would be in everyone's interest. Of course standards take time and are best based on robust understanding of requirements which can often come only from implementation and production experience. If follows that we need something which works well for all Envoy use-cases so we can get that experience. However, given a choice of equals in the design it would be better to choose options which have fewer dependencies (i.e. not requiring protobuf support for basic things like configuration) which may not be available in other environments.

And by not requiring protobufs for configuration, I mean in the WASM code in the argument to the on_configuration call. In the host code in Envoy using protobufs is obviously the way to go.

Per discussion elsewhere I really don't think we should be focusing on standardizing right now unless it provides very clear benefits. Everything seems very immature and we should focus now on doing the right thing ans satisfying real world use cases.

jplevyak · 2019-12-04T23:31:01Z

RE: splitting this PR. This is the smallest portion of the ABI that I could reasonably split out and still make it contain tests. There is a great deal more for the full ABI and API and I was forced to make some throw-away changes to get this bit to be independent. That said, I could split out some portions which are "safe" on their own:

the .proto files
the build system

lizan@ I am not sure what you mean by "the C++ part binding WASM into Envoy (WasmVm with api/**.proto)". The WasmVm doesn't use the .proto files.

I also am reluctant to split out "test extension with tests." since a PR without tests is not trustworthy.

I am very much open to suggestions. This was the smallest portion that I could split out that involved tests, but if there are portions that you feel comfortable reviewing without tests I would be happy to split those out.

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

repokitteh-read-only · 2019-12-05T23:19:34Z

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

🐱

Caused by: #9217 was synchronize by jplevyak.

see: more, trace.

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

mattklein123 · 2019-12-05T23:32:05Z

@jplevyak my advice is to open a fresh PR that has the smallest amount of code as possible (one API potentially) that will allow us to discuss the high level approach. We can then review tests, etc. I want to make sure we agree on the high level approach as there have been many decisions made here that are opaque to us and need to be re-reviewed. Thank you.

jplevyak · 2019-12-06T22:17:48Z

@mattklein123 This was my attempt at pulling out the very smallest amount of code possible. All it has is the life cycle + logging + stats. I can remove the stats and that would leave only the lifecycle + logging which we need for the tests. I don't think that is going to help much though.

I can move out bits like the proto and SDK and they will be unmotivated and not have tests, but perhaps that is a better approach. We can use this PR for motivation and reference.

htuch · 2019-12-06T22:19:36Z

@jplevyak is there no way to have the API minus implementation? I think we want to get agreement at the level of interface header files first.

jplevyak · 2019-12-06T22:23:12Z

Protos: #9256

jplevyak · 2019-12-06T22:23:54Z

I'll pull out the ABI separately. I'll also remove the stats.

jplevyak · 2019-12-06T22:36:13Z

ABI V0 Part1: Lifecycle + logging: #9257

jplevyak · 2019-12-06T22:59:38Z

@lizan regarding the docker build system. We can build either via docker or via "make" and eventually we should be able to build via bazel. We can also build for the Null VM or for .wasm files. There are files which are shared between these environments. If I move the Dockerfile to envoy-build-tools, then I am going to have to add a dependency to that repo on the envoy main repo or wherever the SDK files are since they will not live exclusively in docker.

How would you suggest I proceed?

jplevyak · 2019-12-06T23:32:55Z

This PR included some other changes which were in the envoy-wasm repo.

I have created a new PR with just those changes: #9259

stale · 2019-12-16T21:39:05Z

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!

jplevyak · 2019-12-18T19:03:07Z

This is being divided up.

Upstream ABI Part 1 (logging + stats) from envoy-wasm .

b8648ee

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

repokitteh-read-only Bot added the api label Dec 4, 2019

Fix formatting.

97ff8e8

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

htuch reviewed Dec 4, 2019

View reviewed changes

mattklein123 added the waiting label Dec 4, 2019

mattklein123 self-assigned this Dec 4, 2019

yskopets reviewed Dec 4, 2019

View reviewed changes

Address comments.

0ef1d3b

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

repokitteh-read-only Bot removed the waiting label Dec 5, 2019

jplevyak added 2 commits December 5, 2019 23:20

Address comments.

c42f3f5

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

Address comments.

0aeb55a

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

mattklein123 added the waiting label Dec 13, 2019

stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 16, 2019

jplevyak closed this Dec 18, 2019


		#include "stddef.h"

		//

Conversation

jplevyak commented Dec 4, 2019

Uh oh!

repokitteh-read-only Bot commented Dec 4, 2019

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!

lizan commented Dec 4, 2019

Uh oh!

mattklein123 commented Dec 4, 2019

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!

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!

yskopets Dec 4, 2019 • 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!

jplevyak commented Dec 4, 2019

Uh oh!

repokitteh-read-only Bot commented Dec 5, 2019

Uh oh!

mattklein123 commented Dec 5, 2019

yskopets Dec 4, 2019 •

edited

Loading