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.

Sign up for GitHub

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

Jump to bottom

Precompilation support. #32

Merged
merged 7 commits into from
Mar 12, 2024
Merged

Precompilation support. #32

merged 7 commits into from
Mar 12, 2024

Conversation

radu-matei
Copy link
Member

Note: this now only supports scratch containers pushed without the --platform=wasi/wasm flag set when building the container image.

relevant changes: 7b1e07a
close #31

Signed-off-by: Radu Matei radu@fermyon.com

radu-matei and others added 4 commits March 7, 2024 22:11
@radu-matei @kate-goldenring
Initial support for precompiling Wasm components
09c1a56 
Signed-off-by: Radu Matei <radu@fermyon.com>
@kate-goldenring
Check if layer is already precompiled before precompilation
1184935 
Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
@kate-goldenring
Point to latest Runwasi and Spin crates for precompilation support
9ae9b30 
Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
@radu-matei
fix(precompilation): skip AOT if scratch container
7b1e07a 
This commit skips precompilation when the Spin application is
distributed using a scratch container, and only enables it when
distributing apps using `spin registry push`.

Signed-off-by: Radu Matei <radu@fermyon.com>
@radu-matei radu-matei mentioned this pull request Mar 8, 2024
Closed
devigned
devigned requested changes Mar 8, 2024
View reviewed changes
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.

A small nit, but otherwise looks great!

containerd-shim-spin/src/engine.rs Outdated Show resolved Hide resolved
@radu-matei radu-matei changed the title Initial support for precompiling Wasm components Support scratch containers Mar 8, 2024
@radu-matei radu-matei changed the title Support scratch containers Skip precompilation for scratch containers Mar 8, 2024
@devigned devigned mentioned this pull request Mar 8, 2024
Closed
@kate-goldenring
Copy link
Collaborator

@radu-matei how does the --platform=wasi/wasm affect this behavior?

@kate-goldenring
cleanup: create constants for each OCI layer media type
a57eeeb 
Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
@kate-goldenring
Copy link
Collaborator

I think we should use this PR and close #16 for a couple reasons:

  1. Precompilation should not be added without this patch
  2. The other branch is orphaned (its remote does not exist anymore) so changes cannot be made to it)

If there are no objections, can i close #16? I incorporated your feedback @devigned -- much cleaner!

@radu-matei radu-matei changed the title Skip precompilation for scratch containers Precompilation support. Mar 8, 2024
Mossaka
Mossaka approved these changes Mar 8, 2024
View reviewed changes
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.

LGTM added a few refactoring tips

containerd-shim-spin/src/engine.rs Show resolved Hide resolved
containerd-shim-spin/src/engine.rs Outdated Show resolved Hide resolved
containerd-shim-spin/src/engine.rs Show resolved Hide resolved
@kate-goldenring
Copy link
Collaborator

kate-goldenring commented Mar 11, 2024 •
edited
Loading

@radu-matei I was tinkering with this and similarly failed to run a scratch container built with --platform wasm/wasi. Part of the error was:

Mar 11 06:43:23 kagold-ThinkPad-X1-Carbon-6th containerd[927513]: time="2024-03-11T13:43:23.742903865Z" level=error msg="on non-Windows, at least one process arg entry is required"
Mar 11 06:43:23 kagold-ThinkPad-X1-Carbon-6th containerd[927513]: time="2024-03-11T13:43:23.742939138Z" level=error msg="failed to initialize container process: missing args in the process spec"

However, if I set arg0 in the Entrypoint to be ./spin.toml it runs successfully. Note how adding the last argument in the ctr command makes it work:

sudo ctr run --rm --net-host --runtime io.containerd.spin.v2 ghcr.io/deislabs/containerd-wasm-shims/examples/spin-rust-hello:v0.10.0 sample ./spin.toml

I'm continuing to investigate but wanted to share this for now

@kate-goldenring
fix(engine): use fixed Spin manifest path when loading from file app …
d8b2330 
…source

Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
@kate-goldenring
Copy link
Collaborator

kate-goldenring commented Mar 11, 2024 •
edited
Loading

@radu-matei I believe my latest commit should have resolved the issue. In 09c1a56, we undid always resolving local spin manifest path to ./spin.toml. Commit d8b2330 reconfigures that.

jsturtevant
jsturtevant approved these changes Mar 11, 2024
View reviewed changes
Copy link
Contributor

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

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

LGTM

containerd-shim-spin/src/engine.rs Outdated Show resolved Hide resolved
containerd-shim-spin/src/engine.rs Show resolved Hide resolved
devigned
devigned approved these changes Mar 11, 2024
View reviewed changes
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!

@kate-goldenring
cleanup: simplify precompile implementation with an iterator
6e1b36b 
Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
@radu-matei
Copy link
Member Author

Is there anything before we can get this in and get prebuilt binaries we can use more easily?

Thanks!

@kate-goldenring kate-goldenring merged commit 2214517 into spinkube:main Mar 12, 2024
8 of 9 checks passed
@jsturtevant jsturtevant mentioned this pull request Mar 12, 2024
Closed
@Mossaka
Copy link
Member

Mossaka commented Mar 12, 2024

Thanks for getting this PR in! 🥂

Time to do another release?

@kate-goldenring
Copy link
Collaborator

Time to do another release?

@Mossaka yes indeed. I can do this one to get some practice at it

@Mossaka
Copy link
Member

Mossaka commented Mar 12, 2024

@kate-goldenring yeah let me know if you have questions

@jsturtevant jsturtevant mentioned this pull request Mar 12, 2024
Closed
@radu-matei radu-matei mentioned this pull request Apr 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mossaka Mossaka Mossaka approved these changes

@jsturtevant jsturtevant jsturtevant approved these changes

@devigned devigned devigned approved these changes

@kate-goldenring kate-goldenring Awaiting requested review from kate-goldenring kate-goldenring is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Precompilation breaks Spin applications distributed in scratch containers
5 participants
@radu-matei @kate-goldenring @Mossaka @devigned @jsturtevant