Go 1.21.0 #8957
Merged
Go 1.21.0 #8957
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AkihiroSuda
added
cherry-pick/1.6.x
https://github.com/containerd/containerd/actions/runs/5833675339/job/15821608719?pr=8957 |
AkihiroSuda
force-pushed
the
go-1.21
branch
2 times, most recently
from
61104dc
to
1400127
Compare
https://github.com/containerd/containerd/actions/runs/5834109612/job/15823137201?pr=8957 |
➜ containerd git:(pr-8957) go1.21.0 test -c -o /tmp/test -gcflags -m -v ./pkg/cri/sbserver 2>&1 | grep "seccomp.With"
pkg/cri/sbserver/container_create_linux.go:179:36: inlining call to seccomp.WithDefaultProfile
pkg/cri/sbserver/container_create_linux.go:183:29: inlining call to seccomp.WithProfile
pkg/cri/sbserver/container_create_linux_test.go:906:40: inlining call to seccomp.WithDefaultProfile
pkg/cri/sbserver/container_create_linux_test.go:911:40: inlining call to seccomp.WithDefaultProfile
pkg/cri/sbserver/container_create_linux_test.go:916:33: inlining call to seccomp.WithProfile
pkg/cri/sbserver/container_create_linux_test.go:921:39: inlining call to seccomp.WithProfile
pkg/cri/sbserver/container_create_linux_test.go:926:46: inlining call to seccomp.WithDefaultProfile
pkg/cri/sbserver/container_create_linux_test.go:964:40: inlining call to seccomp.WithDefaultProfile
pkg/cri/sbserver/container_create_linux_test.go:971:33: inlining call to seccomp.WithProfile
pkg/cri/sbserver/container_create_linux_test.go:979:33: inlining call to seccomp.WithProfile
➜ containerd git:(pr-8957) go1.20.4 test -c -o /tmp/test -gcflags -m -v ./pkg/cri/sbserver 2>&1 | grep "seccomp.With"
➜ containerd git:(pr-8957)It seems like that go1.21.0 has improved the inline logic. However, it still fails even if I add noinline (no idea yet). Maybe we should improve our testing code instead of checking function pointer. Update: Use ➜ containerd git:(pr-8957) go1.21.0 test -gcflags=-l ./pkg/cri/sbserver -run TestGenerateSeccompSecurityProfileSpecOpts
ok github.com/containerd/containerd/pkg/cri/sbserver 0.094s
Quick fix: diff --git a/contrib/seccomp/seccomp.go b/contrib/seccomp/seccomp.go
index 5292cbcec..f1408a492 100644
--- a/contrib/seccomp/seccomp.go
+++ b/contrib/seccomp/seccomp.go
@@ -30,6 +30,8 @@ import (
// WithProfile receives the name of a file stored on disk comprising a json
// formatted seccomp profile, as specified by the opencontainers/runtime-spec.
// The profile is read from the file, unmarshaled, and set to the spec.
+//
+//go:noinline
func WithProfile(profile string) oci.SpecOpts {
return func(_ context.Context, _ oci.Client, _ *containers.Container, s *specs.Spec) error {
s.Linux.Seccomp = &specs.LinuxSeccomp{}
@@ -46,6 +48,8 @@ func WithProfile(profile string) oci.SpecOpts {
// WithDefaultProfile sets the default seccomp profile to the spec.
// Note: must follow the setting of process capabilities
+//
+//go:noinline
func WithDefaultProfile() oci.SpecOpts {
return func(_ context.Context, _ oci.Client, _ *containers.Container, s *specs.Spec) error {
s.Linux.Seccomp = DefaultProfile(s)
(END)
|
|
Thank you, added Eventually we should update the tests so that they do not depend on this though. |
|
Windows 2019 tests still failing, although Windows 2022 tests are green https://github.com/containerd/containerd/actions/runs/5866899922/job/15906736005?pr=8957 |
|
@aznashwan when you get a chance, could you have a look? CC @iankingori |
|
@AkihiroSuda TL;DR: the Windows errors are runtime DLL loading errors due to an outdated MinGW. (please see fix in AkihiroSuda#1) While I do not know the exact root cause, I can confirm that upgrading the MinGW version in the 2019 runner avoids the issue entirely. While it sucks to have 2019 workflows take 5 extra minutes to upgrade MinGW, our only real alternatives would either be building custom images for our tests or using some pre-existing actions which set up MinGW such as MSYS2. I personally believe it's better we keep all the setup explicit within the containerd repos/action workflows instead of relying on 3rd party actions, so I have created this PR which seems to fix the issue: AkihiroSuda#1 Based on my test runs, it looks like it fixes the issue: windows-2019, windows-2019-sandboxed Also note that, in my testing of the Azure-based workflow which uses this setup script, it looks like the current MinGW version of 10.2 seems to work fine, and I'd personally like to avoid changing it until it presents issues. Please let me know if you'd like more specific testing done. |
AkihiroSuda
force-pushed
the
go-1.21
branch
2 times, most recently
from
9644fe6
to
7f0d0af
Compare
|
Thank you @aznashwan ! |
> github.com/containerd/containerd/contrib/apparmor > github.com/containerd/containerd/contrib/apparmor > Running go-fuzz -tags gofuzz -func FuzzLoadDefaultProfile -o fuzz_FuzzLoadDefaultProfile.a github.com/containerd/containerd/contrib/apparmor > /usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x25 > fuzz_FuzzLoadDefaultProfile.a(000021.o): in function `_cgo_9c8efe9babca_C2func_res_search': > cgo_unix_cgo_res.cgo2.c:(.text+0x32): undefined reference to `__res_search' > /usr/bin/ld: fuzz_FuzzLoadDefaultProfile.a(000021.o): in function `_cgo_9c8efe9babca_Cfunc_res_search': > cgo_unix_cgo_res.cgo2.c:(.text+0x81): undefined reference to `__res_search' > clang-15: error: linker command failed with exit code 1 (use -v to see invocation) > 2023-08-11 14:25:45,433 - root - ERROR - Building fuzzers failed. > 2023-08-11 14:25:45,433 - root - ERROR - Error building fuzzers for (commit: 432d86b, pr_ref: refs/pull/8957/merge). Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Tests in pkg/cri/[sb]server/container_create_linux_test.go depends on go:noinline since Go 1.21. e.g., > ``` > === FAIL: pkg/cri/sbserver TestGenerateSeccompSecurityProfileSpecOpts/should_set_default_seccomp_when_seccomp_is_runtime/default (0.00s) > container_create_linux_test.go:1013: > Error Trace: /home/runner/work/containerd/containerd/pkg/cri/sbserver/container_create_linux_test.go:1013 > Error: Not equal: > expected: 0x263d880 > actual : 0x263cbc0 > Test: TestGenerateSeccompSecurityProfileSpecOpts/should_set_default_seccomp_when_seccomp_is_runtime/default > ``` See comments in PR 8957. Thanks to Wei Fu for analyzing this. Co-authored-by: Wei Fu <fuweid89@gmail.com> Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
The default version of MinGW and GCC on the GitHub-hosted Windows 2019 runners compile fine but lead to linker errors during runtime. Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
https://go.dev/doc/go1.21 Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
|
ping @AdamKorcz would you please help us fix the fuzz-build issue? thanks |
Yep. I will need to update containerd on the OSS-Fuzz side, so feel free to merge and I will fix right after. |
|
@AdamKorcz Great! Thank you! |
kzys
approved these changes
Aug 18, 2023
|
I don't know if it's been discussed elsewhere, but a nice-to-have for backporting to 1.7 is it would resolve the containerd-side occurrence of microsoft/Windows-Containers#213 when calling Although nothing in the wild is likely to be hitting that codepath today, BuildKit seems likely to get there soonish and hopefully adoption will accelerate from there. Later Docker with (I think) containerd image store enabled will be exposed to this issue too, if using containerd built with Go older than 1.21. So it's be nice (but not essential: there's known Dockerfile workarounds now that we have identified the underlying cause and hence exactly what actions are affected) if we could safely point those budding developments at a 1.7.x containerd release, similar to how we got the Windows layer mount support backported to 1.7.2. |
This was referenced Oct 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://go.dev/doc/go1.21