Fix setting some systemd limits, add more tests #2423
Merged
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
Contributor
Author
|
|
60c8635 to
546788c
Compare
Contributor
Author
|
This one is ready. There might be more similar work like that needed but this part is done. |
Contributor
Author
|
@AkihiroSuda @mrunalp PTAL |
Contributor
Author
|
@lifubang PTAL |
AkihiroSuda
reviewed
May 20, 2020
...so it will be easier to write more tests Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For some reason, this was not set before. Test case is added by the next commit. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. add missing checks for systemd's MemoryMax / MemoryLimit. 2. add checks for systemd's MemoryLow and MemorySwapMax. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. do not allow to set quota without period or period without quota, as we won't be able to calculate new value for CPUQuotaPerSecUSec otherwise. 2. do not ignore setting quota to -1 when a period is not set. 3. update the test case accordingly. Note that systemd value checks will be added in the next commit. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently, both systemd cgroup drivers (v1 and v2) only set "TasksMax" unit property if the value > 0, so there is no way to update the limit to -1 / unlimited / infinity / max. Since systemd driver is backed by fs driver, and both fs and fs2 set the limit of -1 properly, it works, but systemd still has the old value: # runc --systemd-cgroup update $CT --pids-limit 42 # systemctl show runc-$CT.scope | grep TasksMax TasksMax=42 # cat /sys/fs/cgroup/system.slice/runc-$CT.scope/pids.max 42 # ./runc --systemd-cgroup update $CT --pids-limit -1 # systemctl show runc-$CT.scope | grep TasksMax= TasksMax=42 # cat /sys/fs/cgroup/system.slice/runc-xx77.scope/pids.max max Fix by changing the condition to allow -1 as a valid value. NOTE other negative values are still being ignored by systemd drivers (as it was done before). I am not sure whether this is correct, or should we return an error. A test case is added. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
546788c to
5989736
Compare
Member
|
LGTM |
1 similar comment
Contributor
|
LGTM |
This was referenced May 21, 2020
Closed
This was referenced Jun 4, 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 fixes a few systemd cgroup bugs for both v1 and v2, and adds tests for systemd limit values (which uncover the bugs being fixed).
In particular, the bug fixes are:
TasksMax=infinity(akapids.limit: -1) [v1 and v2]MemoryLow(akamemory.reservation) [v2 only]CPUQuotaPerSecUSecfor some cases [v1 only]The integration test improvements are:
check_systemd_valueusageMemoryMax/MemoryLimitMemoryLowandMemorySwapMaxTasksMaxCPUSharesandCPUQuotaPerSecUSec)pids.limit=-1Please see individual commits for details.