pkg/system: move EnsureRemoveAll() to pkg/containerfs

pkg/system historically has been a bit of a kitchen-sink of things that were
somewhat "system" related, but didn't have a good place for. EnsureRemoveAll()
is one of those utilities. EnsureRemoveAll() is used to both unmount and remove
a path, for which it depends on both github.com/moby/sys/mount, which in turn
depends on github.com/moby/sys/mountinfo.

pkg/system is imported in the CLI, but neither EnsureRemoveAll(), nor any of its
moby/sys dependencies are used on the client side, so let's move this function
somewhere else, to remove those dependencies from the CLI.

I looked for plausible locations that were related; it's used in:

- daemon
- daemon/graphdriver/XXX/
- plugin

I considered moving it into a (e.g.) "utils" package within graphdriver (but not
a huge fan of "utils" packages), and given that it felt (mostly) related to
cleaning up container filesystems, I decided to move it there.

Some things to follow-up on after this:

- Verify if this function is still needed (it feels a bit like a big hammer in
  a "YOLO, let's try some things just in case it fails")
- Perhaps it should be integrated in `containerfs.Remove()` (so that it's used
  automatically)
- Look if there's other implementations (and if they should be consolidated),
  although (e.g.) the one in containerd is a copy of ours:
  https://github.com/containerd/containerd/blob/v1.5.9/pkg/cri/server/helpers_linux.go#L200

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

daemon/delete.go

@@ -10,7 +10,7 @@ import (
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/container"
 	"github.com/docker/docker/errdefs"
-	"github.com/docker/docker/pkg/system"
+	"github.com/docker/docker/pkg/containerfs"
 	"github.com/opencontainers/selinux/go-selinux"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -124,7 +124,7 @@ func (daemon *Daemon) cleanupContainer(container *container.Container, forceRemo
 		container.RWLayer = nil
 	}
 
-	if err := system.EnsureRemoveAll(container.Root); err != nil {
+	if err := containerfs.EnsureRemoveAll(container.Root); err != nil {
 		err = errors.Wrapf(err, "unable to remove filesystem for %s", container.ID)
 		container.SetRemovalError(err)
 		return err

daemon/graphdriver/aufs/aufs.go

@@ -42,7 +42,6 @@ import (
 	"github.com/docker/docker/pkg/containerfs"
 	"github.com/docker/docker/pkg/directory"
 	"github.com/docker/docker/pkg/idtools"
-	"github.com/docker/docker/pkg/system"
 	"github.com/moby/locker"
 	"github.com/moby/sys/mount"
 	"github.com/opencontainers/selinux/go-selinux/label"
@@ -164,7 +163,7 @@ func Init(root string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap
 			}
 			if strings.HasSuffix(entry.Name(), "-removing") {
 				logger.WithField("dir", entry.Name()).Debug("Cleaning up stale layer dir")
-				if err := system.EnsureRemoveAll(filepath.Join(p, entry.Name())); err != nil {
+				if err := containerfs.EnsureRemoveAll(filepath.Join(p, entry.Name())); err != nil {
 					logger.WithField("dir", entry.Name()).WithError(err).Error("Error removing stale layer dir")
 				}
 			}
@@ -357,7 +356,7 @@ func atomicRemove(source string) error {
 		return errors.Wrapf(err, "error preparing atomic delete")
 	}
 
-	return system.EnsureRemoveAll(target)
+	return containerfs.EnsureRemoveAll(target)
 }
 
 // Get returns the rootfs path for the id.

daemon/graphdriver/btrfs/btrfs.go

@@ -31,7 +31,6 @@ import (
 	"github.com/docker/docker/pkg/containerfs"
 	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/pkg/parsers"
-	"github.com/docker/docker/pkg/system"
 	units "github.com/docker/go-units"
 	"github.com/moby/sys/mount"
 	"github.com/opencontainers/selinux/go-selinux/label"
@@ -627,7 +626,7 @@ func (d *Driver) Remove(id string) error {
 		//
 		// From https://github.com/containers/storage/pull/508/commits/831e32b6bdcb530acc4c1cb9059d3c6dba14208c
 	}
-	if err := system.EnsureRemoveAll(dir); err != nil {
+	if err := containerfs.EnsureRemoveAll(dir); err != nil {
 		return err
 	}
 	return d.subvolRescanQuota()

daemon/graphdriver/fuse-overlayfs/fuseoverlayfs.go

@@ -23,7 +23,6 @@ import (
 	"github.com/docker/docker/pkg/directory"
 	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/pkg/parsers/kernel"
-	"github.com/docker/docker/pkg/system"
 	"github.com/moby/locker"
 	"github.com/moby/sys/mount"
 	"github.com/opencontainers/selinux/go-selinux/label"
@@ -310,7 +309,7 @@ func (d *Driver) Remove(id string) error {
 		}
 	}
 
-	if err := system.EnsureRemoveAll(dir); err != nil && !os.IsNotExist(err) {
+	if err := containerfs.EnsureRemoveAll(dir); err != nil && !os.IsNotExist(err) {
 		return err
 	}
 	return nil

daemon/graphdriver/overlay/overlay.go

@@ -20,7 +20,6 @@ import (
 	"github.com/docker/docker/pkg/fsutils"
 	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/pkg/parsers"
-	"github.com/docker/docker/pkg/system"
 	"github.com/moby/locker"
 	"github.com/moby/sys/mount"
 	"github.com/opencontainers/selinux/go-selinux/label"
@@ -351,7 +350,7 @@ func (d *Driver) Remove(id string) error {
 	}
 	d.locker.Lock(id)
 	defer d.locker.Unlock(id)
-	return system.EnsureRemoveAll(d.dir(id))
+	return containerfs.EnsureRemoveAll(d.dir(id))
 }
 
 // Get creates and mounts the required file system for the given id and returns the mount path.

daemon/graphdriver/overlay2/overlay.go

@@ -24,7 +24,6 @@ import (
 	"github.com/docker/docker/pkg/fsutils"
 	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/pkg/parsers"
-	"github.com/docker/docker/pkg/system"
 	"github.com/docker/docker/quota"
 	units "github.com/docker/go-units"
 	"github.com/moby/locker"
@@ -514,7 +513,7 @@ func (d *Driver) Remove(id string) error {
 		}
 	}
 
-	if err := system.EnsureRemoveAll(dir); err != nil && !os.IsNotExist(err) {
+	if err := containerfs.EnsureRemoveAll(dir); err != nil && !os.IsNotExist(err) {
 		return err
 	}
 	return nil

daemon/graphdriver/vfs/driver.go

@@ -10,7 +10,6 @@ import (
 	"github.com/docker/docker/pkg/containerfs"
 	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/pkg/parsers"
-	"github.com/docker/docker/pkg/system"
 	"github.com/docker/docker/quota"
 	units "github.com/docker/go-units"
 	"github.com/opencontainers/selinux/go-selinux/label"
@@ -186,7 +185,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 {
-	return system.EnsureRemoveAll(d.dir(id))
+	return containerfs.EnsureRemoveAll(d.dir(id))
 }
 
 // Get returns the directory for the given id.

pkg/containerfs/archiver.go

@@ -2,7 +2,7 @@ package containerfs // import "github.com/docker/docker/pkg/containerfs"
 
 import (
 	"archive/tar"
-	"fmt"
+	"errors"
 	"io"
 	"os"
 	"path/filepath"
@@ -100,7 +100,7 @@ func (archiver *Archiver) CopyFileWithTar(src, dst string) (retErr error) {
 	}
 
 	if srcSt.IsDir() {
-		return fmt.Errorf("Can't copy a directory")
+		return errors.New("cannot copy a directory")
 	}
 
 	// Clean up the trailing slash. This must be done in an operating

pkg/containerfs/rm.go

@@ -1,7 +1,7 @@
 //go:build !darwin && !windows
 // +build !darwin,!windows
 
-package system // import "github.com/docker/docker/pkg/system"
+package containerfs // import "github.com/docker/docker/pkg/containerfs"
 
 import (
 	"os"

pkg/containerfs/rm_nodarwin_test.go

@@ -1,7 +1,7 @@
 //go:build !darwin
 // +build !darwin
 
-package system // import "github.com/docker/docker/pkg/system"
+package containerfs // import "github.com/docker/docker/pkg/containerfs"
 
 import (
 	"os"

pkg/containerfs/rm_test.go

@@ -1,7 +1,7 @@
 //go:build !darwin && !windows
 // +build !darwin,!windows
 
-package system // import "github.com/docker/docker/pkg/system"
+package containerfs // import "github.com/docker/docker/pkg/containerfs"
 
 import (
 	"os"
@@ -10,11 +10,12 @@ import (
 	"time"
 
 	"github.com/moby/sys/mount"
-	"gotest.tools/v3/skip"
 )
 
 func TestEnsureRemoveAllWithMount(t *testing.T) {
-	skip.If(t, os.Getuid() != 0, "skipping test that requires root")
+	if os.Getuid() != 0 {
+		t.Skip("skipping test that requires root")
+	}
 
 	dir1, err := os.MkdirTemp("", "test-ensure-removeall-with-dir1")
 	if err != nil {

pkg/containerfs/rm_windows.go

@@ -1,4 +1,4 @@
-package system
+package containerfs // import "github.com/docker/docker/pkg/containerfs"
 
 import "os"
 

plugin/backend_linux.go

@@ -27,10 +27,10 @@ import (
 	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/pkg/authorization"
 	"github.com/docker/docker/pkg/chrootarchive"
+	"github.com/docker/docker/pkg/containerfs"
 	"github.com/docker/docker/pkg/pools"
 	"github.com/docker/docker/pkg/progress"
 	"github.com/docker/docker/pkg/stringid"
-	"github.com/docker/docker/pkg/system"
 	v2 "github.com/docker/docker/plugin/v2"
 	"github.com/moby/sys/mount"
 	digest "github.com/opencontainers/go-digest"
@@ -803,7 +803,7 @@ func atomicRemoveAll(dir string) error {
 		// even if `dir` doesn't exist, we can still try and remove `renamed`
 	case os.IsExist(err):
 		// Some previous remove failed, check if the origin dir exists
-		if e := system.EnsureRemoveAll(renamed); e != nil {
+		if e := containerfs.EnsureRemoveAll(renamed); e != nil {
 			return errors.Wrap(err, "rename target already exists and could not be removed")
 		}
 		if _, err := os.Stat(dir); os.IsNotExist(err) {
@@ -819,7 +819,7 @@ func atomicRemoveAll(dir string) error {
 		return errors.Wrap(err, "failed to rename dir for atomic removal")
 	}
 
-	if err := system.EnsureRemoveAll(renamed); err != nil {
+	if err := containerfs.EnsureRemoveAll(renamed); err != nil {
 		os.Rename(renamed, dir)
 		return err
 	}

plugin/manager.go

@@ -17,9 +17,9 @@ import (
 	"github.com/docker/distribution/reference"
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/pkg/authorization"
+	"github.com/docker/docker/pkg/containerfs"
 	"github.com/docker/docker/pkg/ioutils"
 	"github.com/docker/docker/pkg/pubsub"
-	"github.com/docker/docker/pkg/system"
 	v2 "github.com/docker/docker/plugin/v2"
 	"github.com/docker/docker/registry"
 	digest "github.com/opencontainers/go-digest"
@@ -193,7 +193,7 @@ func (pm *Manager) reload() error { // todo: restore
 		} else {
 			if validFullID.MatchString(strings.TrimSuffix(v.Name(), "-removing")) {
 				// There was likely some error while removing this plugin, let's try to remove again here
-				if err := system.EnsureRemoveAll(v.Name()); err != nil {
+				if err := containerfs.EnsureRemoveAll(v.Name()); err != nil {
 					logrus.WithError(err).WithField("id", v.Name()).Warn("error while attempting to clean up previously removed plugin")
 				}
 			}

plugin/manager_linux_test.go

@@ -8,8 +8,8 @@ import (
 	"testing"
 
 	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/pkg/containerfs"
 	"github.com/docker/docker/pkg/stringid"
-	"github.com/docker/docker/pkg/system"
 	v2 "github.com/docker/docker/plugin/v2"
 	"github.com/moby/sys/mount"
 	"github.com/moby/sys/mountinfo"
@@ -24,7 +24,7 @@ func TestManagerWithPluginMounts(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	defer system.EnsureRemoveAll(root)
+	defer containerfs.EnsureRemoveAll(root)
 
 	s := NewStore()
 	managerRoot := filepath.Join(root, "manager")
@@ -110,7 +110,7 @@ func TestCreateFailed(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	defer system.EnsureRemoveAll(root)
+	defer containerfs.EnsureRemoveAll(root)
 
 	s := NewStore()
 	managerRoot := filepath.Join(root, "manager")
@@ -181,7 +181,7 @@ func TestPluginAlreadyRunningOnStartup(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	defer system.EnsureRemoveAll(root)
+	defer containerfs.EnsureRemoveAll(root)
 
 	for _, test := range []struct {
 		desc   string
@@ -237,7 +237,7 @@ func TestPluginAlreadyRunningOnStartup(t *testing.T) {
 			if err != nil {
 				t.Fatal(err)
 			}
-			defer system.EnsureRemoveAll(config.ExecRoot)
+			defer containerfs.EnsureRemoveAll(config.ExecRoot)
 
 			m, err := NewManager(config)
 			if err != nil {