Skip to content

Commit

Permalink
Merge branch 'rm_container' into 'huawei-1.11.2'
Browse files Browse the repository at this point in the history
Do not remove containers from memory on error

Before this, if `forceRemove` is set the container data will be removed<br>
no matter what, including if there are issues with removing container<br>
on-disk state (rw layer, container root).   

<br>

In practice this causes a lot of issues with leaked data sitting on<br>
disk that users are not able to clean up themselves.

<br>
This is particularly a problem while the `EBUSY` errors on remove are so<br>
prevalent. So for now let's not keep this behavior.     

<br>

Cherry-pick from <a href="moby#31012" rel="nofollow noreferrer" target="_blank">https://github.com/moby/moby/pull/31012</a>

<br>

Signed-off-by: Brian Goff <a href="mailto:cpuguy83@gmail.com">cpuguy83@gmail.com</a><br>
Signed-off-by: Wentao Zhang <a href="mailto:zhangwentao234@huawei.com">zhangwentao234@huawei.com</a><br>
(cherry picked from commit <a href="/docker/docker/commit/54dcbab25ea4771da303fa95e0c26f2d39487b49" data-original="54dcbab2" data-link="false" data-project="7713" data-commit="54dcbab25ea4771da303fa95e0c26f2d39487b49" data-reference-type="commit" title="" class="gfm gfm-commit has-tooltip" data-original-title="Do not remove containers from memory on error">54dcbab2</a>)     

<br>

Conflicts:<br>
    daemon/delete.go<br>
    daemon/graphdriver/aufs/aufs.go<br>
    daemon/graphdriver/btrfs/btrfs.go<br>
    daemon/graphdriver/devmapper/driver.go<br>
    daemon/graphdriver/overlay/overlay.go<br>
    daemon/monitor.go<br>
    pkg/mount/mount.go



See merge request docker/docker!519
  • Loading branch information
moypray authored and coolljt0725 committed Jun 12, 2017
2 parents 686943e + d7bf814 commit 1f78313
Show file tree
Hide file tree
Showing 11 changed files with 233 additions and 30 deletions.
34 changes: 14 additions & 20 deletions daemon/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/docker/docker/container"
"github.com/docker/docker/errors"
"github.com/docker/docker/layer"
"github.com/docker/docker/pkg/system"
volumestore "github.com/docker/docker/volume/store"
"github.com/docker/engine-api/types"
)
Expand Down Expand Up @@ -99,31 +100,11 @@ 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")
}
}()

// Release accelerator resources
if err = daemon.releaseAccelResources(container, true); err != nil {
return fmt.Errorf("Unable to release accelerator resources: %v", err)
}

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 {
Expand All @@ -134,6 +115,19 @@ func (daemon *Daemon) cleanupContainer(container *container.Container, forceRemo
}
}

if err := system.EnsureRemoveAll(container.Root); err != nil {
return fmt.Errorf("unable to remove filesystem for %s: %v", container.ID, err)
}

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")
return nil
}

Expand Down
5 changes: 3 additions & 2 deletions daemon/graphdriver/aufs/aufs.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import (
"github.com/docker/docker/pkg/locker"
mountpk "github.com/docker/docker/pkg/mount"
"github.com/docker/docker/pkg/stringid"
"github.com/docker/docker/pkg/system"

"github.com/opencontainers/runc/libcontainer/label"
)
Expand Down Expand Up @@ -288,13 +289,13 @@ func (a *Driver) Remove(id string) error {
if err := os.Rename(mountpoint, tmpMntPath); err != nil && !os.IsNotExist(err) {
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) {
Expand Down
3 changes: 2 additions & 1 deletion daemon/graphdriver/btrfs/btrfs.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/docker/docker/daemon/graphdriver"
"github.com/docker/docker/pkg/idtools"
"github.com/docker/docker/pkg/mount"
"github.com/docker/docker/pkg/system"
"github.com/opencontainers/runc/libcontainer/label"
)

Expand Down Expand Up @@ -305,7 +306,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
}
return nil
Expand Down
3 changes: 2 additions & 1 deletion daemon/graphdriver/devmapper/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -155,7 +156,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 {
logrus.Warnf("devmapper: Warning removing %s error: %v", mp, err)
}

Expand Down
6 changes: 2 additions & 4 deletions daemon/graphdriver/overlay/overlay.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/docker/docker/pkg/fsutils"
"github.com/docker/docker/pkg/idtools"
"github.com/docker/docker/pkg/locker"
"github.com/docker/docker/pkg/system"
"github.com/opencontainers/runc/libcontainer/label"
)

Expand Down Expand Up @@ -347,10 +348,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.
Expand Down
3 changes: 2 additions & 1 deletion daemon/graphdriver/overlay2/overlay.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,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/runc/libcontainer/label"
Expand Down Expand Up @@ -462,7 +463,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
Expand Down
3 changes: 2 additions & 1 deletion daemon/graphdriver/vfs/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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/runc/libcontainer/label"
)
Expand Down Expand Up @@ -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
Expand Down
28 changes: 28 additions & 0 deletions pkg/mount/mount.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package mount

import (
"sort"
"strings"
"time"
)

Expand Down Expand Up @@ -72,3 +74,29 @@ func ForceUnmount(target string) (err error) {
}
return
}

// 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
}
14 changes: 14 additions & 0 deletions pkg/mount/mountinfo.go
Original file line number Diff line number Diff line change
Expand Up @@ -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]
}
80 changes: 80 additions & 0 deletions pkg/system/rm.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
84 changes: 84 additions & 0 deletions pkg/system/rm_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}

0 comments on commit 1f78313

Please sign in to comment.