Skip to content

fix: Ignoring CPU realtime on cgroupsv2 if set to zero #3180

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

Merged
merged 5 commits into from
Jun 22, 2025

Conversation

CheatCodeSam
Copy link
Contributor

@CheatCodeSam CheatCodeSam commented Jun 6, 2025

Description

Requested to break up #3174 into two seperate issues by @YJDoc2.

This fixes an auxiliary issue of #2994, where nerdctl will set both linux.resources.cpu.realtimeRuntime and linux.resources.cpu.realtimePeriod to zero in the config.json despite youki trying to use cgroupsv2. This would cause a V2CpuControllerError to be thrown when the container is being created.

My solution is just to ignore them if using cgroupsv2 and they are both set to zero.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test updates
  • CI/CD related changes
  • Other (please describe):

Testing

  • Added new unit tests
  • Added new integration tests
  • Ran existing test suite
  • Tested manually (please provide steps)

Once the initial runtime spec was fixed, this was able to run with no problems on nerdctl.

Related Issues

Fixes part of #2994.

Additional Context

@CheatCodeSam CheatCodeSam changed the title [FIX] Ignoring CPU realtime on cgroupsv2 if set to zero fix: Ignoring CPU realtime on cgroupsv2 if set to zero Jun 6, 2025
@utam0k utam0k added the kind/bug label Jun 7, 2025
@utam0k utam0k requested a review from Copilot June 7, 2025 08:01
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates both the cgroupsv2 and systemd CPU controllers to skip the realtime runtime/period error when both values are explicitly set to zero, avoiding a V2CpuControllerError or SystemdCpuError in that case.

  • Adds a guard in apply to check for (Some(0), Some(0)) and bypass the realtime error.
  • Mirrors the same change in the systemd CPU controller.
  • Does not include new tests for the added branch.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
crates/libcgroups/src/v2/cpu.rs Skip realtime error when both runtime and period are zero
crates/libcgroups/src/systemd/cpu.rs Skip realtime error when both runtime and period are zero (systemd)
Comments suppressed due to low confidence (3)

crates/libcgroups/src/v2/cpu.rs:89

  • Add a unit or integration test covering the case where both realtime_runtime and realtime_period are zero to ensure this new branch is exercised.
let realtime_runtime = cpu.realtime_runtime();

crates/libcgroups/src/systemd/cpu.rs:45

  • Add a test in the systemd CPU controller for the scenario where both realtime values are zero so this branch is verified.
let realtime_runtime = cpu.realtime_runtime();

crates/libcgroups/src/v2/cpu.rs:86

  • The realtime-ignore logic is duplicated in both v2 and systemd modules; consider extracting it into a shared helper function to reduce code duplication.
impl Cpu {

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

Please add the unit test for your change 🙏

@CheatCodeSam
Copy link
Contributor Author

Tests have been added

Signed-off-by: Carson Weeks <[email protected]>
Signed-off-by: Carson Weeks <[email protected]>
Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

Thanks!

@utam0k utam0k merged commit db59aff into youki-dev:main Jun 22, 2025
29 checks passed
@github-actions github-actions bot mentioned this pull request Jun 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants