WASI Key Value 0.2.0-draft2 support #2895

devigned · 2024-10-24T23:21:04Z

This pull request adds support for the [email protected].

I'm opening this as a draft to get some initial feedback as I implement the remaining providers. Currently, the only provider implemented is the SQLite provider.

fixes #2486

itowlson

Thanks for working on this David. Fingers crossed that the SQLite was the worst of it. I left a few comments but it's brilliant how it's coming along!

itowlson · 2024-10-25T00:16:33Z

crates/key-value-spin/src/store.rs

+
+    // The assumption with increment is that if the value for the key does not exist, it will be
+    // assumed to be zero. In the case that we are unable to unmarshal the value into an i64 an error will be returned.
+    async fn increment(&self, key: String, delta: i64) -> Result<i64, Error> {


This is an interesting design niggle - we have not previously defined an encoding for integers, and I don't think wasi-keyvalue defines one either. All the "set" APIs are byte arrays. So how do I set an initial value to which I can later apply increment? Hopefully this won't be an issue in practice, but might be worth considering in the wasi-keyvalue specification.

Nice catch! I'm just going with little endian encoding of i64s. I think the guidance on using increment right now is to start with a key/value that doesn't exist and "increment" it to your value of choice. Then use increment or get from there on out.

I do believe this is something that we'll need to consider for future interface revisions.

crates/key-value-spin/src/store.rs

itowlson · 2024-10-25T00:27:35Z

crates/key-value-spin/src/store.rs

+            let tx = binding.transaction().map_err(log_error)?;
+            for kv in key_values {
+                tx.prepare_cached(
+                    "INSERT INTO spin_key_value (store, key, value) VALUES ($1, $2, $3)


You're right, this is pretty unlovely, but I have no other ideas. SQLite does provide for bulk inserts, but I don't think they let you do an update where the row already existed. Hopefully it will not be too bad because in-process...!

thankfully single-transaction-many-inserts is equally performant to single-transaction-bulk-insert - and this is easier to read (SQLite bulk inserts require the data source to be a SELECT, which is... unintuitive), so I wouldn't personally be too worried about perf here

crates/factor-key-value/src/host.rs

Signed-off-by: itowlson <[email protected]>

Signed-off-by: David Justice <[email protected]>

devigned · 2024-10-31T02:19:17Z

@michelleN, please take a look when you have a minute. Thank you!

itowlson

This looks really good, David, and I'm so grateful to you for taking it on. I have some comments which I don't think are blockers but would appreciate if you have time to address them. If not then no worries, I can work on them after the RC.

crates/factor-key-value/src/lib.rs

crates/key-value-redis/Cargo.toml

crates/key-value-redis/src/store.rs

itowlson · 2024-10-31T02:27:59Z

crates/key-value-redis/src/store.rs

+            .incr(key, delta)
+            .await
+            .map_err(log_error)
+    }


Heh, I look at these implementations compared to the others, and it becomes very clear what the atomics and batch interfaces were modelled on...!

crates/factor-key-value/src/host.rs

itowlson · 2024-10-31T02:36:32Z

tests/test-components/components/wasi-key-value/src/lib.rs

+        ensure_ok!(store.delete("qux"));
+        ensure_matches!(store.exists("bar"), Ok(false));
+        ensure_matches!(store.get("qux"), Ok(None));
+        ensure_matches!(keys(&store.list_keys(None)), Ok(&[]));


Can we add some atomics and batch tests here, please, even if they're only basic smoke tests for now? I know you have tests in the implementations, but just something that verifies that the APIs work from the guest even if we can't practically test their atomicity or batchitude. (Particularly CAS because resources.)

rylev · 2024-10-31T09:05:09Z

crates/factor-key-value/src/host.rs

+    ) -> Result<wasi_keyvalue::store::KeyResponse, wasi_keyvalue::store::Error> {
+        match cursor {
+            Some(_) => Err(wasi_keyvalue::store::Error::Other(
+                "list_keys: cursor not supported".to_owned(),


Is there ever a plan to support a cursor?

This is for data stores that support paging. You could imagine a case where too many keys are in the store and the client would like to fetch them a page at a time.

Personally, I don't like this. I think listing all keys, possibly across partitions, is a really bad idea.

This is an artefact of the original Spin key-value interface not supporting a cursor. There's no technical reason we couldn't implement one in the WASI implementation (and have the Spin interface sit over that, rather than the other way round). It's just down to time and effort.

rylev · 2024-10-31T09:07:02Z

crates/factor-key-value/src/host.rs

+        delta: i64,
+    ) -> Result<i64, wasi_keyvalue::store::Error> {
+        let store = self.get_store_wasi(bucket)?;
+        store.increment(key, delta).await.map_err(to_wasi_err)


Is Store::increment documented as requiring to be atomic?

In what sense? If I give a delta and then ask for it to be applied it will be without regard to the underlying value. The only issue with atomicity would be in the case where one fetches and increments while another has mutated the underlying value. In that case, there should be isolation from each mutation. In the case of Redis, this is handled with the INCR instruction. In other cases, it should be done within a transaction or optimistically.

I guest the short answer is that increment should be serializable and consistent.

rylev · 2024-10-31T09:09:07Z

crates/factor-key-value/src/lib.rs

@@ -40,6 +41,7 @@ impl Factor for KeyValueFactor {
    fn init<T: Send + 'static>(&mut self, mut ctx: InitContext<T, Self>) -> anyhow::Result<()> {
        ctx.link_bindings(spin_world::v1::key_value::add_to_linker)?;
        ctx.link_bindings(spin_world::v2::key_value::add_to_linker)?;
+        ctx.link_bindings(spin_world::wasi::keyvalue::store::add_to_linker)?;


Just a note: We probably don't want to force all embedders of this factor to support wasi key_value. We can leave it as is for now, but we'll probably want to come up with some sort of feature system that allows embedders to choose which interfaces get linked in.

@rylev - would you mind adding an issue so we can track that if we don't already have one?

As Ryan says, this seems like a system-wide consideration and principle which we should approach from a system-wide perspective. For example it would apply equally to "embedders shouldn't be forced to support the v1 WITs." I agree with Michelle, let's capture this but let's not ad-hoc it into each PR.

I added the linker bindings for batch and atomic.

crates/factor-key-value/src/lib.rs

crates/factor-key-value/src/util.rs

rylev · 2024-10-31T09:17:21Z

crates/key-value-azure/src/store.rs

+            .await
+            .map_err(log_error)?;
+        let pair = self.get_pair(key.as_ref()).await?;
+        match pair {


Isn't there a race condition here? If after patching, another request either successfully deletes the key or changes it in some way, pair here will contain the wrong value. Is there no way to get the value back from the patch operation?

I'm going to dig a little deeper. It wasn't clear initially that patch supported that.

Looks like the REST API returns the original document and the Rust SDK throws it all away except the document attributes. (https://github.com/Azure/azure-sdk-for-rust/blob/e64d27c710ea3a9a55c9e044b388d0fc4acd0979/sdk/data_cosmos/src/operations/patch_document.rs#L150)

@itowlson I think this would be the only one of me comments where we might consider blocking the PR on. What are your thoughts on whether the race here is worth blocking the PR?

@rylev My preference is to merge as is. We can document it as a known issue in the RC, continue investigating between now and release, and assuming no joy, we can either:

Keep documenting it as a known issue in the Cosmos back end; or

Change it to always error with a "not supported on this back end" message

before release.

crates/key-value-azure/src/store.rs

crates/key-value-redis/src/store.rs

itowlson

I'm not sure what we can do about the race condition in Cosmos. The rest looks good to me but I'll leave this with you and Ryan to make a decision on the Cosmos woe. Thanks again for all your hard work on this!

endocrimes

Impls seem sound to me - I'd take the cosmos race as a future improvement if the rust sdk makes it difficult (because it's already a net improvement over our current interfaces)

Signed-off-by: David Justice <[email protected]>

devigned · 2024-11-02T00:43:38Z

There's a test failure, but I think it might be unrelated to the PR since it is outbound_mqtt. Not saying it wasn't me, just that it probably isn't me. If it was, I'll gladly fix it. Thank you so much for the reviews, folks. I really appreciate y'all.

itowlson · 2024-11-03T19:08:17Z

It's not you. MQTT is a known flake. Thanks for battling through this!

devigned marked this pull request as draft October 24, 2024 23:21

itowlson reviewed Oct 25, 2024

View reviewed changes

itowlson mentioned this pull request Oct 28, 2024

Implement AWS key value store #2883

Draft

devigned marked this pull request as ready for review October 31, 2024 02:02

itowlson and others added 5 commits October 30, 2024 22:11

Support wasi-keyvalue for key-value store access

1ed6467

Signed-off-by: itowlson <[email protected]>

update wasi keyvalue to draft2

470a264

Signed-off-by: David Justice <[email protected]>

implement kv factor for 0.2.0draft2 and stub impls

5ee121a

Signed-off-by: David Justice <[email protected]>

implement kv 0.2.0draft2 for sqlite

ba4f3e0

Signed-off-by: David Justice <[email protected]>

add tests for sqlite store

a0a132f

Signed-off-by: David Justice <[email protected]>

devigned force-pushed the wasi-kv branch from 7e436ef to d627003 Compare October 31, 2024 02:18

devigned requested a review from itowlson October 31, 2024 02:18

itowlson reviewed Oct 31, 2024

View reviewed changes

rylev reviewed Oct 31, 2024

View reviewed changes

devigned force-pushed the wasi-kv branch from d627003 to 49c96ca Compare October 31, 2024 22:35

devigned requested review from rylev and itowlson October 31, 2024 22:35

devigned force-pushed the wasi-kv branch from 49c96ca to ef45a96 Compare October 31, 2024 22:40

itowlson approved these changes Oct 31, 2024

View reviewed changes

endocrimes approved these changes Nov 1, 2024

View reviewed changes

devigned force-pushed the wasi-kv branch 10 times, most recently from 6ad74fc to 680e5ab Compare November 1, 2024 19:10

devigned force-pushed the wasi-kv branch 3 times, most recently from 79c85bf to 2fdcb0d Compare November 1, 2024 23:52

implement wasi kv 0.2.0-draft2 for redis and cosmosdb

50f22a1

Signed-off-by: David Justice <[email protected]>

devigned force-pushed the wasi-kv branch from 2fdcb0d to 50f22a1 Compare November 2, 2024 00:07

rylev approved these changes Nov 4, 2024

View reviewed changes

vdice merged commit d833301 into fermyon:main Nov 4, 2024
17 checks passed

This was referenced Nov 4, 2024

Proposal: implement the full wasi:keyvalue spec #2873

Closed

Review RDBMS data types e.g. should we add date/time types #2870

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WASI Key Value 0.2.0-draft2 support #2895

WASI Key Value 0.2.0-draft2 support #2895

devigned commented Oct 24, 2024

itowlson left a comment

itowlson Oct 25, 2024

devigned Oct 31, 2024

itowlson Oct 25, 2024

endocrimes Oct 31, 2024

devigned commented Oct 31, 2024

itowlson left a comment

itowlson Oct 31, 2024

itowlson Oct 31, 2024

rylev Oct 31, 2024

devigned Oct 31, 2024

itowlson Oct 31, 2024

rylev Oct 31, 2024

devigned Oct 31, 2024

rylev Oct 31, 2024

michelleN Oct 31, 2024

itowlson Oct 31, 2024

devigned Oct 31, 2024

rylev Oct 31, 2024

devigned Oct 31, 2024

itowlson Oct 31, 2024 •

edited

Loading

rylev Nov 1, 2024

itowlson Nov 3, 2024

itowlson left a comment

endocrimes left a comment

devigned commented Nov 2, 2024

itowlson commented Nov 3, 2024

WASI Key Value 0.2.0-draft2 support #2895

WASI Key Value 0.2.0-draft2 support #2895

Conversation

devigned commented Oct 24, 2024

itowlson left a comment

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

Choose a reason for hiding this comment

devigned commented Oct 31, 2024

itowlson left a comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itowlson Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itowlson left a comment

Choose a reason for hiding this comment

endocrimes left a comment

Choose a reason for hiding this comment

devigned commented Nov 2, 2024

itowlson commented Nov 3, 2024

itowlson Oct 31, 2024 •

edited

Loading