From c9f3fd3fc7a4beb97de40ef8da7330b23397d9d3 Mon Sep 17 00:00:00 2001 From: Solomon Hykes Date: Wed, 6 Aug 2014 08:12:22 +0000 Subject: [PATCH] Cleanup: refactor shutdown and signal handling facility This disentangles the following functions, which were previously all mixed together: * 1) Waiting for jobs to terminate when shutting down * 2) Handling signals in the Docker daemon * 3) Per-subsystem cleanup handlers * 4) pidfile management Responsibilities are dispatched as follows: * Signal traps are set in `main`, and trigger `engine.Shutdown` * `engine.Shutdown` coordinates cleanup by waiting for jobs to complete, and calling shutdown handlers * To perform cleanup at shutdown, each subsystem registers handlers with `engine.OnShutdown` * `daemon` is one subsystem, so it registers cleanup via `engine.OnShutdown`. * `daemon` owns the pidfile, which is used to lock access to `/var/lib/docker`. Part of its cleanup is to remove the pidfile. Signed-off-by: Solomon Hykes --- daemon/daemon.go | 62 ++++++++++++++++++++++++++++-------------------- docker/daemon.go | 2 ++ server/init.go | 9 ++----- server/server.go | 22 ----------------- 4 files changed, 40 insertions(+), 55 deletions(-) diff --git a/daemon/daemon.go b/daemon/daemon.go index 4753219652..3e990f449d 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -667,6 +667,20 @@ func NewDaemon(config *daemonconfig.Config, eng *engine.Engine) (*Daemon, error) } func NewDaemonFromDirectory(config *daemonconfig.Config, eng *engine.Engine) (*Daemon, error) { + // Claim the pidfile first, to avoid any and all unexpected race conditions. + // Some of the init doesn't need a pidfile lock - but let's not try to be smart. + if config.Pidfile != "" { + if err := utils.CreatePidFile(config.Pidfile); err != nil { + return nil, err + } + eng.OnShutdown(func() { + // Always release the pidfile last, just in case + utils.RemovePidFile(config.Pidfile) + }) + } + + // Check that the system is supported and we have sufficient privileges + // FIXME: return errors instead of calling Fatal if runtime.GOOS != "linux" { log.Fatalf("The Docker daemon is only supported on linux") } @@ -819,13 +833,32 @@ func NewDaemonFromDirectory(config *daemonconfig.Config, eng *engine.Engine) (*D eng: eng, Sockets: config.Sockets, } - if err := daemon.checkLocaldns(); err != nil { return nil, err } if err := daemon.restore(); err != nil { return nil, err } + // Setup shutdown handlers + // FIXME: can these shutdown handlers be registered closer to their source? + eng.OnShutdown(func() { + // FIXME: if these cleanup steps can be called concurrently, register + // them as separate handlers to speed up total shutdown time + // FIXME: use engine logging instead of utils.Errorf + if err := daemon.shutdown(); err != nil { + utils.Errorf("daemon.shutdown(): %s", err) + } + if err := portallocator.ReleaseAll(); err != nil { + utils.Errorf("portallocator.ReleaseAll(): %s", err) + } + if err := daemon.driver.Cleanup(); err != nil { + utils.Errorf("daemon.driver.Cleanup(): %s", err.Error()) + } + if err := daemon.containerGraph.Close(); err != nil { + utils.Errorf("daemon.containerGraph.Close(): %s", err.Error()) + } + }) + return daemon, nil } @@ -853,30 +886,6 @@ func (daemon *Daemon) shutdown() error { return nil } -func (daemon *Daemon) Close() error { - errorsStrings := []string{} - if err := daemon.shutdown(); err != nil { - utils.Errorf("daemon.shutdown(): %s", err) - errorsStrings = append(errorsStrings, err.Error()) - } - if err := portallocator.ReleaseAll(); err != nil { - utils.Errorf("portallocator.ReleaseAll(): %s", err) - errorsStrings = append(errorsStrings, err.Error()) - } - if err := daemon.driver.Cleanup(); err != nil { - utils.Errorf("daemon.driver.Cleanup(): %s", err.Error()) - errorsStrings = append(errorsStrings, err.Error()) - } - if err := daemon.containerGraph.Close(); err != nil { - utils.Errorf("daemon.containerGraph.Close(): %s", err.Error()) - errorsStrings = append(errorsStrings, err.Error()) - } - if len(errorsStrings) > 0 { - return fmt.Errorf("%s", strings.Join(errorsStrings, ", ")) - } - return nil -} - func (daemon *Daemon) Mount(container *Container) error { dir, err := daemon.driver.Get(container.ID, container.GetMountLabel()) if err != nil { @@ -967,6 +976,8 @@ func (daemon *Daemon) Kill(c *Container, sig int) error { // from the content root, including images, volumes and // container filesystems. // Again: this will remove your entire docker daemon! +// FIXME: this is deprecated, and only used in legacy +// tests. Please remove. func (daemon *Daemon) Nuke() error { var wg sync.WaitGroup for _, container := range daemon.List() { @@ -977,7 +988,6 @@ func (daemon *Daemon) Nuke() error { }(container) } wg.Wait() - daemon.Close() return os.RemoveAll(daemon.config.Root) } diff --git a/docker/daemon.go b/docker/daemon.go index 4b994e9629..caaf94e06d 100644 --- a/docker/daemon.go +++ b/docker/daemon.go @@ -10,6 +10,7 @@ import ( "github.com/docker/docker/dockerversion" "github.com/docker/docker/engine" flag "github.com/docker/docker/pkg/mflag" + "github.com/docker/docker/pkg/signal" "github.com/docker/docker/sysinit" ) @@ -39,6 +40,7 @@ func mainDaemon() { } eng := engine.New() + signal.Trap(eng.Shutdown) // Load builtins if err := builtins.Register(eng); err != nil { log.Fatal(err) diff --git a/server/init.go b/server/init.go index c18fd90b4a..f815b8e61b 100644 --- a/server/init.go +++ b/server/init.go @@ -10,7 +10,6 @@ import ( "github.com/docker/docker/daemon" "github.com/docker/docker/daemonconfig" "github.com/docker/docker/engine" - "github.com/docker/docker/pkg/signal" "github.com/docker/docker/utils" ) @@ -41,15 +40,11 @@ func InitPidfile(job *engine.Job) engine.Status { // The signals SIGINT, SIGQUIT and SIGTERM are intercepted for cleanup. func InitServer(job *engine.Job) engine.Status { job.Logf("Creating server") - srv, err := NewServer(job.Eng, daemonconfig.ConfigFromJob(job)) + cfg := daemonconfig.ConfigFromJob(job) + srv, err := NewServer(job.Eng, cfg) if err != nil { return job.Error(err) } - job.Logf("Setting up signal traps") - signal.Trap(func() { - utils.RemovePidFile(srv.daemon.Config().Pidfile) - srv.Close() - }) job.Eng.Hack_SetGlobalVar("httpapi.server", srv) job.Eng.Hack_SetGlobalVar("httpapi.daemon", srv.daemon) diff --git a/server/server.go b/server/server.go index e5233a604c..c185a6951e 100644 --- a/server/server.go +++ b/server/server.go @@ -23,7 +23,6 @@ package server import ( "sync" - "time" "github.com/docker/docker/daemon" "github.com/docker/docker/engine" @@ -42,27 +41,6 @@ func (srv *Server) IsRunning() bool { return srv.running } -func (srv *Server) Close() error { - if srv == nil { - return nil - } - srv.SetRunning(false) - done := make(chan struct{}) - go func() { - srv.tasks.Wait() - close(done) - }() - select { - // Waiting server jobs for 15 seconds, shutdown immediately after that time - case <-time.After(time.Second * 15): - case <-done: - } - if srv.daemon == nil { - return nil - } - return srv.daemon.Close() -} - type Server struct { sync.RWMutex daemon *daemon.Daemon