From 276b44608b04f08bdf46ce7c816b1f744bf24b7d Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Mon, 21 Aug 2017 17:38:13 -0400 Subject: [PATCH] Fix error removing diff path In d42dbdd3d48d0134f8bba7ead92a7067791dffab the code was re-arranged to better report errors, and ignore non-errors. In doing so we removed a deferred remove of the AUFS diff path, but did not replace it with a non-deferred one. This fixes the issue and makes the code a bit more readable. Signed-off-by: Brian Goff --- daemon/graphdriver/aufs/aufs.go | 69 ++++++++++++++++++++-------- daemon/graphdriver/aufs/aufs_test.go | 27 ++++++++++- 2 files changed, 75 insertions(+), 21 deletions(-) diff --git a/daemon/graphdriver/aufs/aufs.go b/daemon/graphdriver/aufs/aufs.go index 81edb8ea94..7b574f9f64 100644 --- a/daemon/graphdriver/aufs/aufs.go +++ b/daemon/graphdriver/aufs/aufs.go @@ -142,6 +142,23 @@ func Init(root string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap } } + for _, path := range []string{"mnt", "diff"} { + p := filepath.Join(root, path) + dirs, err := ioutil.ReadDir(p) + if err != nil { + logrus.WithError(err).WithField("dir", p).Error("error reading dir entries") + continue + } + for _, dir := range dirs { + if strings.HasSuffix(dir.Name(), "-removing") { + logrus.WithField("dir", dir.Name()).Debug("Cleaning up stale layer dir") + if err := system.EnsureRemoveAll(filepath.Join(p, dir.Name())); err != nil { + logrus.WithField("dir", dir.Name()).WithError(err).Error("Error removing stale layer dir") + } + } + } + } + a.naiveDiff = graphdriver.NewNaiveDiffDriver(a, uidMaps, gidMaps) return a, nil } @@ -317,26 +334,6 @@ func (a *Driver) Remove(id string) error { 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 - // way (so that docker doesn't find it anymore) before doing removal of - // the whole tree. - tmpMntPath := path.Join(a.mntPath(), fmt.Sprintf("%s-removing", id)) - if err := os.Rename(mountpoint, tmpMntPath); err != nil && !os.IsNotExist(err) { - if err == unix.EBUSY { - logger.WithField("dir", mountpoint).WithError(err).Warn("os.Rename err due to EBUSY") - } - 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) - } - - tmpDiffpath := path.Join(a.diffPath(), fmt.Sprintf("%s-removing", id)) - if err := os.Rename(a.getDiffPath(id), tmpDiffpath); err != nil && !os.IsNotExist(err) { - return errors.Wrapf(err, "error preparing atomic delete of aufs diff dir for id: %s", id) } // Remove the layers file for the id @@ -344,12 +341,44 @@ func (a *Driver) Remove(id string) error { return errors.Wrapf(err, "error removing layers dir for %s", id) } + if err := atomicRemove(a.getDiffPath(id)); err != nil { + return errors.Wrapf(err, "could not remove diff path for id %s", id) + } + + // Atomically remove each directory in turn by first moving it out of the + // way (so that docker doesn't find it anymore) before doing removal of + // the whole tree. + if err := atomicRemove(mountpoint); err != nil { + if errors.Cause(err) == unix.EBUSY { + logger.WithField("dir", mountpoint).WithError(err).Warn("error performing atomic remove due to EBUSY") + } + return errors.Wrapf(err, "could not remove mountpoint for id %s", id) + } + a.pathCacheLock.Lock() delete(a.pathCache, id) a.pathCacheLock.Unlock() return nil } +func atomicRemove(source string) error { + target := source + "-removing" + + err := os.Rename(source, target) + switch { + case err == nil, os.IsNotExist(err): + case os.IsExist(err): + // Got error saying the target dir already exists, maybe the source doesn't exist due to a previous (failed) remove + if _, e := os.Stat(source); !os.IsNotExist(e) { + return errors.Wrapf(err, "target rename dir '%s' exists but should not, this needs to be manually cleaned up") + } + default: + return errors.Wrapf(err, "error preparing atomic delete") + } + + return system.EnsureRemoveAll(target) +} + // Get returns the rootfs path for the id. // This will mount the dir at its given path func (a *Driver) Get(id, mountLabel string) (string, error) { diff --git a/daemon/graphdriver/aufs/aufs_test.go b/daemon/graphdriver/aufs/aufs_test.go index baf0fd89f2..532074eee6 100644 --- a/daemon/graphdriver/aufs/aufs_test.go +++ b/daemon/graphdriver/aufs/aufs_test.go @@ -12,6 +12,8 @@ import ( "sync" "testing" + "path/filepath" + "github.com/docker/docker/daemon/graphdriver" "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/reexec" @@ -147,7 +149,10 @@ func TestRemoveImage(t *testing.T) { for _, p := range paths { if _, err := os.Stat(path.Join(tmp, p, "1")); err == nil { - t.Fatalf("Error should not be nil because dirs with id 1 should be delted: %s", p) + t.Fatalf("Error should not be nil because dirs with id 1 should be deleted: %s", p) + } + if _, err := os.Stat(path.Join(tmp, p, "1-removing")); err == nil { + t.Fatalf("Error should not be nil because dirs with id 1-removing should be deleted: %s", p) } } } @@ -800,3 +805,23 @@ func BenchmarkConcurrentAccess(b *testing.B) { } } } + +func TestInitStaleCleanup(t *testing.T) { + if err := os.MkdirAll(tmp, 0755); err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmp) + + for _, d := range []string{"diff", "mnt"} { + if err := os.MkdirAll(filepath.Join(tmp, d, "123-removing"), 0755); err != nil { + t.Fatal(err) + } + } + + testInit(tmp, t) + for _, d := range []string{"diff", "mnt"} { + if _, err := os.Stat(filepath.Join(tmp, d, "123-removing")); err == nil { + t.Fatal("cleanup failed") + } + } +}