firecracker
firecracker copied to clipboard
refactor cgroup to allow multiple properties per controller
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.
Same as https://github.com/firecracker-microvm/firecracker/pull/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
- [ ] If a specific issue led to this PR, this PR closes the issue.
- [ ] The description of changes is clear and encompassing.
- [ ] Any required documentation changes (code and docs) are included in this PR.
- [ ] API changes follow the Runbook for Firecracker API changes.
- [ ] User-facing changes are mentioned in
CHANGELOG.md. - [ ] All added/changed functionality is tested.
- [ ] New
TODOs link to an issue. - [ ] Commits meet contribution quality standards.
- [ ] This functionality cannot be added in
rust-vmm.
Codecov Report
Attention: Patch coverage is 92.37288% with 9 lines in your changes missing coverage. Please review.
Project coverage is 84.37%. Comparing base (
e2fceff) to head (4e16439). Report is 6 commits behind head on main.
| Files | Patch % | Lines |
|---|---|---|
| src/jailer/src/cgroup.rs | 95.41% | 5 Missing :warning: |
| src/jailer/src/env.rs | 55.55% | 4 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #4387 +/- ##
==========================================
+ Coverage 84.22% 84.37% +0.15%
==========================================
Files 249 249
Lines 27421 27433 +12
==========================================
+ Hits 23095 23147 +52
+ Misses 4326 4286 -40
| Flag | Coverage Δ | |
|---|---|---|
| 5.10-c5n.metal | 84.60% <92.37%> (+0.17%) |
:arrow_up: |
| 5.10-m5n.metal | ? |
|
| 5.10-m6a.metal | 83.87% <92.37%> (+0.17%) |
:arrow_up: |
| 5.10-m6g.metal | 80.91% <92.37%> (+0.17%) |
:arrow_up: |
| 5.10-m6i.metal | 84.58% <92.37%> (+0.16%) |
:arrow_up: |
| 5.10-m7g.metal | 80.91% <92.37%> (+0.17%) |
:arrow_up: |
| 6.1-c5n.metal | 84.59% <92.37%> (+0.15%) |
:arrow_up: |
| 6.1-m5n.metal | 84.58% <92.37%> (?) |
|
| 6.1-m6a.metal | 83.87% <92.37%> (+0.17%) |
:arrow_up: |
| 6.1-m6g.metal | 80.90% <92.37%> (+0.17%) |
:arrow_up: |
| 6.1-m6i.metal | 84.58% <92.37%> (?) |
|
| 6.1-m7g.metal | 80.90% <92.37%> (+0.16%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I've added the integ test @pb8o made but it fails on aarch64. I found it's because it's using mkdirat instead of mkdir for some reason.