cgroup: workaround cpu quota/period issue with v1 #1188
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
giuseppe
reviewed
Apr 6, 2023
src/libcrun/cgroup-resources.c
Outdated
| if (UNLIKELY (ret < 0)) | ||
| return ret; | ||
| { | ||
| if (errno != EINVAL) |
Member
There was a problem hiding this comment.
could you please replace errno with crun_error_get_errno (err)?
Collaborator
Author
There was a problem hiding this comment.
That's exactly what I was thinking, thanks!
Collaborator
Author
There was a problem hiding this comment.
Right (I thought about that but forgot).
In addition, I forgot to add !cpu->quota check, since if we won't be setting quota later, we need to return an error. Fixed now.
| * it is rejected by the kernel (EINVAL) as old_quota/new_period exceeds | ||
| * the parent cgroup quota limit. If this happens and the quota is going | ||
| * to be set, ignore the error for now and retry after setting the quota. | ||
| */ |
Member
There was a problem hiding this comment.
since the err is not used, we'll need to free it with crun_error_release (err);
Sometimes setting CPU quota period fails when a new period is lower, and a parent cgroup has CPU quota limit set. This happens as in cgroup v1 the quota and the period can not be set together (this is fixed in v2), and since the period is being set first, new_limit = old_quota/new_period may be higher than the parent cgroup limit. The fix is to retry setting the period after the quota, to cover all possible scenarios. Tested via runc integration tests. Before the commit, it fails: root@ubu2004:~/git/runc# RUNC=`pwd`/../crun/crun.before bats -f "pod cgroup" -t tests/integration/update.bats 1..1 not ok 1 update cpu period in a pod cgroup with pod limit set # (in test file tests/integration/update.bats, line 424) # `[ "$status" -eq 0 ]' failed # crun.before spec (status=0): # # crun.before run -d --console-socket /tmp/bats-run-30428-dYkMDC/runc.4FdCtn/tty/sock test_update (status=0): # # crun.before update --cpu-quota 600000 test_update (status=1): # writing file `cpu.cfs_quota_us`: Invalid argument # crun.before update --cpu-period 10000 --cpu-quota 3000 test_update (status=1): # writing file `cpu.cfs_period_us`: Invalid argument With the fix, the test passes. Originally reported for runc in opencontainers/runc#3084 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1b71c08 to
6824924
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Sometimes setting CPU quota period fails when a new period is lower, and a parent cgroup has CPU quota limit set.
This happens as in cgroup v1 the quota and the period can not be set together (this is fixed in v2), and since the period is being set first, new_limit = old_quota/new_period may be higher than the parent cgroup limit.
The fix is to retry setting the period after the quota, to cover all possible scenarios.
Tested via runc integration tests. Before the commit, it fails:
With the fix, the test passes.
Originally reported for runc in
opencontainers/runc#3084