From 25ee00c494ff588f6347b4735f2080df9eb05262 Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Wed, 2 Mar 2022 22:43:07 +0100
Subject: [PATCH 1/2] 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 | 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 {
From dee9f422c80e04162e944d4eea0f216efc32e078 Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Wed, 2 Mar 2022 22:44:29 +0100
Subject: [PATCH 2/2] pkg/system: remove github.com/docker/go-units dependency
This is not "very" important, but this dependency was only used
for a single const, which could be satisfied with a comment.
Not very urgent, as github.com/docker/go-units is likely imported
through other ways already (but it's nice to have the package be
more isolated).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
pkg/system/meminfo_linux.go | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/pkg/system/meminfo_linux.go b/pkg/system/meminfo_linux.go
index cd060eff24..d407739858 100644
--- a/pkg/system/meminfo_linux.go
+++ b/pkg/system/meminfo_linux.go
@@ -6,8 +6,6 @@ import (
"os"
"strconv"
"strings"
-
- units "github.com/docker/go-units"
)
// ReadMemInfo retrieves memory statistics of the host system and returns a
@@ -42,7 +40,8 @@ func parseMemInfo(reader io.Reader) (*MemInfo, error) {
if err != nil {
continue
}
- bytes := int64(size) * units.KiB
+ // Convert to KiB
+ bytes := int64(size) * 1024
switch parts[0] {
case "MemTotal:":