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

skip set cpu.weight when cpu.idle is enabled in cgroups v2 #4143

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

qdaxb
Copy link

@qdaxb qdaxb commented Dec 12, 2023

In cgroups v2, setting cpu.weight when cpu.idle==1 will fail with Invalid argument

This pr fixed this situation, consistent with https://github.com/opencontainers/runc/blob/main/libcontainer/cgroups/systemd/v2.go#L119

@qdaxb qdaxb force-pushed the main branch 2 times, most recently from 9079199 to d255be9 Compare December 12, 2023 16:48
@mrunalp
Copy link
Contributor

mrunalp commented Dec 12, 2023

Instead of silently skipping we should fail as a validation error.

@qdaxb
Copy link
Author

qdaxb commented Dec 13, 2023

Instead of silently skipping we should fail as a validation error.

@mrunalp Thanks for reply.

I'm still a little confused about the idle parameter.

There is no description in the spec that shares and idle cannot be applied at the same time. But If I set both shares and idle, systemd driver and fs2 driver will behave differently.

system\driver systemd/v2 fs2/cpu
support idle & systemd < 252 systemd:set cpu.weight=1
fsMgr:set cpu.idle=1 & return error
set cpu.idle=1 & return error
support idle & systemd > 252 systemd:set cpu.idle=1
fsMgr:set cpu.idle=1 & return error
set cpu.idle=1 & return error
not support idle return error return error

So I think fs2 should be compatible with this situation like systemd, or directly add a hint in the document that they cannot be used at the same time?

@AkihiroSuda
Copy link
Member

Instead of silently skipping we should fail as a validation error.

Should be a warning to avoid breaking compatibility?

@kolyshkin
Copy link
Contributor

Yes, this should probably be a warning. I'm going to carry this into #4227.

@kolyshkin
Copy link
Contributor

There's no need for a warning. The problem is, Set is called during runc update with a combination of old and new settings, and thus it's normal for cpuweight and cpuidle both being set.

This PR needs a test case though. Can you add one @qdaxb ?

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.

4 participants