From 15c49406519715370b3f0096770b43afea01668c Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Mon, 6 Jun 2016 20:29:05 -0700 Subject: [PATCH] Update `docker stop` and `docker restart` to allow not specifying timeout and use the one specified at container creation time. Signed-off-by: Yong Tang Upstream-commit: cc703784f39fb3f585ec685cf7aeda6410648446 Component: engine --- .../api/server/router/container/backend.go | 4 +-- .../router/container/container_routes.go | 20 +++++++++-- .../engine/cli/command/container/restart.go | 13 ++++++-- .../engine/cli/command/container/stop.go | 13 ++++++-- components/engine/cmd/dockerd/daemon.go | 12 +++++-- components/engine/container/container.go | 6 ++-- .../engine/container/container_unit_test.go | 4 +-- .../engine/daemon/cluster/executor/backend.go | 2 +- .../cluster/executor/container/adapter.go | 7 ++-- components/engine/daemon/daemon.go | 33 ++++++++++++++++--- components/engine/daemon/restart.go | 9 +++-- components/engine/daemon/stop.go | 8 +++-- components/engine/runconfig/opts/parse.go | 6 ++-- 13 files changed, 103 insertions(+), 34 deletions(-) diff --git a/components/engine/api/server/router/container/backend.go b/components/engine/api/server/router/container/backend.go index 55f2c9aa23..2e8968d2e1 100644 --- a/components/engine/api/server/router/container/backend.go +++ b/components/engine/api/server/router/container/backend.go @@ -37,10 +37,10 @@ type stateBackend interface { ContainerPause(name string) error ContainerRename(oldName, newName string) error ContainerResize(name string, height, width int) error - ContainerRestart(name string, seconds int) error + ContainerRestart(name string, seconds *int) error ContainerRm(name string, config *types.ContainerRmConfig) error ContainerStart(name string, hostConfig *container.HostConfig, validateHostname bool, checkpoint string) error - ContainerStop(name string, seconds int) error + ContainerStop(name string, seconds *int) error ContainerUnpause(name string) error ContainerUpdate(name string, hostConfig *container.HostConfig, validateHostname bool) (types.ContainerUpdateResponse, error) ContainerWait(name string, timeout time.Duration) (int, error) diff --git a/components/engine/api/server/router/container/container_routes.go b/components/engine/api/server/router/container/container_routes.go index 481745082d..001ad3a3ee 100644 --- a/components/engine/api/server/router/container/container_routes.go +++ b/components/engine/api/server/router/container/container_routes.go @@ -169,7 +169,14 @@ func (s *containerRouter) postContainersStop(ctx context.Context, w http.Respons return err } - seconds, _ := strconv.Atoi(r.Form.Get("t")) + var seconds *int + if tmpSeconds := r.Form.Get("t"); tmpSeconds != "" { + valSeconds, err := strconv.Atoi(tmpSeconds) + if err != nil { + return err + } + seconds = &valSeconds + } if err := s.backend.ContainerStop(vars["name"], seconds); err != nil { return err @@ -223,9 +230,16 @@ func (s *containerRouter) postContainersRestart(ctx context.Context, w http.Resp return err } - timeout, _ := strconv.Atoi(r.Form.Get("t")) + var seconds *int + if tmpSeconds := r.Form.Get("t"); tmpSeconds != "" { + valSeconds, err := strconv.Atoi(tmpSeconds) + if err != nil { + return err + } + seconds = &valSeconds + } - if err := s.backend.ContainerRestart(vars["name"], timeout); err != nil { + if err := s.backend.ContainerRestart(vars["name"], seconds); err != nil { return err } diff --git a/components/engine/cli/command/container/restart.go b/components/engine/cli/command/container/restart.go index e370ef4010..fc3ba93c84 100644 --- a/components/engine/cli/command/container/restart.go +++ b/components/engine/cli/command/container/restart.go @@ -13,7 +13,8 @@ import ( ) type restartOptions struct { - nSeconds int + nSeconds int + nSecondsChanged bool containers []string } @@ -28,6 +29,7 @@ func NewRestartCommand(dockerCli *command.DockerCli) *cobra.Command { Args: cli.RequiresMinArgs(1), RunE: func(cmd *cobra.Command, args []string) error { opts.containers = args + opts.nSecondsChanged = cmd.Flags().Changed("time") return runRestart(dockerCli, &opts) }, } @@ -40,9 +42,14 @@ func NewRestartCommand(dockerCli *command.DockerCli) *cobra.Command { func runRestart(dockerCli *command.DockerCli, opts *restartOptions) error { ctx := context.Background() var errs []string + var timeout *time.Duration + if opts.nSecondsChanged { + timeoutValue := time.Duration(opts.nSeconds) * time.Second + timeout = &timeoutValue + } + for _, name := range opts.containers { - timeout := time.Duration(opts.nSeconds) * time.Second - if err := dockerCli.Client().ContainerRestart(ctx, name, &timeout); err != nil { + if err := dockerCli.Client().ContainerRestart(ctx, name, timeout); err != nil { errs = append(errs, err.Error()) } else { fmt.Fprintf(dockerCli.Out(), "%s\n", name) diff --git a/components/engine/cli/command/container/stop.go b/components/engine/cli/command/container/stop.go index 2f22fd09a4..c68ede5368 100644 --- a/components/engine/cli/command/container/stop.go +++ b/components/engine/cli/command/container/stop.go @@ -13,7 +13,8 @@ import ( ) type stopOptions struct { - time int + time int + timeChanged bool containers []string } @@ -28,6 +29,7 @@ func NewStopCommand(dockerCli *command.DockerCli) *cobra.Command { Args: cli.RequiresMinArgs(1), RunE: func(cmd *cobra.Command, args []string) error { opts.containers = args + opts.timeChanged = cmd.Flags().Changed("time") return runStop(dockerCli, &opts) }, } @@ -39,12 +41,17 @@ func NewStopCommand(dockerCli *command.DockerCli) *cobra.Command { func runStop(dockerCli *command.DockerCli, opts *stopOptions) error { ctx := context.Background() - timeout := time.Duration(opts.time) * time.Second + + var timeout *time.Duration + if opts.timeChanged { + timeoutValue := time.Duration(opts.time) * time.Second + timeout = &timeoutValue + } var errs []string errChan := parallelOperation(ctx, opts.containers, func(ctx context.Context, id string) error { - return dockerCli.Client().ContainerStop(ctx, id, &timeout) + return dockerCli.Client().ContainerStop(ctx, id, timeout) }) for _, container := range opts.containers { if err := <-errChan; err != nil { diff --git a/components/engine/cmd/dockerd/daemon.go b/components/engine/cmd/dockerd/daemon.go index f90f93835c..d85cde45c9 100644 --- a/components/engine/cmd/dockerd/daemon.go +++ b/components/engine/cmd/dockerd/daemon.go @@ -296,7 +296,7 @@ func (cli *DaemonCli) start(opts daemonOptions) (err error) { // Wait for serve API to complete errAPI := <-serveAPIWait c.Cleanup() - shutdownDaemon(d, 15) + shutdownDaemon(d) containerdRemote.Cleanup() if errAPI != nil { return fmt.Errorf("Shutting down due to ServeAPI error: %v", errAPI) @@ -342,16 +342,22 @@ func (cli *DaemonCli) stop() { // shutdownDaemon just wraps daemon.Shutdown() to handle a timeout in case // d.Shutdown() is waiting too long to kill container or worst it's // blocked there -func shutdownDaemon(d *daemon.Daemon, timeout time.Duration) { +func shutdownDaemon(d *daemon.Daemon) { + shutdownTimeout := d.ShutdownTimeout() ch := make(chan struct{}) go func() { d.Shutdown() close(ch) }() + if shutdownTimeout < 0 { + <-ch + logrus.Debug("Clean shutdown succeeded") + return + } select { case <-ch: logrus.Debug("Clean shutdown succeeded") - case <-time.After(timeout * time.Second): + case <-time.After(time.Duration(shutdownTimeout) * time.Second): logrus.Error("Force shutdown daemon") } } diff --git a/components/engine/container/container.go b/components/engine/container/container.go index 59bcaa8962..0043c7091c 100644 --- a/components/engine/container/container.go +++ b/components/engine/container/container.go @@ -45,8 +45,8 @@ import ( const configFileName = "config.v2.json" const ( - // defaultStopTimeout is the timeout (in seconds) for the syscall signal used to stop a container. - defaultStopTimeout = 10 + // DefaultStopTimeout is the timeout (in seconds) for the syscall signal used to stop a container. + DefaultStopTimeout = 10 ) var ( @@ -588,7 +588,7 @@ func (container *Container) StopTimeout() int { if container.Config.StopTimeout != nil { return *container.Config.StopTimeout } - return defaultStopTimeout + return DefaultStopTimeout } // InitDNSHostConfig ensures that the dns fields are never nil. diff --git a/components/engine/container/container_unit_test.go b/components/engine/container/container_unit_test.go index 73bdf0fe13..f301f25bbe 100644 --- a/components/engine/container/container_unit_test.go +++ b/components/engine/container/container_unit_test.go @@ -43,8 +43,8 @@ func TestContainerStopTimeout(t *testing.T) { } s := c.StopTimeout() - if s != defaultStopTimeout { - t.Fatalf("Expected %v, got %v", defaultStopTimeout, s) + if s != DefaultStopTimeout { + t.Fatalf("Expected %v, got %v", DefaultStopTimeout, s) } stopTimeout := 15 diff --git a/components/engine/daemon/cluster/executor/backend.go b/components/engine/daemon/cluster/executor/backend.go index d8a7646e4d..fb7c822225 100644 --- a/components/engine/daemon/cluster/executor/backend.go +++ b/components/engine/daemon/cluster/executor/backend.go @@ -25,7 +25,7 @@ type Backend interface { PullImage(ctx context.Context, image, tag string, metaHeaders map[string][]string, authConfig *types.AuthConfig, outStream io.Writer) error CreateManagedContainer(config types.ContainerCreateConfig, validateHostname bool) (types.ContainerCreateResponse, error) ContainerStart(name string, hostConfig *container.HostConfig, validateHostname bool, checkpoint string) error - ContainerStop(name string, seconds int) error + ContainerStop(name string, seconds *int) error ConnectContainerToNetwork(containerName, networkName string, endpointConfig *network.EndpointSettings) error UpdateContainerServiceConfig(containerName string, serviceConfig *clustertypes.ServiceConfig) error ContainerInspectCurrent(name string, size bool) (*types.ContainerJSON, error) diff --git a/components/engine/daemon/cluster/executor/container/adapter.go b/components/engine/daemon/cluster/executor/container/adapter.go index 774333e8a3..56f8c4067e 100644 --- a/components/engine/daemon/cluster/executor/container/adapter.go +++ b/components/engine/daemon/cluster/executor/container/adapter.go @@ -279,11 +279,12 @@ func (c *containerAdapter) wait(ctx context.Context) error { } func (c *containerAdapter) shutdown(ctx context.Context) error { - // Default stop grace period to 10s. - stopgrace := 10 + // Default stop grace period to nil (daemon will use the stopTimeout of the container) + var stopgrace *int spec := c.container.spec() if spec.StopGracePeriod != nil { - stopgrace = int(spec.StopGracePeriod.Seconds) + stopgraceValue := int(spec.StopGracePeriod.Seconds) + stopgrace = &stopgraceValue } return c.backend.ContainerStop(c.container.name(), stopgrace) } diff --git a/components/engine/daemon/daemon.go b/components/engine/daemon/daemon.go index 218c9031cc..adf5e443ae 100644 --- a/components/engine/daemon/daemon.go +++ b/components/engine/daemon/daemon.go @@ -692,6 +692,8 @@ func NewDaemon(config *Config, registryService registry.Service, containerdRemot } func (daemon *Daemon) shutdownContainer(c *container.Container) error { + stopTimeout := c.StopTimeout() + // TODO(windows): Handle docker restart with paused containers if c.IsPaused() { // To terminate a process in freezer cgroup, we should send @@ -708,8 +710,8 @@ func (daemon *Daemon) shutdownContainer(c *container.Container) error { if err := daemon.containerUnpause(c); err != nil { return fmt.Errorf("Failed to unpause container %s with error: %v", c.ID, err) } - if _, err := c.WaitStop(time.Duration(c.StopTimeout()) * time.Second); err != nil { - logrus.Debugf("container %s failed to exit in %d second of SIGTERM, sending SIGKILL to force", c.ID, c.StopTimeout()) + if _, err := c.WaitStop(time.Duration(stopTimeout) * time.Second); err != nil { + logrus.Debugf("container %s failed to exit in %d second of SIGTERM, sending SIGKILL to force", c.ID, stopTimeout) sig, ok := signal.SignalMap["KILL"] if !ok { return fmt.Errorf("System does not support SIGKILL") @@ -721,8 +723,8 @@ func (daemon *Daemon) shutdownContainer(c *container.Container) error { return err } } - // If container failed to exit in c.StopTimeout() seconds of SIGTERM, then using the force - if err := daemon.containerStop(c, c.StopTimeout()); err != nil { + // If container failed to exit in stopTimeout seconds of SIGTERM, then using the force + if err := daemon.containerStop(c, stopTimeout); err != nil { return fmt.Errorf("Failed to stop container %s with error: %v", c.ID, err) } @@ -730,6 +732,29 @@ func (daemon *Daemon) shutdownContainer(c *container.Container) error { return nil } +// ShutdownTimeout returns the shutdown timeout based on the max stopTimeout of the containers +func (daemon *Daemon) ShutdownTimeout() int { + // By default we use container.DefaultStopTimeout + 5s, which is 15s. + // TODO (yongtang): Will need to allow shutdown-timeout once #23036 is in place. + graceTimeout := 5 + shutdownTimeout := container.DefaultStopTimeout + graceTimeout + if daemon.containers != nil { + for _, c := range daemon.containers.List() { + if shutdownTimeout >= 0 { + stopTimeout := c.StopTimeout() + if stopTimeout < 0 { + shutdownTimeout = -1 + } else { + if stopTimeout+graceTimeout > shutdownTimeout { + shutdownTimeout = stopTimeout + graceTimeout + } + } + } + } + } + return shutdownTimeout +} + // Shutdown stops the daemon. func (daemon *Daemon) Shutdown() error { daemon.shutdown = true diff --git a/components/engine/daemon/restart.go b/components/engine/daemon/restart.go index 1172f6c82e..cdf21af293 100644 --- a/components/engine/daemon/restart.go +++ b/components/engine/daemon/restart.go @@ -13,15 +13,20 @@ import ( // timeout, ContainerRestart will wait forever until a graceful // stop. Returns an error if the container cannot be found, or if // there is an underlying error at any stage of the restart. -func (daemon *Daemon) ContainerRestart(name string, seconds int) error { +func (daemon *Daemon) ContainerRestart(name string, seconds *int) error { container, err := daemon.GetContainer(name) if err != nil { return err } - if err := daemon.containerRestart(container, seconds); err != nil { + if seconds == nil { + stopTimeout := container.StopTimeout() + seconds = &stopTimeout + } + if err := daemon.containerRestart(container, *seconds); err != nil { return fmt.Errorf("Cannot restart container %s: %v", name, err) } return nil + } // containerRestart attempts to gracefully stop and then start the diff --git a/components/engine/daemon/stop.go b/components/engine/daemon/stop.go index 90b8898b24..aa7b3820c8 100644 --- a/components/engine/daemon/stop.go +++ b/components/engine/daemon/stop.go @@ -16,7 +16,7 @@ import ( // will wait for a graceful termination. An error is returned if the // container is not found, is already stopped, or if there is a // problem stopping the container. -func (daemon *Daemon) ContainerStop(name string, seconds int) error { +func (daemon *Daemon) ContainerStop(name string, seconds *int) error { container, err := daemon.GetContainer(name) if err != nil { return err @@ -25,7 +25,11 @@ func (daemon *Daemon) ContainerStop(name string, seconds int) error { err := fmt.Errorf("Container %s is already stopped", name) return errors.NewErrorWithStatusCode(err, http.StatusNotModified) } - if err := daemon.containerStop(container, seconds); err != nil { + if seconds == nil { + stopTimeout := container.StopTimeout() + seconds = &stopTimeout + } + if err := daemon.containerStop(container, *seconds); err != nil { return fmt.Errorf("Cannot stop container %s: %v", name, err) } return nil diff --git a/components/engine/runconfig/opts/parse.go b/components/engine/runconfig/opts/parse.go index 81240b8290..e4221dee2d 100644 --- a/components/engine/runconfig/opts/parse.go +++ b/components/engine/runconfig/opts/parse.go @@ -94,6 +94,7 @@ type ContainerOptions struct { cgroupParent string volumeDriver string stopSignal string + stopTimeout int isolation string shmSize string noHealthcheck bool @@ -106,7 +107,6 @@ type ContainerOptions struct { init bool initPath string credentialSpec string - stopTimeout int Image string Args []string @@ -146,7 +146,6 @@ func AddFlags(flags *pflag.FlagSet) *ContainerOptions { ulimits: NewUlimitOpt(nil), volumes: opts.NewListOpts(nil), volumesFrom: opts.NewListOpts(nil), - stopSignal: flags.String("stop-signal", signal.DefaultStopSignal, fmt.Sprintf("Signal to stop a container, %v by default", signal.DefaultStopSignal)), } // General purpose flags @@ -163,6 +162,7 @@ func AddFlags(flags *pflag.FlagSet) *ContainerOptions { flags.BoolVar(&copts.readonlyRootfs, "read-only", false, "Mount the container's root filesystem as read only") flags.StringVar(&copts.restartPolicy, "restart", "no", "Restart policy to apply when a container exits") flags.StringVar(&copts.stopSignal, "stop-signal", signal.DefaultStopSignal, fmt.Sprintf("Signal to stop a container, %v by default", signal.DefaultStopSignal)) + flags.IntVar(&copts.stopTimeout, "stop-timeout", 0, "Timeout (in seconds) to stop a container") flags.Var(copts.sysctls, "sysctl", "Sysctl options") flags.BoolVarP(&copts.tty, "tty", "t", false, "Allocate a pseudo-TTY") flags.Var(copts.ulimits, "ulimit", "Ulimit options") @@ -561,7 +561,7 @@ func Parse(flags *pflag.FlagSet, copts *ContainerOptions) (*container.Config, *c config.StopSignal = copts.stopSignal } if flags.Changed("stop-timeout") { - config.StopTimeout = copts.flStopTimeout + config.StopTimeout = &copts.stopTimeout } hostConfig := &container.HostConfig{