Add experimental `spin up --component` flag to run a subset of app components #2826

kate-goldenring · 2024-09-12T17:06:00Z

Say I have an app with 4 components but only want to run 2, I can do the equivalent of spin up --component-id "foo" --component-id "bar"

Approach: Modifies the locked app to remove undesired components and triggers before the locked app is loaded. Takes somewhat hacky approach of creating a temporary App struct to pull out information mapping components to their triggers.

src/commands/up.rs

itowlson

I think these are mostly nits or "is this the best way" kibbitzing - I do think it's worth adding a test case for templated hosts though. I love that you were able to bring this off so simply!

src/commands/up.rs

itowlson · 2024-09-13T00:17:01Z

src/commands/up.rs

+// Introspects the LockedApp to find and selectively retain the triggers that correspond to those components
+fn retain_components(locked_app: &mut LockedApp, components: &[String]) -> Result<()> {
+    // Create a temporary app to access parsed component and trigger information
+    let tmp_app = spin_app::App::new("tmp", locked_app.clone());


I feel like up should not have to create an App just to look at the triggers and components. It seems like this should be possible to get from the LockedApp but maybe not (I know the loose typing can introduce a bit of faff there...). Or maybe App and LockedApp have largely converged at this point - I see the factors work has removed the dependency from spin-app to spin-core which was always my concern in the past - maybe App is just a helpful wrapper around LockedApp now?

It is really hard to map components to triggers without parsing each trigger type, which the App type does for you. From talking to @lann, this seems like the best approach, but I agree that it feels hacky. I may have missed a recent change in factors that offers a different strategy

src/commands/up.rs

itowlson · 2024-09-13T00:30:02Z

src/commands/up.rs

+    for (c, _) in &component_triggers {
+        let allowed_hosts = allowed_hosts(c)?;
+        allowed_hosts.iter().try_for_each(|host| {
+            let uri = host.parse::<http::Uri>().unwrap();


I think you might have a problem here with templated URIs? I believe they're resolved during trigger startup, unless the factors work has changed that. Might merit a case in the unit tests.

We can't (currently) resolve templates here, so I guess there are two questions:

Will this code panic if it tries to resolve a template? (seems likely to me)

What should happen with templates here? I would probably suggest doing nothing. Using templates for service chaining seems like an uncommon scenario and the consequence here is pretty low: a runtime error instead of this nicer startup validation.

Yes, the current code will panic on an unsubstituted template.

I agree that ignoring the error is likely best. Service chaining via a templated URL is one of those hazy features. It happens to work in the current CLI implementation, and I don't believe we complain if someone does it, but there are no guarantees around it. So it seems reasonable for a failed URL parse to be interpreted as "it's not a service chaining URL" which means it is of no further interest.

If we wanted more belt and braces we could validate during trigger load (after template substitution) that all non-wildcard service chaining URLs pointed to components that exist in the app. Which might not be a bad plan anyway, and can be done outside the scope of this PR.

Good call out here. I agree that we should ignore URLs that cannot be parsed for now and +1 to validating non-wildcard service chaining URLs are to existing components.

src/commands/up.rs

crates/factor-outbound-networking/src/lib.rs

src/commands/up.rs

rylev · 2024-09-16T09:19:29Z

src/commands/up.rs

+        .triggers()
+        .filter_map(|t| match t.component() {
+            Ok(comp) => {
+                if components.contains(&comp.id().to_string()) {


Doesn't id() return &str making the to_string() unnecessary?

This one really baffled me. I get this error if i don't explicitly pass &String:

expected reference `&std::string::String` found reference `&str`

src/commands/up.rs

rylev · 2024-09-16T09:24:35Z

tests/integration.rs

-      no triggers in app
-"#;
-
+        let expected = "Error: No triggers in app\n";


This error might be a bit too terse. I'm not sure what a better wording would be though...

kate-goldenring · 2024-09-16T22:42:05Z

@itowlson I added a test to validate templated hosts are ignored/allowed

itowlson

Thanks for all the changes @kate-goldenring - this reads really clearly to me! I left some minor suggestions but LGTM

crates/factor-outbound-networking/src/lib.rs

src/commands/up.rs

kate-goldenring · 2024-09-16T23:52:46Z

@itowlson @rylev thoughts on moving retain_components to the spin-locked crate or spin-app crate? The shim will want to use it. It uses spin-locked types and functions from spin-app imports so regardless would want to export it from there. I could see it being a method on a LockedApp. This can also be in a follow up PR

kate-goldenring · 2024-09-16T23:54:49Z

@itowlson what are your thoughts on:

spin up --component "foo" --component "bar"

vs

spin up --components "foo,bar,bap"

I think i prefer the latter though it is more prone to parsing errors

itowlson · 2024-09-17T00:14:58Z

@kate-goldenring The comma separated form should be safe to parse, because component IDs can't contain tricksy characters, but even as I write that I hear my ghost gloating "famous last words." I do have reservations about it requiring knowledge (what is the separator), though, and we use the "multiple occurrences" form for most other things.

My suggestion would be to go with multiple occurrences for now, and see if it is used by human users (as opposed to deployment scripts) often enough for people to complain about the verbosity - if so we can add support for a CSV form as well - how about that?

itowlson · 2024-09-17T00:25:16Z

Re moving it to spin-app or spin-locked-app, it feels awkward dragging http and factor-outbound-networking dependencies into those crates. They feel like largely less-opinionated and more-opinionated schema crates right now. It kind of feels closer to a spin-loader behaviour, but the loader is specific to local now, so I'm not sure what a good home would be - maybe the awkwardness is the least worst option.

itowlson · 2024-09-17T02:09:26Z

@kate-goldenring I just remembered we do have precedent for this in spin build -c, which uses multiple occurrences, which reinforces my preference for sticking with that.

I did notice that the build flag is named -c/--component-id instead of --component - I wonder if we should settle on one preferred option, and alias for back compat if necessary. Although that in turn makes me unsure how we would parse spin build --up --component admin if we used the same flag name! What do you reckon?

src/commands/up.rs

michelleN · 2024-09-18T01:43:03Z

src/commands/up.rs

@@ -113,6 +114,11 @@ pub struct UpCommand {
    #[clap(long, takes_value = false, env = ALWAYS_BUILD_ENV)]
    pub build: bool,

+    /// Specific component to run. Can specify multiple. If omitted, all
+    /// components are run.
+    #[clap(hide = true, long = "component")]


Is hide=true here because this is experimental?

Yeah. I am not sure if there is another way we've marked features as experimental in the past. Should i update the comment to say it is experimental too?

yea not sure what the precedent is but I think a comment would be helpful.

radu-matei · 2024-09-18T06:51:03Z

+1 to @itowlson's comment about the spin build flag that controls the same behaviour being named --component-id.

radu-matei

Tested this with applications with several triggers and components and the behaviour LGTM!

Thanks!

kate-goldenring · 2024-09-18T17:19:42Z

I did notice that the build flag is named -c/--component-id instead of --component - I wonder if we should settle on one preferred option, and alias for back compat if necessary.

@itowlson thank you for noticing this. I think we should similarly name the flag --component-id and then we can always add the alias for component later.

Although that in turn makes me unsure how we would parse spin build --up --component admin if we used the same flag name! What do you reckon?

In that case, i think we should ideally apply the flag to both commands. I am not sure if that is possible though

kate-goldenring · 2024-09-18T17:50:53Z

@itowlson ff7feef commit brings in changes to enable running spin build --up --component-id foo which will cause only the foo component to be built and run. What do you think?

itowlson · 2024-09-18T20:57:01Z

@kate-goldenring I am honestly not sure what the right experience is with spin build --component-id foo --up. It feels like two separate concerns are colliding: minimising rebuilds, and running subsets. Looking back at when the feature was introduced (#1515) it seems the original consumer of build --component-id was spin doctor - so I don't really know if it's something people actually use manually. But if they are then they might be surprised that they now only get a subset of their application.

I guess whatever we choose will be surprising to some people, but they always have the get-out clause of spin build --whatever && spin up --whatever.

Perhaps for Spin 3 we should enable spin build --up -- --up-arg-1 --up-arg-2 and deprecate inlining the up-args into the build-args, with a view to phasing that out in Spin 4. That's out of scope for this PR for sure though!

Sorry for the long and indecisive ramble...!

michelleN · 2024-09-19T14:50:43Z

+1 to what @itowlson said about flags for spin 3.

kate-goldenring · 2024-09-19T18:51:37Z

@itowlson I can see that distinction. This is no longer passing component-id forward to up. I think where we are at now is a good place for experimental and enabling spin build --up -- --up-arg-1 --up-arg-2 will clarify this

kate-goldenring · 2024-09-19T18:52:19Z

@lann @rylev I think i have addressed your comments. This is ready for a final review

rylev

I mostly have nits (feel free to ignore them if you like), but I am unsure about whether the service chaining check is correct.

rylev · 2024-09-20T14:17:05Z

crates/factor-outbound-networking/src/lib.rs

@@ -1,8 +1,6 @@
 mod config;
 pub mod runtime_config;

-use std::{collections::HashMap, sync::Arc};


Nit: I don't think it's a hard rule, but we tend to keep std uses separate from external crates (which are both separate from uses local to the crate). It's not blocking, but I would consider reverting this.

I'm not sure i follow. Are you saying to not use these dependencies, not use them in the root of the crate or change the formatting somehow? Other factors use std libs so i am assuming that is not it: https://github.com/kate-goldenring/spin/blob/6d29d49d5b2d9aad5b13f4c95602a2c4a77b16e8/crates/factor-key-value/src/util.rs#L5-L6

Sorry - this is all about formatting of the use statements. Typically use statements are grouped into three groups:

std lib uses (e.g., std::fmt::Display)

external crate uses (e.g., tokio::task::spawn)

uses local to the crate (e.g., crate::foo)

These three groups are then separated by an empty line.

There are a million exceptions, and it's not really important, so I only bring it up since you moved this line for seemingly no other reason than aesthetics. Feel free to ignore my comment 😄

src/commands/up.rs

rylev · 2024-09-20T14:23:49Z

src/commands/up.rs

+            .await
+            .context("Failed to load application")?;
+        if !self.components.is_empty() {
+            retain_components(&mut locked_app, &self.components)?;


I would add some additional context to this function and some of the subfunctions with .context(). Right now an error from this function might simply read "failed to get allowed hosts" which is highly confusing without the context of the code.

src/commands/up.rs

rylev · 2024-09-20T14:27:46Z

src/commands/up.rs

+        if let Ok(component) = t.component() {
+            if retained_components.contains(&component.id().to_string()) {
+            let allowed_hosts = allowed_outbound_hosts(&component).context("failed to get allowed hosts")?;
+            allowed_hosts.iter().try_for_each(|host| {


Nit: for_each and try_for_each aren't super common in my experience. This code would be a bit more readable IMO if for in were used instead.

rylev · 2024-09-20T14:27:56Z

src/commands/up.rs

+            if retained_components.contains(&component.id().to_string()) {
+            let allowed_hosts = allowed_outbound_hosts(&component).context("failed to get allowed hosts")?;
+            allowed_hosts.iter().try_for_each(|host| {
+                // Templated URLs are not resolved at this point, so ignore unresolvable URIs


Does this mean we might allow some components that we shouldn't?

It means that we may spin up something that would later fail to execute. For example, say you have a spin app with 3 components, foo, bar and baz. You run spin up --component-id "foo" --component-id "baz" and foo is configured with allowed_outbound_hosts = [ "https://{{ myvar }}.spin.internal"]. We know we want to retain foo and baz but we also want to check to make sure neither do internal service chaining to bar so that we can catch that error and fail the run. However, we cannot parse that host, so we cannot whether it is service chaining and even if we could we couldn't determine what component this is referencing, so we continue.

No extra components are run but components may be run that cannot service chain. We discuss this a bit in this thread #2826 (comment)

Ah! Sorry I was reading the logic backwards. Sounds good to me!

One nit that might make this less confusing to readers in the future. We could add a comment outlining that we're doing a best effort lookup of components that are allowed to be accessed through service chaining, and we try to error early if a component tries to chain to another component that is not retained.

Good call out. I elaborated in the comment to make the best effort clearer.

src/commands/up.rs

rylev

I still have some suggestions for improvements, but I don't want to block merging this any more!

rylev · 2024-09-23T11:35:48Z

src/commands/up.rs

+            if retained_components.contains(&component.id().to_string()) {
+            let allowed_hosts = allowed_outbound_hosts(&component).context("failed to get allowed hosts")?;
+            allowed_hosts.iter().try_for_each(|host| {
+                // Templated URLs are not resolved at this point, so ignore unresolvable URIs


Ah! Sorry I was reading the logic backwards. Sounds good to me!

One nit that might make this less confusing to readers in the future. We could add a comment outlining that we're doing a best effort lookup of components that are allowed to be accessed through service chaining, and we try to error early if a component tries to chain to another component that is not retained.

rylev · 2024-09-23T11:44:25Z

src/commands/up.rs

+            let allowed_hosts = allowed_outbound_hosts(&component).context("failed to get allowed hosts")?;
+            allowed_hosts.iter().try_for_each(|host| {
+                // Templated URLs are not resolved at this point, so ignore unresolvable URIs
+                if let Ok(uri) = host.parse::<http::Uri>() {


Allowed host configs are very often not valid Uris but are still statically resolvable. For example, *://{foo.spin.internal, bar.spin.internal}. This check wouldn't run because the above cannot be parsed as an http::Uri.

You might want to consider AllowedHostConfig::parse instead. This handles all of the interpolation syntax that allowed host configs are allowed to have. You can then add a new method like AllowedHostConfig::service_chaining_target that returns a Vec<String> with all of the components that that AllowedHostConfig targets.

I didn't know you could chain to multiple targets in one host *://{foo.spin.internal, bar.spin.internal}. Should we update the docs on this: https://developer.fermyon.com/spin/v2/http-outbound#local-service-chaining?

It looks like we have two conditions we want to check for:

List of internal targets: *://{foo.spin.internal, bar.spin.internal}

Templated targets: http://{{ myvar }}.spin.internal

The former, we can parse with AllowedHostConfig::parse and get a "host lists are not supported error", but i don't understand why we would allow that syntax rather than requiring multiple entries. The latter, we cannot parse with AllowedHostConfig::parse, so we may want to add support to catch this case with specific error types for each. I am not sure what this adds at this point though? More specific errors we can surface to users?

I think i will merge this as is but I would like to continue to discuss this and potentially follow up on this in another PR

Signed-off-by: Kate Goldenring <[email protected]>

kate-goldenring requested a review from itowlson September 12, 2024 17:06

kate-goldenring commented Sep 12, 2024

View reviewed changes

src/commands/up.rs Outdated Show resolved Hide resolved

lann reviewed Sep 12, 2024

View reviewed changes

kate-goldenring force-pushed the component-filter-flag branch from c8dc850 to d8834c8 Compare September 12, 2024 18:08

kate-goldenring marked this pull request as ready for review September 12, 2024 18:28

kate-goldenring force-pushed the component-filter-flag branch from d8834c8 to 67af2d3 Compare September 12, 2024 19:49

itowlson reviewed Sep 13, 2024

View reviewed changes

rylev reviewed Sep 16, 2024

View reviewed changes

kate-goldenring force-pushed the component-filter-flag branch from ab2c037 to 67d9b77 Compare September 16, 2024 22:41

itowlson approved these changes Sep 16, 2024

View reviewed changes

crates/factor-outbound-networking/src/lib.rs Outdated Show resolved Hide resolved

src/commands/up.rs Outdated Show resolved Hide resolved

src/commands/up.rs Outdated Show resolved Hide resolved

michelleN reviewed Sep 18, 2024

View reviewed changes

radu-matei approved these changes Sep 18, 2024

View reviewed changes

kate-goldenring force-pushed the component-filter-flag branch from 67d9b77 to ff7feef Compare September 18, 2024 17:49

kate-goldenring force-pushed the component-filter-flag branch from ff7feef to 6d29d49 Compare September 19, 2024 18:48

michelleN approved these changes Sep 20, 2024

View reviewed changes

rylev requested changes Sep 20, 2024

View reviewed changes

rylev approved these changes Sep 23, 2024

View reviewed changes

Add spin up component flag to run a subset of app components

788dec1

Signed-off-by: Kate Goldenring <[email protected]>

kate-goldenring force-pushed the component-filter-flag branch from a666b18 to 788dec1 Compare September 23, 2024 16:49

kate-goldenring merged commit 11e0d32 into fermyon:main Sep 23, 2024
17 checks passed

kate-goldenring mentioned this pull request Sep 30, 2024

Component filtering spinkube/containerd-shim-spin#197

Draft

Add experimental spin up --component flag to run a subset of app components #2826

Add experimental spin up --component flag to run a subset of app components #2826

Conversation

kate-goldenring commented Sep 12, 2024 • edited Loading

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

lann Sep 13, 2024 • edited Loading

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

kate-goldenring commented Sep 16, 2024

itowlson left a comment

Choose a reason for hiding this comment

kate-goldenring commented Sep 16, 2024 • edited Loading

kate-goldenring commented Sep 16, 2024

itowlson commented Sep 17, 2024

itowlson commented Sep 17, 2024

itowlson commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radu-matei commented Sep 18, 2024

radu-matei left a comment

Choose a reason for hiding this comment

kate-goldenring commented Sep 18, 2024

kate-goldenring commented Sep 18, 2024

itowlson commented Sep 18, 2024

michelleN commented Sep 19, 2024

kate-goldenring commented Sep 19, 2024

kate-goldenring commented Sep 19, 2024

rylev 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

rylev 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

kate-goldenring Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Add experimental `spin up --component` flag to run a subset of app components #2826

Add experimental `spin up --component` flag to run a subset of app components #2826

kate-goldenring commented Sep 12, 2024 •

edited

Loading

lann Sep 13, 2024 •

edited

Loading

kate-goldenring commented Sep 16, 2024 •

edited

Loading

kate-goldenring Sep 23, 2024 •

edited

Loading