-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
docker
says You should always set the Memory limit when using Memoryswap limit, see usage.
#10935
Comments
I think we can get in this state, when it says we have support for memory-swap but not for memory memcgSwap := hasMemorySwapCgroup()
memcg := HasMemoryCgroup() It's a weird state, so we should look for it. And avoid using swap if so, but it's probably a cgroups v2 issue. if memcg {
runArgs = append(runArgs, fmt.Sprintf("--memory=%s", p.Memory))
}
if memcgSwap {
// Disable swap by setting the value to match
runArgs = append(runArgs, fmt.Sprintf("--memory-swap=%s", p.Memory))
} Apparently the bug was introduced in 7b0bf57
|
yeah, there is |
What cgroupfs mounts do you have, currently ? The regular (v1) setup looks like:
A pure cgroups v2 would be cleaner. https://www.kernel.org/doc/Documentation/cgroup-v2.txt
|
cgroup-v1 is not mounted. Looks like |
Yeah, it looks like the cgroups checks are broken. And memcgSwap should depend on memcg, like Docker says. Basically the only useful thing that this code does is to mimic the output from Docker, when running on Ubuntu kernel:
|
Compare https://github.com/moby/moby/tree/master/pkg/sysinfo v1// applyMemoryCgroupInfo adds the memory cgroup controller information to the info.
func applyMemoryCgroupInfo(info *SysInfo, cgMounts map[string]string) []string {
var warnings []string
mountPoint, ok := cgMounts["memory"]
if !ok {
warnings = append(warnings, "Your kernel does not support cgroup memory limit")
return warnings
}
info.MemoryLimit = ok
info.SwapLimit = cgroupEnabled(mountPoint, "memory.memsw.limit_in_bytes")
if !info.SwapLimit {
warnings = append(warnings, "Your kernel does not support swap memory limit")
} v2func applyMemoryCgroupInfoV2(info *SysInfo, controllers map[string]struct{}, _ string) []string {
var warnings []string
if _, ok := controllers["memory"]; !ok {
warnings = append(warnings, "Unable to find memory controller")
return warnings
}
info.MemoryLimit = true
info.SwapLimit = true |
So something like #10936 should be enough or more ththoughtful rework is needed? |
Probably needs the same for docker (not only podman), and not sure we need to check any files at all for v2 ? But something like that... Would be nice to have some tests for cgroups v2 during the minikube regression testing. |
memcg could be compiled out in kernel and cgroupv2 will be mounted without memorycg then. Unfortunately I don't have time right now to do a proper PR with tests and stuff, so either I can satisfy bots to get it into mergeable state as is (possibly with minor changes based on review comments) or I can drop it and wait for the proper fix, please advise what should be done here. |
There is no rush, this only affects v2. We can do a proper fix, and see what went wrong in #10371 |
@ojab I believe we fixed this in v1.19.0 do u mind trying the latest release and see if u still have this issue? if yes please reopen this |
Steps to reproduce the issue:
minikube start --driver=docker
Full output of failed command:
(without
--alsologtostderr
because it's 76KB of likely useless data)docker docs says that
--memory
also should be passed if we pass--memory-swap
.The text was updated successfully, but these errors were encountered: