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.

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

pkg/sysinfo: remove libcontainer dependency #43347

Merged
merged 1 commit into from Mar 9, 2022

Conversation

corhere
Copy link
Contributor

@corhere corhere commented Mar 7, 2022

- What I did
Addressed part of #42452. The only remaining place where package github.com/opencontainers/runc/libcontainer/cgroups is used within Moby is

moby/daemon/oci_linux.go

Lines 821 to 843 in 327699c

p := cgroupsPath
if useSystemd {
initPath, err := cgroups.GetInitCgroup("cpu")
if err != nil {
return errors.Wrap(err, "unable to init CPU RT controller")
}
_, err = cgroups.GetOwnCgroup("cpu")
if err != nil {
return errors.Wrap(err, "unable to init CPU RT controller")
}
p = filepath.Join(initPath, s.Linux.CgroupsPath)
}
// Clean path to guard against things like ../../../BAD
parentPath := filepath.Dir(p)
if !filepath.IsAbs(parentPath) {
parentPath = filepath.Clean("/" + parentPath)
}
mnt, root, err := cgroups.FindCgroupMountpointAndRoot("", "cpu")
if err != nil {
return errors.Wrap(err, "unable to init CPU RT controller")
}

- How I did it
Reimplemented GetCgroupMounts in terms of the github.com/containerd/cgroups and github.com/moby/sys/mountinfo packages. I recursively inlined the libcontainer implementation into the findCgroupMountpoints function and refactored the resulting code, deleting dead branches and simplifying what remained.

- How to verify it
Are existing integration tests sufficient?

- Description for the changelog
N/A

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

Reimplement GetCgroupMounts using the github.com/containerd/cgroups and
github.com/moby/sys/mountinfo packages.

Signed-off-by: Cory Snider <csnider@mirantis.com>
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, thanks!!

"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

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 🤗)

// 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" 😂)

return nil, err
}

allSubsystems, err := cgroups.ParseCgroupFile("/proc/self/cgroup")
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.

@thaJeztah thaJeztah added area/daemon kind/refactor PR's that refactor, or clean-up code status/4-merge labels Mar 9, 2022
@thaJeztah thaJeztah added this to the 21.xx milestone Mar 9, 2022
@thaJeztah thaJeztah merged commit 8539d06 into moby:master Mar 9, 2022
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.

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/daemon kind/refactor PR's that refactor, or clean-up code status/4-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants