Cpu quota fixes, try II #2458
Merged
Cpu quota fixes, try II #2458
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AkihiroSuda
reviewed
Jun 5, 2020
Use r instead of c.Resources for readability. No functional change. This commit has been brought to you by '<,'>s/c\.Resources\./r./g Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
826ae7d to
e8f49b0
Compare
Contributor
Author
|
@AkihiroSuda @mrunalp @cyphar PTAL |
mrunalp
reviewed
Jun 8, 2020
| config.Cgroups.Resources.CpuQuota = q | ||
| } else { | ||
| // one is set and the other is not | ||
| if p != 0 { |
Contributor
There was a problem hiding this comment.
This is a very unlikely scenario. Should we just fail here?
Contributor
Author
There was a problem hiding this comment.
Not unlikely. I tried failing when one is set and the other isn't and it ended up not so good so I had to revert it, see #2434
This allows to use runc update $ID --cpu-quota without --cpu-period and vice versa, in which case the old value from the missing setting is used (it is loaded into config earlier).
Contributor
Author
|
One more corner case I have missed: runc --systemd-cgroup run -d xx44 # no cpu quota/limit setting in config
runc --systemd-cgroup update xx44 --cpu-period 5000
cat /sys/fs/cgroup/system.slice/runc-xx44.scope/cpu.max
max 100000
^^^ bug -- the new cpu-period setting is ignored :( |
Contributor
Author
Which is fortunately easy to fix (see the last commit). |
Contributor
Author
It was my own regression, so I fixed it in place. Adding more tests... |
6bed815 to
688eab1
Compare
Contributor
Author
Contributor
Author
|
@AkihiroSuda @mrunalp PTAL |
The code that adds CpuQuotaPerSecUSec is the same in v1 and v2 systemd cgroup driver. Move it to common. No functional change. Note that the comment telling that we always set this property contradicts with the current code, and therefore it is removed. [v2: drop cgroupv1-specific comment] [v3: drop returning error as it's not used] [v4: remove an obsoleted comment] 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. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Seting CPU quota and period independently does not make much sense,
but historically runc allowed it and this needs to be supported
to not break compatibility.
For systemd cgroup drivers to set CPU quota/period correctly,
it needs to know both values. For fs2 cgroup driver to be compatible
with the fs driver, it also needs to know both values.
Here in update, previously set values are available from config.
If only one of {quota,period} is set and the other is not, leave
the unset parameter at the old value (don't overwrite config).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Also, enable tests of setting quota and period separately in case systemd cgroup driver is used, as commit 32746fb ("update: do not overwrite old cpu quota/period") made it possible to do so. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Check that resetting cpu quota works. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Contributor
Author
|
Please wait; I want to polish this one a bit further |
systemd drivers ignore --cpu-quota during update if the CPU period was not set earlier. Fixed by adding the default for the period. The test will be added by the following commit. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add four "corner case" tests that check that the CPU period/quota can be set/updated even in case neither CPU quota nor CPU period (were previously) set. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Contributor
Author
|
Dropped the patch that rejected cpu period without cpu quota on run/start, as it could bring some backward compatibility issues. Instead, we use default quota of This one is ready for review. |
AkihiroSuda
reviewed
Jun 10, 2020
| // 10ms (1% of a second) such that child cgroups can set the cpu.cfs_quota_us they expect. | ||
| if period == 0 { | ||
| // assume the default kernel value of 100000 us (100 ms), same for v1 and v2. | ||
| // v1: https://www.kernel.org/doc/html/latest/scheduler/sched-bwc.html and |
Member
There was a problem hiding this comment.
nit: latest -> v5.6 to avoid potential deadlink?
AkihiroSuda
approved these changes
Jun 10, 2020
Member
|
@cyphar LGTY? |
This was referenced Jun 11, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this is loosely based on #2428 but I am trying to make things right this time
Seting CPU quota and period independently does not make much sense, but historically runc allowed it and this needs to be supported to not break compatibility.
This PR fixes the following issues:
1. Issue #2456
systemd/v1, systemd/v2 and fs2 cgroup drivers reset the period to default 100000 during
runc updateif it is not set explicitly. This makes the behavior inconsistent with the one when fs driver is used (it leaves the period at the old value).The issue is fixed by reusing the old (previously set) value for period/quota, if a new period/quota is not specified during the update.
2. Issue #2464
systemd/v1, systemd/v2 and fs2 cgroup drivers ignore the CPU period during start/run, if CPU quota is not set. The fs (v1) driver sets the period.
The issue is fixed by rejecting the configuration where quota is not set but the period is set during start. NOTE: this can bring some backward compatibility issues but I consider the chances are low.Issue fixed by
3. Issue #2463
systemd v2 driver ignores --cpu-quota during update if CPU period was not set.
Fixed by adding the default period to
addCpuQuotaOther changes
In addition, this PR enables CPU quota and CPU shares tests for v2, adds unlimited quota tests, and some corner test cases to cover all the issues above.
Fixes: #2456, #2463, #2464.