Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

wasm: allow execution of multiple instances of the same plugin. #13753

Merged
merged 26 commits into from
Nov 12, 2020
Merged

wasm: allow execution of multiple instances of the same plugin. #13753

merged 26 commits into from
Nov 12, 2020

Conversation

PiotrSikora
Copy link
Contributor

@PiotrSikora PiotrSikora commented Oct 26, 2020 •
edited
Loading

Fixes #13690.

Signed-off-by: Piotr Sikora piotrsikora@google.com

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Oct 26, 2020
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #13753 was opened by PiotrSikora.

see: more, trace.

@PiotrSikora PiotrSikora mentioned this pull request Oct 26, 2020
Closed
@lizan
Copy link
Member

lizan commented Oct 27, 2020

@mathetake

@mathetake
Copy link
Member

@lizan I got the context by following related issues.

@PiotrSikora If there's anything I can help, let me know

@mathetake mathetake mentioned this pull request Oct 27, 2020
Merged
@mattklein123 mattklein123 self-assigned this Oct 28, 2020
@PiotrSikora
wasm: allow execution of multiple instances of the same plugin.
697dc19 
Fixes envoyproxy#13690.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora PiotrSikora marked this pull request as ready for review October 29, 2020 05:46
@PiotrSikora
Copy link
Contributor Author

@mathetake this is ready for review now (@lizan could we add @mathetake to assignable?)

@PiotrSikora PiotrSikora mentioned this pull request Oct 29, 2020
Closed
PiotrSikora
PiotrSikora commented Oct 29, 2020
View reviewed changes
bazel/repository_locations.bzl Outdated Show resolved Hide resolved
test/extensions/common/wasm/wasm_test.cc Outdated Show resolved Hide resolved
source/extensions/common/wasm/context.cc Show resolved Hide resolved
@PiotrSikora
review: fix clang-tidy.
672c1ff 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
mathetake
mathetake reviewed Oct 29, 2020
View reviewed changes
Copy link
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this against proxy-wasm-go-sdk's e2e and it works as well! LGTM after the fix of proxy-wasm-cpp-host repository location @PiotrSikora

mathetake and others added 3 commits October 30, 2020 08:34
@mathetake @PiotrSikora
wasm: remove the gap between rust and cpp test code for logging
8d49c1d 
Signed-off-by: mathetake <takeshi@tetrate.io>
@PiotrSikora
review: use merged commit.
9f98a40 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm_co…
60003c1 
…nfig_id

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
mathetake
mathetake suggested changes Nov 2, 2020
View reviewed changes
Copy link
Member

@mathetake mathetake left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize that usign tls_slog->get() would cause a crash during shutdown (e.g. handling SIGTERM) if it's not a release build: https://github.com/envoyproxy/envoy/blob/master/source/common/thread_local/thread_local_impl.cc#L66-L66

looks like using currentThreadRegistered would be better to check if the slot exists

source/extensions/filters/network/wasm/wasm_filter.cc Outdated Show resolved Hide resolved
source/extensions/filters/http/wasm/wasm_filter.cc Outdated Show resolved Hide resolved
source/extensions/bootstrap/wasm/config.h Outdated Show resolved Hide resolved
source/extensions/access_loggers/wasm/wasm_access_log_impl.h Outdated Show resolved Hide resolved
@PiotrSikora
review: use PluginHandle for shutdown-on-destroy semantics.
9965dcd 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@repokitteh-read-only repokitteh-read-only bot added deps Approval required for changes to Envoy's external dependencies and removed waiting labels Nov 10, 2020
@mattklein123
Copy link
Member

Thanks for iterating, this looks much better. LMK when this is ready for review again and not pointing at your branch.

/wait

@PiotrSikora
review: update release date.
d128aaf 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Contributor Author

@mattklein123 this should be good to go, but it needs #13930 and #13932 to be merged first.

@mattklein123
Copy link
Member

OK just ping me when this is ready.

/wait

@PiotrSikora
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm_co…
7bc7c9a 
…nfig_id

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora PiotrSikora mentioned this pull request Nov 12, 2020
Merged
@PiotrSikora
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm_co…
e15e498 
…nfig_id

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Contributor Author

@mattklein123 all other PRs are now merged, so this is ready for review / merge. Thanks!

cc @envoyproxy/dependency-shepherds for dependency update.

@PiotrSikora PiotrSikora mentioned this pull request Nov 12, 2020
Merged
mattklein123
mattklein123 approved these changes Nov 12, 2020
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup, thanks.

@mattklein123 mattklein123 merged commit 3c8e56a into envoyproxy:master Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmarantz jmarantz jmarantz left review comments

@mattklein123 mattklein123 mattklein123 approved these changes

@mathetake mathetake mathetake approved these changes

@kyessenov kyessenov Awaiting requested review from kyessenov

@moderation moderation Awaiting requested review from moderation

@htuch htuch Awaiting requested review from htuch

@lizan lizan Awaiting requested review from lizan

Assignees

@jmarantz jmarantz

@mattklein123 mattklein123

Labels
deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

wasm: plugin configuration is racey
6 participants
@PiotrSikora @lizan @mathetake @mattklein123 @jmarantz @moderation