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

Improvements to cgroup support #513

Merged
merged 5 commits into from
Dec 5, 2021
Merged

Conversation

Furisto
Copy link
Collaborator

@Furisto Furisto commented Dec 2, 2021

This contains multiple smaller improvements that did not really warrant their own PR.

  • When a transient unit is created by systemd we now ensure that all controllers that have been delegated are inherited by the subtree
  • Pause/Resume and stats are now enabled for systemd managed cgroups through the v2 cgroup manager
  • The libcgroups crate now provides feature flags for v1, v2 and systemd managed cgroups
  • Fixed some commands that failed due to trying to read the runtime config.json instead of the youki config.json

@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2021

Codecov Report

Merging #513 (af4d403) into main (6cb39e1) will increase coverage by 0.32%.
The diff coverage is 9.52%.

@@            Coverage Diff             @@
##             main     #513      +/-   ##
==========================================
+ Coverage   60.54%   60.86%   +0.32%     
==========================================
  Files          86       87       +1     
  Lines       12604    12672      +68     
==========================================
+ Hits         7631     7713      +82     
+ Misses       4973     4959      -14     

Copy link
Collaborator

@tsturzl tsturzl left a comment

Choose a reason for hiding this comment

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

Looks good. Minor nit picks, and if you disagree feel free to resolve.


#[cfg(not(feature = "v1"))]
fn create_v1_cgroup_manager(_cgroup_path: PathBuf) -> Result<Box<dyn CgroupManager>> {
bail!("cgroup v1 feature is not activated");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe for these features we should add some context around the fact that the build excluded this feature? I know we didn't do this in the past, but maybe it's worth doing moving forward.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I really appreciate this being broken out and put behind feature flags. I think this is much cleaner.

crates/libcgroups/src/systemd/manager.rs Show resolved Hide resolved
@Furisto
Copy link
Collaborator Author

Furisto commented Dec 4, 2021

@tsturzl PTAL

Copy link
Collaborator

@tsturzl tsturzl left a comment

Choose a reason for hiding this comment

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

This looks great

@tsturzl tsturzl merged commit eb083a1 into youki-dev:main Dec 5, 2021
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.

3 participants