Browse Source

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 <cpuguy83@gmail.com>
Brian Goff 8 years ago
parent
commit
d42dbdd3d4
2 changed files with 33 additions and 20 deletions
  1. 1 1
      daemon/delete.go
  2. 32 19
      daemon/graphdriver/aufs/aufs.go

+ 1 - 1
daemon/delete.go

@@ -119,7 +119,7 @@ func (daemon *Daemon) cleanupContainer(container *container.Container, forceRemo
 	if container.RWLayer != nil {
 	if container.RWLayer != nil {
 		metadata, err := daemon.stores[container.Platform].layerStore.ReleaseRWLayer(container.RWLayer)
 		metadata, err := daemon.stores[container.Platform].layerStore.ReleaseRWLayer(container.RWLayer)
 		layer.LogReleaseMetadata(metadata)
 		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.Platform), container.ID)
 			return errors.Wrapf(err, "driver %q failed to remove root filesystem for %s", daemon.GraphDriverName(container.Platform), container.ID)
 		}
 		}
 	}
 	}

+ 32 - 19
daemon/graphdriver/aufs/aufs.go

@@ -46,6 +46,7 @@ import (
 	"github.com/docker/docker/pkg/system"
 	"github.com/docker/docker/pkg/system"
 	rsystem "github.com/opencontainers/runc/libcontainer/system"
 	rsystem "github.com/opencontainers/runc/libcontainer/system"
 	"github.com/opencontainers/selinux/go-selinux/label"
 	"github.com/opencontainers/selinux/go-selinux/label"
+	"github.com/pkg/errors"
 	"github.com/vbatts/tar-split/tar/storage"
 	"github.com/vbatts/tar-split/tar/storage"
 	"golang.org/x/sys/unix"
 	"golang.org/x/sys/unix"
 )
 )
@@ -282,30 +283,41 @@ func (a *Driver) Remove(id string) error {
 		mountpoint = a.getMountpoint(id)
 		mountpoint = a.getMountpoint(id)
 	}
 	}
 
 
+	logger := logrus.WithFields(logrus.Fields{
+		"module": "graphdriver",
+		"driver": "aufs",
+		"layer":  id,
+	})
+
 	var retries int
 	var retries int
 	for {
 	for {
 		mounted, err := a.mounted(mountpoint)
 		mounted, err := a.mounted(mountpoint)
 		if err != nil {
 		if err != nil {
+			if os.IsNotExist(err) {
+				break
+			}
 			return err
 			return err
 		}
 		}
 		if !mounted {
 		if !mounted {
 			break
 			break
 		}
 		}
 
 
-		if err := a.unmount(mountpoint); err != nil {
-			if err != unix.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
 		}
 		}
-		break
+
+		if err != unix.EBUSY {
+			return errors.Wrapf(err, "aufs: unmount error: %s", mountpoint)
+		}
+		if retries >= 5 {
+			return errors.Wrapf(err, "aufs: unmount error after retries: %s", mountpoint)
+		}
+		// 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
 	// Atomically remove each directory in turn by first moving it out of the
@@ -314,21 +326,22 @@ func (a *Driver) Remove(id string) error {
 	tmpMntPath := path.Join(a.mntPath(), fmt.Sprintf("%s-removing", id))
 	tmpMntPath := path.Join(a.mntPath(), fmt.Sprintf("%s-removing", id))
 	if err := os.Rename(mountpoint, tmpMntPath); err != nil && !os.IsNotExist(err) {
 	if err := os.Rename(mountpoint, tmpMntPath); err != nil && !os.IsNotExist(err) {
 		if err == unix.EBUSY {
 		if err == unix.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))
 	tmpDiffpath := path.Join(a.diffPath(), fmt.Sprintf("%s-removing", id))
 	if err := os.Rename(a.getDiffPath(id), tmpDiffpath); err != nil && !os.IsNotExist(err) {
 	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
 	// Remove the layers file for the id
 	if err := os.Remove(path.Join(a.rootPath(), "layers", id)); err != nil && !os.IsNotExist(err) {
 	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()
 	a.pathCacheLock.Lock()