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

systemd: adjust CPUQuotaPerSecUSec to compensate for systemd internal handling #1651

Merged
merged 1 commit into from
Nov 16, 2017

Conversation

sjenning
Copy link
Contributor

@derekwaynecarr
Copy link
Contributor

This was a fun one.

LGTM

@sjenning
Copy link
Contributor Author

wait on this, my rounding is wrong for quotas that don't need adjustment. i'll fix in a minute.

@sjenning sjenning force-pushed the adjust-systemd-cpuquota branch 2 times, most recently from f97fc12 to b9c18b8 Compare November 15, 2017 21:21
@sjenning
Copy link
Contributor Author

ok should be good now

@@ -271,6 +271,13 @@ func (m *Manager) Apply(pid int) error {
// cpu.cfs_quota_us and cpu.cfs_period_us are controlled by systemd.
if c.Resources.CpuQuota != 0 && c.Resources.CpuPeriod != 0 {
cpuQuotaPerSecUSec := uint64(c.Resources.CpuQuota*1000000) / c.Resources.CpuPeriod
// systemd converts CPUQuotaPerSecUSec (microseconds per CPU second) to CPUQuota
// (integer percentage of CPU) internally. This means that if a fractional percent of
// CPU is inidicated by Resources.CpuQuota, we need to round up to the nearest

Choose a reason for hiding this comment

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

nit inidicated to indicated

Copy link

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

comment nit, otherwise LGTM

… handling

Signed-off-by: Seth Jennings <sjenning@redhat.com>
@sjenning
Copy link
Contributor Author

@mrunalp can I get review/approval on this? I need one other approver as well.

@dqminh
Copy link
Contributor

dqminh commented Nov 16, 2017

Its a bit ugly though, do you think we should just let systemd create the slice without limits, then set the limits ourselves ?

@sjenning
Copy link
Contributor Author

@dqminh it is not a good idea to change values in cgroups that systemd thinks it controls. We have encountered issues where things like a daemon-reload will cause those values to be lost.

@dqminh
Copy link
Contributor

dqminh commented Nov 16, 2017

LGTM

@sjenning yah i think i had some similar bugs like that in the past I thought that would be fixed in recent version of systemd though ( but im not too sure as we dont run systemd cgroup now). Probably need more investigations though, meanwhile i think this works fine as a workaround.

Approved with PullApprove

@mrunalp
Copy link
Contributor

mrunalp commented Nov 16, 2017

LGTM

Approved with PullApprove

@mrunalp mrunalp merged commit fb6ec65 into opencontainers:master Nov 16, 2017
openshift-merge-robot added a commit to openshift/origin that referenced this pull request Dec 17, 2017
Automatic merge from submit-queue.

UPSTREAM: opencontainer/runc: 1651: systemd: adjust CPUQuotaPerSecUSec to compensate for systemd internal handling

opencontainers/runc#1651

holding on master rebase

@derekwaynecarr @mrunalp @jupierce
openshift-merge-robot added a commit to openshift/origin that referenced this pull request Jan 5, 2018
Automatic merge from submit-queue.

[3.8] UPSTREAM: opencontainers/runc: 1651: systemd: adjust CPUQuotaPerSecUSec to compensate for systemd internal handling

opencontainers/runc#1651

master PR: #17348

@derekwaynecarr @mrunalp @jupierce @eparis @smarterclayton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants