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

libct/cg/sd: write rounded CPU quota to cgroupfs #4639

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hshiina
Copy link

@hshiina hshiina commented Feb 22, 2025

When CPU quota is updated, the value is converted to CPUQuotaPerSecUSec property for passing to systemd. The value can be rounded in the following cases:

  • The value is rounded up to the nearest 10ms.
  • Depending on CPU period, the value may be rounded during division.

Because of this rounding, systemd and runc systemd driver may write different values to cgroupfs. In order to avoid this inconsistency, this fix makes systemd driver write the same rounded value to cgroupfs by calculating the value from systemd properties.

Fixes #4622

@hshiina hshiina force-pushed the 4622-systemd-update-cpu-quota branch from 8d53945 to 9640365 Compare February 22, 2025 05:46
When CPU quota is updated, the value is converted to CPUQuotaPerSecUSec
property for passing to systemd. The value can be rounded in the
following cases:
- The value is rounded up to the nearest 10ms.
- Depending on CPU period, the value may be rounded during division.
Because of this rounding, systemd and runc systemd driver may write
different values to cgroupfs. In order to avoid this inconsistency,
this fix makes systemd driver write the same rounded value to cgroupfs
by calculating the value from systemd properties.

Signed-off-by: Hironori Shiina <[email protected]>
@hshiina hshiina force-pushed the 4622-systemd-update-cpu-quota branch from 9640365 to bfd4ea5 Compare February 22, 2025 08:03
@hshiina
Copy link
Author

hshiina commented Feb 22, 2025

/cc @rata

@rata
Copy link
Member

rata commented Feb 24, 2025

Oh, this is changing the behavior of existing tests? I'm busy and it's not as trivial as I'd thought. I'd take a while to review, sorry :(

I hope someone else can review meanwhile :)

@rata rata added the backport/1.2-todo A PR in main branch which needs to be backported to release-1.2 label Feb 24, 2025
@kolyshkin
Copy link
Contributor

Ideally, I think what we should do is to only set those parameters via fs cgroup driver which we are not able to convert to systemd. I've been meaning to try that for the last few years but never got around to :(

In this particular case though, I think we can set CpuQuota and CpuPeriod to 0 for fs manager, so it won't be set. The only thing we need to make sure of is systemd actually sets quota and period. From a cursory look it looks like CPUQuota was added in systemd v213 and CPUQuotaPeriodSec is in systemd v242 (and we already check that in addCpuQuota`.

One alternative way is to round up the value, using the same rules as in systemd. This is essentially what your patch does, but in a somewhat complicated way.

Oh well, you actually use the conversion which is performed by addCpuQuota but not systemd itself.

(Will take a deeper look later).

@kolyshkin
Copy link
Contributor

One other thing is, we are in the process of splitting libcontainer/cgroups into a separate repo (see #4618), meaning most probably you'll have to open two PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.2-todo A PR in main branch which needs to be backported to release-1.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

systemd driver updates CPU quota inconsitently
3 participants