wasm: clean up the code #36556

wbpcode · 2024-10-12T17:40:35Z

Commit Message: wasm: clean up the code
Additional Description:

When I doing the #36456, I found there are lots of redundant code in the wasm extensions. And the wasm loading and creations are spread out in multiple different positions.
This redundancy and fragmentation make #36456 become more and more complex.

Finally, I split the code clean up out as an independent PR.

This PR doesn't change any logic but only merge duplicated logic.

Risk Level: n/a.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

kyessenov

LGTM but not 100% if code is exactly the same. I can take a look again.

source/extensions/common/wasm/wasm.cc

kyessenov · 2024-10-14T19:39:14Z

source/extensions/common/wasm/wasm.cc

+                                init_manager, context.mainThreadDispatcher(), context.api(),
+                                context.lifecycleNotifier(), remote_data_provider_,
+                                std::move(callback))) {
+    throw Common::Wasm::WasmException(


Can we restructure this to avoid exceptions?

Great suggestion.

I finally add a TODO first. Because this is in the constructor and we need to use creation_status to handle the error. That means we need to change code of lots of different positions at where this PluginConfig is used.

We can put it to separated PR.

kyessenov · 2024-10-14T19:39:48Z

source/extensions/common/wasm/wasm.cc

+
+  if (is_singleton_handle_) {
+    // Use critical log because this error should not happen in production.
+    ENVOY_LOG(critical, "CreateContext() only works for thread local plugins.");


why not release assert if this never should be called by the implementation?

This check is finally removed at #36456. Finally, we use the same way to treat the singleton handle and thread local handle.
So, I think we can leave it there for now?

kyessenov · 2024-10-14T19:40:56Z

source/extensions/common/wasm/wasm.cc

+    return singleton_handle_ != nullptr ? singleton_handle_->wasmOfHandle() : nullptr;
+  }
+
+  ASSERT(thread_local_handle_ != nullptr);


RELEASE_ASSERT since it would crash anyways?

From the current code base, seems we prefer the ASSERT even in this type case? Let's me know if you think this is a block point.

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

source/extensions/common/wasm/wasm.h

wbpcode · 2024-10-16T04:11:11Z

ping @mpwarres for final review of this wasm related changes. :)

mpwarres

Great idea for refactoring!

source/extensions/access_loggers/wasm/config.cc

source/extensions/bootstrap/wasm/config.cc

-    throw Common::Wasm::WasmException(
-        fmt::format("Unable to create Wasm service {}", plugin->name_));
-  }
+  wasm_service_ = std::make_unique<Common::Wasm::PluginConfig>(


source/extensions/bootstrap/wasm/config.cc

source/extensions/bootstrap/wasm/config.h

mpwarres · 2024-10-16T04:24:33Z

source/extensions/common/wasm/wasm.cc

+    plugin_handle_initialized_ = true;
+
+    if (base_wasm == nullptr) {
+      ENVOY_LOG(critical, "Plugin {} failed to load", plugin_->name_);


Some cases of the old logic that this refactoring consolidates used different log levels here depending on whether or not plugin->fail_open_ was true. Can that be preserved?

Also (optionally) you might consider having the PluginConfig constructor take an additional string param for the role of the plugin (e.g. "stat sink" vs. "access logger" vs. "HTTP filter" etc.), so that can be retained in the log message here. While it isn't strictly necessary functionality-wise, I think it is useful information in the log message, since plugin failure could occur in a number of contexts.

Some cases of the old logic that this refactoring consolidates used different log levels here depending on whether or not plugin->fail_open_ was true. Can that be preserved?

The fail_open self will be deprecated in favor of new FailurePolicy in the Envoy. And I think it acutally make no big sense to keep that logic.

Also (optionally) you might consider having the PluginConfig constructor take an additional string param for the role of the plugin (e.g. "stat sink" vs. "access logger" vs. "HTTP filter" etc.), so that can be retained in the log message here. While it isn't strictly necessary functionality-wise, I think it is useful information in the log message, since plugin failure could occur in a number of contexts.

I personally think the plugin name should be enough to distinguish different extension and their type. But I don't oppose it. I will add a TODO first until some acutally user ask it. (By the way, we created wasm bootstrap extension, wasm network extension, http extension, logger extension, etc, but we don't know how much of them is what the users actually want. At least from recently problems/bug, I think even the http wasm extension is not be accepted/used widely. We do too much things before get actual practising feedbacks and requirements.)

The fail_open self will be deprecated in favor of new FailurePolicy in the Envoy. And I think it acutally make no big sense to keep that logic.

SG--didn't know about that!

I personally think the plugin name should be enough to distinguish different extension and their type. But I don't oppose it. I will add a TODO first until some acutally user ask it.

Fair enough--SGTM.

mpwarres · 2024-10-16T04:27:01Z

source/extensions/common/wasm/wasm.cc

+    return nullptr;
+  }
+
+  Wasm* wasm = plugin_holder->handle->wasmOfHandle();


It seems like most/all of the logic in this method up to this point is the same as PluginConfig::wasmOfHandle(). Could the first half of this method be replaced by a call to PluginConfig::wasmOfHandle()?

We need both Wasm* and PluginHandle here, which make we cannot call the wasmOfHandle directly here. But this is optimized at #36456.

Got it, SG.

source/extensions/common/wasm/wasm.h

mpwarres · 2024-10-16T04:35:44Z

source/extensions/common/wasm/wasm.h

+               const envoy::config::core::v3::Metadata* metadata, bool singleton);
+
+  std::shared_ptr<Context> createContext();
+  Wasm* wasmOfHandle();


How about making this WasmHandleSharedPtr wasmHandle()? That would avoid the possibility of lifetime issues, as well as the name wasmOfHandle which sounds a little awkward to me.

If we return a WasmHandleSharedPtr here, the WasmHandleSharedPtr self may be null. It make the caller need to do additional check to get a Wasm* pointer.

But I agree the naming is not good, may call it wasm() directly?

And the lifetime is another problem I think we should to improve at this code base. We should only return shared pointer if we hope the caller to longer the lifetime of the returned object. This method here is not designed to work at that way.

Thanks, I agree wasm() is a better name, LG.

source/extensions/common/wasm/wasm.h

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

…asm-clean-up

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

wbpcode · 2024-10-17T03:08:12Z

friendly ping for another approval @kyessenov :)

kyessenov

Thanks!

wasm: clean up the code

e472ab7

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

wbpcode assigned kyessenov and mpwarres Oct 12, 2024

wbpcode added 2 commits October 13, 2024 10:32

fix test

6106740

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

support singleton

2cc8acb

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

wbpcode mentioned this pull request Oct 13, 2024

wasm: restart wasm vm if it's failed because runtime error #36456

Merged

wbpcode added 2 commits October 14, 2024 10:03

improve coverage

3a8fd67

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

check check

91f2db4

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

kyessenov reviewed Oct 14, 2024

View reviewed changes

address some comments

8270067

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

kyessenov reviewed Oct 15, 2024

View reviewed changes

source/extensions/common/wasm/wasm.h Outdated Show resolved Hide resolved

kyessenov previously approved these changes Oct 15, 2024

View reviewed changes

mpwarres reviewed Oct 16, 2024

View reviewed changes

address all comments

8ad8cd0

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

wbpcode dismissed kyessenov’s stale review via 8ad8cd0 October 16, 2024 07:52

wbpcode added 4 commits October 16, 2024 15:52

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-w…

99c7386

…asm-clean-up

add todo

80f9eef

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

remove unneeded type name

f89926e

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

improve coverage

0bbd11d

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

wbpcode added a commit to wbpcode/envoy that referenced this pull request Oct 16, 2024

update based on the envoyproxy#36556

eb2f707

Signed-off-by: wangbaiping <wangbaiping@bytedance.com>

mpwarres approved these changes Oct 16, 2024

View reviewed changes

wbpcode enabled auto-merge (squash) October 17, 2024 03:07

kyessenov approved these changes Oct 17, 2024

View reviewed changes

wbpcode merged commit f63b61a into envoyproxy:main Oct 17, 2024
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wasm: clean up the code #36556

wasm: clean up the code #36556

wbpcode commented Oct 12, 2024 •

edited

Loading

kyessenov left a comment

kyessenov Oct 14, 2024

wbpcode Oct 15, 2024 •

edited

Loading

wbpcode Oct 15, 2024 •

edited

Loading

kyessenov Oct 14, 2024

wbpcode Oct 15, 2024 •

edited

Loading

kyessenov Oct 14, 2024

wbpcode Oct 15, 2024

wbpcode commented Oct 16, 2024

mpwarres left a comment

This comment was marked as resolved.

mpwarres Oct 16, 2024

mpwarres Oct 16, 2024

wbpcode Oct 16, 2024

wbpcode Oct 16, 2024 •

edited

Loading

mpwarres Oct 16, 2024

mpwarres Oct 16, 2024

wbpcode Oct 16, 2024 •

edited

Loading

mpwarres Oct 16, 2024

mpwarres Oct 16, 2024

wbpcode Oct 16, 2024 •

edited

Loading

mpwarres Oct 16, 2024

wbpcode commented Oct 17, 2024

kyessenov left a comment

wasm: clean up the code #36556

wasm: clean up the code #36556

Conversation

wbpcode commented Oct 12, 2024 • edited Loading

kyessenov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

wbpcode Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode commented Oct 16, 2024

mpwarres left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode commented Oct 17, 2024

kyessenov left a comment

Choose a reason for hiding this comment

wbpcode commented Oct 12, 2024 •

edited

Loading

wbpcode Oct 15, 2024 •

edited

Loading

wbpcode Oct 15, 2024 •

edited

Loading

wbpcode Oct 15, 2024 •

edited

Loading

wbpcode Oct 16, 2024 •

edited

Loading

wbpcode Oct 16, 2024 •

edited

Loading

wbpcode Oct 16, 2024 •

edited

Loading