Cpu quota fixes #2428
Cpu quota fixes #2428
Conversation
4d353c4 to
0fbba3d
Compare
|
First three (preparatory) commits: CI ✔️ Adding more |
8f1cf04 to
772e04e
Compare
|
This one is almost ready. Added missing tests for removing CPU quota. |
|
@AkihiroSuda @mrunalp @cyphar PTAL |
tests/integration/update.bats
Outdated
| [ "$status" -eq 0 ] | ||
| check_cpu_quota 600000 1000000 "600ms" | ||
|
|
||
| # remove cpu quota |
There was a problem hiding this comment.
thanks for catching this -- fixed.
NOTE that (as I noted in the changelog message) I have converted this code (and the code I've added) to use tab instead of 4 spaces since I like tabs much more than 4 spaces, and I am changing lots of code here now.
Long term, I don't know what to do about it. I want to avoid commits just changing the whitespace as this will make it harder to look through git history, but I don't like the current way of using 4 tabs either.
There was a problem hiding this comment.
Is there any tool we can run to ensure consistency?
There was a problem hiding this comment.
shfmt solves this problem (and we had it in CI for a few months a while ago) but see #2396 which tracks re-adding it.
|
The indentation seems broken. Other parts look good. |
|
LGTM |
|
Needs rebase. |
|
Rebased. I am also going to separate the regression fix out of this. |
See #2434 |
|
container update from the command line is a mess; need more time to fix it |
This reverts commit be54678. The proper fix will follow. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Use r instead of c.Resources for readability. No functional change. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Move setting cgroup.Resources (except Devices) into a separate function, to be used by the following commit. This is best reviewed with --ignore-space-change Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This removes the duplicated functionality of interpreting the data from specs.LinuxResources to fill in configs.Resources. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case CpuQuota is set, and CpuPeriod is not, we should assume the default (which is the same for v1 and v2). Currently, only ConvertCPUQuotaCPUPeriodToCgroupV2Value assumes it. Move the assumption to specconv, making it universally available. This makes all the cgroup drivers behave the same. This commit also fixes the regression introduced by commit 06d7c1d, which made it impossible to only set CpuQuota (without the CpuPeriod). NOTE it is still wrong to set CpuPeriod without setting CpuQuota. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This was added by commit 06d7c1d but is a no-op since CpuPeriod is uint64. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The code that adds CpuQuotaPerSecUSec should be the same in v1 and v2 systemd cgroup driver. Move it to common, using v1 implementation. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Setting or updating CpuPeriod without CpuQuota - does not make sense; - impossible to have with systemd cgroup drivers. Reject such configuration as invalid. Modify the test accordingly. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This (and the converting function) is only used by one of the four cgroup drivers. The other three do some checking and conversion in place, so let the fs2 do the same. Looks like this is also fixing the issue of `--cpu-quota=-1` being ignored by the fs2 driver. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
With this commit, CPU quota/period/weight/shares are now tested for cgroupv2. Since I had to modify almost every line of the test case, I took a liberty to convert spaces to tabs while at it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add a check that CPU quota can be reset. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
rebased on top of #2434 |
|
Going to rework this in a slightly less dramatic fashion :) |
This fixes a few bugs wrt setting cpu quota, including a recent regression, adds more tests for cpu quota, and (hopefully) simplifies the code handling all this.
The bugs being fixed are:
The regression introduced by commit 06d7c1d (PR Fix setting some systemd limits, add more tests #2423), which made
it impossible to only set CpuQuota (without the CpuPeriod). Reported by @haircommander
The discrepancy in how various cgroup drivers interpret the case ofI was wrong, all drivers assume previously set value.missing CpuPeriod. Before this, the fs2 driver assumed the default,
while the fs driver assumed the previously set value. Now the
behavior is the same.
An invalid configuration (CpuPeriod set without CpuQuota) was not rejected
by some drivers.
The fs2 driver ignored
--cpu-quota=-1when CpuPeriod was not set.The test cases added are
CPU quota/period/weight/shares tests for cgroup v2 🎉
Unlimited CPU quota test.