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

Allow completely disabling cgroup manipulation #2892

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

aidanhs
Copy link

@aidanhs aidanhs commented Aug 24, 2024

This enables usage of libcontainer etc in situations where you want a rootless container and you do not have access to systemd.

This has been achieved by setting up cgroups once (on container init) and storing the config in the youki_config.json file, then loading it afterwards when it needs to be referenced rather than deriving it from scratch every time. I suspect this will fix bugs with exec not correctly using systemd for cgroups (because subcommands were not performing the check for user namespaces, they just check for the --systemd-cgroup flag).

This is a breaking change to the API, as now use_systemd cannot be overridden on the Container type - this PR makes it a static property of the container once created (like hooks). But I think if people are doing on-the-fly editing of cgroup management for a created container they're already off the happy path of youki anyway.

Note: you may want to review this via https://github.com/youki-dev/youki/pull/2892/files?w=1 (note the &w=1) to ignore whitespace differences on the cleanup_container function, which has been moved to a free function rather than a method.

@aidanhs aidanhs force-pushed the aphs-disabling-cgroups branch 2 times, most recently from cfae143 to c535084 Compare August 25, 2024 12:26
@utam0k
Copy link
Member

utam0k commented Aug 28, 2024

Sorry for the late. I'll review this PR next weekend. If you have time to check CIs, please fix it before starting reviewing 🙏

@utam0k utam0k self-requested a review August 28, 2024 13:15
@aidanhs aidanhs force-pushed the aphs-disabling-cgroups branch from c535084 to 3783dab Compare August 30, 2024 18:49
@aidanhs
Copy link
Author

aidanhs commented Aug 30, 2024

Have fixed linting and unit test compilation failures. Have run the unit tests and integration tests as best I can (struggled with WASM ones) and the only failures I get also fail on main.

Signed-off-by: Aidan Hobson Sayers <[email protected]>
Signed-off-by: Aidan Hobson Sayers <[email protected]>
Signed-off-by: Aidan Hobson Sayers <[email protected]>
@utam0k
Copy link
Member

utam0k commented Sep 1, 2024

This enables usage of libcontainer etc in situations where you want a rootless container and you do not have access to systemd.

Why don't you directly use cgroupv2 fs instead of systemd? Also, I'd like to know more concrete use case you mentioned.

@aidanhs
Copy link
Author

aidanhs commented Sep 1, 2024

The program I'm working on will be distributed to users and I want it to work natively in WSL, without any additional user setup. Currently I use nsjail for rootless namespace isolation (pid and mount only) and have no need for cgroups or anything else.

I'm doing the isolation rootless, WSL uses the unified setup (microsoft/WSL#6662) and also does not have systemd enabled by default - so afaict none of systemd, cgroupsv1 or cgroupsv2 will work for me without some configuration. libcontainer allows disabling most things (via the bundle), but it will always try to access cgroups to create a cmanager (even if you're not applying resource limits or entering a cgroup namespace) - hence this PR.

There were two ways I could tackle this PR:

  1. auto detecting usage of cgroup namespaces and/or resources, and using cgroup managers on-demand
  2. (current implementation) explicitly disabling cgroup management

I opted against 1 because it removes control from the user - if someone wants the container in a cgroup without resources or cgroup namespaces (e.g. to manage outside of libcontainer), they'd have no way to achieve that.

@aidanhs
Copy link
Author

aidanhs commented Sep 1, 2024

I will say - if your goal is to have libcontainer be a runtime primarily for OCI, rather than a generic 'namespace toolkit' (like nsjail), I understand if you're not interested in this PR.

@utam0k
Copy link
Member

utam0k commented Dec 24, 2024

We understand the demand, but this implementation is still unacceptable. This is because the demand is not very high, whereas the cost of maintenance is too high.
So, how about smoothly harmonizing it with the current implementation: why not create a Dummy CgroupManager and have it do nothing?

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

Successfully merging this pull request may close these issues.

2 participants