From d6379d2d956104e68bd4678cc7305b6d9134ea25 Mon Sep 17 00:00:00 2001 From: Yu Lun Hsu Date: Mon, 6 Apr 2026 15:33:13 -0700 Subject: [PATCH] fix(shim): support non-CRI callers (BuildKit, ctr) in containerd shim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fix two issues that prevent gVisor containers from working when created through containerd's direct API (non-CRI), such as BuildKit's containerd worker or `ctr run`. ### 1. Sandbox detection for non-CRI callers `SpecContainerType` returns `ContainerTypeUnspecified` for non-CRI callers that don't set the container type annotation (`io.kubernetes.cri.container-type`). Previously this resulted in `p.Sandbox=false`, which skipped passing IO file descriptors to `runsc create`. Without IO, container processes receive SIGPIPE on stdout/stderr writes and exit with code 128. **Fix:** treat `ContainerTypeUnspecified` as sandbox, since non-CRI containers are always root/sandbox containers. ### 2. Wait error handling for short-lived containers For short-lived containers, the sandbox may exit before `runsc wait` retrieves the exit status, producing `"sandbox no longer running and its exit status is unavailable"`. This error was treated as fatal (setting `internalErrorCode=128`) even when the container exited successfully (`status=0`). **Fix:** extract `adjustWaitStatus()` function — when status is 0, log the error as a warning but preserve the exit status. ## Test plan - [x] `TestSandboxDetection` — 9 cases covering all container type variants including no-annotation (BuildKit/ctr), containerd sandbox/container, CRI-O sandbox/container, unknown values, dual annotations - [x] `TestAdjustWaitStatus` — 9 cases covering status=0 success, non-zero exit, signal exit (137), benign sandbox error with status=0, non-zero with error, negative status, exit 255 - [x] E2E: BuildKit containerd worker with gVisor on GCE e2-standard-4, containerd v1.7.29, runsc release-20260330.0. Eight consecutive RUN steps (echo, ls, resolv.conf, apk add curl jq + curl HTTPS, exit code, multi-line script, id, uname) all pass with exit code 0. Fixes #12198 FUTURE_COPYBARA_INTEGRATE_REVIEW=https://github.com/google/gvisor/pull/12873 from alan890104:minimal-fix c5fdfd4576a67dd7051c0a1d76ea4b0b199966ff PiperOrigin-RevId: 895531992 --- pkg/shim/v1/proc/BUILD | 11 +++- pkg/shim/v1/proc/init.go | 23 +++++-- pkg/shim/v1/proc/init_test.go | 102 ++++++++++++++++++++++++++++++ pkg/shim/v1/runsc/BUILD | 1 + pkg/shim/v1/runsc/service.go | 6 +- pkg/shim/v1/runsc/service_test.go | 80 +++++++++++++++++++++++ 6 files changed, 216 insertions(+), 7 deletions(-) create mode 100644 pkg/shim/v1/proc/init_test.go diff --git a/pkg/shim/v1/proc/BUILD b/pkg/shim/v1/proc/BUILD index e3bf358468..9123a70be1 100644 --- a/pkg/shim/v1/proc/BUILD +++ b/pkg/shim/v1/proc/BUILD @@ -1,4 +1,4 @@ -load("//tools:defs.bzl", "go_library") +load("//tools:defs.bzl", "go_library", "go_test") package( default_applicable_licenses = ["//:license"], @@ -40,3 +40,12 @@ go_library( "@org_golang_x_sys//unix:go_default_library", ], ) + +go_test( + name = "proc_test", + srcs = ["init_test.go"], + library = ":proc", + deps = [ + "//pkg/shim/v1/runsccmd", + ], +) diff --git a/pkg/shim/v1/proc/init.go b/pkg/shim/v1/proc/init.go index 16413d7072..4c77819c0a 100644 --- a/pkg/shim/v1/proc/init.go +++ b/pkg/shim/v1/proc/init.go @@ -252,11 +252,7 @@ func (p *Init) start(ctx context.Context, restoreConf *extension.RestoreConfig) } go func() { status, err := p.runtime.Wait(context.Background(), p.id) - if err != nil { - log.G(ctx).WithError(err).Errorf("Failed to wait for container %q", p.id) - p.killAllLocked(ctx) - status = internalErrorCode - } + status = adjustWaitStatus(ctx, p, status, err) ExitCh <- Exit{ Timestamp: time.Now(), ID: p.id, @@ -485,6 +481,23 @@ func (p *Init) convertStatus(status string) string { return status } +// adjustWaitStatus handles the exit status from runtime.Wait. When status is +// 0 but an error is returned (e.g., "sandbox no longer running" for short-lived +// containers), the error is benign and the status is preserved. For non-zero +// status with an error, the container is killed and internalErrorCode is used. +func adjustWaitStatus(ctx context.Context, p *Init, status int, err error) int { + if err == nil { + return status + } + if status == 0 { + log.G(ctx).WithError(err).Warnf("Container %q wait error with exit status 0 (ignoring)", p.id) + return 0 + } + log.G(ctx).WithError(err).Errorf("Failed to wait for container %q", p.id) + p.killAllLocked(ctx) + return internalErrorCode +} + func withConditionalIO(c stdio.Stdio) runc.IOOpt { return func(o *runc.IOOption) { o.OpenStdin = c.Stdin != "" diff --git a/pkg/shim/v1/proc/init_test.go b/pkg/shim/v1/proc/init_test.go new file mode 100644 index 0000000000..d03006933c --- /dev/null +++ b/pkg/shim/v1/proc/init_test.go @@ -0,0 +1,102 @@ +// Copyright 2026 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package proc + +import ( + "context" + "fmt" + "testing" + + "gvisor.dev/gvisor/pkg/shim/v1/runsccmd" +) + +func TestAdjustWaitStatus(t *testing.T) { + ctx := context.Background() + + for _, tc := range []struct { + name string + status int + err error + wantStatus int + }{ + { + name: "success with no error", + status: 0, + err: nil, + wantStatus: 0, + }, + { + name: "non-zero exit with no error", + status: 1, + err: nil, + wantStatus: 1, + }, + { + name: "signal exit with no error", + status: 137, + err: nil, + wantStatus: 137, + }, + { + name: "success with benign error (short-lived container)", + status: 0, + err: fmt.Errorf("sandbox no longer running and its exit status is unavailable"), + wantStatus: 0, + }, + { + name: "success with any error preserves zero status", + status: 0, + err: fmt.Errorf("some unexpected error"), + wantStatus: 0, + }, + { + name: "non-zero exit with error becomes internalErrorCode", + status: 1, + err: fmt.Errorf("wait failed"), + wantStatus: internalErrorCode, + }, + { + name: "signal exit with error becomes internalErrorCode", + status: 137, + err: fmt.Errorf("wait failed"), + wantStatus: internalErrorCode, + }, + { + name: "negative status with error becomes internalErrorCode", + status: -1, + err: fmt.Errorf("wait failed unexpectedly"), + wantStatus: internalErrorCode, + }, + { + name: "exit 255 with no error passes through", + status: 255, + err: nil, + wantStatus: 255, + }, + } { + t.Run(tc.name, func(t *testing.T) { + // Create a minimal Init with a dummy runtime to prevent nil + // dereference in killAllLocked for the error path. + p := &Init{ + id: "test-container", + runtime: &runsccmd.Runsc{Command: "false"}, + } + got := adjustWaitStatus(ctx, p, tc.status, tc.err) + if got != tc.wantStatus { + t.Errorf("adjustWaitStatus(status=%d, err=%v) = %d, want %d", tc.status, tc.err, got, tc.wantStatus) + } + }) + } +} diff --git a/pkg/shim/v1/runsc/BUILD b/pkg/shim/v1/runsc/BUILD index b854a22f36..487ed0eade 100644 --- a/pkg/shim/v1/runsc/BUILD +++ b/pkg/shim/v1/runsc/BUILD @@ -63,6 +63,7 @@ go_test( library = ":runsc", deps = [ "//pkg/shim/v1/utils", + "//runsc/specutils", "@com_github_opencontainers_runtime_spec//specs-go:go_default_library", ], ) diff --git a/pkg/shim/v1/runsc/service.go b/pkg/shim/v1/runsc/service.go index cb0b44ca9d..c52aaee2bd 100644 --- a/pkg/shim/v1/runsc/service.go +++ b/pkg/shim/v1/runsc/service.go @@ -656,7 +656,11 @@ func newInit(workDir, namespace string, platform stdio.Platform, r *proc.CreateC p.WorkDir = workDir p.IoUID = int(options.IoUID) p.IoGID = int(options.IoGID) - p.Sandbox = specutils.SpecContainerType(spec) == specutils.ContainerTypeSandbox + // Non-CRI callers (BuildKit, ctr) don't set the container type + // annotation, so SpecContainerType returns ContainerTypeUnspecified. + // These are always root containers that need IO passed to runsc create. + ct := specutils.SpecContainerType(spec) + p.Sandbox = ct == specutils.ContainerTypeSandbox || ct == specutils.ContainerTypeUnspecified p.UserLog = utils.UserLogPath(spec) p.Monitor = reaper.Default return p, nil diff --git a/pkg/shim/v1/runsc/service_test.go b/pkg/shim/v1/runsc/service_test.go index 30543f37ab..b720c6b511 100644 --- a/pkg/shim/v1/runsc/service_test.go +++ b/pkg/shim/v1/runsc/service_test.go @@ -19,6 +19,7 @@ import ( specs "github.com/opencontainers/runtime-spec/specs-go" "gvisor.dev/gvisor/pkg/shim/v1/utils" + "gvisor.dev/gvisor/runsc/specutils" ) func TestCgroupPath(t *testing.T) { @@ -90,6 +91,85 @@ func TestCgroupPath(t *testing.T) { } } +func TestSandboxDetection(t *testing.T) { + for _, tc := range []struct { + name string + annotations map[string]string + wantSandbox bool + }{ + { + name: "no-annotation (non-CRI caller like BuildKit or ctr)", + annotations: nil, + wantSandbox: true, + }, + { + name: "empty-annotations", + annotations: map[string]string{}, + wantSandbox: true, + }, + { + name: "containerd-sandbox", + annotations: map[string]string{ + specutils.ContainerdContainerTypeAnnotation: specutils.ContainerdContainerTypeSandbox, + }, + wantSandbox: true, + }, + { + name: "containerd-container", + annotations: map[string]string{ + specutils.ContainerdContainerTypeAnnotation: specutils.ContainerdContainerTypeContainer, + }, + wantSandbox: false, + }, + { + name: "crio-sandbox", + annotations: map[string]string{ + specutils.CRIOContainerTypeAnnotation: specutils.CRIOContainerTypeSandbox, + }, + wantSandbox: true, + }, + { + name: "crio-container", + annotations: map[string]string{ + specutils.CRIOContainerTypeAnnotation: specutils.CRIOContainerTypeContainer, + }, + wantSandbox: false, + }, + { + name: "unknown-annotation-value", + annotations: map[string]string{ + specutils.ContainerdContainerTypeAnnotation: "unknown-value", + }, + wantSandbox: false, + }, + { + name: "both-annotations-container-wins", + annotations: map[string]string{ + specutils.ContainerdContainerTypeAnnotation: specutils.ContainerdContainerTypeContainer, + specutils.CRIOContainerTypeAnnotation: specutils.CRIOContainerTypeSandbox, + }, + // containerd annotation is checked first + wantSandbox: false, + }, + { + name: "unrelated-annotations-treated-as-unspecified", + annotations: map[string]string{ + "some.other.annotation": "value", + }, + wantSandbox: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + spec := &specs.Spec{Annotations: tc.annotations} + ct := specutils.SpecContainerType(spec) + got := ct == specutils.ContainerTypeSandbox || ct == specutils.ContainerTypeUnspecified + if got != tc.wantSandbox { + t.Errorf("sandbox detection for %v: got %v, want %v (containerType=%v)", tc.annotations, got, tc.wantSandbox, ct) + } + }) + } +} + // Test cases that cgroup path should not be updated. func TestCgroupNoUpdate(t *testing.T) { for _, tc := range []struct {