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

fix cpu.cfs_quota_us changed when systemd daemon-reload using systemd. #1344

Merged
merged 1 commit into from
Mar 6, 2017

Conversation

xuxinkun
Copy link
Contributor

if c.Resources.CpuQuota != 0 && c.Resources.CpuPeriod != 0 {
cpuQuota := c.Resources.CpuQuota * 100 / c.Resources.CpuPeriod
if cpuQuota == 0 {
return fmt.Errorf("cpu_quota divided by cpu_period must be greater than 0.01")
Copy link
Member

Choose a reason for hiding this comment

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

Why? Where does this requirement come from (if it's a kernel requirement -- which I assume it is -- then we generally don't error out for cases like this)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the systemd can not accept that the percentage(cpu_quota divided by cpu_period) is less than 0.01. For example, if the user let CpuQuota be 10 and CpuPeriod be 10000, the cpu.cfs_quota_us will be -1, not 10. So, I think it is necessary to warn the user that the percentage must be greater than 0.01.

Copy link
Member

Choose a reason for hiding this comment

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

Can you provide a link to the systemd documentation which describes this option? I can't find it here. If it's an option added recently, you'll need to make your change backwards compatible with older systemd versions (by conditionally adding it like we do with Delegate for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I did not find it from document. By observing, I found the percentage cannot be lower than 0.01. I found that if i set the percentage lower than 0.01, the cpu.cfs_quota_us will not be set as it should be set.
How about this idea? I delete this check and add docs about it to warn users.

@xuxinkun
Copy link
Contributor Author

xuxinkun commented Mar 1, 2017

@cyphar I modified the code. Pls review it.

@cyphar cyphar force-pushed the fixCPUQuota20170224 branch 2 times, most recently from 6ad2da9 to c44aec9 Compare March 6, 2017 09:07
@cyphar
Copy link
Member

cyphar commented Mar 6, 2017

LGTM.

Approved with PullApprove

@xuxinkun
Copy link
Contributor Author

xuxinkun commented Mar 6, 2017

@dqminh May you help review?

@crosbymichael
Copy link
Member

crosbymichael commented Mar 6, 2017

LGTM

Approved with PullApprove

@crosbymichael crosbymichael merged commit 49a33c4 into opencontainers:master Mar 6, 2017
vieux pushed a commit to mlaventure/docker that referenced this pull request Mar 10, 2017
This fix a conflict with systemd daemon-reload (see
opencontainers/runc#1344)

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
renyufu pushed a commit to renyufu/docker that referenced this pull request Mar 18, 2017
This fix a conflict with systemd daemon-reload (see
opencontainers/runc#1344)

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
openshift-merge-robot added a commit to openshift/origin that referenced this pull request Sep 21, 2017
Automatic merge from submit-queue

UPSTREAM: opencontainers/runc: 1344: fix cpu.cfs_quota_us changed when n systemd daemon-reload using systemd

opencontainers/runc#1344

also see https://bugzilla.redhat.com/show_bug.cgi?id=1455071

@derekwaynecarr @runcom
openshift-merge-robot added a commit to openshift/origin that referenced this pull request Sep 22, 2017
Automatic merge from submit-queue

UPSTREAM: opencontainers/runc: 1344: fix cpu.cfs_quota_us changed when systemd daemon-reload using systemd

opencontainers/runc#1344

master PR:
#16480

also see https://bugzilla.redhat.com/show_bug.cgi?id=1455071

@derekwaynecarr @runcom
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.

None yet

3 participants