Drop tinygo requirement #148

saschagrunert · 2025-03-11T13:22:30Z

Fixes #140

knqyf263 · 2025-03-11T17:07:45Z

main is not called anymore with a WASI reactor.

Lines 27 to 29 in 82c65a7

    
           func main() { 
        
           	api.RegisterPlugin(&plugin{}) 
        
           }

You need to use init() instead.
https://github.com/knqyf263/go-plugin/blob/7819d6147c952d109df10990fe555029aff6c3bc/examples/host-function-library/plugin/plugin.go#L13-L18

Note that main() is still needed for Go to compile Wasm modules.

saschagrunert · 2025-03-12T11:29:38Z

main is not called anymore with a WASI reactor.

nri/plugins/wasm/plugin.go

Lines 27 to 29 in 82c65a7

func main() {

api.RegisterPlugin(&plugin{})

}

You need to use init() instead. https://github.com/knqyf263/go-plugin/blob/7819d6147c952d109df10990fe555029aff6c3bc/examples/host-function-library/plugin/plugin.go#L13-L18

Note that main() is still needed for Go to compile Wasm modules.

Yes, thank you now it works when vendoring the NRI into CRI-O. 👍

saschagrunert · 2025-03-12T12:03:54Z

@knqyf263 CI is green now, do we want to cut a new go-plugin release first?

klihub · 2025-03-12T12:13:43Z

go.mod

 module github.com/containerd/nri

-go 1.22.0
+go 1.24.1


We need to be a bit careful with this and check if this is (already) fine (at this point) for both for containerd/main and containerd/release/2.0. Ideally also for 1.7.x, but I'd say updating that anyway might require rolling a maintenance branch with hand-picked cherry-picks and we probably don't need/want to push WASM support for 1.7.x anyway.

containerd/main seems to be on 1.23 right now, but CI is partially using go 1.24.

Proposing the new golang version in containerd/containerd#11533

@klihub this will also be a breaking change, WASM plugins compiled with tinygo will not work with golang ones because the underlying library changed. I still think it's worth it, having native golang support is brilliant.

@saschagrunert I agree that using the stock/native golang toolchain is clearly preferable over depending on both golang and tinygo. I'm not too worried about the breaking WASM plugin runtime backward compatibility (if you guys are not). We only have had it in one tagged release, and we're not at 1.0.0 yet.

but this is forcing everyone importing this library to move to 1.24, no?

but this is forcing everyone importing this library to move to 1.24, no?

Yes.

We could think about wrapping a // +build go1.24 tag, but this would make a feature dependent on a certain golang version. It would also require more boilerplate.

nod, and the MUST be go 1.24.1 in the go.mod file is forcing everyone that includes this to be dependent on 1.24.1 or higher. This change by golang to make the go tag a MUST is crashing all backwards compatibility testing across the board. Typically you chase it down and someone thought it cool to use a new log feature just introduced.

I see, I was more asking about if was a deliberate change or an oversight, it looks is the only way, I didn;t mean to push in one or other direction

updating that anyway might require rolling a maintenance branch with hand-picked cherry-picks and we probably don't need/want to push WASM support for 1.7.x anyway.

➕ I don't think we should take this for 1.7

this will also be a breaking change, WASM plugins compiled with tinygo will not work with golang ones because the underlying library changed
[...]
Proposing the new golang version in containerd/containerd#11533

Oof. So for containerd, either we:

Require 1.24 in 2.1 (which can make it hard for distros to ship 2.1, delaying adoption), or

Don't take 1.24 in 2.1 and ship 2.1 with the initial tinygo-based WASM support, and then immediately break that in the next release (2.2)

Disable WASM in 2.1 and take the 1.24 minimum version change in 2.2 (which will be more reasonable timing-wise as Go 1.23 will have reached EOL)

None of those sound like great choices to me. I think option 2 sounds the worst though.

Per the community call today, we are not going to upgrade containerd 2.1 to Go 1.24. This leaves us with either option (2) or (3) above.

My preference as-is would be option 3, disabling WASM support entirely until we have moved to Go 1.24 (which will happen for containerd 2.2). Unless there is some way to have the new WASM support sans-tinygo support old tinygo-based plugins?

knqyf263 · 2025-03-12T12:19:58Z

@saschagrunert Thanks for testing it out. I'll cut a new release.

klihub · 2025-03-13T17:00:57Z

@saschagrunert Is this something that would be important to get behind an NRI tag, so we could pull it in on the CRI-O side ?

If it is, then maybe one alternative would be to branch off a 0.9 release branch and merge this there, but keep if off main until all parties involved/affected are content with moving to go 1.24 as the minimum version.

saschagrunert · 2025-03-14T08:30:56Z

If it is, then maybe one alternative would be to branch off a 0.9 release branch and merge this there, but keep if off main until all parties involved/affected are content with moving to go 1.24 as the minimum version.

I think that's a valuable solution, yes. We will move to go 1.24 in the future in any case.

A build tag will not work with the go module requirements if I think about it, because go modules are not tag aware. Means we need the go 1.24 requirement independently of any build constraints.

saschagrunert · 2025-03-25T13:22:30Z

@klihub do you have a new base branch where I could rebase on?

klihub · 2025-03-25T14:08:24Z

@klihub do you have a new base branch where I could rebase on?

No, sorry, not yet.

samuelkarp · 2025-05-23T18:54:59Z

I think this can be rebased on main now. containerd will also move to 1.24 for our next release (2.2).

klihub · 2025-06-12T18:24:06Z

@saschagrunert Now we could rebase this and try to get it merged. I tried to do that, but I was hasty with my push (to your original source branch, argh... sorry 🙈) and now I can't figure out what I screwed up compared to your saved original branch, or if it is something extra that the additions since the base branch of that tree would require, since the WASM plugin compiles in your original but not in the updated version.

klihub · 2025-06-13T06:45:01Z

@saschagrunert Now we could rebase this and try to get it merged. I tried to do that, but I was hasty with my push (to your original source branch, argh... sorry 🙈) and now I can't figure out what I screwed up compared to your saved original branch, or if it is something extra that the additions since the base branch of that tree would require, since the WASM plugin compiles in your original but not in the updated version.

@saschagrunert I think I managed to figure it out now... will update with a fixed branch.

saschagrunert · 2025-06-23T07:38:24Z

@klihub thank you!

mikebrow

The matrix for go still includes 1.23.9 but during the setup for that version in switches to 1.24.3 instead.. thus we end up using 1.24.3 twice in CI..

Prepare all required actions
Getting action download info
Download immutable action package 'actions/setup-go@v5'
Run ./.github/actions/install-go
Run actions/setup-go@v5
Setup go version spec 1.23.9
Found in cache @ /opt/hostedtoolcache/go/1.23.9/x64
Added go to the path
Successfully set up Go version 1.23.9
go: downloading go1.24.3 (linux/amd64)
go version go1.24.3 linux/amd64

nit because this can be said for all the plugins.. TODO: need some readme docs for the WASM implementation... linking to https://github.com/knqyf263/go-plugin docs if nec..; explanation of special pkg/api; something for the plugin; and a test would be nice.

saschagrunert · 2025-07-02T10:27:38Z

The matrix for go still includes 1.23.9 but during the setup for that version in switches to 1.24.3 instead.. thus we end up using 1.24.3 twice in CI..

I removed that version from the matrix.

nit because this can be said for all the plugins.. TODO: need some readme docs for the WASM implementation... linking to https://github.com/knqyf263/go-plugin docs if nec..; explanation of special pkg/api; something for the plugin; and a test would be nice.

Yeah I can follow-up on docs and tests.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com> Signed-off-by: Sascha Grunert <sgrunert@redhat.com>

mikebrow

LGTM

saschagrunert mentioned this pull request Mar 11, 2025

Support standard golang compiler for WASM plugins #140

Closed

saschagrunert force-pushed the tinygo branch from c2f161c to 2225349 Compare March 12, 2025 11:29

saschagrunert force-pushed the tinygo branch 6 times, most recently from 06afcd6 to 44c6377 Compare March 12, 2025 12:01

saschagrunert changed the title ~~WIP: Drop tinygo requirement~~ Drop tinygo requirement Mar 12, 2025

klihub reviewed Mar 12, 2025

View reviewed changes

knqyf263 mentioned this pull request Mar 12, 2025

Can I use go1.21 to compile plugins? knqyf263/go-plugin#58

Closed

saschagrunert mentioned this pull request Mar 12, 2025

Switch to go 1.24 containerd/containerd#11533

Closed

saschagrunert force-pushed the tinygo branch from 44c6377 to c2e2f41 Compare March 12, 2025 12:29

djdongjin mentioned this pull request May 7, 2025

Bump up go version to 1.24 and cri-api to 0.33.0 containerd/containerd#11823

Merged

samuelkarp added this to the 1.0 milestone May 23, 2025

klihub force-pushed the tinygo branch 3 times, most recently from a71d198 to a2a4812 Compare June 12, 2025 16:47

klihub force-pushed the tinygo branch from a2a4812 to 0b70e05 Compare June 13, 2025 06:49

klihub force-pushed the tinygo branch 2 times, most recently from 5af355a to 0af3a71 Compare June 13, 2025 06:58

klihub requested review from aojea, klihub, mikebrow and samuelkarp June 23, 2025 07:10

aojea approved these changes Jun 23, 2025

View reviewed changes

klihub approved these changes Jun 27, 2025

View reviewed changes

mikebrow reviewed Jul 1, 2025

View reviewed changes

saschagrunert force-pushed the tinygo branch from 0af3a71 to 1b8bfd4 Compare July 2, 2025 10:25

saschagrunert force-pushed the tinygo branch from 1b8bfd4 to 8f2c1dd Compare July 2, 2025 10:28

Drop tinygo requirement

f6f8d03

Signed-off-by: Sascha Grunert <sgrunert@redhat.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com> Signed-off-by: Sascha Grunert <sgrunert@redhat.com>

saschagrunert force-pushed the tinygo branch from 8f2c1dd to f6f8d03 Compare July 2, 2025 10:29

mikebrow approved these changes Jul 2, 2025

View reviewed changes

mikebrow merged commit 8ebdb07 into containerd:main Jul 2, 2025
7 checks passed

saschagrunert deleted the tinygo branch July 2, 2025 14:16

Drop tinygo requirement #148

Drop tinygo requirement #148

Uh oh!

Conversation

saschagrunert commented Mar 11, 2025

Uh oh!

knqyf263 commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saschagrunert commented Mar 12, 2025

Uh oh!

saschagrunert commented Mar 12, 2025

Uh oh!

klihub Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

saschagrunert Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klihub Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aojea Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

saschagrunert Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

mikebrow Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aojea Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

samuelkarp Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

samuelkarp Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

knqyf263 commented Mar 12, 2025

Uh oh!

klihub commented Mar 13, 2025

Uh oh!

saschagrunert commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saschagrunert commented Mar 25, 2025

Uh oh!

klihub commented Mar 25, 2025

Uh oh!

samuelkarp commented May 23, 2025

Uh oh!

klihub commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klihub commented Jun 13, 2025

Uh oh!

saschagrunert commented Jun 23, 2025

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

saschagrunert commented Jul 2, 2025

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

knqyf263 commented Mar 11, 2025 •

edited

Loading

saschagrunert Mar 12, 2025 •

edited

Loading

klihub Mar 12, 2025 •

edited

Loading

mikebrow Mar 12, 2025 •

edited

Loading

saschagrunert commented Mar 14, 2025 •

edited

Loading

klihub commented Jun 12, 2025 •

edited

Loading