ConvertCPUSharesToCgroupV2Value: improve #20
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
This was referenced Jun 5, 2025
|
Thank you very much @kolyshkin! |
AkihiroSuda
approved these changes
Jun 11, 2025
thaJeztah
reviewed
Jun 11, 2025
utils_test.go
Outdated
| 262144: 10000, // Maximum. | ||
| 262145: 10000, // Above maximum (out of range). | ||
| } | ||
| for i, expected := range cases { |
There was a problem hiding this comment.
Not directly related to this PR, but we should use a different name for the i var to reduce cognitive load; it's very easy to read i as index value for a slice;
Suggested change
| for i, expected := range cases { | |
| for input, expected := range cases { |
There was a problem hiding this comment.
Done (as a separate patch).
utils.go
Outdated
Comment on lines
428
to
431
| if cpuShares < 2 { | ||
| return 1 | ||
| } | ||
| if cpuShares > 262144 { |
There was a problem hiding this comment.
Really nitty, but these could be <= and >= to avoid doing the calculation
There was a problem hiding this comment.
Done.
Although this also results in formula not being used for boundary values, so we're no longer testing it giving min and max as inputs. In order to test that we now need to add sharesMin+1 and sharesMax-1 values to the test.
Added as well.
| // Since the OCI spec is designed for cgroup v1, in some cases | ||
| // there is need to convert from the cgroup v1 configuration to cgroup v2 | ||
| // the formula for cpuShares is y = (1 + ((x - 2) * 9999) / 262142) | ||
| // convert from [2-262144] to [1-10000] |
There was a problem hiding this comment.
Perhaps including the v1 range ([2-262144]) and v2 range ([1-10000]) is worth including in the documentation.
utils.go
Outdated
Comment on lines
424
to
427
| // Cgroup v1 CPU shares has a range of [2^1...2^18], i.e. [2...262144], | ||
| // and the default value is 1024. | ||
| // Cgroup v2 CPU weight has a range of [10^0...10^4], i.e. [1...10000], | ||
| // and the default value is 100. |
There was a problem hiding this comment.
I guess (see my other comment) this could be part of the GoDoc?
The old formula, while correctly converting the cgroup v1 range to cgroup v2 range, had a few issues: 1. When cgroup v1 value is out of range, it returned invalid cgroup v2 value, leading to vague errors down the line (see [1]). 2. Default value cgroup v1 shares of 1024 is converted to 39, which is much less than the cgroup v2 default weight of 100 (see [2], [3]). The new conversion formula fixes both issues (see discussion in [2] for more details). Amend the test case accordingly. [1]: opencontainers/runc#4755 [2]: kubernetes/kubernetes#131216 [3]: opencontainers/runc#4772 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Rename some variables, and simplify the error message using the got/want pattern. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
@thaJeztah addressed all your comments, PTAL |
kolyshkin
added a commit
to kolyshkin/cri-o
that referenced
this pull request
Jun 20, 2025
It's a long story (see [1], [2], [3], [4]) but both runc and crun is changing the formula to convert cgroup v1 CPU shares to cgroup v2 CPU weight, and it causes a failure in "ctr update resources" test, because it relies on the old conversion formula. Let's modify it so it works either the old or the new conversion. (Ultimately, with cgroup v2 we should switch to setting unified.cpu.weight directly). [1]: kubernetes/kubernetes#131216 [2]: opencontainers/runc#4772 [3]: opencontainers/cgroups#20 [4]: containers/crun#1767 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
added a commit
to kolyshkin/cri-o
that referenced
this pull request
Jun 20, 2025
It's a long story (see [1], [2], [3], [4]) but both runc and crun is changing the formula to convert cgroup v1 CPU shares to cgroup v2 CPU weight, and it causes a failure in "ctr update resources" test, because it relies on the old conversion formula. Let's modify it so it works either the old or the new conversion. (Ultimately, with cgroup v2 we should switch to setting unified.cpu.weight directly). [1]: kubernetes/kubernetes#131216 [2]: opencontainers/runc#4772 [3]: opencontainers/cgroups#20 [4]: containers/crun#1767 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
added a commit
to kolyshkin/cri-o
that referenced
this pull request
Jun 20, 2025
It's a long story (see [1], [2], [3], [4]) but both runc and crun is changing the formula to convert cgroup v1 CPU shares to cgroup v2 CPU weight, and it causes a failure in "ctr update resources" test, because it relies on the old conversion formula. Let's modify it so it works either the old or the new conversion. (Ultimately, with cgroup v2 we should switch to setting unified.cpu.weight directly). [1]: kubernetes/kubernetes#131216 [2]: opencontainers/runc#4772 [3]: opencontainers/cgroups#20 [4]: containers/crun#1767 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
added a commit
to kolyshkin/cri-o
that referenced
this pull request
Jun 23, 2025
It's a long story (see [1], [2], [3], [4]) but both runc and crun is changing the formula to convert cgroup v1 CPU shares to cgroup v2 CPU weight, and it causes a failure in "ctr update resources" test, because it relies on the old conversion formula. Let's modify it so it works either the old or the new conversion. (Ultimately, with cgroup v2 we should switch to setting unified.cpu.weight directly). [1]: kubernetes/kubernetes#131216 [2]: opencontainers/runc#4772 [3]: opencontainers/cgroups#20 [4]: containers/crun#1767 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This was referenced Jul 7, 2025
klihub
pushed a commit
to klihub/cri-o
that referenced
this pull request
Jul 14, 2025
It's a long story (see [1], [2], [3], [4]) but both runc and crun is changing the formula to convert cgroup v1 CPU shares to cgroup v2 CPU weight, and it causes a failure in "ctr update resources" test, because it relies on the old conversion formula. Let's modify it so it works either the old or the new conversion. (Ultimately, with cgroup v2 we should switch to setting unified.cpu.weight directly). [1]: kubernetes/kubernetes#131216 [2]: opencontainers/runc#4772 [3]: opencontainers/cgroups#20 [4]: containers/crun#1767 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
klihub
pushed a commit
to klihub/cri-o
that referenced
this pull request
Jul 14, 2025
It's a long story (see [1], [2], [3], [4]) but both runc and crun is changing the formula to convert cgroup v1 CPU shares to cgroup v2 CPU weight, and it causes a failure in "ctr update resources" test, because it relies on the old conversion formula. Let's modify it so it works either the old or the new conversion. (Ultimately, with cgroup v2 we should switch to setting unified.cpu.weight directly). [1]: kubernetes/kubernetes#131216 [2]: opencontainers/runc#4772 [3]: opencontainers/cgroups#20 [4]: containers/crun#1767 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
klihub
pushed a commit
to klihub/cri-o
that referenced
this pull request
Jul 15, 2025
It's a long story (see [1], [2], [3], [4]) but both runc and crun is changing the formula to convert cgroup v1 CPU shares to cgroup v2 CPU weight, and it causes a failure in "ctr update resources" test, because it relies on the old conversion formula. Let's modify it so it works either the old or the new conversion. (Ultimately, with cgroup v2 we should switch to setting unified.cpu.weight directly). [1]: kubernetes/kubernetes#131216 [2]: opencontainers/runc#4772 [3]: opencontainers/cgroups#20 [4]: containers/crun#1767 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
klihub
pushed a commit
to klihub/cri-o
that referenced
this pull request
Jul 15, 2025
It's a long story (see [1], [2], [3], [4]) but both runc and crun is changing the formula to convert cgroup v1 CPU shares to cgroup v2 CPU weight, and it causes a failure in "ctr update resources" test, because it relies on the old conversion formula. Let's modify it so it works either the old or the new conversion. (Ultimately, with cgroup v2 we should switch to setting unified.cpu.weight directly). [1]: kubernetes/kubernetes#131216 [2]: opencontainers/runc#4772 [3]: opencontainers/cgroups#20 [4]: containers/crun#1767 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
klihub
pushed a commit
to klihub/cri-o
that referenced
this pull request
Jul 16, 2025
It's a long story (see [1], [2], [3], [4]) but both runc and crun is changing the formula to convert cgroup v1 CPU shares to cgroup v2 CPU weight, and it causes a failure in "ctr update resources" test, because it relies on the old conversion formula. Let's modify it so it works either the old or the new conversion. (Ultimately, with cgroup v2 we should switch to setting unified.cpu.weight directly). [1]: kubernetes/kubernetes#131216 [2]: opencontainers/runc#4772 [3]: opencontainers/cgroups#20 [4]: containers/crun#1767 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
klihub
pushed a commit
to klihub/cri-o
that referenced
this pull request
Jul 24, 2025
It's a long story (see [1], [2], [3], [4]) but both runc and crun is changing the formula to convert cgroup v1 CPU shares to cgroup v2 CPU weight, and it causes a failure in "ctr update resources" test, because it relies on the old conversion formula. Let's modify it so it works either the old or the new conversion. (Ultimately, with cgroup v2 we should switch to setting unified.cpu.weight directly). [1]: kubernetes/kubernetes#131216 [2]: opencontainers/runc#4772 [3]: opencontainers/cgroups#20 [4]: containers/crun#1767 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
The old formula, while correctly converting the cgroup v1 range to cgroup v2 range, had a few issues:
The new conversion formula fixes both issues (see discussion in 2 for more details).
Amend the test case accordingly.