Skip to content
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.

Sign up for GitHub

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

Jump to bottom

pkg/sysinfo: remove libcontainer dependency #43347

Merged
merged 1 commit into from Mar 9, 2022
+52 −12
Merged

pkg/sysinfo: remove libcontainer dependency #43347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/sysinfo/cgroup2_linux.go
View file
Open in desktop
Expand Up @@ -5,9 +5,9 @@ import (
"path"
"strings"

"github.com/containerd/cgroups"
cgroupsV2 "github.com/containerd/cgroups/v2"
"github.com/containerd/containerd/pkg/userns"
"github.com/opencontainers/runc/libcontainer/cgroups"
"github.com/sirupsen/logrus"
)
Copy link
Member

Choose a reason for hiding this comment

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

This caused a regression:

Because containerd/cgroups.ParseCgroupFile (purposely) does not support the unified hierarchy:

Copy link
Member

Choose a reason for hiding this comment

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

Ah, dang that's what I initially was afraid of #43347 (comment)


Expand Down
62 changes: 51 additions & 11 deletions pkg/sysinfo/sysinfo_linux.go
View file
Open in desktop
Expand Up @@ -5,22 +5,62 @@ import (
"os"
"path"
"strings"
"sync"

cdcgroups "github.com/containerd/cgroups"
cdseccomp "github.com/containerd/containerd/pkg/seccomp"
"github.com/opencontainers/runc/libcontainer/cgroups"
"github.com/containerd/cgroups"
"github.com/containerd/containerd/pkg/seccomp"
"github.com/moby/sys/mountinfo"
"github.com/sirupsen/logrus"
)

func findCgroupMountpoints() (map[string]string, error) {
cgMounts, err := cgroups.GetCgroupMounts(false)
Copy link
Member

Choose a reason for hiding this comment

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

FTR; the function in runc also deals with V2, but (see rest of this PR) we're not using that, so 👍 https://github.com/opencontainers/runc/blob/eddf35e5462e2a9f24d8279874a84cfc8b8453c2/libcontainer/cgroups/utils.go#L76-L88

var (
readMountinfoOnce sync.Once
readMountinfoErr error
cgroupMountinfo []*mountinfo.Info
)

// readCgroupMountinfo returns a list of cgroup v1 mounts (i.e. the ones
// with fstype of "cgroup") for the current running process.
//
// The results are cached (to avoid re-reading mountinfo which is relatively
// expensive), so it is assumed that cgroup mounts are not being changed.
func readCgroupMountinfo() ([]*mountinfo.Info, error) {
readMountinfoOnce.Do(func() {
Copy link
Member

Choose a reason for hiding this comment

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

So, it looks like findCgroupV1Mountpoints() is only used by findCgroupV1Mountpoints(); would it make sense to put the sync.Once inside findCgroupV1Mountpoints() instead, that way we also skip parsing the results 🤔

Anyway, ok for a follow up (if that would work)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took the conservative route of retaining the existing behavior as I am not familiar enough with the finer points of cgroups to know whether /proc/self/cgroups is as safe to cache as the cgroupfs mounts in /proc/self/mountinfo. I agree that it would make a lot of sense to parse /proc/self/cgroups only once if we know it is safe to cache.

Copy link
Member

Choose a reason for hiding this comment

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

Heh; I was looking along the same lines; initially thought "cache it!" then noticed /proc/self and was in doubt. I think it can be cached (/proc/self being the daemon process everywhere it's called), but definitely should be checked if it's safe.

(hence the "for a follow up" 😂)

cgroupMountinfo, readMountinfoErr = mountinfo.GetMounts(
mountinfo.FSTypeFilter("cgroup"),
)
})

return cgroupMountinfo, readMountinfoErr
}

func findCgroupV1Mountpoints() (map[string]string, error) {
mounts, err := readCgroupMountinfo()
if err != nil {
return nil, err
}

allSubsystems, err := cgroups.ParseCgroupFile("/proc/self/cgroup")
Copy link
Member

Choose a reason for hiding this comment

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

I just recalled there was a subtle difference between the "runc" variant and the "containerd" variant that I noticed when I exported this (containerd/cgroups#197); the same where the containerd variant ignored empty subsystems, but the runc variant didn't;

in containerd/cgroups

for _, subs := range strings.Split(parts[1], ",") {
	if subs != "" {
		cgroups[subs] = parts[2]
	}
}

in libcontainer/cgroups

for _, subs := range strings.Split(parts[1], ",") {
	cgroups[subs] = parts[2]
}

But re-reading the comment in runc opencontainers/runc#3001 (comment), it looks to be only for v2 cgroups (which we aren't using here), so looks like it was fine after all 😅

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for rambling 😂 - had to double check if it was fine here (had that on my list of todo's 🤗)

if err != nil {
return nil, fmt.Errorf("Failed to parse cgroup information: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

(not for this PR) we should fix this error to not start with a capital, and use pkg/errors for this (not sure if we should wrap (as that may return a ENOENT) or keep formatting as-is.

}

allMap := make(map[string]bool)
for s := range allSubsystems {
allMap[s] = false
}

mps := make(map[string]string)
for _, m := range cgMounts {
for _, ss := range m.Subsystems {
mps[ss] = m.Mountpoint
for _, mi := range mounts {
for _, opt := range strings.Split(mi.VFSOptions, ",") {
seen, known := allMap[opt]
if known && !seen {
allMap[opt] = true
mps[strings.TrimPrefix(opt, "name=")] = mi.Mountpoint
}
}
if len(mps) >= len(allMap) {
break
}
}
return mps, nil
Expand All @@ -45,7 +85,7 @@ func WithCgroup2GroupPath(g string) Opt {
// New returns a new SysInfo, using the filesystem to detect which features
// the kernel supports.
func New(options ...Opt) *SysInfo {
if cdcgroups.Mode() == cdcgroups.Unified {
if cgroups.Mode() == cgroups.Unified {
return newV2(options...)
}
return newV1()
Expand All @@ -64,7 +104,7 @@ func newV1() *SysInfo {
applyCgroupNsInfo,
}

sysInfo.cgMounts, err = findCgroupMountpoints()
sysInfo.cgMounts, err = findCgroupV1Mountpoints()
if err != nil {
logrus.Warn(err)
} else {
Expand Down Expand Up @@ -246,7 +286,7 @@ func applyCgroupNsInfo(info *SysInfo) {

// applySeccompInfo checks if Seccomp is supported, via CONFIG_SECCOMP.
func applySeccompInfo(info *SysInfo) {
info.Seccomp = cdseccomp.IsEnabled()
info.Seccomp = seccomp.IsEnabled()
}

func cgroupEnabled(mountPoint, name string) bool {
Expand Down