ConvertCPUSharesToCgroupV2Value: improve #20
Conversation
|
Thank you very much @kolyshkin! |
utils_test.go
Outdated
| 2: 1, // Minimum. | ||
| 1024: 100, // Default. | ||
| 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;
| 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
| 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
| // 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 <[email protected]>
Rename some variables, and simplify the error message using the got/want pattern. Signed-off-by: Kir Kolyshkin <[email protected]>
|
@thaJeztah addressed all your comments, PTAL |
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.