Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Component dependencies not working #238

Open
radu-matei opened this issue Nov 14, 2024 · 14 comments
Open

Component dependencies not working #238

radu-matei opened this issue Nov 14, 2024 · 14 comments
Labels
bug Something isn't working

Comments

@radu-matei
Copy link
Member

Trying to run an application that uses component dependencies -- for example https://github.com/radu-matei/spin-deps-image-manipulation.

Locally everything runs properly. However, when I run the app using SpinKube, I get the following error:

time="2024-11-14T17:27:05.248670334Z" level=error msg="run_wasi ERROR >>>  failed: component imports instance `component:image-manipulation-lib/image-manipulation`, but a matching implementation was not found in the linker

Caused by:
    0: instance export `grayscale` has the wrong type
    1: function implementation is missing"
@radu-matei radu-matei added the bug Something isn't working label Nov 14, 2024
@Mossaka
Copy link
Member

Mossaka commented Nov 14, 2024

Retried this locally and can confirm that this is not working

@kate-goldenring
Copy link
Collaborator

I believe this is because we do not check for component dependencies before precompiling. I'd be curious if packaging the app with docker instead of OCI makes it work since we do not precompile for Docker images.

@fibonacci1729
Copy link

fibonacci1729 commented Nov 21, 2024

Hey all, i'm the author of spin's component dependencies feature and i've been looking into implementing support for the shim.

The obvious place to hook in is at the point of precompiling. My initial thinking was that calling spin_compose::compose(...) would replace the call to componentize (since that happens internally during compose).

To compose, we would need to re-construct the LockedApp from the config layer and perform the composition (and subsequent precompilation) for each component in the order dictated by the order of the layers (to maintain the ordering invariant of the precompile method. I.e.:

Runwasi expects layers to be returned in the same order, so wrap each layer in an option, setting non Wasm layers to None

However, each wasm layer is not 1:1 with a component in the LockedApp. Furthermore, each tuple of (spin-component, wasm-layer) is not 1:1 with a precompiled wasm. This is because, each spin component specifies a bit of configuration to declare how configuration should be inherited (i.e. dependencies_inherit_configuration). The result of this bit instructs the composer whether or not to link in a "deny" adapter to handle the virtualization.

What this means is that for each wasm layer, which may relate to 1-or-more spin components, may yield one of 2 different precompiled wasms, and so we can't assume that len(wasm-layers) == len(precompiled-layers).

I haven't dug into the upstream shim crate (where Engine is defined) to understand the flexibility of this invariant. I thought i'd first brain dump here to see if someone more familiar with the shim can help guide my understanding.

Please let me know if what i said isn't clear or if i'm misunderstanding the invariant!

@jsturtevant
Copy link
Contributor

We could potentially change this. At one-point during the initial designs we didn't have this restraint. The tricky part is communicating back which components are tied to which layer, so we can look them up and send them along properly.

I put some thoughts in containerd/runwasi#504 a while back

@Mossaka
Copy link
Member

Mossaka commented Nov 26, 2024

As far as I know, the len(wasm-layers) == len(precompiled-layers) condition could be relaxed. As @jsturtevant we should be able to map layer to components even if they are not a 1-1 mapping. Would you like to take a look at the upstream containerd-shim-wasm crate to resolve this issue? @fibonacci1729

@jsturtevant
Copy link
Contributor

I was thinking about this a bit more. I have a few questions to help me understand the use case a bit more the Kubernetes env. (From a dev, perspective this is a really cool feature!)

  • In the case of the shim, do we want the components deps being dynamically fetched and linked?
  • Would this cause any type of perf impact on deployments (i.e impact start up times since we are fetching something after the pod is already "running")?
  • How is the dynamic linking controlled? Is this statically built into the OCI package? Would it require a new package to be published to change the component that is linked?

@fibonacci1729
Copy link

Hey @jsturtevant, thanks for taking a look!

  1. When you spin registry push an application, at the moment the dependencies are bundled in as part of the artifact. So at the point of processing the layers in the shim we already have everything we need for static composition.
  2. There's nothing dynamically being fetched so no performance impact. The components in the spin.toml are AOT composed before the trigger is started (just before precompilation).
  3. Yes, to swap in a different dependency you would need to publish a new app artifact. Currently, this is mostly dictated by the fact that we bundle deps into the oci image.

@fibonacci1729
Copy link

fibonacci1729 commented Dec 9, 2024

Capturing the outcome of discussions with @jsturtevant and @Mossaka late last week on a potential solution for support component dependencies. Effectively what needs to be done is to relax the assumption that the input layers to precompile are 1:1 mapped to the output layers. When composing it's typically the case that new layers are created that are composed of one or more of the original layers. This suggests that, in addition to returning the processed layers, implementers of this processing step will need to return a mapping that describes how the original layers are related to new layers (so that containerd can attach the appropriate labels to maintain gc links).

To do this, a rough proposal sketch subject-to-change (inspired by James' original proposal linked above) might look like this:

struct ProcessedLayer {
     layer: Vec<u8>,  // Bytes of processed layer
     mapping: IndexSet<usize>,  // Indices in to original layers using to produced this new layer
}

enum Layer {
    Original(digest), // Untouched
    Processed(ProcessedLayer),
}

fn process_layers(&self, _layers: &[WasmLayer]) -> Result<Vec<Layer>> {
    // do work
}

On return, the shim would use the mappings of each processed layer to attach gc labels to the original layers used to construct it. For untouched layers, we could associate it via label to the original layer using the index as its done today.

@Mossaka
Copy link
Member

Mossaka commented Dec 10, 2024

Something to consider:

I wonder if the full generalized version of the layers is a directed acyclic graph (DAG). In this graph, we have nodes and edges where edges are pointers to the original nodes. In DAG, we could enable multiple processed layers referring to one original layer or one processed layer referring to multiple original layers. We can do a topological ordering of the DAG to traverse through the graph.

struct LayerGraph {
    layers: vec<Layer>, // ID to layer
    dependencies: vec<BTreeSet<usize>>, // Adjacency list of edges
}

struct Layer {
    data: Option<Vec<u8>>,
}


impl LayerGraph {
  fn topological_order(&self) -> Result<Vec<usize>> {
    ...
  }
}

@fibonacci1729
Copy link

Great call out @Mossaka, I think you're right that the full generalized version of this is best represented as a DAG.

@fibonacci1729
Copy link

fibonacci1729 commented Dec 11, 2024

I'm making good progress on evolving the precompile api given the conversation here; but what i'm finding difficult to reconcile given the proposed design is how to support "layer rewrites" along side "a synthesized layer". The former would apply to rewrites of a layer (e.g. mutating a spin.toml given information available during processing) and the latter would handle the case where new layers are synthesized as would be the case for composition / precompilation.

As a strawman, perhaps the api here should evolve to return something like:

struct ProcessedLayers {
     // 1:1 mapping with original (None if original should be used)
     rewritten: Vec<Option<Vec<u8>>>,
     // Layers created during precompile/processing
     created: Vec<NewLayer>,
}

struct NewLayer {
    // Needed to communicate back to the shim for creating a new `oci::image::Descriptor`
    media_type: String,
    // Bytes of the layer
    bytes: Vec<u8>,
    // Indices of original layers used during processing
    dependencies: BTreeSet<usize>,
}

Currently in the shim when a layer is rewritten the descriptor from the original layer is used to construct a WasmLayer to return from load_modules; to evolve this, can i simply use oci::image::Descriptor::new(...) to construct a WasmLayer for the NewLayers returned?

@jsturtevant
Copy link
Contributor

jsturtevant commented Dec 12, 2024

Currently in the shim when a layer is rewritten the descriptor from the original layer is used to construct a WasmLayer to return from load_modules; to evolve this, can i simply use oci::image::Descriptor::new(...) to construct a WasmLayer for the NewLayers returned?

From a runwasi/containerd perspective, I don't think this should be an issue. In spin the "pre-compiled" bytes are written to the cached location using the original digest id so this might cause an issue. Maybe there is a way to track this on spin side?

The layer original digest id is used here but the precompiled bytes are written:

cache
.write_wasm(&artifact.layer, &artifact.config.digest())
.await?;

but what i'm finding difficult to reconcile given the proposed design is how to support "layer rewrites" along side "a synthesized layer". The former would apply to rewrites of a layer (e.g. mutating a spin.toml given information available during processing)

Is the layer different from the original here? I.e. If you want a different layer returned instead of the original, could that be considered "New layer"?

@fibonacci1729
Copy link

fibonacci1729 commented Dec 17, 2024

Is the layer different from the original here? I.e. If you want a different layer returned instead of the original, could that be considered "New layer"?

I think in cases such as wanting to rewrite the spin.toml we would want to re-use the digest for the original because it is used in other oci layers to say the locked app has this digest. This is similar to how what you pointed to above works; where the precompiled bytes are written but original digest id is used. It seems less than ideal to effectively rewrite all the layers with updated digests so everything is consistent but maybe that is the more robust way to go?

@fibonacci1729
Copy link

fibonacci1729 commented Dec 18, 2024

I think we cracked it, I will open WIP PRs in the coming days for 👀 before moving onto polish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants