firecracker icon indicating copy to clipboard operation
firecracker copied to clipboard

refactor cgroup to allow multiple properties per controller

Open mancio-aws opened this issue 1 year ago • 1 comments

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.

mancio-aws avatar Jan 19 '24 16:01 mancio-aws

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.

codecov[bot] avatar Jan 19 '24 16:01 codecov[bot]

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.

Manciukic avatar Aug 12 '24 15:08 Manciukic