Wasmtime GitHub notifications bot (Jul 22 2024 at 07:21): Wasmtime GitHub notifications bot (Jul 22 2024 at 07:21):iawia002 opened PR #8983 from iawia002:wasi-keyvalue to bytecodealliance:main:
The wasi-keyvalue API has entered Phase 2. This patch implements the API, giving wasm components access to key-value storages.
Currently, only the Redis storage backend has been implemented, and support for more kv storage (e.g. etcd, etc.) will be added in the future.
cc @alexcrichton
Wasmtime GitHub notifications bot (Jul 22 2024 at 07:21):iawia002 requested wasmtime-core-reviewers for a review on PR #8983.
Wasmtime GitHub notifications bot (Jul 22 2024 at 07:21):iawia002 requested pchickey for a review on PR #8983.
Wasmtime GitHub notifications bot (Jul 22 2024 at 07:21):iawia002 requested wasmtime-default-reviewers for a review on PR #8983.
Wasmtime GitHub notifications bot (Jul 22 2024 at 07:32):iawia002 updated PR #8983.
Wasmtime GitHub notifications bot (Jul 22 2024 at 07:43):iawia002 updated PR #8983.
Wasmtime GitHub notifications bot (Jul 22 2024 at 08:16):Would it make sense to have a local storage method too? That would allow quick tests without requiring the setup of an entire database.
Wasmtime GitHub notifications bot (Jul 22 2024 at 09:06):iawia002 commented on PR #8983:
Would it make sense to have a local storage method too? That would allow quick tests without requiring the setup of an entire database.
Sure, I'll implement an in-memory backend and make the other database backends optional.
Wasmtime GitHub notifications bot (Jul 22 2024 at 09:52):iawia002 updated PR #8983.
Wasmtime GitHub notifications bot (Jul 22 2024 at 10:18):iawia002 commented on PR #8983:
I will fix the dead-code warning, but I have no idea why the runtime_config_get test failed:
---- runtime_config_get stdout ---- thread 'runtime_config_get' panicked at crates/wasmtime/src/runtime/vm/libcalls.rs:147:5: internal error: entered unreachable code
Wasmtime GitHub notifications bot (Jul 23 2024 at 08:53):iawia002 updated PR #8983.
Wasmtime GitHub notifications bot (Jul 23 2024 at 09:11):iawia002 commented on PR #8983:
I will fix the dead-code warning, but I have no idea why the runtime_config_get test failed:
---- runtime_config_get stdout ---- thread 'runtime_config_get' panicked at crates/wasmtime/src/runtime/vm/libcalls.rs:147:5: internal error: entered unreachable codeI have figured it out (#8997) and made a temporary solution by adding the
wasmtime/wmemcheckfeature towasi-runtime-config:[features] # Do not use, this is only used for testing in CI, # see https://github.com/bytecodealliance/wasmtime/issues/8997 test = ["wasmtime/wmemcheck"]
Wasmtime GitHub notifications bot (Jul 24 2024 at 17:33):alexcrichton submitted PR review:
This is looking quite good to me, thanks for the PR and the work here! The biggest question below I have is about how buckets are opened.
Wasmtime GitHub notifications bot (Jul 24 2024 at 17:33):alexcrichton submitted PR review:
This is looking quite good to me, thanks for the PR and the work here! The biggest question below I have is about how buckets are opened.
Wasmtime GitHub notifications bot (Jul 24 2024 at 17:33):alexcrichton created PR review comment:
Since
::default()is used here it looks like there's no way to configure this in-memory configuration?
Wasmtime GitHub notifications bot (Jul 24 2024 at 17:33):alexcrichton created PR review comment:
Is there perhaps a more "official" action to use here to setup redis? We in theory try to avoid third-party dependencies on actions because of security concerns, but if it's a complicated and/or widely-enough-used action we can make an exception.
Wasmtime GitHub notifications bot (Jul 24 2024 at 17:33):alexcrichton created PR review comment:
This exposes the ability for wasm to make a connection to any redis server it wants without the host being able to intervene. In general we try to model the implementations of WASI proposals to deny access to things by default with explicit opt-ins to provide access.
Perhaps as an alternative design, could the configuration for
WasiKeyValueCtxlook along the lines of one of:
- Provides are added ahead-of-time to
WasiKeyValueCtxwhich means the host has an allow-listed set of provides each component gets access to.- The host has a callback or trait which receives the
opencall here and returns aBox<dyn Host>? That would enable the host to customize whether it wants the logic this method has implemneted or something more custom.
Wasmtime GitHub notifications bot (Jul 24 2024 at 17:33):alexcrichton created PR review comment:
It loosk like this is silently ignoring non-integer previous values and converting them to 0?
Wasmtime GitHub notifications bot (Jul 25 2024 at 07:16):iawia002 submitted PR review.
Wasmtime GitHub notifications bot (Jul 25 2024 at 07:16):iawia002 created PR review comment:
Yes, the in-memory implementation is very simple. Currently, it doesn't have any configurable features, so configuration is not supported here.
Wasmtime GitHub notifications bot (Jul 25 2024 at 07:17):iawia002 submitted PR review.
Wasmtime GitHub notifications bot (Jul 25 2024 at 07:17):iawia002 created PR review comment:
Fixed, now, non-integer data conversion errors will be returned directly.
Wasmtime GitHub notifications bot (Jul 25 2024 at 07:19):iawia002 updated PR #8983.
Wasmtime GitHub notifications bot (Jul 25 2024 at 07:25):iawia002 submitted PR review.
Wasmtime GitHub notifications bot (Jul 25 2024 at 07:25):iawia002 created PR review comment:
I am now using GitHub's Redis service containers to start a Redis service, but the drawback is that it can only run on Linux. Therefore, the previous macOS testing has been removed, but I think this is an acceptable compromise.
Wasmtime GitHub notifications bot (Jul 25 2024 at 07:32):iawia002 submitted PR review.
Wasmtime GitHub notifications bot (Jul 25 2024 at 07:32):iawia002 created PR review comment:
I chose the whitelist strategy and added a general configuration method that applies to all providers. By default, servers at all addresses are blocked from connecting. Which addresses the components can connect to depends on the runtime configuration.
Wasmtime GitHub notifications bot (Jul 25 2024 at 07:32):iawia002 commented on PR #8983:
All comments addressed ec3373a, PTAL
Wasmtime GitHub notifications bot (Jul 25 2024 at 14:53):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jul 25 2024 at 14:53):alexcrichton created PR review comment:
Yeah that sounds reasonable to me too, thanks!
Wasmtime GitHub notifications bot (Jul 25 2024 at 14:56):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jul 25 2024 at 14:56):alexcrichton created PR review comment:
Could this be renamed to
allow_redis_hosts?
Wasmtime GitHub notifications bot (Jul 25 2024 at 14:56):alexcrichton created PR review comment:
I'm at least personally a bit hesitant to include this when it doesn't have any support/tests/etc. Would it be possible to hook this up to the configuration of
WasiKeyValueCtx? Or somehow expose the ability to configure this? Otherwise I think it'd be better to remove the internals ofInMemoryand make it an empty struct which is a noop implementation of the trait.
Wasmtime GitHub notifications bot (Jul 26 2024 at 03:18):iawia002 updated PR #8983.
Wasmtime GitHub notifications bot (Jul 26 2024 at 03:29):iawia002 submitted PR review.
Wasmtime GitHub notifications bot (Jul 26 2024 at 03:29):iawia002 created PR review comment:
I added a configuration option to the In-Memory provider that allows presetting some data. I believe this provider is valuable, especially in testing scenarios, where users can use this provider to mock some data without relying on a real database. PTAL if this is reasonable.
Wasmtime GitHub notifications bot (Jul 26 2024 at 04:19):iawia002 updated PR #8983.
Wasmtime GitHub notifications bot (Jul 26 2024 at 04:38):iawia002 commented on PR #8983:
All comments addressed b9d2bd2, PTAL.
The CI failed due to the feature combinations issue we encountered before. Since the changes are not related to this PR, I have submitted a separate PR #9016 to address that issue. Once that PR is merged, I will rebase this PR, and the CI should be happy then.
Wasmtime GitHub notifications bot (Jul 26 2024 at 14:59):alexcrichton submitted PR review:
Looks good to me, thanks! I'll queue for merge after a rebase makes CI happy
(thanks for helping to debug CI and fix the issue!)
Wasmtime GitHub notifications bot (Jul 26 2024 at 17:51):iawia002 updated PR #8983.
Wasmtime GitHub notifications bot (Jul 26 2024 at 18:14):iawia002 commented on PR #8983:
Hi @alexcrichton, CI is happy now except
cargo vet, I believe that should be handled by you.
Wasmtime GitHub notifications bot (Jul 26 2024 at 20:50):alexcrichton updated PR #8983.
Wasmtime GitHub notifications bot (Jul 26 2024 at 20:56):alexcrichton has enabled auto merge for PR #8983.
Wasmtime GitHub notifications bot (Jul 26 2024 at 21:15):alexcrichton merged PR #8983.
Wasmtime GitHub notifications bot (Jul 31 2024 at 23:11):thomastaylor312 commented on PR #8983:
Hey there! wasi-keyvalue champion here. So I don't think any of the champions knew this was landing. I'm super excited someone implemented it here (thanks @iawia002)! However, we are in the process of incorporating a last round of feedback now that we've had people implementing against it: https://github.com/WebAssembly/wasi-keyvalue/pull/46. We might want to hold off on releasing this until we can make those changes and go to a full 0.2.0 release of wasi-keyvalue
Wasmtime GitHub notifications bot (Jul 31 2024 at 23:13):thomastaylor312 edited a comment on PR #8983:
Hey there! wasi-keyvalue champion here. So I don't think any of the champions knew this was landing. I'm super excited someone implemented it here (thanks @iawia002)! However, we are in the process of incorporating a last round of feedback now that we've had people implementing against it: https://github.com/WebAssembly/wasi-keyvalue/pull/46. We might want to hold off on releasing this until we can make those changes and go to a full 0.2.0 release of wasi-keyvalue.
Also, and in memory KV store makes sense to me, but I don't think we want to take hard deps on other KV stores in something like wasmtime. This was something that we had loosely talked about as being "host plugins" using something like wRPC
Wasmtime GitHub notifications bot (Jul 31 2024 at 23:16):thomastaylor312 edited a comment on PR #8983:
Hey there! wasi-keyvalue champion here. So I don't think any of the champions knew this was landing. I'm super excited someone implemented it here (thanks @iawia002)! However, we are in the process of incorporating a last round of feedback now that we've had people implementing against it: https://github.com/WebAssembly/wasi-keyvalue/pull/46. We might want to hold off on releasing this until we can make those changes and go to a full 0.2.0 release of wasi-keyvalue.
Also, an in memory KV store makes sense to me, but I don't think we want to take hard deps on other KV stores in something like wasmtime. This was something that we had loosely talked about as being "host plugins" using something like wRPC
Wasmtime GitHub notifications bot (Aug 01 2024 at 02:18):iawia002 commented on PR #8983:
Hi @thomastaylor312, basically, when a proposal enters Phase 2, we can start implementing it. If we wait for it to be completely ready before we begin, the timeline would become excessively long. Therefore, we also have corresponding implementations for draft versions of proposals, allowing users to start experimenting with the API.
For the
wasi-keyvalue, the implementation targets a specific tag 219ea36. I will continue to track the latest changes in this proposal to keep the implementation up to date.Regarding the release issue, strictly speaking, this has not yet been landed. According to the wasmtime release schedule, the code will be frozen on the 5th and will not be released until the 20th of this month. So we still have time to follow up on the latest changes in the
wasi-keyvalueproposal.Overall, I think we can go ahead and release this, allowing users of wasmtime v24 to try out the
wasi-keyvalue@0.2.0-draftAPI.but I don't think we want to take hard deps on other KV stores in something like wasmtime. This was something that we had loosely talked about as being "host plugins" using something like wRPC
I agree with this, there are too many KV storage solutions. But I'm not sure if we should move all KV storage implementations out of wasmtime, because depending on Redis/etcd/Memcached, etc., does not affect
wasmtimeitself. It only affects the standalone cratewasmtime-wasi-keyvalueand thewasmtime-cli. I hope to implement some very popular providers in wasmtime and turn the others into "host plugins".
Wasmtime GitHub notifications bot (Aug 02 2024 at 04:03):thomastaylor312 commented on PR #8983:
Yeah we chatted about this today in the wasmtime meeting. So I think we are ok with the current pinned tag, especially since things are behind a feature flag.
However, I still think keeping Redis in tree is a bad idea. The reason why even the super popular ones should be a plugin type model is that now any time the redis client has a bug, you have to update the keyvalue crate and the CLI just to get the bug fix. I know this could happen with any crate, but with client APIs, these kinds of fixes happen quite often. If it happens to be security sensitive, it would necessitate a release of the crate and CLI for anyone depending on it. This wouldn't happen if this was done by loading a plugin separately
Wasmtime GitHub notifications bot (Aug 02 2024 at 04:30):iawia002 commented on PR #8983:
I understand your concerns. I am not sure what the "host plugins" will look like exactly. Since you mentioned wrpc, I assume it will involve a server and a client? If we turn all in-tree providers into plugins, would the user have to go through the following additional steps to access Redis?
- Deploy the plugin server
- Import the plugin
Compared to releasing patch versions for
wasmtime-wasi-keyvalueandwasmtime-cli, I am more worried about the impact on usability. Will this be too difficult for users, especially the step of deploying a separate plugin server?Anyway, before the 0.2 release of
wasi-keyvalue, I don't plan to add more providers. I originally intended to consider these after the official release, but now it seems this will be on hold until we decide how to handle these providers.
Wasmtime GitHub notifications bot (Aug 02 2024 at 07:55):iawia002 deleted a comment on PR #8983:
I understand your concerns. I am not sure what the "host plugins" will look like exactly. Since you mentioned wrpc, I assume it will involve a server and a client? If we turn all in-tree providers into plugins, would the user have to go through the following additional steps to access Redis?
- Deploy the plugin server
- Import the plugin
Compared to releasing patch versions for
wasmtime-wasi-keyvalueandwasmtime-cli, I am more worried about the impact on usability. Will this be too difficult for users, especially the step of deploying a separate plugin server?Anyway, before the 0.2 release of
wasi-keyvalue, I don't plan to add more providers. I originally intended to consider these after the official release, but now it seems this will be on hold until we decide how to handle these providers.
Wasmtime GitHub notifications bot (Aug 02 2024 at 08:06):iawia002 commented on PR #8983:
I have now seen the meeting notes. I apologize for any inconvenience caused by not notifying you during the implementation. I will submit a PR to revert the introduction of Redis.
Wasmtime GitHub notifications bot (Aug 05 2024 at 18:02):thomastaylor312 commented on PR #8983:
No problem! I just wanted to make sure we had our ducks in a row here as we move forward adding new interfaces
Last updated: Nov 22 2024 at 16:03 UTC