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

Add ability to generate new artifact type #631

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

jsturtevant
Copy link
Contributor

@jsturtevant jsturtevant commented Jun 25, 2024

The CNCF wg-wasm has published an OCI artifact format for packaging wasm modules and components. The artifact can be produced locally by running the --as-artifact flag:

cargo run --bin oci-tar-builder -- --name wasi-demo-oci --repo ghcr.io/containerd/runwasi --tag latest --as-artifact --module ./target/wasm32-wasi/debug/wasi-demo-app.wasm -o target/wasm32-wasi/debug/img-oci-artifact.tar
regctl image import localhost:5000/wasi-artifact:latest target/wasm32-wasi/debug/img-oci-artifact.tar
regctl manifest get localhost:5000/wasi-artifact:latest

Name:                                localhost:5000/wasi-artifact:latest
MediaType:                           application/vnd.oci.image.manifest.v1+json
Digest:                              sha256:7c31e635b3bef8b6c727a316e9a2dae777dbd184318d66a97da040fb11e37d70
Annotations:
  io.containerd.image.name:          ghcr.io/containerd/runwasi/wasi-demo-oci:latest
  org.opencontainers.image.ref.name: latest
Total Size:                          2.006MB

Config:
  Digest:                            sha256:24f30be41b447bbaf3644dad1e1c23dd28b597f36a5f455399657d65945816ea
  MediaType:                         application/vnd.wasm.config.v0+json
  Size:                              235B

Layers:

  Digest:                            sha256:0db51ed1c94837f422b2259c473758f298eef69605eaae6195bc043e25971e94
  MediaType:                         application/wasm
  Size:                              2.006MB

Note: this does not add support to runwasi to run these yet. Some work in Containerd is required: containerd/containerd#10179

@jsturtevant jsturtevant force-pushed the oci-artifact branch 2 times, most recently from 9836cd1 to 3085cc0 Compare June 28, 2024 21:22
Mossaka
Mossaka previously requested changes Jul 2, 2024
Copy link
Member

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Added some refactoring suggestions

Makefile Outdated Show resolved Hide resolved
crates/oci-tar-builder/README.md Show resolved Hide resolved
crates/oci-tar-builder/README.md Show resolved Hide resolved
crates/oci-tar-builder/src/bin.rs Outdated Show resolved Hide resolved
crates/oci-tar-builder/src/bin.rs Outdated Show resolved Hide resolved
Comment on lines +22 to +27
pub trait OciConfig {
fn os(&self) -> String;
fn architecture(&self) -> String;
fn layers(&self) -> Vec<String>;
fn to_string(&self) -> String;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion:

Suggested change
pub trait OciConfig {
fn os(&self) -> String;
fn architecture(&self) -> String;
fn layers(&self) -> Vec<String>;
fn to_string(&self) -> String;
}
pub trait OciConfig {
fn os(&self) -> &str;
fn architecture(&self) -> &str;
fn layers(&self) -> &[String];
}
impl<T: OciConfig> ToString for T {
fn to_string(&self) -> String {
serde_json::to_string_pretty(self).unwrap()
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this out but I get various errors like:

define and implement a trait or new type insteadrustc[Click for full compiler diagnostic](rust-analyzer-diagnostics-view:/diagnostic%20message%20%5B0%5D?0#file%3A%2F%2F%2Fhome%2Fjstur%2Fprojects%2Frunwasi%2Fcrates%2Foci-tar-builder%2Fsrc%2Flib.rs)
lib.rs(43, 19): `WasmConfig` is not defined in the current crate
| impl<T: OciConfig> ToString for T {
   |      ^ type parameter `T` must be used as the type parameter for some local type

I think this is again an issue with the fact that I am extending types outside of the current crate

crates/oci-tar-builder/src/lib.rs Show resolved Hide resolved
crates/oci-tar-builder/src/lib.rs Show resolved Hide resolved
crates/oci-tar-builder/src/lib.rs Show resolved Hide resolved
Comment on lines +74 to +81
impl Default for Builder<ImageConfiguration> {
fn default() -> Self {
Self {
configs: Vec::new(),
layers: Vec::new(),
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove as the above suggestion derives Default trait

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this suggestion but default marcro doesn't work since we have two different types, one of which doesn't have a default implementation

@jsturtevant jsturtevant force-pushed the oci-artifact branch 9 times, most recently from 4d109d5 to 40a6d0c Compare August 7, 2024 22:32
@jsturtevant
Copy link
Contributor Author

I published an example to https://github.com/users/jsturtevant/packages/container/package/wasi-artifact

Can use the new tooling wkg to pull and run:

wkg oci pull ghcr.io/jsturtevant/wasi-artifact:latest
wasmtime run jsturtevant_wasi-artifact.wasm

This is a song that never ends.
Yes, it goes on and on my friends.

@jsturtevant
Copy link
Contributor Author

I am not sure why the smoke test is failing, I've run the same tests locally and it doesn't timeout. I've also not modified any the shim or the test image.

@jsturtevant
Copy link
Contributor Author

it is also only failing on ubuntu 20.04

@jsturtevant
Copy link
Contributor Author

The musl build of wasmtime and the image built with these changes is working on ubuntu 20.04 for me locally

@jsturtevant
Copy link
Contributor Author

/assign @devigned

Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! Great work, @jsturtevant!

@jsturtevant jsturtevant dismissed Mossaka’s stale review August 8, 2024 15:53

Address concerns and Joe is away from keyboard right now

@jsturtevant jsturtevant merged commit 8326190 into containerd:main Aug 8, 2024
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants