diff --git a/daemon/delete.go b/daemon/delete.go index 483241db53..4da442c007 100644 --- a/daemon/delete.go +++ b/daemon/delete.go @@ -12,6 +12,7 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/container" "github.com/docker/docker/layer" + "github.com/docker/docker/pkg/system" volumestore "github.com/docker/docker/volume/store" "github.com/pkg/errors" ) @@ -111,37 +112,30 @@ func (daemon *Daemon) cleanupContainer(container *container.Container, forceRemo logrus.Errorf("Error saving dying container to disk: %v", err) } - // If force removal is required, delete container from various - // indexes even if removal failed. - defer func() { - if err == nil || forceRemove { - daemon.nameIndex.Delete(container.ID) - daemon.linkIndex.delete(container) - selinuxFreeLxcContexts(container.ProcessLabel) - daemon.idIndex.Delete(container.ID) - daemon.containers.Delete(container.ID) - if e := daemon.removeMountPoints(container, removeVolume); e != nil { - logrus.Error(e) - } - daemon.LogContainerEvent(container, "destroy") - stateCtr.del(container.ID) - } - }() - - if err = os.RemoveAll(container.Root); err != nil { - return fmt.Errorf("Unable to remove filesystem for %v: %v", container.ID, err) - } - // When container creation fails and `RWLayer` has not been created yet, we // do not call `ReleaseRWLayer` if container.RWLayer != nil { metadata, err := daemon.layerStore.ReleaseRWLayer(container.RWLayer) layer.LogReleaseMetadata(metadata) if err != nil && err != layer.ErrMountDoesNotExist { - return fmt.Errorf("Driver %s failed to remove root filesystem %s: %s", daemon.GraphDriverName(), container.ID, err) + return errors.Wrapf(err, "driver %q failed to remove root filesystem for %s", daemon.GraphDriverName(), container.ID) } } + if err := system.EnsureRemoveAll(container.Root); err != nil { + return errors.Wrapf(err, "unable to remove filesystem for %s", container.ID) + } + + daemon.nameIndex.Delete(container.ID) + daemon.linkIndex.delete(container) + selinuxFreeLxcContexts(container.ProcessLabel) + daemon.idIndex.Delete(container.ID) + daemon.containers.Delete(container.ID) + if e := daemon.removeMountPoints(container, removeVolume); e != nil { + logrus.Error(e) + } + stateCtr.del(container.ID) + daemon.LogContainerEvent(container, "destroy") return nil } diff --git a/daemon/graphdriver/aufs/aufs.go b/daemon/graphdriver/aufs/aufs.go index 92684199b9..85cd5d1d26 100644 --- a/daemon/graphdriver/aufs/aufs.go +++ b/daemon/graphdriver/aufs/aufs.go @@ -46,6 +46,7 @@ import ( "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/locker" mountpk "github.com/docker/docker/pkg/mount" + "github.com/docker/docker/pkg/system" rsystem "github.com/opencontainers/runc/libcontainer/system" "github.com/opencontainers/selinux/go-selinux/label" @@ -319,13 +320,13 @@ func (a *Driver) Remove(id string) error { } return err } - defer os.RemoveAll(tmpMntPath) + 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 } - defer os.RemoveAll(tmpDiffpath) + 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) { diff --git a/daemon/graphdriver/btrfs/btrfs.go b/daemon/graphdriver/btrfs/btrfs.go index 00d3462097..2ff41a0b8b 100644 --- a/daemon/graphdriver/btrfs/btrfs.go +++ b/daemon/graphdriver/btrfs/btrfs.go @@ -27,6 +27,7 @@ import ( "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/mount" "github.com/docker/docker/pkg/parsers" + "github.com/docker/docker/pkg/system" "github.com/docker/go-units" "github.com/opencontainers/selinux/go-selinux/label" ) @@ -535,7 +536,7 @@ func (d *Driver) Remove(id string) error { if err := subvolDelete(d.subvolumesDir(), id); err != nil { return err } - if err := os.RemoveAll(dir); err != nil && !os.IsNotExist(err) { + if err := system.EnsureRemoveAll(dir); err != nil { return err } if err := d.subvolRescanQuota(); err != nil { diff --git a/daemon/graphdriver/devmapper/driver.go b/daemon/graphdriver/devmapper/driver.go index 91de5cd12a..243d88a8bb 100644 --- a/daemon/graphdriver/devmapper/driver.go +++ b/daemon/graphdriver/devmapper/driver.go @@ -16,6 +16,7 @@ import ( "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/locker" "github.com/docker/docker/pkg/mount" + "github.com/docker/docker/pkg/system" units "github.com/docker/go-units" ) @@ -160,7 +161,7 @@ func (d *Driver) Remove(id string) error { } mp := path.Join(d.home, "mnt", id) - if err := os.RemoveAll(mp); err != nil && !os.IsNotExist(err) { + if err := system.EnsureRemoveAll(mp); err != nil { return err } diff --git a/daemon/graphdriver/overlay/overlay.go b/daemon/graphdriver/overlay/overlay.go index 805385c138..88d6602e07 100644 --- a/daemon/graphdriver/overlay/overlay.go +++ b/daemon/graphdriver/overlay/overlay.go @@ -21,6 +21,7 @@ import ( "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/locker" "github.com/docker/docker/pkg/mount" + "github.com/docker/docker/pkg/system" "github.com/opencontainers/selinux/go-selinux/label" ) @@ -339,10 +340,7 @@ func (d *Driver) dir(id string) string { func (d *Driver) Remove(id string) error { d.locker.Lock(id) defer d.locker.Unlock(id) - if err := os.RemoveAll(d.dir(id)); err != nil && !os.IsNotExist(err) { - return err - } - return nil + return system.EnsureRemoveAll(d.dir(id)) } // Get creates and mounts the required file system for the given id and returns the mount path. diff --git a/daemon/graphdriver/overlay2/overlay.go b/daemon/graphdriver/overlay2/overlay.go index b4d73554dd..b337bed975 100644 --- a/daemon/graphdriver/overlay2/overlay.go +++ b/daemon/graphdriver/overlay2/overlay.go @@ -31,6 +31,7 @@ import ( "github.com/docker/docker/pkg/mount" "github.com/docker/docker/pkg/parsers" "github.com/docker/docker/pkg/parsers/kernel" + "github.com/docker/docker/pkg/system" units "github.com/docker/go-units" "github.com/opencontainers/selinux/go-selinux/label" @@ -464,7 +465,7 @@ func (d *Driver) Remove(id string) error { } } - if err := os.RemoveAll(dir); err != nil && !os.IsNotExist(err) { + if err := system.EnsureRemoveAll(dir); err != nil && !os.IsNotExist(err) { return err } return nil diff --git a/daemon/graphdriver/vfs/driver.go b/daemon/graphdriver/vfs/driver.go index 6812dfd590..846215e810 100644 --- a/daemon/graphdriver/vfs/driver.go +++ b/daemon/graphdriver/vfs/driver.go @@ -8,6 +8,7 @@ import ( "github.com/docker/docker/daemon/graphdriver" "github.com/docker/docker/pkg/chrootarchive" "github.com/docker/docker/pkg/idtools" + "github.com/docker/docker/pkg/system" "github.com/opencontainers/selinux/go-selinux/label" ) @@ -114,7 +115,7 @@ func (d *Driver) dir(id string) string { // Remove deletes the content from the directory for a given id. func (d *Driver) Remove(id string) error { - if err := os.RemoveAll(d.dir(id)); err != nil && !os.IsNotExist(err) { + if err := system.EnsureRemoveAll(d.dir(id)); err != nil { return err } return nil diff --git a/daemon/monitor.go b/daemon/monitor.go index b243b74784..6861a5c3c8 100644 --- a/daemon/monitor.go +++ b/daemon/monitor.go @@ -41,17 +41,6 @@ func (daemon *Daemon) StateChanged(id string, e libcontainerd.StateInfo) error { daemon.updateHealthMonitor(c) daemon.LogContainerEvent(c, "oom") case libcontainerd.StateExit: - // if container's AutoRemove flag is set, remove it after clean up - autoRemove := func() { - c.Lock() - ar := c.HostConfig.AutoRemove - c.Unlock() - if ar { - if err := daemon.ContainerRm(c.ID, &types.ContainerRmConfig{ForceRemove: true, RemoveVolume: true}); err != nil { - logrus.Errorf("can't remove container %s: %v", c.ID, err) - } - } - } c.Lock() c.StreamConfig.Wait() @@ -63,7 +52,7 @@ func (daemon *Daemon) StateChanged(id string, e libcontainerd.StateInfo) error { c.SetRestarting(platformConstructExitStatus(e)) } else { c.SetStopped(platformConstructExitStatus(e)) - defer autoRemove() + defer daemon.autoRemove(c) } // cancel healthcheck here, they will be automatically @@ -85,7 +74,7 @@ func (daemon *Daemon) StateChanged(id string, e libcontainerd.StateInfo) error { } if err != nil { c.SetStopped(platformConstructExitStatus(e)) - defer autoRemove() + defer daemon.autoRemove(c) if err != restartmanager.ErrRestartCanceled { logrus.Errorf("restartmanger wait error: %+v", err) } @@ -153,3 +142,24 @@ func (daemon *Daemon) StateChanged(id string, e libcontainerd.StateInfo) error { } return nil } + +func (daemon *Daemon) autoRemove(c *container.Container) { + c.Lock() + ar := c.HostConfig.AutoRemove + c.Unlock() + if !ar { + return + } + + var err error + if err = daemon.ContainerRm(c.ID, &types.ContainerRmConfig{ForceRemove: true, RemoveVolume: true}); err == nil { + return + } + if c := daemon.containers.Get(c.ID); c == nil { + return + } + + if err != nil { + logrus.WithError(err).WithField("container", c.ID).Error("error removing container") + } +} diff --git a/pkg/mount/mount.go b/pkg/mount/mount.go index a57f3979b5..c9fdfd6942 100644 --- a/pkg/mount/mount.go +++ b/pkg/mount/mount.go @@ -1,5 +1,10 @@ package mount +import ( + "sort" + "strings" +) + // GetMounts retrieves a list of mounts for the current running process. func GetMounts() ([]*Info, error) { return parseMountTable() @@ -53,3 +58,29 @@ func Unmount(target string) error { } return unmount(target, mntDetach) } + +// RecursiveUnmount unmounts the target and all mounts underneath, starting with +// the deepsest mount first. +func RecursiveUnmount(target string) error { + mounts, err := GetMounts() + if err != nil { + return err + } + + // Make the deepest mount be first + sort.Sort(sort.Reverse(byMountpoint(mounts))) + + for i, m := range mounts { + if !strings.HasPrefix(m.Mountpoint, target) { + continue + } + if err := Unmount(m.Mountpoint); err != nil && i == len(mounts)-1 { + if mounted, err := Mounted(m.Mountpoint); err != nil || mounted { + return err + } + // Ignore errors for submounts and continue trying to unmount others + // The final unmount should fail if there ane any submounts remaining + } + } + return nil +} diff --git a/pkg/mount/mountinfo.go b/pkg/mount/mountinfo.go index e3fc3535e9..ff4cc1d86b 100644 --- a/pkg/mount/mountinfo.go +++ b/pkg/mount/mountinfo.go @@ -38,3 +38,17 @@ type Info struct { // VfsOpts represents per super block options. VfsOpts string } + +type byMountpoint []*Info + +func (by byMountpoint) Len() int { + return len(by) +} + +func (by byMountpoint) Less(i, j int) bool { + return by[i].Mountpoint < by[j].Mountpoint +} + +func (by byMountpoint) Swap(i, j int) { + by[i], by[j] = by[j], by[i] +} diff --git a/pkg/system/rm.go b/pkg/system/rm.go new file mode 100644 index 0000000000..ca0621f04f --- /dev/null +++ b/pkg/system/rm.go @@ -0,0 +1,80 @@ +package system + +import ( + "os" + "syscall" + "time" + + "github.com/docker/docker/pkg/mount" + "github.com/pkg/errors" +) + +// EnsureRemoveAll wraps `os.RemoveAll` to check for specific errors that can +// often be remedied. +// Only use `EnsureRemoveAll` if you really want to make every effort to remove +// a directory. +// +// Because of the way `os.Remove` (and by extension `os.RemoveAll`) works, there +// can be a race between reading directory entries and then actually attempting +// to remove everything in the directory. +// These types of errors do not need to be returned since it's ok for the dir to +// be gone we can just retry the remove operation. +// +// This should not return a `os.ErrNotExist` kind of error under any cirucmstances +func EnsureRemoveAll(dir string) error { + notExistErr := make(map[string]bool) + + // track retries + exitOnErr := make(map[string]int) + maxRetry := 5 + + // Attempt to unmount anything beneath this dir first + mount.RecursiveUnmount(dir) + + for { + err := os.RemoveAll(dir) + if err == nil { + return err + } + + pe, ok := err.(*os.PathError) + if !ok { + return err + } + + if os.IsNotExist(err) { + if notExistErr[pe.Path] { + return err + } + notExistErr[pe.Path] = true + + // There is a race where some subdir can be removed but after the parent + // dir entries have been read. + // So the path could be from `os.Remove(subdir)` + // If the reported non-existent path is not the passed in `dir` we + // should just retry, but otherwise return with no error. + if pe.Path == dir { + return nil + } + continue + } + + if pe.Err != syscall.EBUSY { + return err + } + + if mounted, _ := mount.Mounted(pe.Path); mounted { + if e := mount.Unmount(pe.Path); e != nil { + if mounted, _ := mount.Mounted(pe.Path); mounted { + return errors.Wrapf(e, "error while removing %s", dir) + } + } + } + + if exitOnErr[pe.Path] == maxRetry { + return err + } + exitOnErr[pe.Path]++ + time.Sleep(100 * time.Millisecond) + } +} diff --git a/pkg/system/rm_test.go b/pkg/system/rm_test.go new file mode 100644 index 0000000000..fc2821f897 --- /dev/null +++ b/pkg/system/rm_test.go @@ -0,0 +1,84 @@ +package system + +import ( + "io/ioutil" + "os" + "path/filepath" + "runtime" + "testing" + "time" + + "github.com/docker/docker/pkg/mount" +) + +func TestEnsureRemoveAllNotExist(t *testing.T) { + // should never return an error for a non-existent path + if err := EnsureRemoveAll("/non/existent/path"); err != nil { + t.Fatal(err) + } +} + +func TestEnsureRemoveAllWithDir(t *testing.T) { + dir, err := ioutil.TempDir("", "test-ensure-removeall-with-dir") + if err != nil { + t.Fatal(err) + } + if err := EnsureRemoveAll(dir); err != nil { + t.Fatal(err) + } +} + +func TestEnsureRemoveAllWithFile(t *testing.T) { + tmp, err := ioutil.TempFile("", "test-ensure-removeall-with-dir") + if err != nil { + t.Fatal(err) + } + tmp.Close() + if err := EnsureRemoveAll(tmp.Name()); err != nil { + t.Fatal(err) + } +} + +func TestEnsureRemoveAllWithMount(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("mount not supported on Windows") + } + + dir1, err := ioutil.TempDir("", "test-ensure-removeall-with-dir1") + if err != nil { + t.Fatal(err) + } + dir2, err := ioutil.TempDir("", "test-ensure-removeall-with-dir2") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir2) + + bindDir := filepath.Join(dir1, "bind") + if err := os.MkdirAll(bindDir, 0755); err != nil { + t.Fatal(err) + } + + if err := mount.Mount(dir2, bindDir, "none", "bind"); err != nil { + t.Fatal(err) + } + + done := make(chan struct{}) + go func() { + err = EnsureRemoveAll(dir1) + close(done) + }() + + select { + case <-done: + if err != nil { + t.Fatal(err) + } + case <-time.After(5 * time.Second): + t.Fatal("timeout waiting for EnsureRemoveAll to finish") + } + + if _, err := os.Stat(dir1); !os.IsNotExist(err) { + t.Fatalf("expected %q to not exist", dir1) + } +}