cgroup-cgroupfs: use caller's cgroup as base for unspecified paths

[mdionisio]

Currently, if the OCI config.json does not specify a linux.cgroupsPath, crun defaults to creating the container's cgroups at the root of the cgroup hierarchy. However, "my" expected behavior (and the one implemented by runc) is to create the container cgroup as a child of the cgroup where the runtime is being executed.

This is particularly important for:

Unprivileged/Nested environments: Where the caller might not have permissions to create directories at the cgroup root.

Consistency: Ensuring that container orchestration tools that rely on runc's default behavior work seamlessly with crun.

The proposed changes in src/libcrun/cgroup-cgroupfs.c implement a helper to parse /proc/self/cgroup. It correctly identifies the current path for both cgroup v2 (unified) and v1 (searching for the cpuset controller). If found, this path is used as the prefix for the new container cgroup ID.

I have NOT test is in all the possible environment. I have done comparative test with cgroup v1 and with patch attached crun and runc works more similar.

I think that this "fix" can help also to reduce impact of
#1625
because if we start crun from "well" configured environment, the new created cgroup will copy cpuset property from parent.

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refines crun's cgroup creation mechanism by changing the default behavior for unspecified cgroup paths. Instead of always creating cgroups at the root, crun will now attempt to use the caller's cgroup as the parent, aligning its functionality with runc. This modification addresses permission issues in restricted environments and ensures greater consistency for container orchestration workflows.

Highlights

• Default Cgroup Path Alignment: crun now defaults to creating container cgroups as children of the runtime's cgroup when linux.cgroupsPath is not specified, mirroring runc's behavior.
• Enhanced Environment Compatibility: This change improves functionality in unprivileged/nested environments where creating cgroups at the root might not be permitted.
• Improved Orchestration Tool Consistency: Ensures smoother integration with container orchestration tools that rely on runc's default cgroup behavior.
• New Cgroup Path Helper: Introduced get_current_cpuset_group to parse /proc/self/cgroup and identify the current cgroup path for both cgroup v1 (cpuset) and v2.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request modifies crun to use the caller's cgroup as a base for the container's cgroup when no explicit path is provided, aligning its behavior with runc. The implementation adds a new helper function to parse /proc/self/cgroup to determine the current cgroup.

My review has identified a critical issue related to memory management where a const pointer is being freed, which is undefined behavior. I have also provided a suggestion to refactor the new helper function for better readability and consistency with the existing codebase style. Please address these points.

[packit-as-a-service]

TMT tests failed. @containers/packit-build please check.

[giuseppe]

Currently, if the OCI config.json does not specify a linux.cgroupsPath, crun defaults to creating the container's cgroups at the root of the cgroup hierarchy. However, "my" expected behavior (and the one implemented by runc) is to create the container cgroup as a child of the cgroup where the runtime is being executed.

Thanks for the patch, although I don't think this is a good behavior. With cgroup v2, processes can be placed only in leaf nodes, so creating a sub-cgroup at the current path is a problem because you'd first need to migrate all the processes in the current cgroup to a sibling cgroup.

Is runc doing this also on cgroup v2?

Also, we already have a function to read the current cgroup (libcrun_get_cgroup_process), so we can just reuse that if we agree on this behavior

[lsm5]

I'll defer to @giuseppe . Just making an observation that all testing-farm jobs are failing which I think should be handled if we're going ahead with this.

[giuseppe]

I think we can't accept it in this form, it breaks cgroup2

[mdionisio]

Hi, I'm not test yet runc on v2. In any case, what do you suggest? to apply the change only on v1 ?

[giuseppe]

I'd suggest to not make this change at all because: 1) we will have an unnecessary difference between the two versions, 2) cgroup v1 support is going to be dropped soon

[kolyshkin]

runtime-spec (https://github.com/opencontainers/runtime-spec/blob/main/config-linux.md#cgroups-path) says:

If the value is not specified, the runtime MAY define the default cgroups path.

Indeed, runc uses $CURRENT_CGROUP/$CONTAINER_ID as cgroup v1 path in case it is not explicitly set, but this is highly optional (and maybe helps some old docker-in-docker with no cgroupns cases, which are now obsoleted since we have cgroup namespaces). I would not like it in crun.

I also agree with @giuseppe that cgroup v1 is deprecated, so we don't add new features to it (I think this is a feature not a bug).

@mdionisio I suggest you to switch to cgroup v2 (it is so much cleaner!) and use cgroupns, if at all possible.