From 6830a70e65f9de5826bfb2a52e87936fb7e9f555 Mon Sep 17 00:00:00 2001 From: Jonas Heinle Date: Thu, 6 Aug 2026 11:00:21 +0200 Subject: [PATCH] shim: make container teardown timeouts configurable The shim gives a container 30s to shut down and another 30s to terminate before it stops waiting, DeleteExec waits 30s for resource cleanup, and the delete command waits 30s for a leftover compute system to terminate. All of these limits are hardcoded. Tearing a Windows Server (process isolated) container down is host-side work whose cost scales with how much the container touched the filesystem: the layer filter stack has to be detached and the container's registry hives flushed back into its scratch. For filesystem-heavy workloads such as source builds this takes minutes. We measured 117s for a single OpenCV build container on Windows 11 26200 with ltsc2025 base images (HcsShutDownComputeSystem returned promptly; the completion notification arrived 117s later). When the 30s limits expire mid-flush, the container is terminated while its scratch is still being written. The scratch is then left in a state the platform refuses to export: every subsequent finalize of that snapshot fails with hcsshim::ExportLayer 0x3, and the damage survives fresh snapshots and host reboots. All silo processes do exit, so nothing in the container is at fault and no in-container mitigation helps - verified by overriding WaitToKillServiceTimeout inside the payload and by a full pre-exit teardown of non-essential services, both of which lost the notification identically. Make the limits configurable via environment variables, which the shim inherits from containerd, following the naming of the existing CONTAINERD_SHIM_RUNHCS_V1_WAIT_DEBUGGER: CONTAINERD_SHIM_RUNHCS_V1_TEARDOWN_TIMEOUT each wait in hcsTask.close, and the delete command's wait for termination CONTAINERD_SHIM_RUNHCS_V1_TASK_CLOSE_TIMEOUT the hcsTask.DeleteExec wait The first two are coupled, and the coupling is why the second knob is not simply independent: DeleteExec waits on the channel close() closes, so a task close timeout below close()'s worst case of 2*teardown abandons a teardown that is still progressing - precisely the outcome these knobs exist to prevent. Raising only the teardown timeout would therefore silently not help. When the task close timeout is not set explicitly and the teardown timeout has been raised above the default, the task close timeout is derived to cover close()'s worst case with room to spare. delete.go waits on the same host-side work as hcsTask.close and runs exactly when the shim died with a container still going down, so it takes the same bound rather than a knob of its own. Defaults are unchanged at 30s, so behaviour is identical unless a host opts in. An empty, unparseable or non-positive value is treated as unset, since resolution happens during package initialization, before logging is available, and a bad value must not stop the shim from starting. Also log how long a successful shutdown actually took. That duration is what is needed to size the timeout for a given workload and it was not otherwise observable - the absence of this number is a large part of why the failure above was hard to attribute. Deliberately left alone: the 30s timer in the SIGKILL path, which guards the hosting UVM (ht.host != nil) rather than process isolated teardown; cmd/runhcs, a separate binary that already allows 5 minutes; and cmd/containerd-shim-lcow-v2, which is a different shim. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Jonas Heinle --- cmd/containerd-shim-runhcs-v1/delete.go | 6 +- cmd/containerd-shim-runhcs-v1/task_hcs.go | 92 ++++++++++++++++++- .../task_hcs_test.go | 69 ++++++++++++++ 3 files changed, 163 insertions(+), 4 deletions(-) diff --git a/cmd/containerd-shim-runhcs-v1/delete.go b/cmd/containerd-shim-runhcs-v1/delete.go index c1eb3375fa..7029551820 100644 --- a/cmd/containerd-shim-runhcs-v1/delete.go +++ b/cmd/containerd-shim-runhcs-v1/delete.go @@ -90,7 +90,11 @@ The delete command will be executed in the container's bundle as its cwd. } else { ch := make(chan error, 1) go func() { ch <- sys.Wait() }() - t := time.NewTimer(time.Second * 30) + // Same bound as the normal teardown path in [hcsTask.close]: + // this waits on the same host-side work, and it runs precisely + // when the shim died with a container still going down, so a + // filesystem-heavy container is if anything more likely here. + t := time.NewTimer(tearDownTimeout) select { case <-t.C: sys.Close() diff --git a/cmd/containerd-shim-runhcs-v1/task_hcs.go b/cmd/containerd-shim-runhcs-v1/task_hcs.go index afadc50c5e..07d52fcb3a 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs.go @@ -51,6 +51,86 @@ import ( "google.golang.org/protobuf/types/known/timestamppb" ) +// Container teardown timeouts. +// +// Tearing a container down is host-side work: detaching the layer filter stack +// and flushing the container's registry hives back into its scratch. For most +// workloads that finishes in well under a second. For Windows Server (process +// isolated) containers it scales with how much the container touched the +// filesystem, and filesystem-heavy workloads such as source builds have been +// measured needing minutes. +// +// Cutting that short is not free: terminating the container mid-flush can leave +// its scratch in a state the platform subsequently refuses to export, and the +// damage outlives the container, the snapshot and the host. +// +// The defaults below are unchanged. Hosts that run such workloads can raise +// them by setting the environment variables on the containerd service; the +// shim inherits its environment from containerd. +const ( + // tearDownTimeoutEnvVar overrides defaultTearDownTimeout. The value is a Go + // duration string, e.g. "45m". + tearDownTimeoutEnvVar = "CONTAINERD_SHIM_RUNHCS_V1_TEARDOWN_TIMEOUT" + // taskCloseTimeoutEnvVar overrides the task close timeout. The value is a Go + // duration string, e.g. "100m". When it is not set, the timeout is derived + // from tearDownTimeoutEnvVar; see [resolveTeardownTimeouts]. + taskCloseTimeoutEnvVar = "CONTAINERD_SHIM_RUNHCS_V1_TASK_CLOSE_TIMEOUT" + + defaultTearDownTimeout = 30 * time.Second + defaultTaskCloseTimeout = 30 * time.Second +) + +var tearDownTimeout, taskCloseTimeout = resolveTeardownTimeouts( + os.Getenv(tearDownTimeoutEnvVar), + os.Getenv(taskCloseTimeoutEnvVar), +) + +// resolveTeardownTimeouts computes the effective teardown timeouts from the raw +// environment values. +// +// tearDown bounds each of the two waits in [hcsTask.close]: first for a graceful +// shutdown to complete, then for a terminate. taskClose bounds the wait for +// container resource cleanup in [hcsTask.DeleteExec]. +// +// The two are coupled: DeleteExec waits on the channel that [hcsTask.close] +// closes, so a taskClose below close's worst case of 2*tearDown abandons a +// teardown that is still making progress - exactly the outcome these knobs exist +// to prevent. Raising only tearDown would therefore silently not help, so when +// taskClose is not set explicitly and tearDown has been raised above the +// default, taskClose is derived to cover close's worst case with room to spare. +// +// A raw value that is empty, unparseable or not positive is treated as unset. +// This runs during package initialization, before the shim's logging exists, so +// an invalid value must not keep the shim from starting; the effective values +// are logged wherever they are used. +func resolveTeardownTimeouts(tearDownRaw, taskCloseRaw string) (tearDown, taskClose time.Duration) { + tearDown = defaultTearDownTimeout + if d, ok := parsePositiveDuration(tearDownRaw); ok { + tearDown = d + } + + if d, ok := parsePositiveDuration(taskCloseRaw); ok { + return tearDown, d + } + if tearDown <= defaultTearDownTimeout { + return tearDown, defaultTaskCloseTimeout + } + return tearDown, 2*tearDown + defaultTaskCloseTimeout +} + +// parsePositiveDuration parses a Go duration string, reporting false when it is +// empty, malformed or not positive. +func parsePositiveDuration(s string) (time.Duration, bool) { + if s == "" { + return 0, false + } + d, err := time.ParseDuration(s) + if err != nil || d <= 0 { + return 0, false + } + return d, true +} + func newHcsStandaloneTask(ctx context.Context, events publisher, req *task.CreateTaskRequest, s *specs.Spec) (shimTask, error) { log.G(ctx).WithField("tid", req.ID).Debug("newHcsStandaloneTask") @@ -536,7 +616,7 @@ func (ht *hcsTask) DeleteExec(ctx context.Context, eid string) (int, uint32, tim // If the shim exits before resources are cleaned up, those resources // will remain locked and untracked, which leads to lingering sandboxes // and container resources like base vhdx. - const timeout = 30 * time.Second + timeout := taskCloseTimeout entry.WithField(logfields.Timeout, timeout).Trace("waiting for task to be closed") select { case <-time.After(timeout): @@ -702,8 +782,6 @@ func (ht *hcsTask) close(ctx context.Context) { // method or interface for ht.c operations that we can stub for // testing. if ht.c != nil { - const tearDownTimeout = 30 * time.Second - // Do our best attempt to tear down the container. // TODO: unify timeout select statements and use [ht.c.WaitCtx] and [context.WithTimeout] var werr error @@ -717,11 +795,19 @@ func (ht *hcsTask) close(ctx context.Context) { if err != nil { entry.WithError(err).Error("failed to shutdown container") } else { + shutdownStart := time.Now() t := time.NewTimer(tearDownTimeout) select { case <-ch: err = werr t.Stop() + // How long teardown actually took is the number needed to + // size tearDownTimeout for a given workload, and it is not + // otherwise observable. + entry.WithFields(logrus.Fields{ + logfields.Timeout: tearDownTimeout, + "duration": time.Since(shutdownStart).String(), + }).Debug("container shutdown completed") if err != nil { entry.WithError(err).Error("failed to wait for container shutdown") } diff --git a/cmd/containerd-shim-runhcs-v1/task_hcs_test.go b/cmd/containerd-shim-runhcs-v1/task_hcs_test.go index d922d8e2f3..4f8c4e330c 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs_test.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs_test.go @@ -550,3 +550,72 @@ func Test_hcsTask_updateWCOWContainerCPUAffinity_XenonNotImplemented(t *testing. t.Fatalf("expected ErrNotImplemented for hypervisor-isolated container, got %v", err) } } + +func Test_resolveTeardownTimeouts(t *testing.T) { + for _, tc := range []struct { + name string + tearDownRaw string + taskCloseRaw string + tearDown time.Duration + taskClose time.Duration + }{ + { + name: "unset keeps the historical defaults", + tearDown: 30 * time.Second, + taskClose: 30 * time.Second, + }, + { + name: "raising teardown derives a task close that covers it", + tearDownRaw: "45m", + tearDown: 45 * time.Minute, + taskClose: 2*45*time.Minute + 30*time.Second, + }, + { + name: "explicit task close wins over the derived value", + tearDownRaw: "45m", + taskCloseRaw: "100m", + tearDown: 45 * time.Minute, + taskClose: 100 * time.Minute, + }, + { + name: "task close alone is honoured", + taskCloseRaw: "5m", + tearDown: 30 * time.Second, + taskClose: 5 * time.Minute, + }, + { + name: "lowering teardown does not derive", + tearDownRaw: "10s", + tearDown: 10 * time.Second, + taskClose: 30 * time.Second, + }, + { + name: "malformed and negative values fall back", + tearDownRaw: "soon", + taskCloseRaw: "-5m", + tearDown: 30 * time.Second, + taskClose: 30 * time.Second, + }, + { + name: "zero is not a positive duration", + tearDownRaw: "0s", + tearDown: 30 * time.Second, + taskClose: 30 * time.Second, + }, + } { + t.Run(tc.name, func(t *testing.T) { + tearDown, taskClose := resolveTeardownTimeouts(tc.tearDownRaw, tc.taskCloseRaw) + if tearDown != tc.tearDown { + t.Errorf("tearDown: expected %v, got %v", tc.tearDown, tearDown) + } + if taskClose != tc.taskClose { + t.Errorf("taskClose: expected %v, got %v", tc.taskClose, taskClose) + } + // The invariant the derivation exists to protect: DeleteExec must + // not give up while close() may still be making progress. + if tc.taskCloseRaw == "" && taskClose <= 2*tearDown && tearDown > defaultTearDownTimeout { + t.Errorf("derived taskClose %v does not cover close()'s worst case of 2*%v", taskClose, tearDown) + } + }) + } +}