Skip to content

cgroup2: implement docker info #40662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 29, 2020
Merged

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Mar 10, 2020

- What I did

Implemented docker info for cgroup v2.

The REST API now includes .CgroupVersion field.

- How I did it
Updated pkg/sysinfo to parse cgroup v2 info.

ref: https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html

- How to verify it
Run curl --unix-socket /var/run/docker.sock http://localhost/v1.41/info.

Rootful:

json.CgroupDriver = "cgroupfs";
json.CgroupVersion = "2";
...
json.Warnings[0] = "WARNING: No kernel memory limit support";
json.Warnings[1] = "WARNING: No kernel memory TCP limit support";
json.Warnings[2] = "WARNING: No oom kill disable support";
json.Warnings[3] = "WARNING: Support for cgroup v2 is experimental";

Rootless (No systemd):

json.CgroupDriver = "none";
json.CgroupVersion = "2";
...
json.Warnings[0] = "WARNING: No cgroup support";

Rootless (With --exec-opt native.cgroupdriver=systemd, Delegate=pids memory cpu):

json.CgroupDriver = "systemd";
json.CgroupVersion = "2";
...
json.Warnings[0] = "WARNING: No kernel memory limit support";
json.Warnings[1] = "WARNING: No kernel memory TCP limit support";
json.Warnings[2] = "WARNING: No oom kill disable support";
json.Warnings[3] = "WARNING: No cpuset support";
json.Warnings[4] = "WARNING: Support for cgroup v2 is experimental";

- Description for the changelog

cgroup2: implement docker info

- A picture of a cute animal (not mandatory but encouraged)
🐧

@AkihiroSuda AkihiroSuda force-pushed the cgroup2-dockerinfo branch 2 times, most recently from e6feebc to 58b3385 Compare March 12, 2020 09:24

const platformSupported = false

func setupResolvConf(config *config.Config) {
}

func (daemon *Daemon) RawSysInfo(quiet bool) *sysinfo.SysInfo {
return sysinfo.New(quiet)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this just return nil or panic(unsupported) ?

or not implement this at all? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is up to pkg/sysinfo implementation.

rootlesskitParentEUID := os.Getenv("ROOTLESSKIT_PARENT_EUID")
if rootlesskitParentEUID != "" {
groupPath := fmt.Sprintf("/user.slice/user-%s.slice", rootlesskitParentEUID)
opts = append(opts, sysinfo.WithCgroup2GroupPath(groupPath))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any situation where we want sysinfo.New() to not use this path if ROOTLESSKIT_PARENT_EUID is set?

Just wondering if we should make sysinfo.New() just do the right thing. I see it already uses a findCgroupMountpoints() function

Also; looks like we may want to sanitise rootlesskitParentEUID before using

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A very quick attempt;
suggestions.patch.txt

diff --git a/cmd/dockerd/daemon.go b/cmd/dockerd/daemon.go
index 46a62b5d2f..8b59189d57 100644
--- a/cmd/dockerd/daemon.go
+++ b/cmd/dockerd/daemon.go
@@ -45,7 +45,6 @@ import (
 	"github.com/docker/docker/pkg/pidfile"
 	"github.com/docker/docker/pkg/plugingetter"
 	"github.com/docker/docker/pkg/signal"
-	"github.com/docker/docker/pkg/sysinfo"
 	"github.com/docker/docker/pkg/system"
 	"github.com/docker/docker/plugin"
 	"github.com/docker/docker/rootless"
@@ -459,11 +458,7 @@ func warnOnDeprecatedConfigOptions(config *config.Config) {
 }
 
 func initRouter(opts routerOptions) {
-	decoder := runconfig.ContainerDecoder{
-		GetSysInfo: func() *sysinfo.SysInfo {
-			return opts.daemon.RawSysInfo(true)
-		},
-	}
+	decoder := runconfig.ContainerDecoder{}
 
 	routers := []router.Router{
 		// we need to add the checkpoint router before the container router or the DELETE gets masked
diff --git a/daemon/daemon.go b/daemon/daemon.go
index 35d83d6148..7df8d7f957 100644
--- a/daemon/daemon.go
+++ b/daemon/daemon.go
@@ -20,10 +20,6 @@ import (
 	"sync"
 	"time"
 
-	"github.com/docker/docker/pkg/fileutils"
-	"google.golang.org/grpc"
-	"google.golang.org/grpc/backoff"
-
 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/defaults"
 	"github.com/containerd/containerd/pkg/dialer"
@@ -38,27 +34,23 @@ import (
 	"github.com/docker/docker/daemon/discovery"
 	"github.com/docker/docker/daemon/events"
 	"github.com/docker/docker/daemon/exec"
+	_ "github.com/docker/docker/daemon/graphdriver/register" // register graph drivers
 	"github.com/docker/docker/daemon/images"
 	"github.com/docker/docker/daemon/logger"
 	"github.com/docker/docker/daemon/network"
-	"github.com/docker/docker/errdefs"
-	"github.com/moby/buildkit/util/resolver"
-	"github.com/moby/buildkit/util/tracing"
-	rsystem "github.com/opencontainers/runc/libcontainer/system"
-	"github.com/sirupsen/logrus"
-
-	// register graph drivers
-	_ "github.com/docker/docker/daemon/graphdriver/register"
 	"github.com/docker/docker/daemon/stats"
 	dmetadata "github.com/docker/docker/distribution/metadata"
 	"github.com/docker/docker/dockerversion"
+	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/image"
 	"github.com/docker/docker/layer"
 	"github.com/docker/docker/libcontainerd"
 	libcontainerdtypes "github.com/docker/docker/libcontainerd/types"
+	"github.com/docker/docker/pkg/fileutils"
 	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/pkg/locker"
 	"github.com/docker/docker/pkg/plugingetter"
+	"github.com/docker/docker/pkg/sysinfo"
 	"github.com/docker/docker/pkg/system"
 	"github.com/docker/docker/pkg/truncindex"
 	"github.com/docker/docker/plugin"
@@ -70,8 +62,14 @@ import (
 	"github.com/docker/libnetwork"
 	"github.com/docker/libnetwork/cluster"
 	nwconfig "github.com/docker/libnetwork/config"
+	"github.com/moby/buildkit/util/resolver"
+	"github.com/moby/buildkit/util/tracing"
+	rsystem "github.com/opencontainers/runc/libcontainer/system"
 	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
 	"golang.org/x/sync/semaphore"
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/backoff"
 )
 
 // ContainersNamespace is the name of the namespace used for users containers
@@ -1044,7 +1042,7 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S
 		return nil, err
 	}
 
-	sysInfo := d.RawSysInfo(false)
+	sysInfo := sysinfo.New(false)
 	// Check if Devices cgroup is mounted, it is hard requirement for container security,
 	// on Linux.
 	if runtime.GOOS == "linux" && !sysInfo.CgroupDevicesEnabled && !rsystem.RunningInUserNS() {
diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go
index 04a9beda4d..5763298dbc 100644
--- a/daemon/daemon_unix.go
+++ b/daemon/daemon_unix.go
@@ -643,7 +643,7 @@ func verifyPlatformContainerSettings(daemon *Daemon, hostConfig *containertypes.
 	if hostConfig == nil {
 		return nil, nil
 	}
-	sysInfo := daemon.RawSysInfo(true)
+	sysInfo := sysinfo.New(true)
 
 	w, err := verifyPlatformContainerResources(&hostConfig.Resources, sysInfo, update)
 
@@ -1631,11 +1631,11 @@ func (daemon *Daemon) initCgroupsPath(path string) error {
 	}
 
 	path = filepath.Join(mnt, root, path)
-	sysinfo := daemon.RawSysInfo(true)
-	if err := maybeCreateCPURealTimeFile(sysinfo.CPURealtimePeriod, daemon.configStore.CPURealtimePeriod, "cpu.rt_period_us", path); err != nil {
+	sysInfo := sysinfo.New(true)
+	if err := maybeCreateCPURealTimeFile(sysInfo.CPURealtimePeriod, daemon.configStore.CPURealtimePeriod, "cpu.rt_period_us", path); err != nil {
 		return err
 	}
-	return maybeCreateCPURealTimeFile(sysinfo.CPURealtimeRuntime, daemon.configStore.CPURealtimeRuntime, "cpu.rt_runtime_us", path)
+	return maybeCreateCPURealTimeFile(sysInfo.CPURealtimeRuntime, daemon.configStore.CPURealtimeRuntime, "cpu.rt_runtime_us", path)
 }
 
 func maybeCreateCPURealTimeFile(sysinfoPresent bool, configValue int64, file string, path string) error {
@@ -1665,16 +1665,3 @@ func (daemon *Daemon) setupSeccompProfile() error {
 func (daemon *Daemon) useShimV2() bool {
 	return cgroups.IsCgroup2UnifiedMode()
 }
-
-// RawSysInfo returns *sysinfo.SysInfo .
-func (daemon *Daemon) RawSysInfo(quiet bool) *sysinfo.SysInfo {
-	var opts []sysinfo.Opt
-	if daemon.getCgroupDriver() == cgroupSystemdDriver {
-		rootlesskitParentEUID := os.Getenv("ROOTLESSKIT_PARENT_EUID")
-		if rootlesskitParentEUID != "" {
-			groupPath := fmt.Sprintf("/user.slice/user-%s.slice", rootlesskitParentEUID)
-			opts = append(opts, sysinfo.WithCgroup2GroupPath(groupPath))
-		}
-	}
-	return sysinfo.New(quiet, opts...)
-}
diff --git a/daemon/daemon_unsupported.go b/daemon/daemon_unsupported.go
index 4c2476edcf..e6bcbdaab1 100644
--- a/daemon/daemon_unsupported.go
+++ b/daemon/daemon_unsupported.go
@@ -4,15 +4,9 @@ package daemon // import "github.com/docker/docker/daemon"
 
 import (
 	"github.com/docker/docker/daemon/config"
-	"github.com/docker/docker/pkg/sysinfo"
 )
 
 const platformSupported = false
 
 func setupResolvConf(config *config.Config) {
 }
-
-// RawSysInfo returns *sysinfo.SysInfo .
-func (daemon *Daemon) RawSysInfo(quiet bool) *sysinfo.SysInfo {
-	return sysinfo.New(quiet)
-}
diff --git a/daemon/daemon_windows.go b/daemon/daemon_windows.go
index 7fffbfb609..021b7b8f0a 100644
--- a/daemon/daemon_windows.go
+++ b/daemon/daemon_windows.go
@@ -657,8 +657,3 @@ func setupResolvConf(config *config.Config) {
 func (daemon *Daemon) useShimV2() bool {
 	return true
 }
-
-// RawSysInfo returns *sysinfo.SysInfo .
-func (daemon *Daemon) RawSysInfo(quiet bool) *sysinfo.SysInfo {
-	return sysinfo.New(quiet)
-}
diff --git a/daemon/info.go b/daemon/info.go
index 91eb826f8a..cb1d39b04b 100644
--- a/daemon/info.go
+++ b/daemon/info.go
@@ -28,7 +28,7 @@ import (
 func (daemon *Daemon) SystemInfo() (*types.Info, error) {
 	defer metrics.StartTimer(hostInfoFunctions.WithValues("system_info"))()
 
-	sysInfo := daemon.RawSysInfo(true)
+	sysInfo := sysinfo.New(true)
 	cRunning, cPaused, cStopped := stateCtr.get()
 
 	v := &types.Info{
diff --git a/pkg/sysinfo/sysinfo_linux.go b/pkg/sysinfo/sysinfo_linux.go
index c619559e12..de15166329 100644
--- a/pkg/sysinfo/sysinfo_linux.go
+++ b/pkg/sysinfo/sysinfo_linux.go
@@ -106,7 +106,11 @@ func newV2(quiet bool, opts *opts) *SysInfo {
 	}
 	g := opts.cg2GroupPath
 	if g == "" {
-		g = "/"
+		if euid := os.Getenv("ROOTLESSKIT_PARENT_EUID"); euid != "" {
+			g = fmt.Sprintf("/user.slice/user-%s.slice", euid)
+		} else {
+			g = "/"
+		}
 	}
 	m, err := cgroupsV2.LoadManager("/sys/fs/cgroup", g)
 	if err != nil {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

g = fmt.Sprintf("/user.slice/user-%s.slice", euid) is expected only when running in systemd mode

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"systemd mode" == "cgroupv2" or something else? (my patch above handles it in newV2(), or is that incorrect?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"systemd mode" == "--exec-opt native.cgroupdriver=systemd" (&& v2, in this context)

@AkihiroSuda AkihiroSuda force-pushed the cgroup2-dockerinfo branch 3 times, most recently from 0505663 to e219912 Compare March 14, 2020 10:14
@AkihiroSuda
Copy link
Member Author

@thaJeztah PTAL

@AkihiroSuda
Copy link
Member Author

@kolyshkin PTAL?

@AkihiroSuda
Copy link
Member Author

updated

@AkihiroSuda
Copy link
Member Author

@thaJeztah @kolyshkin PTAL?

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@AkihiroSuda
Copy link
Member Author

rebased

@@ -1779,3 +1779,16 @@ func (daemon *Daemon) setupSeccompProfile() error {
func (daemon *Daemon) useShimV2() bool {
return cgroups.IsCgroup2UnifiedMode()
}

// RawSysInfo returns *sysinfo.SysInfo .
func (daemon *Daemon) RawSysInfo(quiet bool) *sysinfo.SysInfo {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the need for this.
If we are adding options to the wrapped function call, why not add the operations in RawSysInfo as an option that callers can add when neccessary?

Copy link
Member Author

@AkihiroSuda AkihiroSuda Apr 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because sysinfo.New() should be decoupled from the daemon, and should not call daemon.getCgroupDriver() and os.Getenv("ROOTLESSKIT_PARENT_EUID")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we expose the sysinfo option fields?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you be more specific?

@AkihiroSuda AkihiroSuda force-pushed the cgroup2-dockerinfo branch 2 times, most recently from a795d44 to f7dbbe6 Compare April 16, 2020 22:12
ref: https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I'm not sure what to do with this new info function right now, but it's not changing any public interfaces, so I think it's fine.

@AkihiroSuda
Copy link
Member Author

@thaJeztah Ready to merge?

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I'm not sure what to do with this new info function right now, but it's not changing any public interfaces, so I think it's fine.

We discussed this in the maintainers meeting, and (hopefully) we could find a slightly cleaner/simpler approach, but as @cpuguy83 mentioned; it's not an "public" interface, some should still be able to make such changes in future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants