Add `spin up --key-value` option #1442

itowlson · 2023-05-02T00:57:31Z

Note this currently assumes the answer to my questions there is "default store suffices." The core logic is easy enough to extend to support non-default stores; the difficulty is only in the CLI syntax.

This PR contains (in key-value/src/host_component.rs) the greatest Rust crime I have ever committed. @dicej it would be great to get your thoughts on the right way to do this. The trouble I kept running into was that StoreManagerManager would only dispense a Store if you gave it an AppComponent. My guess is I need to add a get_for_app method to StoreManagerManager but then we will not longer be able to implement it via a function...

Also @lann does the proposed entry point for carrying out before-run host component initialisation seem reasonable? I was wondering about using builder.hooks to set an app_loaded hook. The place where hooks are currently set didn't seem to have access to the host component itself, but would it be reasonable to set a hook where we instantiate and add the key/value DynamicHostComponent (TriggerExecutorBuilder::build)?

Or are these questions two sides of the same coin, and if I set the right hook in the right place then I wouldn't need to go through StoreManagerManager at all?

itowlson · 2023-05-02T01:48:27Z

Before I added the is_empty check, this made a couple of tests (spin-trigger-http tests::test_spin_httptests::test_{spin|wagi}_http and spin-redis-engine tests::test_pubsub) fail. The problem was that the SQLite provider's StoreManager::get(name) calls tokio::task::block_in_place, which is forbidden in the Tokio test runtime. This occurs because they instantiate and invoke trigger objects directly in the unit tests, not hosted in main (which uses the multi-threaded runtime and is perfectly happy). This makes me worry that there is some hidden brittleness here, but I am not sure if it is in the way the initialisation code uses the StoreManager, or in the way the tests load the trigger.

dicej · 2023-05-02T01:55:05Z

FYI, you can annotate Tokio tests using #[tokio::test(flavor = "multi_thread")], which should allow task::block_in_place to work.

itowlson · 2023-05-03T01:49:00Z

This has been manually tested, but doesn't have automated tests - I'm not sure how best to go about those.

dicej

LGTM. Regarding testing: perhaps we could add a variation on the key_value_works test in tests/spinup_tests.rs, tests/testcases/mod.rs, and tests/testcases/key-value.

itowlson · 2023-05-04T00:20:58Z

#1448 is thwarting my ability to implement the suggested test. I am thinking I will merge this as-is and open a new issue to get it under e2e test coverage.

lann · 2023-05-04T13:34:12Z

crates/key-value/src/host_component.rs

+    fn get_for_init(&self) -> Option<Arc<dyn StoreManager>>; // Some stores might not support init
+}
+
+pub struct SingletonStoreManagerManager {


lann · 2023-05-04T13:44:08Z

crates/trigger/src/lib.rs

+                        runtime_config::key_value::build_key_value_component(&runtime_config)?,
+                        init_data.kv,


Unless I'm missing something you should be able to pass this init data into the function that creates the component here:

Suggested change

runtime_config::key_value::build_key_value_component(&runtime_config)?,

init_data.kv,

runtime_config::key_value::build_key_value_component(&runtime_config, init_data.kv)?,

and then in build_key_value_component do something like:

pub fn build_key_value_component(runtime_config: &RuntimeConfig, initial_values: Vec<(String, String)>) -> Result<KeyValueComponent> { let mut stores = runtime_config .key_value_stores() .context("Failed to build key-value component")? .collect::<HashMap<_, _>>(); if !initial_values.is_empty() { if let Some(manager) = stores.get("default") { let store = manager.get("default").unwrap(); for (key, value) in initial_values { store.set(&key, value.as_bytes())?; } } else { // bail? log warning? } } ...

I was kind of thinking it would be nice to have the initialisation login in the component itself rather than as a side effect in a function in the runtime_config module. But maybe I should get over myself!

https://github.com/fermyon/spin/pull/1442/files/09bedc013db837176e3b63631b5e21f6a1688524#r1185541401

lann

I don't think the DynamicHostComponent::initialize method is necessary.

lann · 2023-05-04T21:35:54Z

crates/trigger/src/lib.rs

+                self.loader
+                    .add_dynamic_host_component(
+                        &mut builder,
+                        runtime_config::key_value::build_key_value_component(&runtime_config)?,
+                        init_data.kv,


I was kind of thinking it would be nice to have the initialisation logic in the component itself

Sure!

Suggested change

self.loader

.add_dynamic_host_component(

&mut builder,

runtime_config::key_value::build_key_value_component(&runtime_config)?,

init_data.kv,

let component = runtime_config::key_value::build_key_value_component(&runtime_config)?;

component.set_initial_values(init_data.kv)?;

self.loader

.add_dynamic_host_component(

&mut builder,

component

impl KeyValueComponent { pub fn set_intitial_values ... }

...or some variation thereof... maybe the set_initial_values call would be better off inside build_key_value_component 🤷

lann · 2023-05-04T21:52:21Z

crates/app/src/lib.rs

@@ -79,7 +79,7 @@ impl AppLoader {
    ///
    /// This calls [`EngineBuilder::add_host_component`] for you; it should not
    /// be called separately.
-    pub fn add_dynamic_host_component<T: Send + Sync, DHC: DynamicHostComponent>(
+    pub async fn add_dynamic_host_component<T: Send + Sync, DHC: DynamicHostComponent>(


I think this can be removed now?

Aargh, good spot, thanks!

lann · 2023-05-04T21:53:07Z

crates/trigger/src/cli.rs

@@ -243,6 +254,16 @@ async fn warn_slow_wasm_build() {
    println!();
 }

+// Parse the key/values passed in as `key=value` pairs.
+// TODO: `spin-utils` WHEN


lann · 2023-05-04T21:55:46Z

crates/trigger/src/cli.rs

+    let parts: Vec<_> = s.splitn(2, '=').collect();
+    if parts.len() != 2 {
+        bail!("Key/Values must be of the form `key=value`");
+    }
+    Ok((parts[0].to_owned(), parts[1].to_owned()))


Slightly nicer:

Suggested change

let parts: Vec<_> = s.splitn(2, '=').collect();

if parts.len() != 2 {

bail!("Key/Values must be of the form `key=value`");

}

Ok((parts[0].to_owned(), parts[1].to_owned()))

let Some((key, value)) = s.split_once('=') else {

bail!("Key/Values must be of the form `key=value`");

}

Ok((key.to_owned(), value.to_owned()))

Not only nicer, but more correct (if a value contains an = sign e.g. base64). But I wanted to keep it exactly the same as the deploy one (and didn't want to change deploy) so that when spin-utils came along we wouldn't need to eyeball them for subtle differences.

But yeah now I can (rebase and) move to common.

I think splitn(2, ...) makes it equivalent. (I had the exact same thought on first look)

And you are, of course, correct. Can I start today again please, this time with more tea...

move to common

Ooo yeah we're probably duplicating a few of these arg parsers.

How similar this is to the environment variable parser will shock you.

lann · 2023-05-04T21:57:12Z

crates/app/src/host_component.rs

@@ -8,6 +8,7 @@ use crate::AppComponent;
 ///
 /// This extends [`HostComponent`] to support per-[`AppComponent`] dynamic
 /// runtime configuration.
+#[async_trait::async_trait]


lann · 2023-05-04T21:57:28Z

crates/key-value/src/host_component.rs

@@ -54,6 +54,7 @@ impl HostComponent for KeyValueComponent {
    }
 }

+#[async_trait]


Signed-off-by: itowlson <ivan.towlson@fermyon.com>

Mossaka · 2023-05-18T16:54:55Z

I saw there is a comment in HostComponentInitData saying that this is only for testing purposes, are you planning to remove it @itowlson ?

The new v1.2.0 release of spin has this struct as a argument to builder.build().

itowlson · 2023-05-18T22:27:50Z

@Mossaka HostComponentInitData is for real, but the #[derive(Default)] macro is only there to support the testing infrastructure. There's no meaningful default for HostComponentInitData, it has to be determined by the CLI; but there's a bunch of testing infra that relies on being able to default things. I'll clarify the comment - thanks for flagging this.

itowlson added the do-not-merge label May 2, 2023

itowlson requested review from lann and dicej May 2, 2023 00:57

itowlson force-pushed the kv-local-init branch from 3e82b2d to 8f206ce Compare May 2, 2023 01:34

itowlson marked this pull request as ready for review May 3, 2023 01:44

itowlson removed the do-not-merge label May 3, 2023

itowlson force-pushed the kv-local-init branch from b104071 to a810efe Compare May 3, 2023 01:45

itowlson force-pushed the kv-local-init branch from a810efe to 09bedc0 Compare May 3, 2023 02:36

dicej approved these changes May 3, 2023

View reviewed changes

lann reviewed May 4, 2023

View reviewed changes

lann requested changes May 4, 2023

View reviewed changes

lann reviewed May 4, 2023

View reviewed changes

itowlson force-pushed the kv-local-init branch from 09bedc0 to 6b0340d Compare May 4, 2023 21:51

itowlson requested a review from lann May 4, 2023 21:51

lann reviewed May 4, 2023

View reviewed changes

Add spin up --key-value option

5739969

Signed-off-by: itowlson <ivan.towlson@fermyon.com>

itowlson force-pushed the kv-local-init branch from 6b0340d to 5739969 Compare May 4, 2023 22:40

itowlson requested a review from lann May 4, 2023 22:45

lann approved these changes May 4, 2023

View reviewed changes

itowlson merged commit ab801b0 into fermyon:main May 4, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `spin up --key-value` option #1442

Add `spin up --key-value` option #1442

itowlson commented May 2, 2023

itowlson commented May 2, 2023

dicej commented May 2, 2023

itowlson commented May 3, 2023

dicej left a comment

itowlson commented May 4, 2023

lann May 4, 2023

lann May 4, 2023

itowlson May 4, 2023

lann May 4, 2023

lann left a comment

lann May 4, 2023 •

edited

lann May 4, 2023

lann May 4, 2023

itowlson May 4, 2023

lann May 4, 2023

lann May 4, 2023

itowlson May 4, 2023

lann May 4, 2023 •

edited

itowlson May 4, 2023

lann May 4, 2023

itowlson May 4, 2023

lann May 4, 2023

lann May 4, 2023

Mossaka commented May 18, 2023

itowlson commented May 18, 2023

		runtime_config::key_value::build_key_value_component(&runtime_config)?,
		init_data.kv,

Add spin up --key-value option #1442

Add spin up --key-value option #1442

Conversation

itowlson commented May 2, 2023

itowlson commented May 2, 2023

dicej commented May 2, 2023

itowlson commented May 3, 2023

dicej left a comment

Choose a reason for hiding this comment

itowlson commented May 4, 2023

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

lann left a comment

Choose a reason for hiding this comment

lann May 4, 2023 • edited

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

lann May 4, 2023 • edited

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

Mossaka commented May 18, 2023

itowlson commented May 18, 2023

Add `spin up --key-value` option #1442

Add `spin up --key-value` option #1442

lann May 4, 2023 •

edited

lann May 4, 2023 •

edited