Validate target environments #2806

itowlson · 2024-09-06T00:05:25Z

~~EXTREMELY WIP.~~

There are plenty of outstanding questions here (e.g. ~~environment definition,~~ Redis trigger, ~~hybrid componentisation~~) ~~, and reintegration work (e.g. composing dependencies before validating)~~ (ETA: component dependencies now work). ~~In its current state, the PR is more for visibility than out of any sense of readiness. Also, it has not had any kind of tidying pass, so a lot of naming is not great.~~

But it does work, sorta kinda, some of the time. So we progress.

cc @tschneidereit

itowlson · 2024-09-17T03:16:04Z

Checking environments adds an appreciable bump to the time to run spin build. I looked into caching the environments but a robust way didn't really help - the bulk of the time, in my unscientific test, was in the initial registry request, which retrieval of the Wasm bytes (for the Spin environment) about a quarter of the time. (In my debug build, from a land far away from Fermyon Cloud and GHCR where my test registry is hosted: approx 2500ms to get the digest, then 600ms to get the bytes. The cache eliminated only the 600ms.)

We could, of course, assume that environments are immutable, and key the cache by package reference instead of digest. But that would certainly be an assumption and not guaranteed to be true.

itowlson · 2024-09-18T23:56:04Z

This is now sort of a thing and can possibly be looked at.

Outstanding questions:

Where shall we publish the initial set of environments?
- The wkg configuration will need to reflect this. At the moment it uses the user's default registry. Yeah nah.
Where shall we maintain the environment WITs?
- Separate repo in the Fermyon org?
What does that initial set contain?
- Currently I've created a Spin CLI "2.5" world with just the HTTP trigger (WASI 0.2 and WASI RC).
Testing

Possibly longer term questions:

How can we manage environments where the set of triggers is variable - specifically the CLI with trigger plugins?
How to avoid a lengthy network round-trip on every build
Better error reporting for environments where a trigger supports multiple worlds (like, y'know, the Spin CLI).

If folks want to play with this, add the following to your favourite spin.toml:

[application]
targets = ["spin:[email protected]"]

and set your wkg config (~/.config/wasm-pkg/config.toml) to:

default_registry = "registrytest-vfztdiyy.fermyon.app"

[registry."registrytest-vfztdiyy.fermyon.app"]
type = "oci"

(No, that is not the cat walking across the keyboard... this is my test registry which backs onto my ghcr.)

lann · 2024-09-19T13:15:52Z

Where shall we publish the initial set of environments?

fermyon.com?

Where shall we maintain the environment WITs?

I'd suggest "next to the code that implements them"; ideally generated by that code.

crates/build/src/manifest.rs

rylev · 2024-09-19T14:37:33Z

crates/build/src/manifest.rs

+            let dt = deployment_targets_from_manifest(&manifest);
+            Ok((bc, dt, Ok(manifest)))
+        }
+        Err(e) => {


It's not obvious to me what's happening here. It reads like we're trying to get build and deployment configs even when we can't parse the manifest? I think some terse one-line comments on each branch of this match would go a long way to helping this be a bit more understandable.

Reading the code more, I'm unsure why we go through all these great lengths to read the targets config here when later on we only seem to run the targets check if the manifest was successfully parsed. Won't this information just be thrown away?

@rylev You are absolutely right - this was a holdover from an earlier iteration before I realised I was going to need to full manifest - thanks for catching it. I've pared this back to "has deployment targets", which I think is worth keeping so we can warn if the manifest errors have caused us to bypass checking.

rylev · 2024-09-19T14:39:39Z

crates/build/src/manifest.rs

+            Ok((bc, dt, Ok(manifest)))
+        }
+        Err(e) => {
+            let bc = fallback_load_build_configs(&manifest_file).await?;


It seems like there's some need for better error messages here through liberal use of .context. As the code stands now, component_build_configs function might return an error saying only "expected table found some other type" which would be very confusing.

On reflection these should preserve (and immediately return) the original manifest load error rather than blatting it with whatever went awry during the fallback attempt.

rylev · 2024-09-19T14:40:20Z

crates/build/src/manifest.rs

+            let table: toml::value::Table = toml::from_str(&manifest_text)?;
+            let target_environments = table
+                .get("application")
+                .and_then(|a| a.as_table())
+                .and_then(|t| t.get("targets"))
+                .and_then(|arr| arr.as_array())
+                .map(|v| v.as_slice())
+                .unwrap_or_default()
+                .iter()
+                .filter_map(|t| t.as_str())
+                .map(|s| s.to_owned())
+                .collect();


Would serializing to a type through serde::Deserialize making this easier to read?

rylev · 2024-09-19T14:42:00Z

crates/build/src/manifest.rs

@@ -57,6 +75,30 @@ async fn fallback_load_build_configs(
    })
 }

+async fn fallback_load_deployment_targets(


Nit: is "deployment targets" the right nomenclature? That sounds like what you would be targeting for spin deploy and not the environment you're targeting. Perhaps we could play with the wording here?

@rylev I'm very open to naming on "deployment targets." The current name emerged in a woolly manner from a sense of "these are possible targets for deployment" or "the target environments we want to be able to deploy to." I do feel, albeit not strongly, that it's worth specifying 'deployment' (cf. e.g. 'compilation target') - but for sure let's improve this!

"runtime targets"?

crates/build/src/manifest.rs

rylev · 2024-09-19T15:05:50Z

crates/compose/src/lib.rs

+    async fn load_component_source(&self, source: &Self::Component) -> anyhow::Result<Vec<u8>>;
+    async fn load_dependency_source(&self, source: &Self::Dependency) -> anyhow::Result<Vec<u8>>;


Are we implementing ComponentSourceLoader more than once? I'm a bit lost why we need the flexibility of defining the type of dependency and component instead of hard coding. Can you explain what this buys us?

The current build system doesn't create a lockfile. The current composition implementation depends on the locked types (because it runs at trigger time). This allows us to implement composition on the raw AppManifest as well as on the LockedApp.

rylev · 2024-09-19T15:10:22Z

crates/environments/src/lib.rs

+    "#
+    );
+
+    let doc = wac_parser::Document::parse(&wac_text)


We really should implement a programatic API in wac for this so that we don't need to manipulate a wac script.

Yeah! I tried constructing a wac document AST, which seemed cleaner than parsing a document, but I foundered on - if memory serves - something as trivial as the miette spans. I tried it with dummy spans but something internal seems to have been trying to use the length of the name in error or something and anyway there was anguish. Using the underlying model would have been awesome, but was (for me) hard to understand and to line up inputs for.

crates/environments/src/lib.rs

rylev · 2024-09-19T15:15:22Z

crates/environments/src/loader.rs

+}
+
+pub async fn load_and_resolve_all<'a>(
+    app: &'a spin_manifest::schema::v2::AppManifest,


Nit: seems like we pass app and resolution_context around a lot. It might be nicer to put those into a Resolver struct and implement these functions as methods on that struct.

I had an explore of this, and have pushed something like it as a separate commit for easy reversion. I ended up naming the proposed struct ApplicationToValidate because otherwise I ended up with no "application" visible in a module that was ostensibly all about validating applications. I still have mixed feelings but there's definitely some nice aspects to encapsulating the component loading stuff - I was able to tuck some more clutter away and split things up in a way that's hopefully more readable...

tschneidereit · 2024-09-20T17:15:11Z

We could, of course, assume that environments are immutable, and key the cache by package reference instead of digest. But that would certainly be an assumption and not guaranteed to be true.

@itowlson I think that's an okay assumption to make, personally. Perhaps paired with a way to flush the local cache explicitly?

tschneidereit · 2024-09-20T17:18:49Z

We could also consider introducing something like target-environments.lock which would be stored next to Spin.toml, and which would contain the environments. Then that explicit way to flush the cache would become "remove the file".

itowlson · 2024-09-24T03:47:28Z

Okay I have pencilled in fermyon.com as the default registry for environment definitions (with !NEW! the option to override it or use a local path !NEW!). One thing that occurred to me though was that, at the moment, an environment reference is a package name. Are we okay with environments occupying the same namespace as worlds and components? Or should we adopt an implicit prefix e.g. instead of spin:[email protected] have users write [email protected] and translate this to e.g. environment:[email protected]? I'm not sure I have an opinion, but hopefully someone else does...

tschneidereit · 2024-09-24T09:49:06Z

One thing that occurred to me though was that, at the moment, an environment reference is a package name.

Aren't environments also just packages though? Ones that we apply some additional semantics to, yes, but not ones that are someone non-standard.

Which actually raises the question: could we use the exact packages to generate bindings for Spin and the SDKs as well? (Not necessarily as part of this effort, but as a future thing.)

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

itowlson · 2024-10-04T01:21:19Z

Implemented caching on the basis of immutability as per Till's comment #2806 (comment) (and double checked on Windows).

I'll have a think about the lockfile idea. A lockfile mapping environment IDs to digests would allow us to reuse the existing Wasm by-digest caching infra instead of having something custom for name-based lookup. If we generated it into .spin then it would get picked up by .gitignore which is probably desirable, although it would be less discoverable for deletion purposes (.spin already contains a bunch of other stuff of varying deletability).

ETA: I implemented Till's lockfile-based cache strategy. It does make some things cleaner for sure. I've done it as a separate commit for now, but will squash once we agree on it.

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

itowlson added the do-not-merge label Sep 6, 2024

itowlson force-pushed the validate-target-environment branch 6 times, most recently from 30aa1fc to 3b25dad Compare September 9, 2024 22:10

itowlson force-pushed the validate-target-environment branch 2 times, most recently from e74895b to 828cb70 Compare September 17, 2024 00:57

itowlson marked this pull request as ready for review September 18, 2024 23:56

rylev reviewed Sep 19, 2024

View reviewed changes

itowlson force-pushed the validate-target-environment branch from 4870bba to 531e4fb Compare September 24, 2024 00:27

itowlson added 9 commits September 25, 2024 09:00

Validate against target environments during build

42f9d82

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

Load and compose components once at start of validation

3baee56

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

WIT-based environment syntax

fd45f2f

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

Less meaningless error messages

cadbf91

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

Enable skipping target validation for offline or to bypass delays

cd1c3cc

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

Report all target validation fails not just the first

897e4dd

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

Updated after initial review

bc2cc17

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

Encapsulate the application and loader

68fbd1a

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

Load target env from other registry or local dir

76cb8af

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

itowlson force-pushed the validate-target-environment branch from 51a09f4 to 76cb8af Compare September 24, 2024 21:08

Cache environment definitions

2a3a213

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

Use Till's lockfile

6d068a4

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate target environments #2806

Validate target environments #2806

itowlson commented Sep 6, 2024 •

edited

Loading

itowlson commented Sep 17, 2024

itowlson commented Sep 18, 2024

lann commented Sep 19, 2024

rylev Sep 19, 2024

rylev Sep 19, 2024

itowlson Sep 23, 2024

rylev Sep 19, 2024

itowlson Sep 22, 2024

rylev Sep 19, 2024

rylev Sep 19, 2024

itowlson Sep 22, 2024

lann Sep 23, 2024

rylev Sep 19, 2024

itowlson Sep 23, 2024

rylev Sep 19, 2024

itowlson Sep 23, 2024

rylev Sep 19, 2024

itowlson Sep 23, 2024

tschneidereit commented Sep 20, 2024

tschneidereit commented Sep 20, 2024

itowlson commented Sep 24, 2024

tschneidereit commented Sep 24, 2024

itowlson commented Oct 4, 2024 •

edited

Loading

		async fn load_component_source(&self, source: &Self::Component) -> anyhow::Result<Vec<u8>>;
		async fn load_dependency_source(&self, source: &Self::Dependency) -> anyhow::Result<Vec<u8>>;

Validate target environments #2806

Are you sure you want to change the base?

Validate target environments #2806

Conversation

itowlson commented Sep 6, 2024 • edited Loading

itowlson commented Sep 17, 2024

itowlson commented Sep 18, 2024

lann commented Sep 19, 2024

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

Choose a reason for hiding this comment

tschneidereit commented Sep 20, 2024

tschneidereit commented Sep 20, 2024

itowlson commented Sep 24, 2024

tschneidereit commented Sep 24, 2024

itowlson commented Oct 4, 2024 • edited Loading

itowlson commented Sep 6, 2024 •

edited

Loading

itowlson commented Oct 4, 2024 •

edited

Loading