-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Conversation
105b8c2
to
a649a6d
Compare
e6feebc
to
58b3385
Compare
|
||
const platformSupported = false | ||
|
||
func setupResolvConf(config *config.Config) { | ||
} | ||
|
||
func (daemon *Daemon) RawSysInfo(quiet bool) *sysinfo.SysInfo { | ||
return sysinfo.New(quiet) |
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 {
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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)
0505663
to
e219912
Compare
@thaJeztah PTAL |
@kolyshkin PTAL? |
e219912
to
51fb1b9
Compare
updated |
@thaJeztah @kolyshkin PTAL? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
rebased |
51fb1b9
to
daf5d13
Compare
@@ -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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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")
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
a795d44
to
f7dbbe6
Compare
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>
f7dbbe6
to
f350b53
Compare
There was a problem hiding this 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.
@thaJeztah Ready to merge? |
There was a problem hiding this 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
- 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:
Rootless (No systemd):
Rootless (With
--exec-opt native.cgroupdriver=systemd
,Delegate=pids memory cpu
):- Description for the changelog
cgroup2: implement
docker info
- A picture of a cute animal (not mandatory but encouraged)
🐧