From 8f7dd9a1c4326c4221d0078b0e0b32fb94b5f48b Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 23 Jan 2018 17:17:13 -0800 Subject: [PATCH 1/5] devmapper cleanup: improve error msg 1. Make sure it's clear the error is from unmount. 2. Simplify the code a bit to make it more readable. [v2: use errors.Wrap] [v3: use errors.Wrapf] [v4: lowercase the error message] Signed-off-by: Kir Kolyshkin Upstream-commit: 9d00aedebc25507042c5afd4ab8fc6b333ca7c53 Component: engine --- .../engine/daemon/graphdriver/devmapper/driver.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/components/engine/daemon/graphdriver/devmapper/driver.go b/components/engine/daemon/graphdriver/devmapper/driver.go index e27f3c85cc..6bed6634cf 100644 --- a/components/engine/daemon/graphdriver/devmapper/driver.go +++ b/components/engine/daemon/graphdriver/devmapper/driver.go @@ -16,6 +16,7 @@ import ( "github.com/docker/docker/pkg/locker" "github.com/docker/docker/pkg/mount" units "github.com/docker/go-units" + "github.com/pkg/errors" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" ) @@ -121,12 +122,18 @@ func (d *Driver) GetMetadata(id string) (map[string]string, error) { // Cleanup unmounts a device. func (d *Driver) Cleanup() error { err := d.DeviceSet.Shutdown(d.home) + umountErr := mount.RecursiveUnmount(d.home) - if err2 := mount.RecursiveUnmount(d.home); err == nil { - err = err2 + // in case we have two errors, prefer the one from Shutdown() + if err != nil { + return err } - return err + if umountErr != nil { + return errors.Wrapf(umountErr, "error unmounting %s", d.home) + } + + return nil } // CreateReadWrite creates a layer that is writable for use as a container From 8b5e7a334b4cbaab050c80e347fda828f7574fdd Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 13 Feb 2018 16:10:53 -0800 Subject: [PATCH 2/5] devmapper.shutdown: optimize Move the "unmount and deactivate" code into a separate method, and optimize it a bit: 1. Do not use filepath.Walk() as there's no requirement to recursively go into every directory under home/mnt; a list of directories in mnt is sufficient. With filepath.Walk(), in case some container will fail to unmount, it'll go through the whole container filesystem which is excessive and useless. 2. Do not use GetMounts() and check if a directory is mounted; just unmount it and ignore "not mounted" error. Note the same error is returned in case of wrong flags set, but as flags are hardcoded we can safely ignore such case. While at it, promote "can't unmount" log level from debug to warning. Signed-off-by: Kir Kolyshkin Upstream-commit: f1a459229724f5e8e440b49f058167c2eeeb2dc6 Component: engine --- .../daemon/graphdriver/devmapper/deviceset.go | 72 +++++++++---------- 1 file changed, 33 insertions(+), 39 deletions(-) diff --git a/components/engine/daemon/graphdriver/devmapper/deviceset.go b/components/engine/daemon/graphdriver/devmapper/deviceset.go index 3926648e29..ca6251023b 100644 --- a/components/engine/daemon/graphdriver/devmapper/deviceset.go +++ b/components/engine/daemon/graphdriver/devmapper/deviceset.go @@ -2223,6 +2223,38 @@ func (devices *DeviceSet) cancelDeferredRemoval(info *devInfo) error { return err } +func (devices *DeviceSet) unmountAndDeactivateAll(dir string) { + files, err := ioutil.ReadDir(dir) + if err != nil { + logrus.Warnf("devmapper: unmountAndDeactivate: %s", err) + return + } + + for _, d := range files { + if !d.IsDir() { + continue + } + + name := d.Name() + fullname := path.Join(dir, name) + + // We use MNT_DETACH here in case it is still busy in some running + // container. This means it'll go away from the global scope directly, + // and the device will be released when that container dies. + if err := unix.Unmount(fullname, unix.MNT_DETACH); err != nil && err != unix.EINVAL { + logrus.Warnf("devmapper: Shutdown unmounting %s, error: %s", fullname, err) + } + + if devInfo, err := devices.lookupDevice(name); err != nil { + logrus.Debugf("devmapper: Shutdown lookup device %s, error: %s", name, err) + } else { + if err := devices.deactivateDevice(devInfo); err != nil { + logrus.Debugf("devmapper: Shutdown deactivate %s, error: %s", devInfo.Hash, err) + } + } + } +} + // Shutdown shuts down the device by unmounting the root. func (devices *DeviceSet) Shutdown(home string) error { logrus.Debugf("devmapper: [deviceset %s] Shutdown()", devices.devicePrefix) @@ -2244,45 +2276,7 @@ func (devices *DeviceSet) Shutdown(home string) error { // will be killed and we will not get a chance to save deviceset // metadata. Hence save this early before trying to deactivate devices. devices.saveDeviceSetMetaData() - - // ignore the error since it's just a best effort to not try to unmount something that's mounted - mounts, _ := mount.GetMounts() - mounted := make(map[string]bool, len(mounts)) - for _, mnt := range mounts { - mounted[mnt.Mountpoint] = true - } - - if err := filepath.Walk(path.Join(home, "mnt"), func(p string, info os.FileInfo, err error) error { - if err != nil { - return err - } - if !info.IsDir() { - return nil - } - - if mounted[p] { - // We use MNT_DETACH here in case it is still busy in some running - // container. This means it'll go away from the global scope directly, - // and the device will be released when that container dies. - if err := unix.Unmount(p, unix.MNT_DETACH); err != nil { - logrus.Debugf("devmapper: Shutdown unmounting %s, error: %s", p, err) - } - } - - if devInfo, err := devices.lookupDevice(path.Base(p)); err != nil { - logrus.Debugf("devmapper: Shutdown lookup device %s, error: %s", path.Base(p), err) - } else { - if err := devices.deactivateDevice(devInfo); err != nil { - logrus.Debugf("devmapper: Shutdown deactivate %s , error: %s", devInfo.Hash, err) - } - } - - return nil - }); err != nil && !os.IsNotExist(err) { - devices.Unlock() - return err - } - + devices.unmountAndDeactivateAll(path.Join(home, "mnt")) devices.Unlock() info, _ := devices.lookupDeviceWithLock("") From db90a864e8ac0ef4d6212d4b0c5bda56c2bc8c8a Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 25 Jan 2018 20:30:24 -0800 Subject: [PATCH 3/5] pkg/mount unit tests: skip some test under non-root This makes `go test .` to pass if run as non-root user, skipping those tests that require superuser privileges (for `mount`). Signed-off-by: Kir Kolyshkin Upstream-commit: 4aae77602a7540b4f977572f3fbdc0891ac57cab Component: engine --- components/engine/pkg/mount/mount_unix_test.go | 8 ++++++++ .../engine/pkg/mount/mounter_linux_test.go | 2 +- .../engine/pkg/mount/sharedsubtree_linux_test.go | 16 ++++++++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/components/engine/pkg/mount/mount_unix_test.go b/components/engine/pkg/mount/mount_unix_test.go index f3333d5b31..84699eee5e 100644 --- a/components/engine/pkg/mount/mount_unix_test.go +++ b/components/engine/pkg/mount/mount_unix_test.go @@ -25,6 +25,10 @@ func TestMountOptionsParsing(t *testing.T) { } func TestMounted(t *testing.T) { + if os.Getuid() != 0 { + t.Skip("root required") + } + tmp := path.Join(os.TempDir(), "mount-tests") if err := os.MkdirAll(tmp, 0777); err != nil { t.Fatal(err) @@ -76,6 +80,10 @@ func TestMounted(t *testing.T) { } func TestMountReadonly(t *testing.T) { + if os.Getuid() != 0 { + t.Skip("root required") + } + tmp := path.Join(os.TempDir(), "mount-tests") if err := os.MkdirAll(tmp, 0777); err != nil { t.Fatal(err) diff --git a/components/engine/pkg/mount/mounter_linux_test.go b/components/engine/pkg/mount/mounter_linux_test.go index ac858e269b..e5da438efe 100644 --- a/components/engine/pkg/mount/mounter_linux_test.go +++ b/components/engine/pkg/mount/mounter_linux_test.go @@ -14,7 +14,7 @@ import ( func TestMount(t *testing.T) { if os.Getuid() != 0 { - t.Skip("not root tests would fail") + t.Skip("root required") } source, err := ioutil.TempDir("", "mount-test-source-") diff --git a/components/engine/pkg/mount/sharedsubtree_linux_test.go b/components/engine/pkg/mount/sharedsubtree_linux_test.go index 0cb2b959cd..019514491f 100644 --- a/components/engine/pkg/mount/sharedsubtree_linux_test.go +++ b/components/engine/pkg/mount/sharedsubtree_linux_test.go @@ -12,6 +12,10 @@ import ( // nothing is propagated in or out func TestSubtreePrivate(t *testing.T) { + if os.Getuid() != 0 { + t.Skip("root required") + } + tmp := path.Join(os.TempDir(), "mount-tests") if err := os.MkdirAll(tmp, 0777); err != nil { t.Fatal(err) @@ -110,6 +114,10 @@ func TestSubtreePrivate(t *testing.T) { // Testing that when a target is a shared mount, // then child mounts propagate to the source func TestSubtreeShared(t *testing.T) { + if os.Getuid() != 0 { + t.Skip("root required") + } + tmp := path.Join(os.TempDir(), "mount-tests") if err := os.MkdirAll(tmp, 0777); err != nil { t.Fatal(err) @@ -178,6 +186,10 @@ func TestSubtreeShared(t *testing.T) { // testing that mounts to a shared source show up in the slave target, // and that mounts into a slave target do _not_ show up in the shared source func TestSubtreeSharedSlave(t *testing.T) { + if os.Getuid() != 0 { + t.Skip("root required") + } + tmp := path.Join(os.TempDir(), "mount-tests") if err := os.MkdirAll(tmp, 0777); err != nil { t.Fatal(err) @@ -282,6 +294,10 @@ func TestSubtreeSharedSlave(t *testing.T) { } func TestSubtreeUnbindable(t *testing.T) { + if os.Getuid() != 0 { + t.Skip("root required") + } + tmp := path.Join(os.TempDir(), "mount-tests") if err := os.MkdirAll(tmp, 0777); err != nil { t.Fatal(err) From b6d64a77d2ec2b4fa3300dd035fef16348007d0b Mon Sep 17 00:00:00 2001 From: Eli Uriegas Date: Wed, 7 Mar 2018 21:29:10 +0000 Subject: [PATCH 4/5] Build containerd, runc, and proxy statically These were originally static binaries in the first place, this changes them back to that. Signed-off-by: Eli Uriegas Upstream-commit: 63c7bb24637fdbfd905096ecc75b435ecefd31e9 Component: engine --- .../hack/dockerfile/install/containerd.installer | 13 +++++++++---- .../engine/hack/dockerfile/install/proxy.installer | 3 ++- .../engine/hack/dockerfile/install/runc.installer | 7 ++++++- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/components/engine/hack/dockerfile/install/containerd.installer b/components/engine/hack/dockerfile/install/containerd.installer index 5731a6d560..1b42a48bf6 100755 --- a/components/engine/hack/dockerfile/install/containerd.installer +++ b/components/engine/hack/dockerfile/install/containerd.installer @@ -14,10 +14,15 @@ install_containerd() { ( - if [ "$1" == "static" ]; then - export BUILDTAGS='static_build netgo' - export EXTRA_FLAGS='-buildmod pie' - export EXTRA_LDFLAGS='-extldflags "-fno-PIC -static"' + export BUILDTAGS='static_build netgo' + export EXTRA_FLAGS='-buildmod pie' + export EXTRA_LDFLAGS='-extldflags "-fno-PIC -static"' + + # Reset build flags to nothing if we want a dynbinary + if [ "$1" == "dynamic" ]; then + export BUILDTAGS='' + export EXTRA_FLAGS='' + export EXTRA_LDFLAGS='' fi make diff --git a/components/engine/hack/dockerfile/install/proxy.installer b/components/engine/hack/dockerfile/install/proxy.installer index 598f67c3fb..ed9ea7cbce 100755 --- a/components/engine/hack/dockerfile/install/proxy.installer +++ b/components/engine/hack/dockerfile/install/proxy.installer @@ -23,6 +23,7 @@ install_proxy() { install_proxy_dynamic() { export PROXY_LDFLAGS="-linkmode=external" install_proxy + export BUILD_MODE="-buildmode=pie" _install_proxy } @@ -31,7 +32,7 @@ _install_proxy() { git clone https://github.com/docker/libnetwork.git "$GOPATH/src/github.com/docker/libnetwork" cd "$GOPATH/src/github.com/docker/libnetwork" git checkout -q "$LIBNETWORK_COMMIT" - go build -buildmode=pie -ldflags="$PROXY_LDFLAGS" -o ${PREFIX}/docker-proxy github.com/docker/libnetwork/cmd/proxy + go build $BUILD_MODE -ldflags="$PROXY_LDFLAGS" -o ${PREFIX}/docker-proxy github.com/docker/libnetwork/cmd/proxy } diff --git a/components/engine/hack/dockerfile/install/runc.installer b/components/engine/hack/dockerfile/install/runc.installer index 8ec5efc44e..054f95d66a 100755 --- a/components/engine/hack/dockerfile/install/runc.installer +++ b/components/engine/hack/dockerfile/install/runc.installer @@ -11,7 +11,12 @@ install_runc() { git clone https://github.com/opencontainers/runc.git "$GOPATH/src/github.com/opencontainers/runc" cd "$GOPATH/src/github.com/opencontainers/runc" git checkout -q "$RUNC_COMMIT" - make BUILDTAGS="$RUNC_BUILDTAGS" $1 + if [ -z "$1" ]; then + target=static + else + target="$1" + fi + make BUILDTAGS="$RUNC_BUILDTAGS" "$target" mkdir -p ${PREFIX} cp runc ${PREFIX}/docker-runc } From 3398cf7be9db2dad30b9d10e01424696d7237b58 Mon Sep 17 00:00:00 2001 From: Eli Uriegas Date: Wed, 7 Mar 2018 21:36:23 +0000 Subject: [PATCH 5/5] buildmod => buildmode There was a typo with the buildmode flag for containerd Signed-off-by: Eli Uriegas Upstream-commit: 5e4885b9afb1de30133627ce751af2c0e7b72a4e Component: engine --- components/engine/hack/dockerfile/install/containerd.installer | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/engine/hack/dockerfile/install/containerd.installer b/components/engine/hack/dockerfile/install/containerd.installer index 1b42a48bf6..d6ab8371ae 100755 --- a/components/engine/hack/dockerfile/install/containerd.installer +++ b/components/engine/hack/dockerfile/install/containerd.installer @@ -15,7 +15,7 @@ install_containerd() { ( export BUILDTAGS='static_build netgo' - export EXTRA_FLAGS='-buildmod pie' + export EXTRA_FLAGS='-buildmode=pie' export EXTRA_LDFLAGS='-extldflags "-fno-PIC -static"' # Reset build flags to nothing if we want a dynbinary