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

wasmtime 20.0.2, enabled async, networking and ip name lookup #589

Merged
merged 11 commits into from
May 27, 2024

Conversation

vescoc
Copy link
Contributor

@vescoc vescoc commented May 14, 2024

No description provided.

@vescoc vescoc marked this pull request as draft May 14, 2024 20:07
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.

Thanks for the for upgrade to wasmtime. I am interested in the use cases that require the additional features you've enabled. Ultimately, we might need a way customize the runtimes engines. This has come up in spinkube/containerd-shim-spin#115 as well

@@ -29,6 +28,7 @@ impl WasiConfig for DefaultConfig {
fn new_config() -> Config {
let mut config = wasmtime::Config::new();
config.wasm_component_model(true); // enable component linking
config.async_support(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

What features are you looking to enable with the async support?

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 a sync implementation but got an error when running the demo app. If I remember correctly the error looked like this. I solved the problem by enabling async and using async linker and so on.

.inherit_network()
.allow_tcp(true)
.allow_udp(true)
.allow_ip_name_lookup(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the use case you are trying to unblock?

resource_table: ResourceTable::default(),
};
Ok(wasi_data)
.inherit_network()
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, what are the use cases?

Comment on lines +292 to +293
.allow_tcp(true)
.allow_udp(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

These two are on by default, but being explicit is probably good idea.

@vescoc
Copy link
Contributor Author

vescoc commented May 15, 2024

Thanks for the for upgrade to wasmtime. I am interested in the use cases that require the additional features you've enabled. Ultimately, we might need a way customize the runtimes engines. This has come up in spinkube/containerd-shim-spin#115 as well

Well, the use case that triggered these configuration sets is trivial. I'm investigating the current limits of using WASI on non-trivial "tokio-type" basic applications. I'm converting my protohackers problem solutions which are tokio based, to solutions based on WASI 0.2 components. To run the solutions I need IP name lookup, networking, tcp and udp enabled. Simple 😄

If you want to see what I'm doing on protohackers, this is my repository. Into the directory wasi there are the solutions using Rust / WASI.

I have currently converted the first 4 solutions. I'll proceed with the others in the next few days. I need to implement the missing pieces in my wasi_async framework (next step: UDP).

For the configurability, I'm thinking of using env vars with the WASMTIME_ or RUNWASI_ prefix, not passed to the invoked process but managed by the shim.

I looked at the approach used in spim but it didn't convince me, I probably need to understand it better.

@Mossaka
Copy link
Member

Mossaka commented May 17, 2024

Yeah containerd config options for the shim would be a good place to put configurations that might change the runtime behaviours.

@vescoc vescoc marked this pull request as ready for review May 20, 2024 18:11
@Mossaka Mossaka requested a review from jsturtevant May 21, 2024 22:58
]}
wasmtime-wasi = { version = "17.0", features = ["exit"] }
wasi-common = "17.0"
wasmtime = { version = "20.0.2", features = ["async"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasmtime 21 was released
I saw that by updating the version the repository compiles and the tests pass. If you prefer, I open a new PR or rename this one with the transition directly to wasmtime 21

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, Would you mind doing the bump to Wasmtime 21 as a separate PR? I am still not sure the implications of turning on the async support and a few of the other options enabled here. I need a little more time to understand what turning these features means. Maybe other reviewers have some thoughts too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, Would you mind doing the bump to Wasmtime 21 as a separate PR? I am still not sure the implications of turning on the async support and a few of the other options enabled here. I need a little more time to understand what turning these features means. Maybe other reviewers have some thoughts too

For the transition to wasmtime 21 in my opinion we can wait as long as, we simply need to update the dependencies of Cargo.toml.

I'm currently testing the new version on my k8s.

Please take the time to evaluate the impacts of the proposed changes. As already indicated I attempted to keep the code with async disabled but I had got failed tests so I opted to switch to async mode. This should not impact other shims.

I hope other people can try my modifications on more serious implementations than the my toy one with protohackers.

@jsturtevant
Copy link
Contributor

Well, the use case that triggered these configuration sets is trivial. I'm investigating the current limits of using WASI on non-trivial "tokio-type" basic applications. I'm converting my protohackers problem solutions which are tokio based, to solutions based on WASI 0.2 components. To run the solutions I need IP name lookup, networking, tcp and udp enabled. Simple 😄

If you want to see what I'm doing on protohackers, this is my repository. Into the directory wasi there are the solutions using Rust / WASI.

This is really cool!

Mossaka
Mossaka previously approved these changes May 27, 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.

LGTM!

@Mossaka
Copy link
Member

Mossaka commented May 27, 2024

Need a rebase

@Mossaka
Copy link
Member

Mossaka commented May 27, 2024

Thanks, again! @vescoc

@Mossaka Mossaka merged commit 0f7817a into containerd:main May 27, 2024
51 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.

3 participants