Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runc update: fix logic by distinguishing nil from zero #4227

Closed
wants to merge 2 commits into from

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Mar 19, 2024

(1) Prior to this commit, commands like runc update --cpuset-cpus=1 were implying to set cpu burst to "0" (which does not mean "leave it as is"). This was failing when the kernel does not support cpu burst: openat2 /sys/fs/cgroup/runc-cgroups-integration-test/test-cgroup-22167/cpu.max.burst: no such file or directory

(the above is extracted from #4142 by @AkihiroSuda and further simplified)

(2) Also, solve the issue of re-setting cpu-burst to 0 when other
parameters are being set.

(3) This also solves the issue of re-setting cpu-period to default value when cpu-quota was not set (#4225).

Test cases for (2) and (3) are added (mostly taken from #4226 by @lifubang)

@kolyshkin
Copy link
Contributor Author

Now I need to update other tests :) most probably by fixing the drivers.

@kolyshkin kolyshkin marked this pull request as draft March 19, 2024 01:22
In case cpu.idle=1, cpu.weight can't be set (write to cpu.weights fails
with EINVAL). The systemd v2 cgroup driver already handles this case,
but the fs2 driver did not, resulting in an error when both cpu.idle
and cpu.weight is set.

Fix by not setting cpu.weight (with a warning).

Reported-by: axb <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]>
Prior to this commit, commands like `runc update --cpuset-cpus=1`
were implying to set cpu burst to "0" (which does not mean "leave it as is").
This was failing when the kernel does not support cpu burst:
`openat2 /sys/fs/cgroup/runc-cgroups-integration-test/test-cgroup-22167/cpu.max.burst: no such file or directory`

Also, solve the issue of re-setting cpu-burst to 0 when other
parameters are being set.

Also, solve the issue of re-setting cpu-period to default value when
cpu-quota was not set.

Add test cases for known issues.

Co-authored-by: Akihiro Suda <[email protected]>
Co-authored-by: lifubang <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Contributor Author

I can only say that runc update is a mess and needs to be redone. In particular, it reads all resource values from state.json and then writes all the resources (and not just the ones specified), when writes back the updated state.json.

Problem 1 is performance. Say we just want to change one single parameter -- instead we're writing (to cgroupfs and systemd) all of them, with all but one being updated to the very same value. We are reading and writing some json files, etc.

Problem 2 is how to merge the resource config from state.json with the new ones from runc update. This is problematic when resources are specified via the unified map (https://github.com/opencontainers/runtime-spec/blob/main/config-linux.md#unified).

Problem 3 is parameters that are inter-dependent, like cpu shares (called cpu weight in v2) and cpu idle. Consider the scenario when we have a running container, and want to enable CPU idle scheduling (as in runc update --cpu-idle 1). Naturally, this also makes runc update all the other resource parameters, which may include previously set cpu shares (cpu weight). This won't work (can't assign cpu shares/weight when idle is set), so runc update fails. To work around that, we need to add a kludge to zero out cpu shares/weight, when cpu.idle=1 is set. It's getting even more complicated with the unified resource support.

From my perspective, we should remove resource limits from state.json, and make runc update only update those resources that are explicitly specified. If there are some interdependent resources (say, cpu quota and period), cgroup drivers should read those from cgroupfs and/or systemd.

Closing this for now

@kolyshkin kolyshkin closed this Mar 27, 2024
@AkihiroSuda
Copy link
Member

Couldn't we just merge this as a workaround until the major refactoring?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants