From 5edf9acf9c3e5122384d91a80505eabd522867bc Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 20 Aug 2021 22:43:13 +0200 Subject: [PATCH] daemon: move default stop-timeout to containerStop() This avoids having to determine what the default is in various parts of the code. If no custom timeout is passed (nil), the default will be used. Also remove the named return variable from cleanupContainer(), as it wasn't used. Signed-off-by: Sebastiaan van Stijn --- daemon/daemon.go | 4 +--- daemon/delete.go | 14 +++++++++++++- daemon/restart.go | 10 +++------- daemon/stop.go | 29 +++++++++++++++++------------ 4 files changed, 34 insertions(+), 23 deletions(-) diff --git a/daemon/daemon.go b/daemon/daemon.go index 2a04e33a13..ebb0a51e8f 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -1116,10 +1116,8 @@ func (daemon *Daemon) waitForStartupDone() { } func (daemon *Daemon) shutdownContainer(c *container.Container) error { - stopTimeout := c.StopTimeout() - // If container failed to exit in stopTimeout seconds of SIGTERM, then using the force - if err := daemon.containerStop(c, stopTimeout); err != nil { + if err := daemon.containerStop(c, nil); err != nil { return fmt.Errorf("Failed to stop container %s with error: %v", c.ID, err) } diff --git a/daemon/delete.go b/daemon/delete.go index f1a2ed737a..05536a67d2 100644 --- a/daemon/delete.go +++ b/daemon/delete.go @@ -97,7 +97,19 @@ func (daemon *Daemon) cleanupContainer(container *container.Container, config ty // if stats are currently getting collected. daemon.statsCollector.StopCollection(container) - if err := daemon.containerStop(container, 3); err != nil { + // stopTimeout is the number of seconds to wait for the container to stop + // gracefully before forcibly killing it. + // + // Why 3 seconds? The timeout specified here was originally added in commit + // 1615bb08c7c3fc6c4b22db0a633edda516f97cf0, which added a custom timeout to + // some commands, but lacking an option for a timeout on "docker rm", was + // hardcoded to 10 seconds. Commit 28fd289b448164b77affd8103c0d96fd8110daf9 + // later on updated this to 3 seconds (but no background on that change). + // + // If you arrived here and know the answer, you earned yourself a picture + // of a cute animal of your own choosing. + var stopTimeout = 3 + if err := daemon.containerStop(container, &stopTimeout); err != nil { return err } diff --git a/daemon/restart.go b/daemon/restart.go index da3deec4bf..5ac271c5e0 100644 --- a/daemon/restart.go +++ b/daemon/restart.go @@ -19,11 +19,8 @@ func (daemon *Daemon) ContainerRestart(name string, seconds *int) error { if err != nil { return err } - if seconds == nil { - stopTimeout := ctr.StopTimeout() - seconds = &stopTimeout - } - if err := daemon.containerRestart(ctr, *seconds); err != nil { + err = daemon.containerRestart(ctr, seconds) + if err != nil { return fmt.Errorf("Cannot restart container %s: %v", name, err) } return nil @@ -34,8 +31,7 @@ func (daemon *Daemon) ContainerRestart(name string, seconds *int) error { // container. When stopping, wait for the given duration in seconds to // gracefully stop, before forcefully terminating the container. If // given a negative duration, wait forever for a graceful stop. -func (daemon *Daemon) containerRestart(container *container.Container, seconds int) error { - +func (daemon *Daemon) containerRestart(container *container.Container, seconds *int) error { // Determine isolation. If not specified in the hostconfig, use daemon default. actualIsolation := container.HostConfig.Isolation if containertypes.Isolation.IsDefault(actualIsolation) { diff --git a/daemon/stop.go b/daemon/stop.go index 18d0354f02..2f67925375 100644 --- a/daemon/stop.go +++ b/daemon/stop.go @@ -24,34 +24,39 @@ func (daemon *Daemon) ContainerStop(name string, timeout *int) error { return err } if !ctr.IsRunning() { - return containerNotModifiedError{running: false} + return containerNotModifiedError{} } - if timeout == nil { - stopTimeout := ctr.StopTimeout() - timeout = &stopTimeout - } - if err := daemon.containerStop(ctr, *timeout); err != nil { + err = daemon.containerStop(ctr, timeout) + if err != nil { return errdefs.System(errors.Wrapf(err, "cannot stop container: %s", name)) } return nil } // containerStop sends a stop signal, waits, sends a kill signal. -func (daemon *Daemon) containerStop(ctr *container.Container, seconds int) error { +func (daemon *Daemon) containerStop(ctr *container.Container, seconds *int) error { // TODO propagate a context down to this function ctx := context.TODO() if !ctr.IsRunning() { return nil } + + var ( + stopSignal = ctr.StopSignal() + stopTimeout = ctr.StopTimeout() + ) + if seconds != nil { + stopTimeout = *seconds + } + var wait time.Duration - if seconds >= 0 { - wait = time.Duration(seconds) * time.Second + if stopTimeout >= 0 { + wait = time.Duration(stopTimeout) * time.Second } success := func() error { daemon.LogContainerEvent(ctr, "stop") return nil } - stopSignal := ctr.StopSignal() // 1. Send a stop signal err := daemon.killPossiblyDeadProcess(ctr, stopSignal) @@ -61,7 +66,7 @@ func (daemon *Daemon) containerStop(ctr *container.Container, seconds int) error var subCtx context.Context var cancel context.CancelFunc - if seconds >= 0 { + if stopTimeout >= 0 { subCtx, cancel = context.WithTimeout(ctx, wait) } else { subCtx, cancel = context.WithCancel(ctx) @@ -77,7 +82,7 @@ func (daemon *Daemon) containerStop(ctr *container.Container, seconds int) error // the container has still not exited, and the kill function errored, so log the error here: logrus.WithError(err).WithField("container", ctr.ID).Errorf("Error sending stop (signal %d) to container", stopSignal) } - if seconds < 0 { + if stopTimeout < 0 { // if the client requested that we never kill / wait forever, but container.Wait was still // interrupted (parent context cancelled, for example), we should propagate the signal failure return err