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

parseCgroupFromReader(): Ignore unified paths with no subsystem #3001

Closed

Conversation

thaJeztah
Copy link
Member

When working on containerd/cgroups#197, I noticed this change, which was not in the libcontainer variant of the same function. I thought I'd open a pull request in case this is relevant in this repository as well.

This ports the changes from containerd's cgroup package:
containerd/cgroups@fe19473 (containerd/cgroups#21)

This ports the changes from containerd's cgroup package:
containerd/cgroups@fe19473

Co-authored-by: Michael Crosby <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

Comment on lines +184 to +186
if subs != "" {
cgroups[subs] = parts[2]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually we rely on this functionality in cgroup v2, where the subsystem is empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

By "we" I mean not only runc itself, but also kubernetes. I was going to introduce a v2-specific function but this one does the job just fine, the only overhead is it returns a map with a a single element with empty key, rather than just a string.

Copy link
Contributor

Choose a reason for hiding this comment

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

IOW, NACK

@kolyshkin
Copy link
Contributor

This ports the changes from containerd's cgroup package:
containerd/cgroups@fe19473 (containerd/cgroups#21)

More to say, this change ^^^ needs to be reverted, if you rely on ParseCgroupsFile in your v2 code.

@thaJeztah
Copy link
Member Author

More to say, this change ^^^ needs to be reverted, if you rely on ParseCgroupsFile in your v2 code.

Yes, I'm trying to make sense of it, so the containerd/cgroups package has a v1 and v2 packages in it.
Similar to libcontainer here, both also have a parseCgroupFromReader() one variant returns a single path, and one variant returns multiple (from comma-separated);

func parseCgroupFromReader(r io.Reader) (string, error) {

func parseCgroupFromReader(r io.Reader) (map[string]string, error) {

From your comment, I understand that both v1 and v2 cgroups can have the comma-separated format? If v2 never has a comma-separated list; should all v2 code use the variant that returns a string instead?

@kolyshkin
Copy link
Contributor

If v2 never has a comma-separated list; should all v2 code use the variant that returns a string instead?

Yes, this was the original implementation of it when I was working on untangling the v1/v2 mess. Had to revert this change later as a result of this discussion: #2411 (review)

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

Successfully merging this pull request may close these issues.

2 participants