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) + } + }) + } +}