From 068e200711034930dda536187e55f9fd29f690cb Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Tue, 17 Apr 2018 11:30:39 -0400 Subject: [PATCH] Extra check before unmounting on shutdown This makes sure that if the daemon root was already a self-binded mount (thus meaning the daemonc only performed a remount) that the daemon does not try to unmount. Example: ``` $ sudo mount --bind /var/lib/docker /var/lib/docker $ sudo dockerd & ``` Signed-off-by: Brian Goff Upstream-commit: c403f0036b9945bd58a84e2c93f794ed9861fe99 Component: engine --- components/engine/daemon/daemon_linux.go | 10 +- components/engine/daemon/daemon_linux_test.go | 100 ++++++++++++++++++ components/engine/daemon/daemon_unix.go | 51 ++++++++- 3 files changed, 156 insertions(+), 5 deletions(-) diff --git a/components/engine/daemon/daemon_linux.go b/components/engine/daemon/daemon_linux.go index d27e7333b7..024c48b8bf 100644 --- a/components/engine/daemon/daemon_linux.go +++ b/components/engine/daemon/daemon_linux.go @@ -89,8 +89,16 @@ func (daemon *Daemon) cleanupMounts() error { return nil } + unmountFile := getUnmountOnShutdownPath(daemon.configStore) + if _, err := os.Stat(unmountFile); err != nil { + return nil + } + logrus.WithField("mountpoint", daemon.root).Debug("unmounting daemon root") - return mount.Unmount(daemon.root) + if err := mount.Unmount(daemon.root); err != nil { + return err + } + return os.Remove(unmountFile) } func getCleanPatterns(id string) (regexps []*regexp.Regexp) { diff --git a/components/engine/daemon/daemon_linux_test.go b/components/engine/daemon/daemon_linux_test.go index 195afb1e0f..a90cffeb12 100644 --- a/components/engine/daemon/daemon_linux_test.go +++ b/components/engine/daemon/daemon_linux_test.go @@ -3,11 +3,15 @@ package daemon // import "github.com/docker/docker/daemon" import ( + "io/ioutil" + "os" + "path/filepath" "strings" "testing" containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/container" + "github.com/docker/docker/daemon/config" "github.com/docker/docker/oci" "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/mount" @@ -228,3 +232,99 @@ func TestShouldUnmountRoot(t *testing.T) { }) } } + +func checkMounted(t *testing.T, p string, expect bool) { + t.Helper() + mounted, err := mount.Mounted(p) + assert.Check(t, err) + assert.Check(t, mounted == expect, "expected %v, actual %v", expect, mounted) +} + +func TestRootMountCleanup(t *testing.T) { + t.Parallel() + + testRoot, err := ioutil.TempDir("", t.Name()) + assert.Assert(t, err) + defer os.RemoveAll(testRoot) + cfg := &config.Config{} + + err = mount.MakePrivate(testRoot) + assert.Assert(t, err) + defer mount.Unmount(testRoot) + + cfg.ExecRoot = filepath.Join(testRoot, "exec") + cfg.Root = filepath.Join(testRoot, "daemon") + + err = os.Mkdir(cfg.ExecRoot, 0755) + assert.Assert(t, err) + err = os.Mkdir(cfg.Root, 0755) + assert.Assert(t, err) + + d := &Daemon{configStore: cfg, root: cfg.Root} + unmountFile := getUnmountOnShutdownPath(cfg) + + t.Run("regular dir no mountpoint", func(t *testing.T) { + err = setupDaemonRootPropagation(cfg) + assert.Assert(t, err) + _, err = os.Stat(unmountFile) + assert.Assert(t, err) + checkMounted(t, cfg.Root, true) + + assert.Assert(t, d.cleanupMounts()) + checkMounted(t, cfg.Root, false) + + _, err = os.Stat(unmountFile) + assert.Assert(t, os.IsNotExist(err)) + }) + + t.Run("root is a private mountpoint", func(t *testing.T) { + err = mount.MakePrivate(cfg.Root) + assert.Assert(t, err) + defer mount.Unmount(cfg.Root) + + err = setupDaemonRootPropagation(cfg) + assert.Assert(t, err) + assert.Check(t, ensureShared(cfg.Root)) + + _, err = os.Stat(unmountFile) + assert.Assert(t, os.IsNotExist(err)) + assert.Assert(t, d.cleanupMounts()) + checkMounted(t, cfg.Root, true) + }) + + // mount is pre-configured with a shared mount + t.Run("root is a shared mountpoint", func(t *testing.T) { + err = mount.MakeShared(cfg.Root) + assert.Assert(t, err) + defer mount.Unmount(cfg.Root) + + err = setupDaemonRootPropagation(cfg) + assert.Assert(t, err) + + if _, err := os.Stat(unmountFile); err == nil { + t.Fatal("unmount file should not exist") + } + + assert.Assert(t, d.cleanupMounts()) + checkMounted(t, cfg.Root, true) + assert.Assert(t, mount.Unmount(cfg.Root)) + }) + + // does not need mount but unmount file exists from previous run + t.Run("old mount file is cleaned up on setup if not needed", func(t *testing.T) { + err = mount.MakeShared(testRoot) + assert.Assert(t, err) + defer mount.MakePrivate(testRoot) + err = ioutil.WriteFile(unmountFile, nil, 0644) + assert.Assert(t, err) + + err = setupDaemonRootPropagation(cfg) + assert.Assert(t, err) + + _, err = os.Stat(unmountFile) + assert.Check(t, os.IsNotExist(err), err) + checkMounted(t, cfg.Root, false) + assert.Assert(t, d.cleanupMounts()) + }) + +} diff --git a/components/engine/daemon/daemon_unix.go b/components/engine/daemon/daemon_unix.go index 670d564b8a..f01040ac9b 100644 --- a/components/engine/daemon/daemon_unix.go +++ b/components/engine/daemon/daemon_unix.go @@ -1177,14 +1177,57 @@ func setupDaemonRoot(config *config.Config, rootDir string, rootIDs idtools.IDPa } } - if err := ensureSharedOrSlave(config.Root); err != nil { - if err := mount.MakeShared(config.Root); err != nil { - logrus.WithError(err).WithField("dir", config.Root).Warn("Could not set daemon root propagation to shared, this is not generally critical but may cause some functionality to not work or fallback to less desirable behavior") - } + if err := setupDaemonRootPropagation(config); err != nil { + logrus.WithError(err).WithField("dir", config.Root).Warn("Error while setting daemon root propagation, this is not generally critical but may cause some functionality to not work or fallback to less desirable behavior") } return nil } +func setupDaemonRootPropagation(cfg *config.Config) error { + rootParentMount, options, err := getSourceMount(cfg.Root) + if err != nil { + return errors.Wrap(err, "error getting daemon root's parent mount") + } + + var cleanupOldFile bool + cleanupFile := getUnmountOnShutdownPath(cfg) + defer func() { + if !cleanupOldFile { + return + } + if err := os.Remove(cleanupFile); err != nil && !os.IsNotExist(err) { + logrus.WithError(err).WithField("file", cleanupFile).Warn("could not clean up old root propagation unmount file") + } + }() + + if hasMountinfoOption(options, sharedPropagationOption, slavePropagationOption) { + cleanupOldFile = true + return nil + } + + if err := mount.MakeShared(cfg.Root); err != nil { + return errors.Wrap(err, "could not setup daemon root propagation to shared") + } + + // check the case where this may have already been a mount to itself. + // If so then the daemon only performed a remount and should not try to unmount this later. + if rootParentMount == cfg.Root { + cleanupOldFile = true + return nil + } + + if err := ioutil.WriteFile(cleanupFile, nil, 0600); err != nil { + return errors.Wrap(err, "error writing file to signal mount cleanup on shutdown") + } + return nil +} + +// getUnmountOnShutdownPath generates the path to used when writing the file that signals to the daemon that on shutdown +// the daemon root should be unmounted. +func getUnmountOnShutdownPath(config *config.Config) string { + return filepath.Join(config.ExecRoot, "unmount-on-shutdown") +} + // registerLinks writes the links to a file. func (daemon *Daemon) registerLinks(container *container.Container, hostConfig *containertypes.HostConfig) error { if hostConfig == nil || hostConfig.NetworkMode.IsUserDefined() {