Skip to content

Update the containerd-shim-wasm to v1.0#299

Merged
Mossaka merged 4 commits intomainfrom
bump-containerd-shim-wasm-v1
Mar 31, 2025
Merged

Update the containerd-shim-wasm to v1.0#299
Mossaka merged 4 commits intomainfrom
bump-containerd-shim-wasm-v1

Conversation

@Mossaka
Copy link
Copy Markdown
Member

@Mossaka Mossaka commented Mar 26, 2025 •
edited
Loading

Preparing for the upcoming runwasi v1.0 release

Signed-off-by: Jiaxiao (mossaka) Zhou duibao55328@gmail.com

jprendes
jprendes approved these changes Mar 26, 2025
View reviewed changes
Copy link
Copy Markdown

@jprendes jprendes left a comment

Choose a reason for hiding this comment

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

LGTM, left a few nits

Comment thread containerd-shim-spin/src/engine.rs Outdated
Comment thread containerd-shim-spin/src/trigger.rs Outdated
@Mossaka Mossaka force-pushed the bump-containerd-shim-wasm-v1 branch from 1e6050c to 88df5c0 Compare March 27, 2025 03:26
jprendes
jprendes approved these changes Mar 27, 2025
View reviewed changes
Copy link
Copy Markdown

@jprendes jprendes 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 @Mossaka !

@Mossaka Mossaka force-pushed the bump-containerd-shim-wasm-v1 branch from 88df5c0 to 402d9a6 Compare March 27, 2025 19:27
@Mossaka
Update the containerd-shim-wasm to v1.0.0-rc.1
2dd59d7 
Signed-off-by: Jiaxiao (mossaka) Zhou <duibao55328@gmail.com>
@Mossaka Mossaka force-pushed the bump-containerd-shim-wasm-v1 branch from 5b80069 to 2dd59d7 Compare March 28, 2025 21:48
@Mossaka Mossaka marked this pull request as ready for review March 28, 2025 21:48
kate-goldenring
kate-goldenring approved these changes Mar 28, 2025
View reviewed changes
Copy link
Copy Markdown
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.

Thank you @Mossaka

@kate-goldenring kate-goldenring mentioned this pull request Mar 28, 2025
Merged
@kate-goldenring
Copy link
Copy Markdown
Collaborator

kate-goldenring commented Mar 28, 2025

@Mossaka, it looks like the Systemd cgroup is not being set correctly for k3s: SystemdCgroup is not set to true, Maybe there is some issue with these changes: #301?

jprendes
jprendes approved these changes Mar 28, 2025
View reviewed changes
Copy link
Copy Markdown

@jprendes jprendes left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread containerd-shim-spin/Cargo.toml
@shirok1
Copy link
Copy Markdown
Contributor

shirok1 commented Mar 29, 2025

@Mossaka, it looks like the Systemd cgroup is not being set correctly for k3s: SystemdCgroup is not set to true, Maybe there is some issue with these changes: #301?

On my machine K3S generated SystemdCgroup = true itself:

# k3s -v
k3s version v1.32.2+k3s1 (381620ef)
go version go1.23.6
# ls /var/lib/rancher/k3s/agent/etc/containerd/
certs.d  config.toml
# cat /var/lib/rancher/k3s/agent/etc/containerd/config.toml
# irrelevant skipped...
[plugins.'io.containerd.cri.v1.runtime'.containerd.runtimes.'spin']
  runtime_type = "io.containerd.spin.v2"

[plugins.'io.containerd.cri.v1.runtime'.containerd.runtimes.'spin'.options]
  BinaryName = "/opt/kwasm/bin/containerd-shim-spin-v2"
  SystemdCgroup = true

All runtimes K3S found will share a global SystemdCgroup setting

In templates: https://github.com/k3s-io/k3s/blob/441a42e8ce5a850e32546593ecad750a7ccc3ab1/pkg/agent/templates/templates.go#L25

Also see https://github.com/k3s-io/k3s/blob/441a42e8ce5a850e32546593ecad750a7ccc3ab1/pkg/agent/containerd/config_linux.go#L88 where this value is from

@shirok1
Copy link
Copy Markdown
Contributor

shirok1 commented Mar 29, 2025

@kate-goldenring So I found the problem in #299 (comment) might be

containerd-shim-spin/node-installer/tests/integration-test-k3s.sh

Line 45 in 69dac2c

if sudo cat /var/lib/rancher/k3s/agent/etc/containerd/config.toml | grep -A2 "runtimes.spin.options" | grep -q "SystemdCgroup = true"; then

requiring runtimes.spin.options in config.toml, but K3S generates runtimes."spin".options for containerd v3 config (and runtimes.'spin'.options for v2), see https://github.com/search?q=repo%3Ak3s-io%2Fk3s%20containerd.runtimes&type=code

To totally avoid such bug we might need something like jq but for toml

@Mossaka
node-installer: update the k3s integration test script to account for…
257c991 
… different quoting styles for spin

Signed-off-by: Jiaxiao (mossaka) Zhou <duibao55328@gmail.com>
@shirok1 shirok1 mentioned this pull request Mar 31, 2025
Merged
@Mossaka
node-installer: update symbolic link to the right binary name
eac30f6 
Signed-off-by: Jiaxiao (mossaka) Zhou <duibao55328@gmail.com>
@Mossaka
Copy link
Copy Markdown
Member Author

Mossaka commented Mar 31, 2025 •
edited
Loading

Okay the CI's passing now! I did push two commits to fix it:

Please take a look, @kate-goldenring and @jprendes

Thank you for helping, @shirok1

jprendes
jprendes approved these changes Mar 31, 2025
View reviewed changes
Copy link
Copy Markdown

@jprendes jprendes left a comment

Choose a reason for hiding this comment

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

LGTM

jprendes
jprendes reviewed Mar 31, 2025
View reviewed changes
Comment thread node-installer/script/installer.sh
# A bug in containerd makes BinaryName not work with shim not in PATH, so this statically links the kwasm installation to path
# https://github.com/containerd/containerd/issues/11480
mkdir -p $NODE_ROOT/usr/local/bin/
ln -s $KWASM_DIR/bin/containerd-shim-spin-v2 $NODE_ROOT/usr/local/bin/containerd-shim-spin
Copy link
Copy Markdown

@jprendes jprendes Mar 31, 2025

Choose a reason for hiding this comment

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

How did this ever work? Or is this change not released yet?

Copy link
Copy Markdown
Member Author

@Mossaka Mossaka Mar 31, 2025

Choose a reason for hiding this comment

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

Not released yet but I am not sure how the CI was not able to detect it

Copy link
Copy Markdown
Member Author

@Mossaka Mossaka Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh wait, the CI was skipped for this change - I don't know why though: https://github.com/spinframework/containerd-shim-spin/actions/runs/14123812940. But this explains why this error went undetected to the main tree

@Mossaka
Removed unused dependencies and moved tokio to dev-dependency
47e4593 
Signed-off-by: Jiaxiao (mossaka) Zhou <duibao55328@gmail.com>
@Mossaka Mossaka requested a review from jprendes March 31, 2025 14:12
jprendes
jprendes approved these changes Mar 31, 2025
View reviewed changes
Copy link
Copy Markdown

@jprendes jprendes left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks @Mossaka !

@Mossaka Mossaka merged commit 57712f0 into main Mar 31, 2025
13 checks passed
@Mossaka Mossaka deleted the bump-containerd-shim-wasm-v1 branch March 31, 2025 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kate-goldenring kate-goldenring kate-goldenring approved these changes

@jsturtevant jsturtevant Awaiting requested review from jsturtevant jsturtevant is a code owner

@radu-matei radu-matei Awaiting requested review from radu-matei radu-matei is a code owner

@devigned devigned Awaiting requested review from devigned devigned is a code owner

+1 more reviewer

@jprendes jprendes jprendes approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Mossaka @kate-goldenring @shirok1 @jprendes