From 25ee00c494ff588f6347b4735f2080df9eb05262 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 2 Mar 2022 22:43:07 +0100 Subject: [PATCH] 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 --- daemon/delete.go | 4 ++-- daemon/graphdriver/aufs/aufs.go | 5 ++--- daemon/graphdriver/btrfs/btrfs.go | 3 +-- daemon/graphdriver/fuse-overlayfs/fuseoverlayfs.go | 3 +-- daemon/graphdriver/overlay/overlay.go | 3 +-- daemon/graphdriver/overlay2/overlay.go | 3 +-- daemon/graphdriver/vfs/driver.go | 3 +-- pkg/containerfs/archiver.go | 4 ++-- pkg/{system => containerfs}/rm.go | 2 +- pkg/{system => containerfs}/rm_nodarwin_test.go | 2 +- pkg/{system => containerfs}/rm_test.go | 7 ++++--- pkg/{system => containerfs}/rm_windows.go | 2 +- plugin/backend_linux.go | 6 +++--- plugin/manager.go | 4 ++-- plugin/manager_linux_test.go | 10 +++++----- 15 files changed, 28 insertions(+), 33 deletions(-) rename pkg/{system => containerfs}/rm.go (96%) rename pkg/{system => containerfs}/rm_nodarwin_test.go (90%) rename pkg/{system => containerfs}/rm_test.go (86%) rename pkg/{system => containerfs}/rm_windows.go (59%) diff --git a/daemon/delete.go b/daemon/delete.go index 70f12023cb..e796b95c0b 100644 --- a/daemon/delete.go +++ b/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 diff --git a/daemon/graphdriver/aufs/aufs.go b/daemon/graphdriver/aufs/aufs.go index 0935e447c0..6a4afae180 100644 --- a/daemon/graphdriver/aufs/aufs.go +++ b/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. diff --git a/daemon/graphdriver/btrfs/btrfs.go b/daemon/graphdriver/btrfs/btrfs.go index ee842cce09..bac84c9271 100644 --- a/daemon/graphdriver/btrfs/btrfs.go +++ b/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() diff --git a/daemon/graphdriver/fuse-overlayfs/fuseoverlayfs.go b/daemon/graphdriver/fuse-overlayfs/fuseoverlayfs.go index 909e91cb0d..137b7b189a 100644 --- a/daemon/graphdriver/fuse-overlayfs/fuseoverlayfs.go +++ b/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 diff --git a/daemon/graphdriver/overlay/overlay.go b/daemon/graphdriver/overlay/overlay.go index 6a51f59d55..ae89b86212 100644 --- a/daemon/graphdriver/overlay/overlay.go +++ b/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. diff --git a/daemon/graphdriver/overlay2/overlay.go b/daemon/graphdriver/overlay2/overlay.go index 444f600f55..71b785fcc7 100644 --- a/daemon/graphdriver/overlay2/overlay.go +++ b/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 diff --git a/daemon/graphdriver/vfs/driver.go b/daemon/graphdriver/vfs/driver.go index f903393da2..7b61d2c227 100644 --- a/daemon/graphdriver/vfs/driver.go +++ b/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. diff --git a/pkg/containerfs/archiver.go b/pkg/containerfs/archiver.go index 450c986785..a47d7db760 100644 --- a/pkg/containerfs/archiver.go +++ b/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 diff --git a/pkg/system/rm.go b/pkg/containerfs/rm.go similarity index 96% rename from pkg/system/rm.go rename to pkg/containerfs/rm.go index f2d81597c9..7abca3a804 100644 --- a/pkg/system/rm.go +++ b/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" diff --git a/pkg/system/rm_nodarwin_test.go b/pkg/containerfs/rm_nodarwin_test.go similarity index 90% rename from pkg/system/rm_nodarwin_test.go rename to pkg/containerfs/rm_nodarwin_test.go index f29137ca87..03828970cb 100644 --- a/pkg/system/rm_nodarwin_test.go +++ b/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" diff --git a/pkg/system/rm_test.go b/pkg/containerfs/rm_test.go similarity index 86% rename from pkg/system/rm_test.go rename to pkg/containerfs/rm_test.go index f3d0390e39..6734d8ec04 100644 --- a/pkg/system/rm_test.go +++ b/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 { diff --git a/pkg/system/rm_windows.go b/pkg/containerfs/rm_windows.go similarity index 59% rename from pkg/system/rm_windows.go rename to pkg/containerfs/rm_windows.go index ed9c5dcb8a..779979ed3d 100644 --- a/pkg/system/rm_windows.go +++ b/pkg/containerfs/rm_windows.go @@ -1,4 +1,4 @@ -package system +package containerfs // import "github.com/docker/docker/pkg/containerfs" import "os" diff --git a/plugin/backend_linux.go b/plugin/backend_linux.go index 34313e8fd3..1fe67aa82e 100644 --- a/plugin/backend_linux.go +++ b/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 } diff --git a/plugin/manager.go b/plugin/manager.go index cfc49f285e..d9515b9243 100644 --- a/plugin/manager.go +++ b/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") } } diff --git a/plugin/manager_linux_test.go b/plugin/manager_linux_test.go index b9f71e3702..91f700beb6 100644 --- a/plugin/manager_linux_test.go +++ b/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 {