cgroup: change conversion from CPU shares to weight #1767
Conversation
Reviewer's GuideThis PR replaces the linear CPU shares→cpu.weight mapping with a log₂-based quadratic formula, adds configure-time log2 detection, updates tests and documentation to match the new conversion, and adjusts build dependencies for Alpine. Class diagram for updated CPU shares to weight conversionclassDiagram
class CgroupInternal {
+uint64_t convert_shares_to_weight(uint64_t shares)
}
CgroupInternal : +convert_shares_to_weight(uint64_t shares)
CgroupInternal : <<static>>
%% The function now uses log2, pow, and ceil for the new formula
%% No new classes, but the method implementation is significantly changed
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
giuseppe
force-pushed
the
change-formula-cpu-shares-conversion
branch
from
53159c3 to
fceb48c
Compare
|
hmm wouldnt' this need to be opt-in? I think we'll need some way of coordinating this across the ecosystem before a runtime makes this decision |
I am not going to merge this yet, I've marked it as a Draft and queued it for "crun 2" with a bunch of other breaking changes: https://github.com/containers/crun/milestone/1 I don't see the risk of different runtimes/engines using a different formula. cpu shares/weight are relative to their peer cgroups. e.g. if you've two cgroups ( Also, the entire conversion from shares to weight is an undocumented hack. We should start using the native weight value using the |
giuseppe
force-pushed
the
change-formula-cpu-shares-conversion
branch
from
fceb48c to
9f5e272
Compare
giuseppe
changed the title
|
TMT tests failed. @containers/packit-build please check. |
giuseppe
force-pushed
the
change-formula-cpu-shares-conversion
branch
from
9f5e272 to
be13845
Compare
There was a problem hiding this comment.
Thank you very much @giuseppe! Overall looks great!
Left some small comments below :)
src/libcrun/cgroup-internal.h
Outdated
|
|
||
| #ifdef HAVE_LOG2 | ||
| /* Converts OCI shares (2-262144) to cgroup v2 cpu.weight (1-10000). | ||
| This uses the same formula as systemd, differing from the earlier linear conversion. |
There was a problem hiding this comment.
This uses the same formula as systemd
I think this is no longer correct
src/libcrun/cgroup-internal.h
Outdated
|
|
||
| return (uint64_t) ceil (pow (10, exponent)); | ||
| #else | ||
| /* Simplified version if the math library is not present. */ |
There was a problem hiding this comment.
In which cases do we expect this to happen?
Would it be better to simply fail in this case, to ensure we're not using a different formula silently?
Do you think there's a risk here?
There was a problem hiding this comment.
it didn't work on Alpine for example (with the default configuration), I've not really investigated it but I think it is valuable to have a fallback if -lm is not usable
There was a problem hiding this comment.
it didn't work on Alpine for example (with the default configuration), I've not really investigated it but I think it is valuable to have a fallback if -lm is not usable
I'm not sure if we should have a fallback -- it just adds another dimension with its own potential issues.
As to why it's not working for Alpine, I checked and it looks like in musl
- log2 is available (as it should, since it's part of C99 and POSIX.1-2001, both are quite old);
-lmis not needed.
Tested with alpine-3.22 image from docker hub.
The reason it is not working in CI is because of
crun/tests/alpine-build/run-tests.sh
Line 6 in d89542c
- autoconf explicitly adds bad log2 prototype, resulting in a compiler error:
configure:18660: checking whether log2 is declared
configure:18660: gcc -c -Werror conftest.c >&5
configure:18660: $? = 0
configure:18660: result: yes
configure:18670: checking for library containing log2
configure:18706: gcc -o conftest -Werror conftest.c >&5
conftest.c:66:6: error: conflicting types for built-in function 'log2'; expected 'double(double)' [-Werror=builtin-declaration-mismatch]
66 | char log2 (void);
| ^~~~
conftest.c:1:1: note: 'log2' is declared in header '<math.h>'
1 | /* confdefs.h */
cc1: all warnings being treated as errors
configure:18706: $? = 1
I tried to fix this but failed (I mean, other than removing -Werror from run-tests.sh
There was a problem hiding this comment.
My auto* knowledge is a tad rusty but I'll keep trying.
There was a problem hiding this comment.
thanks for looking into this.
I see log2 is a builtin, so we need to use TRY_LINK and provide a proper code snippet
tests/test_resources.py
Outdated
| @@ -291,13 +291,14 @@ def test_resources_cpu_weight_systemd(): | |||
| sys.stderr.write("found wrong CPUWeight for the systemd scope\n") | |||
| return 1 | |||
|
|
|||
| run_crun_command(['update', '--cpu-share', '4321', cid]) | |||
| cpu_shares = 4321 | |||
There was a problem hiding this comment.
Maybe it's better to also check some important values, e.g. min, max and default?
There was a problem hiding this comment.
the commit tests: improve cpu_weight_systemd coverage is testing these cases
giuseppe
force-pushed
the
change-formula-cpu-shares-conversion
branch
2 times, most recently
from
1b1168b to
02614df
Compare
@haircommander any thoughts? |
the KEP would have impact only the conversion performed by Kubernetes or the CRI runtime. Decisions in this project (and I'd assume runc too) are guided only by the OCI runtime-specs, not a KEP. |
giuseppe
force-pushed
the
change-formula-cpu-shares-conversion
branch
from
02614df to
566d368
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
TMT tests failed. @containers/packit-build please check. |
I was referring to whether the CRI would have a way to opt-into (or out of) this to ensure the new conversion takes place on all OCIs at the same k8s version. Obviously decisions in this repo aren't driven by Kubernetes KEPs, I'm just raising questions about their use-case from a user perspective. I wonder if for the k8s ecosystem (which is a significant user) treating the conversion as an implementation detail is a problem, and how important it is to have an opt-in/out mechanism. |
I think we are overthinking it. The current mechanism is quite broken but you are the first one who noticed and complained about it :-) Most people won't notice the difference, which is an improvement anyway. We need to move completely away from cgroup v1. A real improvement would be how to use |
+100! 👍 |
giuseppe
force-pushed
the
change-formula-cpu-shares-conversion
branch
from
566d368 to
2cebcff
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
giuseppe
force-pushed
the
change-formula-cpu-shares-conversion
branch
2 times, most recently
from
4d51e0a to
85db66e
Compare
|
TMT tests failed. @containers/packit-build please check. |
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>
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>
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>
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>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @giuseppe - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/libcrun/cgroup-internal.h:82` </location>
<code_context>
{
- /* convert linearly from 2-262144 to 1-10000. */
- return (1 + ((shares - 2) * 9999) / 262142);
+ double l, exponent;
+
+ /* The value of 0 means "unset". */
</code_context>
<issue_to_address>
Variable naming could be more descriptive for clarity.
Consider renaming 'l' to 'log_shares' and 'exponent' to 'weight_exponent' for better readability and maintainability.
Suggested implementation:
```c
double log_shares, weight_exponent;
```
```c
log_shares = log2 ((double) shares);
```
</issue_to_address>
### Comment 2
<location> `tests/test_resources.py:294` </location>
<code_context>
- run_crun_command(['update', '--cpu-share', '4321', cid])
- # this is the expected cpu weight after the conversion from the CPUShares
- expected_weight = "165"
+ for values in [(2, 1), (1024, 100), (262144, 10000)]:
+ cpu_shares = values[0]
+ # this is the expected cpu weight after the conversion from the CPUShares
</code_context>
<issue_to_address>
Test only covers three specific share-to-weight mappings; edge cases and intermediate values are not tested.
Please add tests for edge and intermediate values (e.g., 0, 1, 3, 262143, 512, 2048, 10000) to better validate the conversion logic and catch potential off-by-one or rounding errors.
</issue_to_address>
### Comment 3
<location> `tests/test_resources.py:299` </location>
<code_context>
+ expected_weight = str(values[1])
- out = run_crun_command(["exec", cid, "/init", "cat", "/sys/fs/cgroup/cpu.weight"])
- if expected_weight not in out:
- sys.stderr.write("# found wrong CPUWeight %s for the container cgroup\n" % out)
- return -1
</code_context>
<issue_to_address>
Test uses substring matching for weight validation, which may lead to false positives.
`if expected_weight not in out:` may yield false positives (e.g., '100' matches '1000'). Use exact matching or strip whitespace before comparison for accurate validation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -76,8 +79,22 @@ int libcrun_cgroup_pause_unpause_path (const char *cgroup_path, const bool pause | |||
| static inline uint64_t | |||
| convert_shares_to_weight (uint64_t shares) | |||
| { | |||
| /* convert linearly from 2-262144 to 1-10000. */ | |||
| return (1 + ((shares - 2) * 9999) / 262142); | |||
| double l, exponent; | |||
There was a problem hiding this comment.
suggestion: Variable naming could be more descriptive for clarity.
Consider renaming 'l' to 'log_shares' and 'exponent' to 'weight_exponent' for better readability and maintainability.
Suggested implementation:
double log_shares, weight_exponent; log_shares = log2 ((double) shares);| if expected_weight not in out: | ||
| sys.stderr.write("# found wrong CPUWeight %s for the container cgroup\n" % out) | ||
| return -1 | ||
| run_crun_command(['update', '--cpu-share', str(cpu_shares), cid]) |
There was a problem hiding this comment.
suggestion (testing): Test uses substring matching for weight validation, which may lead to false positives.
if expected_weight not in out: may yield false positives (e.g., '100' matches '1000'). Use exact matching or strip whitespace before comparison for accurate validation.
tests/test_resources.py
Outdated
| for values in [(2, 1), (1024, 100), (262144, 10000)]: | ||
| cpu_shares = values[0] | ||
| # this is the expected cpu weight after the conversion from the CPUShares | ||
| expected_weight = str(values[1]) | ||
|
|
||
| out = run_crun_command(["exec", cid, "/init", "cat", "/sys/fs/cgroup/cpu.weight"]) | ||
| if expected_weight not in out: | ||
| sys.stderr.write("# found wrong CPUWeight %s for the container cgroup\n" % out) | ||
| return -1 | ||
| run_crun_command(['update', '--cpu-share', str(cpu_shares), cid]) | ||
|
|
||
| out = subprocess.check_output(['systemctl', 'show','-PCPUWeight', scope ], close_fds=False).decode().strip() | ||
| # as above | ||
| if out != expected_weight: | ||
| out = subprocess.check_output(['systemctl', '--user', 'show','-PCPUWeight', scope ], close_fds=False).decode().strip() | ||
| out = run_crun_command(["exec", cid, "/init", "cat", "/sys/fs/cgroup/cpu.weight"]) | ||
| if expected_weight not in out: | ||
| sys.stderr.write("found wrong CPUWeight %s instead of %s for the container cgroup\n" % (out, expected_weight)) | ||
| return -1 | ||
|
|
||
| if out != expected_weight: | ||
| sys.stderr.write("# found wrong CPUWeight for the systemd scope\n") | ||
| return 1 | ||
| out = subprocess.check_output(['systemctl', 'show','-PCPUWeight', scope ], close_fds=False).decode().strip() | ||
| # as above | ||
| if out != expected_weight: | ||
| out = subprocess.check_output(['systemctl', '--user', 'show','-PCPUWeight', scope ], close_fds=False).decode().strip() | ||
|
|
||
| if out != expected_weight: | ||
| sys.stderr.write("found wrong CPUWeight for the systemd scope\n") | ||
| return 1 |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if expected_weight not in out: | ||
| sys.stderr.write("found wrong CPUWeight %s instead of %s for the container cgroup\n" % (out, expected_weight)) | ||
| return -1 |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if out != expected_weight: | ||
| out = subprocess.check_output(['systemctl', '--user', 'show','-PCPUWeight', scope ], close_fds=False).decode().strip() |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if out != expected_weight: | ||
| sys.stderr.write("found wrong CPUWeight for the systemd scope\n") | ||
| return 1 |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
giuseppe
force-pushed
the
change-formula-cpu-shares-conversion
branch
from
85db66e to
3cbdc1f
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The OCI CPU shares (range [2-262144]) to cgroup v2 `cpu.weight` (range [1-10000]) conversion formula has been updated to use a quadratic function so that min, max and default values match. Closes: containers#1721 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The `test_resources_cpu_weight_systemd` function previously tested the CPU shares update with a single value. This change expands the test to cover boundary values. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
change-formula-cpu-shares-conversion
branch
from
3cbdc1f to
72e5468
Compare
The OCI CPU shares (range [2-262144]) to cgroup v2
cpu.weight(range [1-10000]) conversion formula has been updatedCloses: #1721
Summary by Sourcery
Update the CPU shares to cgroup v2 cpu.weight conversion to use a log-quadratic function, ensure log2 availability at build time, and refresh tests and documentation to reflect the new mapping
Enhancements:
Build:
Documentation:
Tests:
Chores: