diff --git a/internal/erofs/vmdk.go b/internal/erofs/vmdk.go index 36e85eff..0cc148c0 100644 --- a/internal/erofs/vmdk.go +++ b/internal/erofs/vmdk.go @@ -33,6 +33,64 @@ const ( hwVersion = "4" ) +// The names the shim writes into a bundle directory. They live here, beside +// the writers that derive them, so that shim teardown — which has to recognise +// and remove them — cannot drift from what was written. +// +// There is deliberately no "_tail.bin" suffix: the GPT disk is synthetic and +// read-only, so no secondary GPT is written. +const ( + bundleDescriptorPrefix = "merged_fs" + GPTDescriptorName = bundleDescriptorPrefix + "_gpt.vmdk" + + auxHeaderSuffix = "_header.bin" + auxPadSuffix = "_pad.bin" +) + +var auxSuffixes = []string{auxHeaderSuffix, auxPadSuffix} + +// FlatDescriptorName is the basename of the flat-concat VMDK descriptor for a +// multi-device erofs mount assigned the given disk letter. The letter keeps +// several such mounts in one bundle from overwriting each other. +func FlatDescriptorName(letter byte) string { + return fmt.Sprintf("%s_%c.vmdk", bundleDescriptorPrefix, letter) +} + +// auxFilePath names one auxiliary blob after its descriptor, replacing the +// descriptor's extension with suffix. +func auxFilePath(vmdkPath, suffix string) string { + dir := filepath.Dir(vmdkPath) + base := strings.TrimSuffix(filepath.Base(vmdkPath), filepath.Ext(vmdkPath)) + return filepath.Join(dir, base+suffix) +} + +// IsBundleArtifact reports whether name — a bare directory entry name, not a +// path — is a descriptor or auxiliary blob the shim wrote into a bundle. +// +// Shim teardown removes these before containerd deletes the bundle, because on +// Windows a descriptor extent still mapped by the VM cannot be unlinked, and one +// leftover file makes containerd's recursive bundle removal fail — permanently, +// since the stale bundle then also blocks every later start of that container. +// +// Matching by name rather than globbing is deliberate: a bundle path holding a +// glob metacharacter would make the pattern match nothing and report no error, +// and on Windows it cannot even be escaped, the separator being the escape +// character. +func IsBundleArtifact(name string) bool { + if !strings.HasPrefix(name, bundleDescriptorPrefix) { + return false + } + if filepath.Ext(name) == ".vmdk" { + return true + } + for _, suffix := range auxSuffixes { + if strings.HasSuffix(name, suffix) { + return true + } + } + return false +} + // vmdkDescAddExtent writes extent lines to the writer. // Each extent line follows the format: RW FLAT "" // A single device > 2 GiB is split across multiple RW lines, each referencing @@ -229,9 +287,7 @@ func DumpGPTVMDKDescriptorToFile(vmdkPath string, cid uint32, devices []string) return err } - dir := filepath.Dir(vmdkPath) - base := strings.TrimSuffix(filepath.Base(vmdkPath), filepath.Ext(vmdkPath)) - headerPath := filepath.Join(dir, base+"_header.bin") + headerPath := auxFilePath(vmdkPath, auxHeaderSuffix) if err := writeBlob(headerPath, layout.WriteHeader); err != nil { return err @@ -248,7 +304,7 @@ func DumpGPTVMDKDescriptorToFile(vmdkPath string, cid uint32, devices []string) if !gptUseZeroExtents { // The maximum single padding region is bounded by gptAlignSectors // (1 MiB minus a sector); a 1 MiB pad file always suffices. - padFile = filepath.Join(dir, base+"_pad.bin") + padFile = auxFilePath(vmdkPath, auxPadSuffix) if err := writePadFile(padFile, gptAlignSectors*gptSectorSize); err != nil { cleanup() return err diff --git a/internal/erofs/vmdk_names_test.go b/internal/erofs/vmdk_names_test.go new file mode 100644 index 00000000..4fb6f7ec --- /dev/null +++ b/internal/erofs/vmdk_names_test.go @@ -0,0 +1,134 @@ +/* + Copyright The containerd 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 + + http://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 erofs + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// bundleArtifacts lists the entries of dir that IsBundleArtifact classifies as +// shim-written, as full paths — the same enumeration shim teardown performs. +func bundleArtifacts(t *testing.T, dir string) []string { + t.Helper() + entries, err := os.ReadDir(dir) + require.NoError(t, err) + var matched []string + for _, e := range entries { + if IsBundleArtifact(e.Name()) { + matched = append(matched, filepath.Join(dir, e.Name())) + } + } + return matched +} + +// TestBundleArtifactsMatchWhatIsWritten is the anti-drift test: it writes the +// descriptors the shim actually writes into a bundle and asserts the +// classifier used by teardown matches every one of them. If a writer starts +// emitting a new name, this fails rather than silently leaving a file behind +// that would wedge the container id. +func TestBundleArtifactsMatchWhatIsWritten(t *testing.T) { + t.Cleanup(func() { + gptUseZeroExtents = true + }) + // Disable ZERO extents so the pad file is written too, putting the full set + // of auxiliary blobs in front of the classifier. + gptUseZeroExtents = false + + bundle := t.TempDir() + devices := []string{ + makeLayerFile(t, bundle, "a.img", 4*1024*1024), + makeLayerFile(t, bundle, "b.img", 4*1024*1024), + } + + // The GPT descriptor plus its auxiliary blobs. + gptPath := filepath.Join(bundle, GPTDescriptorName) + require.NoError(t, DumpGPTVMDKDescriptorToFile(gptPath, 0xfffffffe, devices)) + + // A flat-concat descriptor, as written for a multi-device erofs mount. + flatPath := filepath.Join(bundle, FlatDescriptorName('b')) + require.NoError(t, DumpVMDKDescriptorToFile(flatPath, 0xfffffffe, devices)) + + want := []string{ + gptPath, + filepath.Join(bundle, "merged_fs_gpt_header.bin"), + filepath.Join(bundle, "merged_fs_gpt_pad.bin"), + flatPath, + } + for _, p := range want { + require.FileExists(t, p, "expected the writers to produce this file") + } + + matched := bundleArtifacts(t, bundle) + + assert.ElementsMatch(t, want, matched, + "IsBundleArtifact must match exactly the descriptors and blobs written into the bundle") + + // Classification must not sweep up the layer images, which live outside + // the bundle in production and are not the shim's to delete. + for _, dev := range devices { + assert.NotContains(t, matched, dev) + } +} + +// TestIsBundleArtifactWithGlobMetacharactersInPath guards the reason teardown +// enumerates instead of globbing: a bundle directory whose path contains a glob +// metacharacter must still have its artifacts found. filepath.Glob would return +// no matches and no error here, silently leaving the files that wedge the +// container id. +func TestIsBundleArtifactWithGlobMetacharactersInPath(t *testing.T) { + bundle := filepath.Join(t.TempDir(), "state[1]") + require.NoError(t, os.MkdirAll(bundle, 0o755)) + + devices := []string{ + makeLayerFile(t, bundle, "a.img", 4*1024*1024), + makeLayerFile(t, bundle, "b.img", 4*1024*1024), + } + gptPath := filepath.Join(bundle, GPTDescriptorName) + require.NoError(t, DumpGPTVMDKDescriptorToFile(gptPath, 0xfffffffe, devices)) + + assert.ElementsMatch(t, + []string{gptPath, filepath.Join(bundle, "merged_fs_gpt_header.bin")}, + bundleArtifacts(t, bundle)) +} + +func TestBundleArtifactsRemoveEverything(t *testing.T) { + bundle := t.TempDir() + devices := []string{ + makeLayerFile(t, bundle, "a.img", 4*1024*1024), + makeLayerFile(t, bundle, "b.img", 4*1024*1024), + } + require.NoError(t, DumpGPTVMDKDescriptorToFile( + filepath.Join(bundle, GPTDescriptorName), 0xfffffffe, devices)) + + for _, p := range bundleArtifacts(t, bundle) { + require.NoError(t, os.RemoveAll(p)) + } + + entries, err := os.ReadDir(bundle) + require.NoError(t, err) + var left []string + for _, e := range entries { + left = append(left, e.Name()) + } + assert.ElementsMatch(t, []string{"a.img", "b.img"}, left, + "only the layer images should survive; every shim-written artifact must be gone") +} diff --git a/internal/shim/task/mount.go b/internal/shim/task/mount.go index 4f626eaf..d3491d03 100644 --- a/internal/shim/task/mount.go +++ b/internal/shim/task/mount.go @@ -136,7 +136,7 @@ func transformMounts(ctx context.Context, id string, ms []*types.Mount, da *disk // Use the disk letter in the filename so that multiple // multi-device erofs mounts within the same bundle each get // a distinct descriptor and don't overwrite each other. - mergedfsPath := filepath.Join(bundleDir, fmt.Sprintf("merged_fs_%c.vmdk", letter)) + mergedfsPath := filepath.Join(bundleDir, erofs.FlatDescriptorName(letter)) if err := erofs.DumpVMDKDescriptorToFile(mergedfsPath, 0xfffffffe, devices); err != nil { log.G(ctx).WithError(err).WithField("path", mergedfsPath).Warn("failed to generate erofs vmdk descriptor") return nil, nil, fmt.Errorf("erofs vmdk: %w", errdefs.ErrNotImplemented) @@ -268,7 +268,7 @@ func finalizeErofsCandidates(ctx context.Context, id string, da *diskAllocator, // shim does not mutate the source image directories. Cache by // stat-check: setupMounts may be called more than once per bundle // (e.g. on restore) but the layer set for a given bundle is fixed. - gptPath := filepath.Join(bundleDir, "merged_fs_gpt.vmdk") + gptPath := filepath.Join(bundleDir, erofs.GPTDescriptorName) if _, err := os.Stat(gptPath); err != nil { if !os.IsNotExist(err) { log.G(ctx).WithError(err).WithField("path", gptPath).Warn("failed to stat erofs gpt vmdk descriptor") diff --git a/internal/shim/task/service.go b/internal/shim/task/service.go index d3c81998..ffd9be27 100644 --- a/internal/shim/task/service.go +++ b/internal/shim/task/service.go @@ -216,11 +216,49 @@ func (s *service) RegisterTTRPC(server *ttrpc.Server) error { return nil } +// Shutdown is bounded by its caller's context — containerd's shutdown service +// gives it a deadline — so only the best-effort phases carry a timeout of their +// own. Stopping the VM gets whatever time is left, because it is the phase that +// releases the bundle's disk files and a VM left running is the one outcome +// nothing downstream can recover from. +const ( + // Flushing guest journals before power-off is worth attempting, not worth + // the whole shutdown: an unhealthy guest keeps failing transiently for as + // long as it is given, and would leave nothing for the VM stop. + guestUnmountTimeout = 5 * time.Second + + // What the VM stop runs under when the caller's context is spent before it + // is reached, so the one unrecoverable phase still gets a real attempt + // rather than an already-cancelled context. + exhaustedStopBudget = 2 * time.Second + + eventSentinelTimeout = 1 * time.Second +) + func (s *service) shutdown(ctx context.Context) error { s.mu.Lock() defer s.mu.Unlock() var errs []error + // Deferred so that a phase below hanging cannot skip them: without the + // rootfs removal containerd's bundle cleanup attempts a bind filter unmount + // that fails on Windows, and without the sentinel the event forwarder never + // returns. + defer func() { + removeRootfsDir(ctx) + + // Bounded rather than an unconditional send: the forwarder may be + // blocked in Publish or already gone, and waiting on it forever here + // would defeat the point of the deferral. + timer := time.NewTimer(eventSentinelTimeout) + defer timer.Stop() + select { + case s.events <- nil: + case <-timer.C: + log.G(ctx).Warn("timed out sending shutdown sentinel; event forwarder may still be running") + } + }() + for id, c := range s.containers { if err := c.shutdown(ctx); err != nil { errs = append(errs, fmt.Errorf("container %q shutdown: %w", id, err)) @@ -230,28 +268,62 @@ func (s *service) shutdown(ctx context.Context) error { if s.sb != nil { // Unmount all block volumes inside the guest before stopping the VM, // to flush ext4 journals and dirty pages to the virtio-blk devices. - // Best-effort with a short retry for transient EBUSY. + // Best-effort with a short retry for transient EBUSY — bounded, because + // an unhealthy guest keeps failing transiently for as long as it is + // given and would starve the VM stop below. if vmc, err := s.sb.Client(); err != nil { log.G(ctx).WithError(err).Warn("failed to get VM client; skipping unmount of block volumes before VM shutdown") } else { - if err := unmountAllWithRetry(ctx, mountAPI.NewTTRPCMountClient(vmc)); err != nil { - log.G(ctx).WithError(err).Warn("failed to unmount all block volumes before VM shutdown") + unmountCtx, cancel := context.WithTimeout(ctx, guestUnmountTimeout) + err := unmountAllWithRetry(unmountCtx, mountAPI.NewTTRPCMountClient(vmc)) + cancel() + if err != nil { + log.G(ctx).WithError(err). + Warn("failed to unmount all block volumes before VM shutdown") } } - if err := s.sb.Stop(ctx); err != nil { - errs = append(errs, fmt.Errorf("sandbox shutdown: %w", err)) + if err := s.stopSandbox(ctx); err != nil { + errs = append(errs, err) } } - // Remove the rootfs directory on Windows so containerd's bundle cleanup - // doesn't attempt a bind filter unmount (no-op on other platforms). - removeRootfsDir(ctx) + return errors.Join(errs...) +} - // Signal last event and stop forwarding - s.events <- nil +// stopSandbox stops the VM, abandoning the wait if it outlasts its share of the +// shutdown budget. +// +// Stop cannot be relied on to honour its context — it delegates to a vm.Instance +// implementation, and one blocked on an internal handoff is not cancellable from +// here. Abandoning the wait is what keeps the deferred bundle cleanup in +// [service.shutdown] reachable. +func (s *service) stopSandbox(ctx context.Context) error { + stopCtx := ctx + if ctx.Err() != nil { + // An earlier phase spent the caller's budget. Running Stop on the dead + // context would abandon it before the goroutine below even started, so + // give it one of its own — see [exhaustedStopBudget]. + var cancel context.CancelFunc + stopCtx, cancel = context.WithTimeout(context.WithoutCancel(ctx), exhaustedStopBudget) + defer cancel() + log.G(ctx).WithField("stop_budget", exhaustedStopBudget). + Warn("shutdown budget spent before stopping the VM; stopping it anyway") + } - return errors.Join(errs...) + done := make(chan error, 1) + go func() { done <- s.sb.Stop(stopCtx) }() + + select { + case err := <-done: + if err != nil { + return fmt.Errorf("sandbox shutdown: %w", err) + } + return nil + case <-stopCtx.Done(): + log.G(ctx).Error("gave up waiting for the VM to stop; it may still hold files in the bundle, which will prevent containerd from deleting the bundle and block subsequent starts of this container") + return fmt.Errorf("sandbox shutdown: %w", stopCtx.Err()) + } } // unmountAllWithRetry asks the guest to unmount all tracked mounts, retrying diff --git a/internal/shim/task/service_shutdown_test.go b/internal/shim/task/service_shutdown_test.go new file mode 100644 index 00000000..33d8e458 --- /dev/null +++ b/internal/shim/task/service_shutdown_test.go @@ -0,0 +1,274 @@ +/* + Copyright The containerd 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 + + http://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 task + +import ( + "context" + "errors" + "net" + "testing" + "time" + + "github.com/containerd/ttrpc" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/containerd/nerdbox/internal/shim/sandbox" +) + +// fakeSandbox is a sandbox.Sandbox whose Stop behaviour is controlled by the +// test. Client always fails, which makes service.shutdown skip the guest +// unmount phase and go straight to stopping the VM — the phase under test. +type fakeSandbox struct { + // stopBlocks, when true, makes Stop block until released is closed, + // ignoring its context. This models a vm.Instance implementation that + // blocks on an internal handoff and cannot be cancelled. + stopBlocks bool + released chan struct{} + + stopErr error + stopCalled chan struct{} + // stopCtxErr records ctx.Err() as observed on entry to Stop, so a test can + // assert Stop was not handed an already-dead context. + stopCtxErr error +} + +func newFakeSandbox() *fakeSandbox { + return &fakeSandbox{ + released: make(chan struct{}), + stopCalled: make(chan struct{}, 1), + } +} + +func (f *fakeSandbox) Start(context.Context, ...sandbox.Opt) error { return nil } + +func (f *fakeSandbox) Stop(ctx context.Context) error { + f.stopCtxErr = ctx.Err() + select { + case f.stopCalled <- struct{}{}: + default: + } + if f.stopBlocks { + // Deliberately ignores ctx. + <-f.released + } + return f.stopErr +} + +func (f *fakeSandbox) Client() (*ttrpc.Client, error) { + return nil, errors.New("no VM client in test") +} + +func (f *fakeSandbox) StartStream(context.Context, string) (net.Conn, error) { + return nil, errors.New("not implemented") +} + +func (f *fakeSandbox) ReservedDisks() int { return 0 } + +// newTestService builds a service with the given sandbox, wired up the way +// NewTaskService does but without the shim framework. +func newTestService(sb sandbox.Sandbox) *service { + return &service{ + context: context.Background(), + sb: sb, + events: make(chan any, 128), + containers: make(map[string]*container), + } +} + +// TestShutdownReturnsWhenVMStopHangs is the regression test for the wedge: +// a VM that never finishes stopping must not hold shutdown open, because the +// steps after it are what let containerd delete the bundle. Before the fix, +// service.shutdown blocked in sb.Stop forever and never reached them. +func TestShutdownReturnsWhenVMStopHangs(t *testing.T) { + sb := newFakeSandbox() + sb.stopBlocks = true + t.Cleanup(func() { close(sb.released) }) + + s := newTestService(sb) + + // A deadline stands in for the shutdown service's overall budget. + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + done := make(chan error, 1) + go func() { done <- s.shutdown(ctx) }() + + select { + case err := <-done: + // The hang must be reported, not swallowed. + require.Error(t, err) + assert.Contains(t, err.Error(), "sandbox shutdown") + case <-time.After(5 * time.Second): + t.Fatal("service.shutdown did not return while sb.Stop was blocked") + } + + // Stop was actually attempted. + select { + case <-sb.stopCalled: + default: + t.Error("expected sb.Stop to have been called") + } + + // The forwarder sentinel must still have been sent, so the event + // forwarding goroutine can exit. + select { + case ev := <-s.events: + assert.Nil(t, ev, "expected the nil shutdown sentinel") + default: + t.Error("shutdown sentinel was not sent when sb.Stop hung") + } +} + +// TestShutdownStopWaitEndsWithCallerContext pins where the VM stop's patience +// comes from: the caller's deadline, not a timeout of its own. It should wait +// out the context it was given, then stop waiting. +func TestShutdownStopWaitEndsWithCallerContext(t *testing.T) { + sb := newFakeSandbox() + sb.stopBlocks = true + t.Cleanup(func() { close(sb.released) }) + + s := newTestService(sb) + + const budget = time.Second + ctx, cancel := context.WithTimeout(context.Background(), budget) + defer cancel() + + start := time.Now() + err := s.shutdown(ctx) + elapsed := time.Since(start) + + require.Error(t, err) + assert.GreaterOrEqual(t, elapsed, budget, + "the VM stop should wait out the caller's context, not give up early") + // Generous upper bound: the point is that it stops waiting, not that it is + // prompt to the millisecond. + assert.Less(t, elapsed, budget+5*time.Second, + "the VM stop should not outlive the caller's context") +} + +// TestShutdownStopsVMWhenBudgetAlreadyExhausted covers the case where an +// earlier phase (a container IO shutdown waiting out the whole budget on a hung +// guest) leaves no time on the clock. Stopping the VM is what releases the +// bundle's disk files, so it must still be attempted with a live context rather +// than skipped — otherwise the VM survives and wedges the bundle, which is the +// failure the bounded shutdown exists to prevent. +func TestShutdownStopsVMWhenBudgetAlreadyExhausted(t *testing.T) { + sb := newFakeSandbox() + s := newTestService(sb) + + // An already-expired budget. + ctx, cancel := context.WithTimeout(context.Background(), -time.Second) + defer cancel() + + require.NoError(t, s.shutdown(ctx)) + + select { + case <-sb.stopCalled: + default: + t.Fatal("sb.Stop was not called when the shutdown budget was already spent") + } + assert.NoError(t, sb.stopCtxErr, "sb.Stop must not be handed an already-cancelled context") +} + +func TestShutdownSucceedsWhenVMStops(t *testing.T) { + sb := newFakeSandbox() + s := newTestService(sb) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + require.NoError(t, s.shutdown(ctx)) + + select { + case ev := <-s.events: + assert.Nil(t, ev) + default: + t.Error("shutdown sentinel was not sent") + } +} + +func TestShutdownPropagatesVMStopError(t *testing.T) { + sb := newFakeSandbox() + sb.stopErr = errors.New("boom") + s := newTestService(sb) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + err := s.shutdown(ctx) + require.Error(t, err) + assert.ErrorContains(t, err, "boom") +} + +// TestShutdownSentinelDoesNotBlockOnFullChannel guards the other way shutdown +// used to be able to wedge: an unconditional send on a full event channel with +// no forwarder draining it. +func TestShutdownSentinelDoesNotBlockOnFullChannel(t *testing.T) { + sb := newFakeSandbox() + s := newTestService(sb) + + // Fill the event channel so the sentinel send cannot proceed. + for len(s.events) < cap(s.events) { + s.events <- struct{}{} + } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + done := make(chan struct{}) + go func() { + defer close(done) + _ = s.shutdown(ctx) + }() + + select { + case <-done: + case <-time.After(eventSentinelTimeout + 3*time.Second): + t.Fatal("service.shutdown blocked sending the sentinel on a full event channel") + } +} + +// TestShutdownWaitsIndefinitelyWithoutDeadline documents the consequence of +// deferring to the caller: given a context with no deadline, the VM stop waits. +// The shim framework always supplies one, so this is the contract, not a hazard. +func TestShutdownWaitsIndefinitelyWithoutDeadline(t *testing.T) { + sb := newFakeSandbox() + sb.stopBlocks = true + + s := newTestService(sb) + + done := make(chan struct{}) + go func() { + defer close(done) + _ = s.shutdown(context.Background()) + }() + + select { + case <-done: + t.Fatal("shutdown returned while sb.Stop was still blocked and no deadline was set") + case <-time.After(500 * time.Millisecond): + } + + // Releasing Stop lets it finish, proving it was waiting on Stop and nothing else. + close(sb.released) + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("shutdown did not return after sb.Stop was released") + } +} diff --git a/pkg/shim/manager/manager_unix.go b/pkg/shim/manager/manager_unix.go index 81b3301d..b785e379 100644 --- a/pkg/shim/manager/manager_unix.go +++ b/pkg/shim/manager/manager_unix.go @@ -37,6 +37,7 @@ import ( "github.com/containerd/containerd/v2/pkg/namespaces" "github.com/containerd/containerd/v2/pkg/shim" "github.com/containerd/errdefs" + "github.com/containerd/log" "golang.org/x/sys/unix" ) @@ -292,11 +293,28 @@ func (manager) Start(ctx context.Context, bparams *bootapi.BootstrapParams) (_ * }, nil } +const ( + // Bounds the wait for the shim to release its shim.pid lock after SIGKILL, + // leaving slack inside containerd's "io.containerd.timeout.shim.cleanup" + // (5s by default), which bounds the whole `shim delete` binary call. Being + // killed by containerd instead would report no exit status at all. + shimExitWaitTimeout = 3 * time.Second + + shimExitPollInterval = 50 * time.Millisecond +) + func (manager) Stop(ctx context.Context, id string) (shim.StopStatus, error) { - pid, err := waitForShimPidLock() + pid, exited, err := waitForShimPidLock(ctx) if err != nil { return shim.StopStatus{}, err } + if !exited { + log.G(ctx).WithFields(log.Fields{ + "pid": pid, + "id": id, + "timeout": shimExitWaitTimeout, + }).Error("shim process did not exit before the wait budget expired; it may still hold bundle files") + } return shim.StopStatus{ ExitedAt: time.Now(), ExitStatus: 128 + int(unix.SIGKILL), @@ -304,46 +322,89 @@ func (manager) Stop(ctx context.Context, id string) (shim.StopStatus, error) { }, nil } -// waitForShimPidLock opens shim.pid, reads the shim PID, and blocks until the -// shim process releases the exclusive flock it holds on that file (i.e. until -// the shim exits). It returns the PID on success. +// waitForShimPidLock SIGKILLs the shim if it is still running and waits for it +// to release the exclusive flock on shim.pid, which the OS does only once it +// exits. It reports the shim's PID and whether that exit was observed. // -// The shim is the only process that acquires the lock, so after SIGKILL it will -// inevitably exit and release it. Blocking unconditionally — rather than racing -// against a timeout — guarantees the caller does not return while the shim is -// still alive. -func waitForShimPidLock() (int, error) { +// The shim is the only process that takes the lock, so after SIGKILL it will +// almost always exit promptly; the wait is bounded anyway, and reports a shim +// that outlives its budget rather than blocking on it. See [shimExitWaitTimeout]. +func waitForShimPidLock(ctx context.Context) (int, bool, error) { f, err := os.Open("shim.pid") if err != nil { - return 0, err + return 0, false, err } defer f.Close() p, err := io.ReadAll(f) if err != nil { - return 0, err + return 0, false, err } pid, err := strconv.Atoi(strings.TrimSpace(string(p))) if err != nil { - return 0, err + return 0, false, err + } + // Reject non-positive pids before they reach Kill: kill(0) signals every + // process in the caller's process group and kill(-n) a whole other group, + // so a truncated or zeroed shim.pid would take down the `shim delete` + // invocation (and whatever else shares its group) instead of the shim. + if pid <= 0 { + return 0, false, fmt.Errorf("invalid shim pid %d in shim.pid", pid) + } + + // tryLock reports whether the lock is now free, i.e. the shim has exited. + tryLock := func() (bool, error) { + err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) + if err == nil { + return true, nil + } + if errors.Is(err, syscall.EWOULDBLOCK) { + return false, nil + } + return false, fmt.Errorf("flock shim.pid: %w", err) } // Try a non-blocking acquire first. If it succeeds the shim has already // exited and the lock is free. - if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err == nil { - return pid, nil - } else if !errors.Is(err, syscall.EWOULDBLOCK) { - return 0, fmt.Errorf("flock shim.pid: %w", err) + free, err := tryLock() + if err != nil { + return 0, false, err + } + if free { + return pid, true, nil } - // Lock is held — shim is still running. Kill it and block until the OS - // releases the lock on shim exit. There is no timeout: the shim cannot - // hold the lock after it dies, and returning early would leave it alive. + // Lock is held — the shim is still running. Kill it, then poll for the + // lock rather than blocking on LOCK_EX, so the wait stays bounded. if kerr := unix.Kill(pid, unix.SIGKILL); kerr != nil && !errors.Is(kerr, unix.ESRCH) { - return 0, fmt.Errorf("kill shim: %w", kerr) + return 0, false, fmt.Errorf("kill shim: %w", kerr) + } + + deadline := time.Now().Add(shimExitWaitTimeout) + if dl, ok := ctx.Deadline(); ok && dl.Before(deadline) { + deadline = dl } - if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX); err != nil { - return 0, fmt.Errorf("flock shim.pid (wait): %w", err) + for { + free, err := tryLock() + if err != nil { + return 0, false, err + } + if free { + return pid, true, nil + } + + remaining := time.Until(deadline) + if remaining <= 0 { + return pid, false, nil + } + wait := shimExitPollInterval + if remaining < wait { + wait = remaining + } + select { + case <-ctx.Done(): + return pid, false, nil + case <-time.After(wait): + } } - return pid, nil } diff --git a/pkg/shim/manager/manager_windows.go b/pkg/shim/manager/manager_windows.go index 4979f48d..96052976 100644 --- a/pkg/shim/manager/manager_windows.go +++ b/pkg/shim/manager/manager_windows.go @@ -38,6 +38,8 @@ import ( "github.com/containerd/containerd/v2/pkg/shim" "github.com/containerd/log" "golang.org/x/sys/windows" + + "github.com/containerd/nerdbox/internal/erofs" ) func newCommand(ctx context.Context, id, containerdAddress, containerdTTRPCAddress string, debug bool) (*exec.Cmd, error) { @@ -164,6 +166,17 @@ const ( shimPipeReadyTimeout = 10 * time.Second shimPipeDialPerAttempt = 1 * time.Second shimPipeRetryDelay = 10 * time.Millisecond + + // Stop waits for the shim to exit and then clears the bundle. The sum of + // the two budgets must leave slack inside containerd's + // "io.containerd.timeout.shim.cleanup" (5s by default), which bounds the + // whole `shim delete` binary call: if containerd kills us instead, the + // deferred bundle cleanup never runs at all. + shimExitWaitTimeout = 2 * time.Second + bundleRemoveWindow = 1 * time.Second + + shimExitPollInterval = 50 * time.Millisecond + bundleRemoveRetryDelay = 200 * time.Millisecond ) // waitForShimPipe polls a named pipe address with a short per-attempt DialPipe timeout @@ -263,23 +276,111 @@ func bundlePath(ctx context.Context) string { return "" } -// removeRootfs removes the rootfs directory from the bundle so that -// containerd's bundle cleanup doesn't attempt a bind filter unmount. -// On Windows, Unmount calls bindfilter.RemoveFileBinding which fails with -// ERROR_ACCESS_DENIED on directories that were never bind filter mounts -// (nerdbox uses VM-based virtio block devices instead). Removing the -// directory makes UnmountAll a no-op. -func removeRootfs(ctx context.Context) { - if bp := bundlePath(ctx); bp != "" { - os.RemoveAll(filepath.Join(bp, "rootfs")) +// removeBundleArtifacts removes everything the shim itself put in the bundle +// directory, leaving containerd's own bundle cleanup nothing to trip over. Two +// Windows failure modes make it necessary: Unmount calls +// bindfilter.RemoveFileBinding, which fails with ERROR_ACCESS_DENIED on a rootfs +// that was never a bind filter mount (nerdbox uses virtio block devices +// instead), and a VMDK extent still mapped by the VM cannot be unlinked at all — +// see [erofs.IsBundleArtifact]. +func removeBundleArtifacts(ctx context.Context) { + bp := bundlePath(ctx) + if bp == "" { + return + } + + targets := []string{filepath.Join(bp, "rootfs")} + entries, err := os.ReadDir(bp) + if err != nil { + log.G(ctx).WithError(err).WithField("bundle", bp). + Error("failed to list bundle directory; shim-written artifacts may be left behind") + } + for _, entry := range entries { + if erofs.IsBundleArtifact(entry.Name()) { + targets = append(targets, filepath.Join(bp, entry.Name())) + } + } + + // A shim being terminated can hold its mappings for a moment after + // TerminateProcess returns, so retry — but over the whole remaining set + // under one deadline. Retrying per target would multiply by the target + // count and could push this call past containerd's cleanup timeout, which + // is the very failure being fixed. + deadline := time.Now().Add(bundleRemoveWindow) + if dl, ok := ctx.Deadline(); ok && dl.Before(deadline) { + deadline = dl + } + + var failures map[string]error + for { + var remaining []string + failures = make(map[string]error, len(targets)) + for _, target := range targets { + if err := os.RemoveAll(target); err != nil { + failures[target] = err + remaining = append(remaining, target) + } + } + targets = remaining + if len(targets) == 0 || time.Until(deadline) <= bundleRemoveRetryDelay { + break + } + time.Sleep(bundleRemoveRetryDelay) + } + + // Name the survivors: this is the file that will wedge subsequent starts. + for _, target := range targets { + log.G(ctx).WithError(failures[target]).WithField("path", target). + Error("failed to remove bundle artifact; containerd bundle cleanup and subsequent starts of this container will fail until it is released") + } +} + +// waitForProcessExit blocks until the process handle h is signalled, ctx is +// done, or timeout elapses — whichever comes first. It reports whether the +// process exited. +// +// The wait is polled rather than a single WaitForSingleObject(INFINITE) so it +// can honour ctx: containerd bounds the `shim delete` binary call, and being +// killed mid-call would skip our deferred bundle cleanup. +func waitForProcessExit(ctx context.Context, h windows.Handle, timeout time.Duration) bool { + deadline := time.Now().Add(timeout) + if dl, ok := ctx.Deadline(); ok && dl.Before(deadline) { + deadline = dl + } + + for { + remaining := time.Until(deadline) + if remaining <= 0 { + return false + } + if err := ctx.Err(); err != nil { + return false + } + + wait := shimExitPollInterval + if remaining < wait { + wait = remaining + } + // WaitForSingleObject returns WAIT_OBJECT_0 (0) when the process has + // exited and WAIT_TIMEOUT when the interval elapsed first. + event, err := windows.WaitForSingleObject(h, uint32(wait.Milliseconds())) + if err != nil { + return false + } + if event == uint32(windows.WAIT_OBJECT_0) { + return true + } } } func (manager) Stop(ctx context.Context, id string) (shim.StopStatus, error) { // must run on all exits (including when the process is already gone) - // to ensure containerd's bundle cleanup is successful. See [removeRootfs] - // for more details. - defer removeRootfs(ctx) + // to ensure containerd's bundle cleanup is successful. See + // [removeBundleArtifacts] for more details. Every wait below is bounded so + // that this defer actually gets to run — containerd kills the `shim delete` + // call once "io.containerd.timeout.shim.cleanup" expires, and a killed + // process runs no defers. + defer removeBundleArtifacts(ctx) p, err := os.ReadFile(filepath.Join(bundlePath(ctx), "shim.pid")) if err != nil { @@ -326,11 +427,24 @@ func (manager) Stop(ctx context.Context, id string) (shim.StopStatus, error) { return shim.StopStatus{}, fmt.Errorf("terminate shim process: %w", err) } - // Block until the process has fully exited. There is no timeout: the - // shim is the only target and TerminateProcess is unconditional, so - // WaitForSingleObject will always complete. - if _, err := windows.WaitForSingleObject(h, windows.INFINITE); err != nil { - return shim.StopStatus{}, fmt.Errorf("wait for shim process: %w", err) + // Wait for the process to fully exit, but only for a bounded period. + // TerminateProcess is not instantaneous: a VM-backed shim can have threads + // parked in kernel-mode hypervisor or memory-mapping calls, and the process + // does not die until those return. containerd bounds this whole binary call + // by "io.containerd.timeout.shim.cleanup" (5s), so waiting forever means + // being killed and skipping the bundle cleanup deferred above — which is + // what leaves a locked VMDK extent behind and wedges the container id. + // + // Report success either way: containerd proceeds to delete the bundle + // regardless of what we return here, so the useful thing is to finish under + // our own control with the cleanup done, and to say loudly when the shim + // outlived us. + if !waitForProcessExit(ctx, h, shimExitWaitTimeout) { + log.G(ctx).WithFields(log.Fields{ + "pid": pid, + "id": id, + "timeout": shimExitWaitTimeout, + }).Error("shim process did not exit before the wait budget expired; it may still hold bundle files") } return shim.StopStatus{ diff --git a/pkg/shim/manager/manager_windows_stop_test.go b/pkg/shim/manager/manager_windows_stop_test.go new file mode 100644 index 00000000..bea95596 --- /dev/null +++ b/pkg/shim/manager/manager_windows_stop_test.go @@ -0,0 +1,335 @@ +//go:build windows + +/* + Copyright The containerd 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 + + http://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 manager + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strconv" + "testing" + "time" + + "github.com/containerd/containerd/v2/pkg/shim" + "golang.org/x/sys/windows" +) + +// bundleCtx returns a context carrying bundle path dir, the way the shim +// framework supplies it from the -bundle flag. +func bundleCtx(dir string) context.Context { + return context.WithValue(context.Background(), shim.OptsKey{}, shim.Opts{BundlePath: dir}) +} + +// seedBundle populates dir with the artifacts the shim writes into a bundle, +// plus a file that must survive cleanup. It returns the paths that must be +// removed and the paths that must remain. +func seedBundle(t *testing.T, dir string) (removed, kept []string) { + t.Helper() + + rootfs := filepath.Join(dir, "rootfs") + if err := os.MkdirAll(filepath.Join(rootfs, "nested"), 0o755); err != nil { + t.Fatalf("mkdir rootfs: %v", err) + } + removed = append(removed, rootfs) + + for _, name := range []string{ + "merged_fs_gpt.vmdk", + "merged_fs_gpt_header.bin", + "merged_fs_gpt_pad.bin", + "merged_fs_b.vmdk", + } { + p := filepath.Join(dir, name) + if err := os.WriteFile(p, []byte("x"), 0o644); err != nil { + t.Fatalf("write %s: %v", name, err) + } + removed = append(removed, p) + } + + // containerd owns these; cleanup must not touch them. + for _, name := range []string{"config.json", "shim.pid", "bootstrap.json"} { + p := filepath.Join(dir, name) + if err := os.WriteFile(p, []byte("1"), 0o644); err != nil { + t.Fatalf("write %s: %v", name, err) + } + kept = append(kept, p) + } + + return removed, kept +} + +func assertBundleCleaned(t *testing.T, removed, kept []string) { + t.Helper() + for _, p := range removed { + if _, err := os.Stat(p); !os.IsNotExist(err) { + t.Errorf("expected %s to be removed, stat err = %v", filepath.Base(p), err) + } + } + for _, p := range kept { + if _, err := os.Stat(p); err != nil { + t.Errorf("expected %s to survive cleanup: %v", filepath.Base(p), err) + } + } +} + +// TestRemoveBundleArtifacts verifies that cleanup removes the rootfs directory +// and every VMDK descriptor and auxiliary blob the shim wrote, and nothing else. +// A single leftover descriptor is enough to make containerd's bundle removal +// fail and wedge the container id, so the coverage here is deliberately exact. +func TestRemoveBundleArtifacts(t *testing.T) { + dir := t.TempDir() + removed, kept := seedBundle(t, dir) + + removeBundleArtifacts(bundleCtx(dir)) + + assertBundleCleaned(t, removed, kept) +} + +func TestRemoveBundleArtifactsNoBundlePath(t *testing.T) { + // Must not panic or touch the working directory when the context carries + // no bundle path. + removeBundleArtifacts(context.Background()) +} + +// TestRemoveBundleArtifactsBudgetIsGlobal guards the sum that matters: cleanup +// must stay inside bundleRemoveWindow no matter how many artifacts are locked, +// because Stop's total (shim wait + cleanup) has to fit inside containerd's 5s +// cleanup timeout. Retrying per-target instead would multiply by target count. +func TestRemoveBundleArtifactsBudgetIsGlobal(t *testing.T) { + dir := t.TempDir() + + // Many artifacts, all of them undeletable: a directory that is not empty + // and whose child is held open cannot be removed on Windows. + var held []*os.File + for _, name := range []string{ + "merged_fs_gpt.vmdk", "merged_fs_gpt_header.bin", "merged_fs_gpt_pad.bin", + "merged_fs_a.vmdk", "merged_fs_b.vmdk", "merged_fs_c.vmdk", + } { + p := filepath.Join(dir, name) + f, err := os.OpenFile(p, os.O_CREATE|os.O_RDWR, 0o644) + if err != nil { + t.Fatalf("create %s: %v", name, err) + } + held = append(held, f) + } + t.Cleanup(func() { + for _, f := range held { + f.Close() + } + }) + + start := time.Now() + removeBundleArtifacts(bundleCtx(dir)) + elapsed := time.Since(start) + + // Generous headroom for slow CI, but far below targets × window. + if limit := bundleRemoveWindow + 2*time.Second; elapsed > limit { + t.Errorf("cleanup took %s, beyond the %s bound", elapsed, limit) + } + if shimExitWaitTimeout+bundleRemoveWindow >= 5*time.Second { + t.Errorf("shimExitWaitTimeout+bundleRemoveWindow = %s, which does not fit inside containerd's 5s cleanup timeout", + shimExitWaitTimeout+bundleRemoveWindow) + } +} + +// startSleeper launches a process that stays alive for the duration of the +// test and returns its pid and an open handle with terminate/synchronize +// rights. Cleanup kills it. +func startSleeper(t *testing.T) (int, windows.Handle) { + t.Helper() + + cmd := exec.Command("ping", "-n", "60", "127.0.0.1") + if err := cmd.Start(); err != nil { + t.Skipf("cannot start helper process: %v", err) + } + pid := cmd.Process.Pid + + t.Cleanup(func() { + _ = cmd.Process.Kill() + _, _ = cmd.Process.Wait() + }) + + h, err := windows.OpenProcess(windows.PROCESS_TERMINATE|windows.SYNCHRONIZE, false, uint32(pid)) + if err != nil { + t.Fatalf("OpenProcess(%d): %v", pid, err) + } + t.Cleanup(func() { windows.CloseHandle(h) }) + + return pid, h +} + +// TestWaitForProcessExitTimeout is the core regression test: a process that +// does not exit must not hold the wait open. Before the fix this was +// WaitForSingleObject(INFINITE), so containerd killed the whole `shim delete` +// call at its 5s cleanup timeout and the deferred bundle cleanup never ran. +func TestWaitForProcessExitTimeout(t *testing.T) { + _, h := startSleeper(t) + + const budget = 300 * time.Millisecond + start := time.Now() + exited := waitForProcessExit(context.Background(), h, budget) + elapsed := time.Since(start) + + if exited { + t.Error("expected waitForProcessExit to report the process as still running") + } + if elapsed < budget { + t.Errorf("returned after %s, before the %s budget elapsed", elapsed, budget) + } + if elapsed > budget+2*time.Second { + t.Errorf("returned after %s, far beyond the %s budget", elapsed, budget) + } +} + +func TestWaitForProcessExitProcessExits(t *testing.T) { + cmd := exec.Command("cmd.exe", "/c", "exit", "0") + if err := cmd.Start(); err != nil { + t.Skipf("cannot start helper process: %v", err) + } + pid := cmd.Process.Pid + + h, err := windows.OpenProcess(windows.PROCESS_TERMINATE|windows.SYNCHRONIZE, false, uint32(pid)) + if err != nil { + // The process may already have exited and been reaped; nothing to wait on. + t.Skipf("OpenProcess(%d): %v", pid, err) + } + defer windows.CloseHandle(h) + defer func() { _, _ = cmd.Process.Wait() }() + + if !waitForProcessExit(context.Background(), h, shimExitWaitTimeout) { + t.Error("expected waitForProcessExit to observe the process exiting") + } +} + +// TestWaitForProcessExitCtxCancelled verifies the wait honours context +// cancellation, which is how containerd signals that the cleanup budget is +// spent. +func TestWaitForProcessExitCtxCancelled(t *testing.T) { + _, h := startSleeper(t) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + start := time.Now() + if waitForProcessExit(ctx, h, shimExitWaitTimeout) { + t.Error("expected waitForProcessExit to report failure on a cancelled context") + } + if elapsed := time.Since(start); elapsed > time.Second { + t.Errorf("took %s on a pre-cancelled context; expected a prompt return", elapsed) + } +} + +// TestStopCleansBundleWhenShimAlreadyGone covers the common path: the shim +// exited and removed its own pid file, so Stop has nothing to terminate but +// must still clear the bundle artifacts. +func TestStopCleansBundleWhenShimAlreadyGone(t *testing.T) { + dir := t.TempDir() + removed, kept := seedBundle(t, dir) + + // Drop the pid file so Stop takes the "already exited" branch. + pidFile := filepath.Join(dir, "shim.pid") + if err := os.Remove(pidFile); err != nil { + t.Fatalf("remove shim.pid: %v", err) + } + kept = filterOut(kept, pidFile) + + status, err := manager{}.Stop(bundleCtx(dir), "test-id") + if err != nil { + t.Fatalf("Stop: %v", err) + } + if status.ExitStatus != 128+9 { + t.Errorf("ExitStatus = %d, want %d", status.ExitStatus, 128+9) + } + + assertBundleCleaned(t, removed, kept) +} + +// TestStopCleansBundleWhenPidIsStale covers a stale pid file pointing at a pid +// that is no longer in the process table: Stop must still succeed and clean up +// rather than erroring out and leaving the bundle behind. +func TestStopCleansBundleWhenPidIsStale(t *testing.T) { + dir := t.TempDir() + removed, kept := seedBundle(t, dir) + + // Start and reap a process so its pid is almost certainly gone. + cmd := exec.Command("cmd.exe", "/c", "exit", "0") + if err := cmd.Start(); err != nil { + t.Skipf("cannot start helper process: %v", err) + } + pid := cmd.Process.Pid + _, _ = cmd.Process.Wait() + + if err := os.WriteFile(filepath.Join(dir, "shim.pid"), + []byte(strconv.Itoa(pid)), 0o644); err != nil { + t.Fatalf("write shim.pid: %v", err) + } + + if _, err := (manager{}).Stop(bundleCtx(dir), "test-id"); err != nil { + t.Fatalf("Stop: %v", err) + } + + assertBundleCleaned(t, removed, kept) +} + +// TestStopTerminatesLiveShimAndCleansBundle verifies the full path: a live +// process is terminated, waited for, and the bundle is cleaned. +func TestStopTerminatesLiveShimAndCleansBundle(t *testing.T) { + dir := t.TempDir() + removed, kept := seedBundle(t, dir) + + cmd := exec.Command("ping", "-n", "60", "127.0.0.1") + if err := cmd.Start(); err != nil { + t.Skipf("cannot start helper process: %v", err) + } + t.Cleanup(func() { + _ = cmd.Process.Kill() + _, _ = cmd.Process.Wait() + }) + + if err := os.WriteFile(filepath.Join(dir, "shim.pid"), + []byte(strconv.Itoa(cmd.Process.Pid)), 0o644); err != nil { + t.Fatalf("write shim.pid: %v", err) + } + + start := time.Now() + status, err := manager{}.Stop(bundleCtx(dir), "test-id") + elapsed := time.Since(start) + if err != nil { + t.Fatalf("Stop: %v", err) + } + if status.Pid != cmd.Process.Pid { + t.Errorf("Pid = %d, want %d", status.Pid, cmd.Process.Pid) + } + // The process is killable, so this must not approach the wait budget. + if elapsed > shimExitWaitTimeout { + t.Errorf("Stop took %s, beyond the %s wait budget", elapsed, shimExitWaitTimeout) + } + + assertBundleCleaned(t, removed, kept) +} + +func filterOut(paths []string, drop string) []string { + out := make([]string, 0, len(paths)) + for _, p := range paths { + if p != drop { + out = append(out, p) + } + } + return out +}