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
refactor cgroup to allow multiple properties per controller #4387
base: main
Are you sure you want to change the base?
Conversation
c46ca08
to
5c43d12
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4387 +/- ##
==========================================
+ Coverage 81.52% 81.63% +0.11%
==========================================
Files 241 241
Lines 29296 29322 +26
==========================================
+ Hits 23883 23938 +55
+ Misses 5413 5384 -29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
5c43d12
to
5510bd5
Compare
This commit adds a new unit test that checks that cgroupv1 can set a simple property. Signed-off-by: Riccardo Mancini <mancio@amazon.com>
The previous Cgroup implementation used a separate object per every cgroup property. This lead to additional calls to create_dir_all and writes to cgroup.procs, especially in cgroupsv2 where there is only one hierarchy. This change prevents the duplication by refactoring the code into: - CgroupConfiguration: this holds multiple cgroup properties in multiple cgroup controllers and hierarchies - Cgroup: this holds the configuration for a hierarchy and holds multiple properties - CgroupProperty: a file to value mapping of the cgroup properties to be written. The CgroupBuilder is changed to a CgroupConfigurationBuilder so that the setup of the Cgroup is abstracted away from the environment. Additionally, in the CgroupV2 the available controllers read from cgroup.controllers are cached to avoid multiple unnecessary reads. Signed-off-by: Riccardo Mancini <mancio@amazon.com>
5510bd5
to
d6b681f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly good to go from my side, just a few minor comments and I think we need a solution for that unreachable!
.
let cgroup = CgroupV2::new(file, value, id, parent_cg, path)?; | ||
Ok(Box::new(cgroup)) | ||
let CgroupConfiguration::V2(cgroup_conf_v2) = &mut self.cgroup_conf else { | ||
unreachable!() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return an error here?
CgroupConfigurationImpl::<T>(HashMap::new()) | ||
} | ||
|
||
/// TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover?
|
||
writeln_special(location, pid)?; | ||
|
||
Ok(()) | ||
} | ||
} | ||
|
||
impl<T: Cgroup> CgroupConfigurationImpl<T> { | ||
/// TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover?
The previous Cgroup implementation used a separate object per every cgroup property. This lead to additional calls to create_dir_all and writes to cgroup.procs, especially in cgroupsv2 where there is only one hierarchy.
This change prevents the duplication by refactoring the code into:
The CgroupBuilder is changed to a CgroupConfigurationBuilder so that the setup of the Cgroup is abstracted away from the environment.
Additionally, in the CgroupV2 the available controllers read from cgroup.controllers are cached to avoid multiple unnecessary reads.
Same as #4384 but without the hacky integ test
Fixes: #2856
Changes
...
Reason
...
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following
Developer Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md
.PR Checklist
CHANGELOG.md
.TODO
s link to an issue.rust-vmm
.