-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
Now I need to update other tests :) most probably by fixing the drivers. |
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 <uaxb@hotmail.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
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 <akihiro.suda.cz@hco.ntt.co.jp> Co-authored-by: lifubang <lifubang@acmcoder.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
6a9411d
to
029a767
Compare
I can only say that 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 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 From my perspective, we should remove resource limits from state.json, and make Closing this for now |
Couldn't we just merge this as a workaround until the major refactoring? |
(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)