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 systemd cpu quota for -1 #1805

Merged
merged 1 commit into from
May 24, 2018

Conversation

derekwaynecarr
Copy link
Contributor

if a caller passes -1 for cpu quota, no quota setting should be applied at all with systemd.

after #1651, passing -1 caused a side-effect of the value being converted to 10000 incorrectly.

this fixes an issue with the systemd cgroup driver in kubernetes when cfs quota is disabled.

@derekwaynecarr
Copy link
Contributor Author

/cc @mrunalp @sjenning

@derekwaynecarr
Copy link
Contributor Author

If the more appropriate change is to have Kubernetes send 0 whether cgroupfs or systemd driver, it would be good to get clarity from maintainers.

@derekwaynecarr
Copy link
Contributor Author

it looks like passing 0 would be consistent across cgroupfs and systemd as any non-zero value results in writing out to cfs_quota_us, and since the default cfs_quota_us is -1, writing nothing would be fine.

but if for example, we ever want to change from quota set to unset on an existing container, passing 0 would not induce a write with the cgroupfs driver, so maybe -1 is the right approach after all.

@derekwaynecarr
Copy link
Contributor Author

Ok, hold on this until I make sure that -1 is consistently applied on create and update of existing cgroups.

@derekwaynecarr derekwaynecarr force-pushed the systemd-cpuquota-fix branch 2 times, most recently from 3dcd6de to 3d5f4f3 Compare May 23, 2018 04:29
@derekwaynecarr
Copy link
Contributor Author

ok, this is ready to go, and ensures that we can set and unset quota.

PTAL @mrunalp @sjenning

@derekwaynecarr derekwaynecarr changed the title apply cpu quota only if value is greater than zero fix systemd cpu quota for -1 May 23, 2018
@derekwaynecarr
Copy link
Contributor Author

/hold this for a moment while i do a few more tests in a kube context.

// (integer percentage of CPU) internally. This means that if a fractional percent of
// CPU is indicated by Resources.CpuQuota, we need to round up to the nearest
// 10ms (1% of a second) such that child cgroups can set the cpu.cfs_quota_us they expect.
cpuQuotaPerSecUSec := uint64(c.Resources.CpuQuota*1000000) / c.Resources.CpuPeriod
Copy link
Contributor Author

Choose a reason for hiding this comment

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

knew i messed something up, this shadows the var

@derekwaynecarr
Copy link
Contributor Author

ok, all set, sorry for spam.

@sjenning
Copy link
Contributor

I verified this fix. With cpu-cfs-quota="true" limits were enforced at the pod and container level. When set to "false" the quota is not set (-1).

@mrunalp
Copy link
Contributor

mrunalp commented May 23, 2018

@mrunalp
Copy link
Contributor

mrunalp commented May 23, 2018

LGTM

Approved with PullApprove

1 similar comment
@dqminh
Copy link
Contributor

dqminh commented May 24, 2018

LGTM

Approved with PullApprove

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.

4 participants