Merge pull request #34587 from cpuguy83/aufs_rm_errors

Fix error removing diff path

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) {

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")
+		}
+	}
+}