From b6df63d5f680663f95081a1e8c97fdeff9ea3f29 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 5 Jul 2017 10:22:57 -0400 Subject: [PATCH] Fix error handling with not-exist errors on remove Specifically, none of the graphdrivers are supposed to return a not-exist type of error on remove (or at least that's how they are currently handled). Found that AUFS still had one case where a not-exist error could escape, when checking if the directory is mounted we call a `Statfs` on the path. This fixes AUFS to not return an error in this case, but also double-checks at the daemon level on layer remove that the error is not a `not-exist` type of error. Signed-off-by: Brian Goff (cherry picked from commit d42dbdd3d48d0134f8bba7ead92a7067791dffab) Signed-off-by: Brian Goff --- components/engine/daemon/delete.go | 2 +- .../engine/daemon/graphdriver/aufs/aufs.go | 54 +++++++++++-------- 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/components/engine/daemon/delete.go b/components/engine/daemon/delete.go index af709445cef..50d2f2972ba 100644 --- a/components/engine/daemon/delete.go +++ b/components/engine/daemon/delete.go @@ -117,7 +117,7 @@ func (daemon *Daemon) cleanupContainer(container *container.Container, forceRemo if container.RWLayer != nil { metadata, err := daemon.layerStore.ReleaseRWLayer(container.RWLayer) layer.LogReleaseMetadata(metadata) - if err != nil && err != layer.ErrMountDoesNotExist { + if err != nil && err != layer.ErrMountDoesNotExist && !os.IsNotExist(errors.Cause(err)) { return errors.Wrapf(err, "driver %q failed to remove root filesystem for %s", daemon.GraphDriverName(), container.ID) } } diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index 85cd5d1d269..163d28727a7 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -37,8 +37,6 @@ import ( "time" "github.com/Sirupsen/logrus" - "github.com/vbatts/tar-split/tar/storage" - "github.com/docker/docker/daemon/graphdriver" "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/chrootarchive" @@ -47,9 +45,11 @@ import ( "github.com/docker/docker/pkg/locker" mountpk "github.com/docker/docker/pkg/mount" "github.com/docker/docker/pkg/system" + "github.com/vbatts/tar-split/tar/storage" rsystem "github.com/opencontainers/runc/libcontainer/system" "github.com/opencontainers/selinux/go-selinux/label" + "github.com/pkg/errors" ) var ( @@ -284,30 +284,41 @@ func (a *Driver) Remove(id string) error { mountpoint = a.getMountpoint(id) } + logger := logrus.WithFields(logrus.Fields{ + "module": "graphdriver", + "driver": "aufs", + "layer": id, + }) + var retries int for { mounted, err := a.mounted(mountpoint) if err != nil { + if os.IsNotExist(err) { + break + } return err } if !mounted { break } - if err := a.unmount(mountpoint); err != nil { - if err != syscall.EBUSY { - return fmt.Errorf("aufs: unmount error: %s: %v", mountpoint, err) - } - if retries >= 5 { - return fmt.Errorf("aufs: unmount error after retries: %s: %v", mountpoint, err) - } - // If unmount returns EBUSY, it could be a transient error. Sleep and retry. - retries++ - logrus.Warnf("unmount failed due to EBUSY: retry count: %d", retries) - time.Sleep(100 * time.Millisecond) - continue + err = a.unmount(mountpoint) + if err == nil { + break + } + + if err != syscall.EBUSY { + return errors.Wrapf(err, "aufs: unmount error: %s", mountpoint) + } + if retries >= 5 { + return errors.Wrapf(err, "aufs: unmount error after retries: %s", mountpoint) } - break + // If unmount returns EBUSY, it could be a transient error. Sleep and retry. + retries++ + logger.Warnf("unmount failed due to EBUSY: retry count: %d", retries) + time.Sleep(100 * time.Millisecond) + continue } // Atomically remove each directory in turn by first moving it out of the @@ -316,21 +327,22 @@ func (a *Driver) Remove(id string) error { tmpMntPath := path.Join(a.mntPath(), fmt.Sprintf("%s-removing", id)) if err := os.Rename(mountpoint, tmpMntPath); err != nil && !os.IsNotExist(err) { if err == syscall.EBUSY { - logrus.Warn("os.Rename err due to EBUSY") + logger.WithField("dir", mountpoint).WithError(err).Warn("os.Rename err due to EBUSY") } - return err + return errors.Wrapf(err, "error preparing atomic delete of aufs mountpoint for id: %s", id) + } + if err := system.EnsureRemoveAll(tmpMntPath); err != nil { + return errors.Wrapf(err, "error removing aufs layer %s", id) } - defer system.EnsureRemoveAll(tmpMntPath) tmpDiffpath := path.Join(a.diffPath(), fmt.Sprintf("%s-removing", id)) if err := os.Rename(a.getDiffPath(id), tmpDiffpath); err != nil && !os.IsNotExist(err) { - return err + return errors.Wrapf(err, "error preparing atomic delete of aufs diff dir for id: %s", id) } - defer system.EnsureRemoveAll(tmpDiffpath) // Remove the layers file for the id if err := os.Remove(path.Join(a.rootPath(), "layers", id)); err != nil && !os.IsNotExist(err) { - return err + return errors.Wrapf(err, "error removing layers dir for %s", id) } a.pathCacheLock.Lock()