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

feat(shim): support archive layers #121

Merged
merged 2 commits into from
Jun 25, 2024
Merged

Conversation

vdice
Copy link
Contributor

@vdice vdice commented May 22, 2024

Closes #28

Depends on fermyon/spin#2523

Draft for now as 2523 is based on Spin's main branch which has since bumped wasmtime to 21.0.1. Can either wait for that same bump to occur in this project (presumably in tandem with pinning to the next Spin release with said version) or can explore backporting 2523 to the v2.5 branch and cutting a patch release with it.

TODO

  • Depends on PR(s) bumping spin + co repos to next release tag and wasmtime to 21.0.1 in Cargo.toml. Rebase after this lands.
  • decide on merge approach (avoid wasmtime/spin crates bump or wait for them to land)
    • I think waiting for the upcoming 2.6 Spin release and bumping wasmtime to suit will be fine; no big hurry to get this in.
  • add test app/test case

@vdice
Copy link
Contributor Author

vdice commented Jun 3, 2024

In 8e83775 I added a new spin-static-assets app and incorporated into the integration tests, to cover the archive layer functionality. I haven't had success running the integration tests locally, so leaning on CI here to check. I'm also happy to relegate this commit (or general approach if adjustments are requested) as a follow-up instead.

@vdice vdice force-pushed the feat/archive-layers branch 5 times, most recently from 2a331be to 8e83775 Compare June 4, 2024 17:17
Signed-off-by: Vaughn Dice <vaughn.dice@fermyon.com>
… testing

Signed-off-by: Vaughn Dice <vaughn.dice@fermyon.com>
@vdice vdice marked this pull request as ready for review June 21, 2024 16:49
@vdice vdice requested a review from jsturtevant June 21, 2024 16:49
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!

@@ -40,10 +40,6 @@ const SPIN_HTTP_LISTEN_ADDR_ENV: &str = "SPIN_HTTP_LISTEN_ADDR";
const RUNTIME_CONFIG_PATH: &str = "/runtime-config.toml";
/// Describes an OCI layer with Wasm content
const OCI_LAYER_MEDIA_TYPE_WASM: &str = "application/vnd.wasm.content.layer.v1+wasm";
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: make spin_oci::client::WASM_LAYER_MEDIA_TYPE public and use it in the Spin Shim.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, Spin itself is waiting on the canonical type/string upstream: https://github.com/fermyon/spin/blob/main/crates/oci/src/client.rs#L34-L35. To be defined in https://github.com/bytecodealliance/registry perhaps? Therefore expecting both Spin and the shim would pull this from the upstream crate.

Copy link
Collaborator

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

LGTM!

// which will fall back to /tmp if TMPDIR is not set. /tmp is either not found or not accessible
// in the shim environment, hence setting to current working directory.
if env::var("TMPDIR").is_err() {
log::debug!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

When do we expect TMPDIR to be set? I am wondering if this log is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point... it appears it isn't ever set, currently. Glad to remove this debug log if you'd like.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we would want to leave it, otherwise we might overwrite it. I don't know what scenario it would be set in, but if it was set and we overwrite I don't think that would be an expected behavior?

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

@vdice
Copy link
Contributor Author

vdice commented Jun 24, 2024

Thanks, all. Please merge when ready.

@Mossaka Mossaka merged commit 95bfe75 into spinkube:main Jun 25, 2024
9 checks passed
@vdice vdice deleted the feat/archive-layers branch June 25, 2024 12:57
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.

Support archive layers
4 participants