Upstreaming of proxy-wasm Part 1. #10262

jplevyak · 2020-03-05T05:55:53Z

Signed-off-by: John Plevyak [email protected]

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

Description: Upstreaming of proxy-wasm. This is the first of what will be several PRs.
Risk Level: Low
Testing: Unit tests
Docs Changes: N/A
Release Notes: NA/
[Optional Fixes #Issue] partial #4272
[Optional Deprecated:]

Signed-off-by: John Plevyak <[email protected]>

mattklein123

Flushing out an initial set of questions/comments. Also needs a format fix I think. Thanks for working on this! Very excited to see this land.

bazel/repository_locations.bzl

source/extensions/common/wasm/BUILD

source/extensions/common/wasm/context.h

source/extensions/common/wasm/context.cc

mattklein123 · 2020-03-05T18:37:58Z

source/extensions/common/wasm/context.cc

+    if (it != wasm()->counters_.end()) {
+      if (offset > 0) {
+        it->second->add(offset);
+        return WasmResult::Ok;


The coverage build is broken right now, but please make sure all of the C-style error handling this file has full test coverage. I will be checking before merge. Thank you!

I'll make a coverage pass once we get the first set of reviews done and have resolved any high level changes which might invalidate tests for low level details like error handling conditionals.

source/extensions/common/wasm/context.cc

source/extensions/common/wasm/context.h

Signed-off-by: John Plevyak <[email protected]>

source/extensions/common/wasm/context.cc

Signed-off-by: John Plevyak <[email protected]>

jplevyak · 2020-03-10T18:01:06Z

/retest

repokitteh-read-only · 2020-03-10T18:01:09Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #10262 (comment) was created by @jplevyak.

see: more, trace.

Signed-off-by: John Plevyak <[email protected]>

source/extensions/common/wasm/context.cc

jmarantz · 2020-03-10T18:59:08Z

source/common/stats/symbol_table_impl.h

@@ -837,6 +837,7 @@ class StatNameSet {
  using StringStatNameMap = absl::flat_hash_map<std::string, Stats::StatName>;
  StringStatNameMap builtin_stat_names_;
 };
+using StatNameSetSharedPtr = std::shared_ptr<Stats::StatNameSet>;


I suspect this is not a great pattern for using StatNameSet; I'm inclined not to institutionalize it by centralizing a shortcut for it.

But it's worth exploring what your data model is. Is the StatNameSet shared across all threads, all VMs?

Yes. The VMs are running the same code and there are many of them (e.g 60 on a large system), so the replication would be large without sharing.

envoyproxy/envoy-wasm#452

As discussed on envoyproxy/envoy-wasm#452 we have a new solution and are testing this now. I will upstream that change assuming it works out. Let's not let that hold up this review.

source/extensions/common/wasm/context.cc

jplevyak · 2020-03-10T19:14:52Z

Is something busted with CI ? The failure that this is reporting doesn't make any sense. It is saying that a constructor requires more arguments than the code very clearly requires. Moreover it compiles for some targets and not for others and it compiles fine on my machine. Also, it started to fail after a pushed a comment change. It had worked before that, so it isn't the code AFAICT

mattklein123 · 2020-03-10T20:33:02Z

@jplevyak AZP merges master for you. Merge master.

Also, stepping back, can you update us on how we are supposed to review this? I'm confused about whether we are waiting on ABI docs, doing reviews in the other repo, etc. Thanks!

/wait

jplevyak · 2020-03-10T21:44:10Z

The ABI doc: https://github.com/proxy-wasm/spec

You can do this review independently if you like. I will be merging the host repo review branch back into master. Then I can do a true-up PR here which will update the host repo SHA after that is fully reviewed.

Changes to the ABI will be done in the SDK and host repos first, then integrated here as well. I'll take care of the dependencies, you just review what you feel comfortable with and I'll worry about how to make it all consistent.

mattklein123 · 2020-03-10T22:09:28Z

Changes to the ABI will be done in the SDK and host repos first, then integrated here as well. I'll take care of the dependencies, you just review what you feel comfortable with and I'll worry about how to make it all consistent.

I would like to start with the ABI review as the first thing, and get the most eyes on that. After that, I will take care of the code reviews which hope will move quickly.

So given ^ can we make a PR of the ABI spec/headers/whatever like you did for the other code and then we can have a bunch of people review that?

Signed-off-by: John Plevyak <[email protected]>

PiotrSikora · 2020-03-11T19:42:04Z

I would like to start with the ABI review as the first thing, and get the most eyes on that. After that, I will take care of the code reviews which hope will move quickly.

So given ^ can we make a PR of the ABI spec/headers/whatever like you did for the other code and then we can have a bunch of people review that?

@mattklein123 I'll do that later tonight / tomorrow morning (I need to wrap one thing first). Do you want to have a bunch of small PRs of ABI groups (root, http, network, queues, shared data, etc.) or just one big PR?

mattklein123 · 2020-03-11T20:11:26Z

@mattklein123 I'll do that later tonight / tomorrow morning (I need to wrap one thing first). Do you want to have a bunch of small PRs of ABI groups (root, http, network, queues, shared data, etc.) or just one big PR?

I'm fine with 1 big PR, but see proxy-wasm/proxy-wasm-cpp-host#2 (comment). I think we can just do inline docs on that PR?

Update the host dependency. Move the BUILD files for the proxy-wasm dependencies into envoy external. Signed-off-by: John Plevyak <[email protected]>

Signed-off-by: John Plevyak <[email protected]>

mattklein123 · 2020-03-12T15:45:20Z

cc @lizan ^ our build/bazel expert.

lizan · 2020-03-12T22:27:10Z

Builds seems OK now, what was the issue?

jplevyak · 2020-03-13T17:15:43Z

@lizan I figured it out. Had to remove @envoy from the subpackage. It works for some CI targets and not for others.

jplevyak · 2020-03-16T15:37:09Z

If we are good with this overall, I will make a coverage pass.

mattklein123 · 2020-03-16T16:13:46Z

@jplevyak I think it's fine to start a coverage pass but I have not really reviewed this yet. I'm going to review the ABI header/docs this today.

bazel/external/proxy_wasm_cpp_host.BUILD

Shikugawa · 2020-03-19T04:18:54Z

include/abi/wasm/proxy_wasm_imports.h

 * be nullptr.
 */
-extern "C" WasmResult proxy_get_configuration((uint32_t start, uint32_t length,
-      const char** configuration_ptr, size_t* configuration_size);
+enum class WasmBufferType : int32_t {


I think that WasmConfigurationBufferType may be easier to interpret.

Shikugawa · 2020-03-19T04:39:28Z

bazel/external/proxy_wasm_cpp_host.BUILD

+package(default_visibility = ["//visibility:public"])
+
+cc_library(
+    name = "include",


I think that name = "proxy_wasm_interface" is better.

Shikugawa · 2020-03-19T04:40:01Z

bazel/external/proxy_wasm_cpp_host.BUILD

+)
+
+cc_library(
+    name = "lib",


name = "proxy_wasm_lib" may be better.

Shikugawa · 2020-03-19T04:45:57Z

source/extensions/common/wasm/context.cc

+}
+
+WasmResult Context::incrementMetric(uint32_t metric_id, int64_t offset) {
+  auto type = static_cast<MetricType>(metric_id & Wasm::kMetricTypeMask);


It seems that Hungarian notation is not used much in Envoy.

Signed-off-by: John Plevyak <[email protected]>

stale · 2020-03-26T18:13:34Z

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!

stale · 2020-04-02T21:15:27Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Upstreaming of proxy-wasm Part 1.

972166c

Signed-off-by: John Plevyak <[email protected]>

jplevyak requested review from lizan and PiotrSikora as code owners March 5, 2020 05:55

mattklein123 self-assigned this Mar 5, 2020

mattklein123 reviewed Mar 5, 2020

View reviewed changes

jplevyak added 5 commits March 6, 2020 01:45

Format.

8b5efdc

Signed-off-by: John Plevyak <[email protected]>

Address comments.

65bd8ae

Signed-off-by: John Plevyak <[email protected]>

Address comments.

b21914e

Signed-off-by: John Plevyak <[email protected]>

Address comments.

3204e82

Signed-off-by: John Plevyak <[email protected]>

Address comments.

3a25b41

Signed-off-by: John Plevyak <[email protected]>

mandarjog reviewed Mar 6, 2020

View reviewed changes

source/extensions/common/wasm/context.cc Show resolved Hide resolved

jplevyak added 6 commits March 7, 2020 18:17

Format.

7548daf

Signed-off-by: John Plevyak <[email protected]>

Format.

3518e10

Signed-off-by: John Plevyak <[email protected]>

Format.

c9bfb2e

Signed-off-by: John Plevyak <[email protected]>

Fix formatting.

b6f1ce6

Signed-off-by: John Plevyak <[email protected]>

Remove local repo reference.

7832462

Signed-off-by: John Plevyak <[email protected]>

Remove WasmState which is not needed in this PR.

489f8f8

Signed-off-by: John Plevyak <[email protected]>

jplevyak requested a review from mattklein123 March 9, 2020 17:10

jplevyak added 2 commits March 9, 2020 23:12

Address comments.

b21857a

Signed-off-by: John Plevyak <[email protected]>

Format.

bbc7922

Signed-off-by: John Plevyak <[email protected]>

Move pointer using statement to core.

6f29c2a

Signed-off-by: John Plevyak <[email protected]>

jmarantz reviewed Mar 10, 2020

View reviewed changes

source/extensions/common/wasm/context.cc Outdated Show resolved Hide resolved

Merge remote-tracking branch 'envoyproxy/master' into wasm-upstream-base

b3a3072

jmarantz reviewed Mar 10, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 10, 2020

Add Random as required by RemoteAsyncDataProvider.

e85d273

Signed-off-by: John Plevyak <[email protected]>

repokitteh-read-only bot removed the waiting label Mar 11, 2020

jplevyak added 2 commits March 12, 2020 04:20

Attempt to fix the @envoy dependency problem.

167c344

Update the host dependency. Move the BUILD files for the proxy-wasm dependencies into envoy external. Signed-off-by: John Plevyak <[email protected]>

Sort alphabetically because that was sooooo important.

06581c7

Signed-off-by: John Plevyak <[email protected]>

lizan reviewed Mar 16, 2020

View reviewed changes

bazel/external/proxy_wasm_cpp_host.BUILD Show resolved Hide resolved

mattklein123 added the waiting label Mar 16, 2020

Shikugawa reviewed Mar 19, 2020

View reviewed changes

Address some coverage.

4119e6b

Signed-off-by: John Plevyak <[email protected]>

repokitteh-read-only bot removed the waiting label Mar 19, 2020

mattklein123 added the waiting label Mar 22, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 26, 2020

jplevyak mentioned this pull request Mar 30, 2020

Review of the ContextBase include file. proxy-wasm/proxy-wasm-cpp-host#2

Merged

stale bot closed this Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upstreaming of proxy-wasm Part 1. #10262

Upstreaming of proxy-wasm Part 1. #10262

jplevyak commented Mar 5, 2020

mattklein123 left a comment

mattklein123 Mar 5, 2020

jplevyak Mar 7, 2020

jplevyak commented Mar 10, 2020

repokitteh-read-only bot commented Mar 10, 2020

jmarantz Mar 10, 2020

jplevyak Mar 10, 2020

jplevyak Mar 10, 2020

jplevyak Mar 12, 2020

jplevyak commented Mar 10, 2020

mattklein123 commented Mar 10, 2020

jplevyak commented Mar 10, 2020

mattklein123 commented Mar 10, 2020

PiotrSikora commented Mar 11, 2020 •

edited

Loading

mattklein123 commented Mar 11, 2020

mattklein123 commented Mar 12, 2020

lizan commented Mar 12, 2020

jplevyak commented Mar 13, 2020

jplevyak commented Mar 16, 2020

mattklein123 commented Mar 16, 2020

Shikugawa Mar 19, 2020

Shikugawa Mar 19, 2020

Shikugawa Mar 19, 2020

Shikugawa Mar 19, 2020

stale bot commented Mar 26, 2020

stale bot commented Apr 2, 2020

Upstreaming of proxy-wasm Part 1. #10262

Upstreaming of proxy-wasm Part 1. #10262

Conversation

jplevyak commented Mar 5, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplevyak commented Mar 10, 2020

repokitteh-read-only bot commented Mar 10, 2020

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

jplevyak commented Mar 10, 2020

mattklein123 commented Mar 10, 2020

jplevyak commented Mar 10, 2020

mattklein123 commented Mar 10, 2020

PiotrSikora commented Mar 11, 2020 • edited Loading

mattklein123 commented Mar 11, 2020

mattklein123 commented Mar 12, 2020

lizan commented Mar 12, 2020

jplevyak commented Mar 13, 2020

jplevyak commented Mar 16, 2020

mattklein123 commented Mar 16, 2020

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

stale bot commented Mar 26, 2020

stale bot commented Apr 2, 2020

PiotrSikora commented Mar 11, 2020 •

edited

Loading