From c81448bb56c0083877ec45fffe2fc4d196bbde3a Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 22 Oct 2018 18:30:34 -0700 Subject: [PATCH] pkg/mount: wrap mount/umount errors The errors returned from Mount and Unmount functions are raw syscall.Errno errors (like EPERM or EINVAL), which provides no context about what has happened and why. Similar to os.PathError type, introduce mount.Error type with some context. The error messages will now look like this: > mount /tmp/mount-tests/source:/tmp/mount-tests/target, flags: 0x1001: operation not permitted or > mount tmpfs:/tmp/mount-test-source-516297835: operation not permitted Before this patch, it was just > operation not permitted [v2: add Cause()] [v3: rename MountError to Error, document Cause()] [v4: fixes; audited all users] [v5: make Error type private; changes after @cpuguy83 reviews] Signed-off-by: Kir Kolyshkin (cherry picked from commit 65331369617e89ce54cc9be080dba70f3a883d1c) Signed-off-by: Kir Kolyshkin Upstream-commit: 7f1c6bf5a745c8faeba695d3556dff4c4ff5f473 Component: engine --- components/engine/container/container_unix.go | 5 +-- .../engine/daemon/graphdriver/btrfs/btrfs.go | 2 +- .../daemon/graphdriver/devmapper/deviceset.go | 4 +-- .../daemon/graphdriver/devmapper/driver.go | 7 +--- .../engine/daemon/graphdriver/zfs/zfs.go | 3 +- components/engine/pkg/mount/mount.go | 35 +++++++++++++++++++ .../engine/pkg/mount/mounter_freebsd.go | 11 ++++-- components/engine/pkg/mount/mounter_linux.go | 25 +++++++++++-- .../pkg/mount/sharedsubtree_linux_test.go | 3 +- components/engine/pkg/mount/unmount_unix.go | 11 ++++-- components/engine/plugin/manager_linux.go | 2 +- components/engine/volume/local/local.go | 2 +- components/engine/volume/local/local_unix.go | 2 +- 13 files changed, 88 insertions(+), 24 deletions(-) diff --git a/components/engine/container/container_unix.go b/components/engine/container/container_unix.go index 592a6dc375..6d402be3a0 100644 --- a/components/engine/container/container_unix.go +++ b/components/engine/container/container_unix.go @@ -191,7 +191,7 @@ func (container *Container) UnmountIpcMount() error { return nil } if err = mount.Unmount(shmPath); err != nil && !os.IsNotExist(err) { - return errors.Wrapf(err, "umount %s", shmPath) + return err } return nil } @@ -381,7 +381,8 @@ func (container *Container) DetachAndUnmount(volumeEventLog func(name, action st for _, mountPath := range mountPaths { if err := mount.Unmount(mountPath); err != nil { - logrus.Warnf("%s unmountVolumes: Failed to do lazy umount for volume '%s': %v", container.ID, mountPath, err) + logrus.WithError(err).WithField("container", container.ID). + Warn("Unable to unmount") } } return container.UnmountVolumes(volumeEventLog) diff --git a/components/engine/daemon/graphdriver/btrfs/btrfs.go b/components/engine/daemon/graphdriver/btrfs/btrfs.go index 7ce7edef36..fcaedc6eab 100644 --- a/components/engine/daemon/graphdriver/btrfs/btrfs.go +++ b/components/engine/daemon/graphdriver/btrfs/btrfs.go @@ -178,7 +178,7 @@ func (d *Driver) Cleanup() error { } if umountErr != nil { - return errors.Wrapf(umountErr, "error unmounting %s", d.home) + return umountErr } return nil diff --git a/components/engine/daemon/graphdriver/devmapper/deviceset.go b/components/engine/daemon/graphdriver/devmapper/deviceset.go index 5dc01d71d9..c41b50c119 100644 --- a/components/engine/daemon/graphdriver/devmapper/deviceset.go +++ b/components/engine/daemon/graphdriver/devmapper/deviceset.go @@ -1200,7 +1200,7 @@ func (devices *DeviceSet) growFS(info *devInfo) error { options = joinMountOptions(options, devices.mountOptions) if err := mount.Mount(info.DevName(), fsMountPoint, devices.BaseDeviceFilesystem, options); err != nil { - return fmt.Errorf("Error mounting '%s' on '%s' (fstype='%s' options='%s'): %s\n%v", info.DevName(), fsMountPoint, devices.BaseDeviceFilesystem, options, err, string(dmesg.Dmesg(256))) + return errors.Wrapf(err, "Failed to mount; dmesg: %s", string(dmesg.Dmesg(256))) } defer unix.Unmount(fsMountPoint, unix.MNT_DETACH) @@ -2381,7 +2381,7 @@ func (devices *DeviceSet) MountDevice(hash, path, mountLabel string) error { options = joinMountOptions(options, label.FormatMountLabel("", mountLabel)) if err := mount.Mount(info.DevName(), path, fstype, options); err != nil { - return fmt.Errorf("devmapper: Error mounting '%s' on '%s' (fstype='%s' options='%s'): %s\n%v", info.DevName(), path, fstype, options, err, string(dmesg.Dmesg(256))) + return errors.Wrapf(err, "Failed to mount; dmesg: %s", string(dmesg.Dmesg(256))) } if fstype == "xfs" && devices.xfsNospaceRetries != "" { diff --git a/components/engine/daemon/graphdriver/devmapper/driver.go b/components/engine/daemon/graphdriver/devmapper/driver.go index 899b1f8670..a42a03ba90 100644 --- a/components/engine/daemon/graphdriver/devmapper/driver.go +++ b/components/engine/daemon/graphdriver/devmapper/driver.go @@ -16,7 +16,6 @@ import ( "github.com/docker/docker/pkg/locker" "github.com/docker/docker/pkg/mount" "github.com/docker/go-units" - "github.com/pkg/errors" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" ) @@ -129,11 +128,7 @@ func (d *Driver) Cleanup() error { return err } - if umountErr != nil { - return errors.Wrapf(umountErr, "error unmounting %s", d.home) - } - - return nil + return umountErr } // CreateReadWrite creates a layer that is writable for use as a container diff --git a/components/engine/daemon/graphdriver/zfs/zfs.go b/components/engine/daemon/graphdriver/zfs/zfs.go index 8a798778d2..c83446cf8f 100644 --- a/components/engine/daemon/graphdriver/zfs/zfs.go +++ b/components/engine/daemon/graphdriver/zfs/zfs.go @@ -19,6 +19,7 @@ import ( "github.com/docker/docker/pkg/parsers" "github.com/mistifyio/go-zfs" "github.com/opencontainers/selinux/go-selinux/label" + "github.com/pkg/errors" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" ) @@ -390,7 +391,7 @@ func (d *Driver) Get(id, mountLabel string) (_ containerfs.ContainerFS, retErr e } if err := mount.Mount(filesystem, mountpoint, "zfs", options); err != nil { - return nil, fmt.Errorf("error creating zfs mount of %s to %s: %v", filesystem, mountpoint, err) + return nil, errors.Wrap(err, "error creating zfs mount") } // this could be our first mount after creation of the filesystem, and the root dir may still have root diff --git a/components/engine/pkg/mount/mount.go b/components/engine/pkg/mount/mount.go index 2d79bc390d..4afd63c427 100644 --- a/components/engine/pkg/mount/mount.go +++ b/components/engine/pkg/mount/mount.go @@ -2,11 +2,46 @@ package mount // import "github.com/docker/docker/pkg/mount" import ( "sort" + "strconv" "strings" "github.com/sirupsen/logrus" ) +// mountError records an error from mount or unmount operation +type mountError struct { + op string + source, target string + flags uintptr + data string + err error +} + +func (e *mountError) Error() string { + out := e.op + " " + + if e.source != "" { + out += e.source + ":" + e.target + } else { + out += e.target + } + + if e.flags != uintptr(0) { + out += ", flags: 0x" + strconv.FormatUint(uint64(e.flags), 16) + } + if e.data != "" { + out += ", data: " + e.data + } + + out += ": " + e.err.Error() + return out +} + +// Cause returns the underlying cause of the error +func (e *mountError) Cause() error { + return e.err +} + // FilterFunc is a type defining a callback function // to filter out unwanted entries. It takes a pointer // to an Info struct (not fully populated, currently diff --git a/components/engine/pkg/mount/mounter_freebsd.go b/components/engine/pkg/mount/mounter_freebsd.go index 8a46eaf454..09ad360608 100644 --- a/components/engine/pkg/mount/mounter_freebsd.go +++ b/components/engine/pkg/mount/mounter_freebsd.go @@ -11,8 +11,8 @@ package mount // import "github.com/docker/docker/pkg/mount" import "C" import ( - "fmt" "strings" + "syscall" "unsafe" ) @@ -47,8 +47,13 @@ func mount(device, target, mType string, flag uintptr, data string) error { } if errno := C.nmount(&rawOptions[0], C.uint(len(options)), C.int(flag)); errno != 0 { - reason := C.GoString(C.strerror(*C.__error())) - return fmt.Errorf("Failed to call nmount: %s", reason) + return &mountError{ + op: "mount", + source: device, + target: target, + flags: flag, + err: syscall.Errno(errno), + } } return nil } diff --git a/components/engine/pkg/mount/mounter_linux.go b/components/engine/pkg/mount/mounter_linux.go index 2c189de0ae..48837adde5 100644 --- a/components/engine/pkg/mount/mounter_linux.go +++ b/components/engine/pkg/mount/mounter_linux.go @@ -33,20 +33,41 @@ func mount(device, target, mType string, flags uintptr, data string) error { // Initial call applying all non-propagation flags for mount // or remount with changed data if err := unix.Mount(device, target, mType, oflags, data); err != nil { - return err + return &mountError{ + op: "mount", + source: device, + target: target, + flags: oflags, + data: data, + err: err, + } } } if flags&ptypes != 0 { // Change the propagation type. if err := unix.Mount("", target, "", flags&pflags, ""); err != nil { + return &mountError{ + op: "remount", + target: target, + flags: flags & pflags, + err: err, + } return err } } if oflags&broflags == broflags { // Remount the bind to apply read only. - return unix.Mount("", target, "", oflags|unix.MS_REMOUNT, "") + if err := unix.Mount("", target, "", oflags|unix.MS_REMOUNT, ""); err != nil { + return &mountError{ + op: "remount-ro", + target: target, + flags: oflags | unix.MS_REMOUNT, + err: err, + } + + } } return nil diff --git a/components/engine/pkg/mount/sharedsubtree_linux_test.go b/components/engine/pkg/mount/sharedsubtree_linux_test.go index 019514491f..7a37f66098 100644 --- a/components/engine/pkg/mount/sharedsubtree_linux_test.go +++ b/components/engine/pkg/mount/sharedsubtree_linux_test.go @@ -7,6 +7,7 @@ import ( "path" "testing" + "github.com/pkg/errors" "golang.org/x/sys/unix" ) @@ -326,7 +327,7 @@ func TestSubtreeUnbindable(t *testing.T) { }() // then attempt to mount it to target. It should fail - if err := Mount(sourceDir, targetDir, "none", "bind,rw"); err != nil && err != unix.EINVAL { + if err := Mount(sourceDir, targetDir, "none", "bind,rw"); err != nil && errors.Cause(err) != unix.EINVAL { t.Fatal(err) } else if err == nil { t.Fatalf("%q should not have been bindable", sourceDir) diff --git a/components/engine/pkg/mount/unmount_unix.go b/components/engine/pkg/mount/unmount_unix.go index d027e3086d..4be4276851 100644 --- a/components/engine/pkg/mount/unmount_unix.go +++ b/components/engine/pkg/mount/unmount_unix.go @@ -6,12 +6,17 @@ import "golang.org/x/sys/unix" func unmount(target string, flags int) error { err := unix.Unmount(target, flags) - if err == unix.EINVAL { + if err == nil || err == unix.EINVAL { // Ignore "not mounted" error here. Note the same error // can be returned if flags are invalid, so this code // assumes that the flags value is always correct. - err = nil + return nil } - return err + return &mountError{ + op: "umount", + target: target, + flags: uintptr(flags), + err: err, + } } diff --git a/components/engine/plugin/manager_linux.go b/components/engine/plugin/manager_linux.go index df1fe5b73d..86ada8d02f 100644 --- a/components/engine/plugin/manager_linux.go +++ b/components/engine/plugin/manager_linux.go @@ -61,7 +61,7 @@ func (pm *Manager) enable(p *v2.Plugin, c *controller, force bool) error { if err := pm.executor.Create(p.GetID(), *spec, stdout, stderr); err != nil { if p.PluginObj.Config.PropagatedMount != "" { if err := mount.Unmount(propRoot); err != nil { - logrus.Warnf("Could not unmount %s: %v", propRoot, err) + logrus.WithField("plugin", p.Name()).WithError(err).Warn("Failed to unmount vplugin propagated mount root") } } return errors.WithStack(err) diff --git a/components/engine/volume/local/local.go b/components/engine/volume/local/local.go index 7190de9ed6..d3119cb2ff 100644 --- a/components/engine/volume/local/local.go +++ b/components/engine/volume/local/local.go @@ -344,7 +344,7 @@ func (v *localVolume) unmount() error { if v.opts != nil { if err := mount.Unmount(v.path); err != nil { if mounted, mErr := mount.Mounted(v.path); mounted || mErr != nil { - return errdefs.System(errors.Wrapf(err, "error while unmounting volume path '%s'", v.path)) + return errdefs.System(err) } } v.active.mounted = false diff --git a/components/engine/volume/local/local_unix.go b/components/engine/volume/local/local_unix.go index b1c68b931b..5ee2ed894b 100644 --- a/components/engine/volume/local/local_unix.go +++ b/components/engine/volume/local/local_unix.go @@ -86,7 +86,7 @@ func (v *localVolume) mount() error { } } err := mount.Mount(v.opts.MountDevice, v.path, v.opts.MountType, mountOpts) - return errors.Wrapf(err, "error while mounting volume with options: %s", v.opts) + return errors.Wrap(err, "failed to mount local volume") } func (v *localVolume) CreatedAt() (time.Time, error) {