Use local "ensureRemoveAll" instead of docker/pkg/system

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

pkg/server/container_remove.go

@@ -20,7 +20,6 @@ import (
 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/log"
-	"github.com/docker/docker/pkg/system"
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
 	runtime "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
@@ -76,12 +75,12 @@ func (c *criService) RemoveContainer(ctx context.Context, r *runtime.RemoveConta
 	}
 
 	containerRootDir := c.getContainerRootDir(id)
-	if err := system.EnsureRemoveAll(containerRootDir); err != nil {
+	if err := ensureRemoveAll(ctx, containerRootDir); err != nil {
 		return nil, errors.Wrapf(err, "failed to remove container root directory %q",
 			containerRootDir)
 	}
 	volatileContainerRootDir := c.getVolatileContainerRootDir(id)
-	if err := system.EnsureRemoveAll(volatileContainerRootDir); err != nil {
+	if err := ensureRemoveAll(ctx, volatileContainerRootDir); err != nil {
 		return nil, errors.Wrapf(err, "failed to remove volatile container root directory %q",
 			volatileContainerRootDir)
 	}

pkg/server/helpers_test.go

@@ -18,6 +18,7 @@ package server
 
 import (
 	"context"
+	"io/ioutil"
 	"testing"
 
 	"github.com/BurntSushi/toml"
@@ -467,3 +468,31 @@ func TestPassThroughAnnotationsFilter(t *testing.T) {
 		})
 	}
 }
+
+func TestEnsureRemoveAllNotExist(t *testing.T) {
+	// should never return an error for a non-existent path
+	if err := ensureRemoveAll(context.Background(), "/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(context.Background(), 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(context.Background(), tmp.Name()); err != nil {
+		t.Fatal(err)
+	}
+}

pkg/server/helpers_unix.go

@@ -19,17 +19,24 @@ limitations under the License.
 package server
 
 import (
+	"context"
 	"fmt"
 	"os"
 	"path"
 	"path/filepath"
 	"regexp"
+	"sort"
 	"strings"
+	"syscall"
+	"time"
 
+	"github.com/containerd/containerd/log"
+	"github.com/containerd/containerd/mount"
 	runcapparmor "github.com/opencontainers/runc/libcontainer/apparmor"
 	runcseccomp "github.com/opencontainers/runc/libcontainer/seccomp"
 	"github.com/opencontainers/selinux/go-selinux/label"
 	"github.com/pkg/errors"
+	"golang.org/x/sys/unix"
 	runtime "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
 )
 
@@ -141,3 +148,106 @@ func (c *criService) seccompEnabled() bool {
 func openLogFile(path string) (*os.File, error) {
 	return os.OpenFile(path, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0640)
 }
+
+// unmountRecursive unmounts the target and all mounts underneath, starting with
+// the deepest mount first.
+func unmountRecursive(ctx context.Context, target string) error {
+	mounts, err := mount.Self()
+	if err != nil {
+		return err
+	}
+
+	var toUnmount []string
+	for _, m := range mounts {
+		p, err := filepath.Rel(target, m.Mountpoint)
+		if err != nil {
+			return err
+		}
+		if !strings.HasPrefix(p, "..") {
+			toUnmount = append(toUnmount, m.Mountpoint)
+		}
+	}
+
+	// Make the deepest mount be first
+	sort.Slice(toUnmount, func(i, j int) bool {
+		return len(toUnmount[i]) > len(toUnmount[j])
+	})
+
+	for i, mountPath := range toUnmount {
+		if err := mount.UnmountAll(mountPath, unix.MNT_DETACH); err != nil {
+			if i == len(toUnmount)-1 { // last mount
+				return err
+			}
+			// This is some submount, we can ignore this error for now, the final unmount will fail if this is a real problem
+			log.G(ctx).WithError(err).Debugf("failed to unmount submount %s", mountPath)
+		}
+	}
+	return nil
+}
+
+// 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 circumstances
+func ensureRemoveAll(ctx context.Context, dir string) error {
+	notExistErr := make(map[string]bool)
+
+	// track retries
+	exitOnErr := make(map[string]int)
+	maxRetry := 50
+
+	// Attempt to unmount anything beneath this dir first.
+	if err := unmountRecursive(ctx, dir); err != nil {
+		log.G(ctx).WithError(err).Debugf("failed to do initial unmount of %s", dir)
+	}
+
+	for {
+		err := os.RemoveAll(dir)
+		if err == nil {
+			return nil
+		}
+
+		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 e := mount.Unmount(pe.Path, unix.MNT_DETACH); e != nil {
+			return errors.Wrapf(e, "error while removing %s", dir)
+		}
+
+		if exitOnErr[pe.Path] == maxRetry {
+			return err
+		}
+		exitOnErr[pe.Path]++
+		time.Sleep(100 * time.Millisecond)
+	}
+}

pkg/server/helpers_unix_test.go

@@ -19,9 +19,15 @@ limitations under the License.
 package server
 
 import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
+	"golang.org/x/net/context"
+	"golang.org/x/sys/unix"
 )
 
 func TestGetCgroupsPath(t *testing.T) {
@@ -56,3 +62,47 @@ func TestGetCgroupsPath(t *testing.T) {
 		assert.Equal(t, test.expected, got)
 	}
 }
+
+func TestEnsureRemoveAllWithMount(t *testing.T) {
+	if os.Getuid() != 0 {
+		t.Skip("skipping test that requires root")
+	}
+
+	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 := unix.Mount(dir2, bindDir, "none", unix.MS_BIND, ""); err != nil {
+		t.Fatal(err)
+	}
+
+	done := make(chan struct{})
+	go func() {
+		err = ensureRemoveAll(context.Background(), 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)
+	}
+}

pkg/server/helpers_windows.go

@@ -19,9 +19,11 @@ limitations under the License.
 package server
 
 import (
+	"context"
 	"os"
 	"path/filepath"
 	"syscall"
+	"time"
 )
 
 // openLogFile opens/creates a container log file.
@@ -156,3 +158,62 @@ func fixLongPath(path string) string {
 	}
 	return string(pathbuf[:w])
 }
+
+// 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 circumstances
+func ensureRemoveAll(_ context.Context, dir string) error {
+	notExistErr := make(map[string]bool)
+
+	// track retries
+	exitOnErr := make(map[string]int)
+	maxRetry := 50
+
+	for {
+		err := os.RemoveAll(dir)
+		if err == nil {
+			return nil
+		}
+
+		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 exitOnErr[pe.Path] == maxRetry {
+			return err
+		}
+		exitOnErr[pe.Path]++
+		time.Sleep(100 * time.Millisecond)
+	}
+}

pkg/server/restart.go

@@ -30,7 +30,6 @@ import (
 	"github.com/containerd/containerd/log"
 	"github.com/containerd/containerd/platforms"
 	"github.com/containerd/typeurl"
-	"github.com/docker/docker/pkg/system"
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
 	runtime "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
@@ -474,7 +473,7 @@ func cleanupOrphanedIDDirs(ctx context.Context, cntrs []containerd.Container, ba
 			continue
 		}
 		dir := filepath.Join(base, d.Name())
-		if err := system.EnsureRemoveAll(dir); err != nil {
+		if err := ensureRemoveAll(ctx, dir); err != nil {
 			log.G(ctx).WithError(err).Warnf("Failed to remove id directory %q", dir)
 		} else {
 			log.G(ctx).Debugf("Cleanup orphaned id directory %q", dir)

pkg/server/sandbox_remove.go

@@ -20,7 +20,6 @@ import (
 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/log"
-	"github.com/docker/docker/pkg/system"
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
 	runtime "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
@@ -80,12 +79,12 @@ func (c *criService) RemovePodSandbox(ctx context.Context, r *runtime.RemovePodS
 
 	// Cleanup the sandbox root directories.
 	sandboxRootDir := c.getSandboxRootDir(id)
-	if err := system.EnsureRemoveAll(sandboxRootDir); err != nil {
+	if err := ensureRemoveAll(ctx, sandboxRootDir); err != nil {
 		return nil, errors.Wrapf(err, "failed to remove sandbox root directory %q",
 			sandboxRootDir)
 	}
 	volatileSandboxRootDir := c.getVolatileSandboxRootDir(id)
-	if err := system.EnsureRemoveAll(volatileSandboxRootDir); err != nil {
+	if err := ensureRemoveAll(ctx, volatileSandboxRootDir); err != nil {
 		return nil, errors.Wrapf(err, "failed to remove volatile sandbox root directory %q",
 			volatileSandboxRootDir)
 	}