Feat: use memory limit from cgroup when set and lower

[louiznk]

Motivation

When you run kubelet in container if you set some memory limit cadvisor still uses system memory. But the memory is limit by the cgroup and the process may be killed if it uses more memory than the limit.

This is describe in issue #2698

The strategy for resolve this is:

• by default use the system memory limit
• if there is cgroup memory limit read it
  • compare the system memory limit and the cgroup memory limit, take the lower
• return the lower memory limit found

Tests

For testing the idea, I directly change the code of cAdvisor in K3S (only and only for test), and I make a container of this custom K3S.
This is available on docker hub and github.

Start k3s in docker

$ docker run --privileged --rm -d -p 6443:6443 -p 80:80 -p 443:443 --memory=2g --memory-swap=-1 louiznk/k3s:v1.19.2-poc-mem server
2063b98137b27da3350f6fb479cf2578807d54feba421d049c8e0dd560129a0d

✅ the container is started

Check the memory limit of the container

$ docker stats --no-stream                                                                                                                
CONTAINER ID        NAME                CPU %               MEM USAGE / LIMIT   MEM %               NET I/O             BLOCK I/O           PIDS
2063b98137b2        wonderful_snyder    10.72%              624MiB / 2GiB       30.47%              115MB / 515kB       369kB / 70.2MB      223

✅ the container use 2 GiB

Check the memory available for kubelet

Open a shell in the container

$ docker exec -it 2063b98137b27da3350f6fb479cf2578807d54feba421d049c8e0dd560129a0d sh
/ # grep MemTotal /proc/meminfo 
MemTotal:       32486464 kB

❗ the system memory is still 32 GiB

/ # cat /sys/fs/cgroup/memory/memory.limit_in_bytes
2147483648

✅ and the memory is still limit at 2 GiB

/ # kubectl get node -o=jsonpath="{.items[*]['status.capacity.memory']}"
2Gi

/ # kubectl top node
NAME           CPU(cores)   CPU%   MEMORY(bytes)   MEMORY%   
2063b98137b2   150m         1%     667Mi           32%

✅ 🎉 and for kubelet the memory available is 2 GiB, kubelet use the memory limit fix by the docker container 🎉

Usefull documentation

https://www.kernel.org/doc/Documentation/cgroup-v1/memory.txt
https://docs.docker.com/config/containers/resource_constraints/

[k8s-ci-robot]

Hi @louiznk. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[iwankgb]

/ok-to-test

[iwankgb]

@louiznk I don't think it's a good idea to modify GetMachineMemoryCapacity() to achieve goal that you stated. This function is responsible for determining memory capacity of the server. By doing what you suggest we could easily end up in following scenarion:

• a server with 64 gigabytes of memory is launhed
• kubelet is launched with cgroup-imposed limit of 512 megabytes
• cAdvisor reports that the server has 512 megabytes of memory available
• kubelet advertises this information to api-server
• no workload is scheduled to the cluster.

See:
https://github.com/kubernetes/kubernetes/blob/dd466bccde8176bd390fcf712c0752ae94444742/pkg/kubelet/nodestatus/setters.go#L275
If the pointer returned from the function stores wrong information on memory capacity, then kubelet will use the information eventually:
https://github.com/kubernetes/kubernetes/blob/dd466bccde8176bd390fcf712c0752ae94444742/pkg/kubelet/kubelet.go#L800

Let me know if I misunderstand your intention, but for the time being I think that the PR should be closed.

[louiznk]

@louiznk I don't think it's a good idea to modify GetMachineMemoryCapacity() to achieve goal that you stated. This function is responsible for determining memory capacity of the server. By doing what you suggest we could easily end up in following scenarion:

• a server with 64 gigabytes of memory is launhed
• kubelet is launched with cgroup-imposed limit of 512 megabytes
• cAdvisor reports that the server has 512 megabytes of memory available
• kubelet advertises this information to api-server
• no workload is scheduled to the cluster.

See:
https://github.com/kubernetes/kubernetes/blob/dd466bccde8176bd390fcf712c0752ae94444742/pkg/kubelet/nodestatus/setters.go#L275
If the pointer returned from the function stores wrong information on memory capacity, then kubelet will use the information eventually:
https://github.com/kubernetes/kubernetes/blob/dd466bccde8176bd390fcf712c0752ae94444742/pkg/kubelet/kubelet.go#L800

Let me know if I misunderstand your intention, but for the time being I think that the PR should be closed.

Hello @iwankgb
Thanks for your reply and explanation.

Say me if I well understood your point: If the kubelet is started with some cgroup isolation and with cgroup memory limitation, but not the rest of the node, then the memory sends to the api-server for this node is wrong.
When I propose this change I don't think to this point and effectively if it happens it will be bad for the node.

My intention is that a cluster running in docker (kind or k3d for example) knows the real memory available. If it doesn't, your nodes that run in some containers could be oom killed.

I don't know now how to deal with these both points, do you have an idea of how to achieve this? (even if it was not for production I think it will be great to run kubernetes in docker without risk of oom kill and it's will be a pleasure to help on this)

For my curiosity is there much kubernetes distribution that running kubelet in a container ?

[iwankgb]

@louiznk at the moment kubelet will send correct information. With your patch applied it would send wrong information and the node would effectively be unschedulable. AFAIK there were some attempts for running kubelet inside a container in the past but they were abandoned.

[iwankgb]

See: kubernetes/kubernetes#4869

[louiznk]

@louiznk at the moment kubelet will send correct information. With your patch applied it would send wrong information and the node would effectively be unschedulable. AFAIK there were some attempts for running kubelet inside a container in the past but they were abandoned.

@iwankgb perhaps if I create another function said GetCGroupMemoryCapacity with does this (and reset GetMachineMemoryCapacity) and use it in kubelet depending on the kube-config-file configuration it's will work without side effect.
It's mean that there is a change in cAdvisor and kubernetes. Do you think there is a chance that this will be accepted?

[BenTheElder]

AFAIK there were some attempts for running kubelet inside a container in the past but they were abandoned.

To be clear those efforts were running kubelet within a container relative to the host, as in alongside the workload containers. that was indeed abandoned, in part because this makes everything with the filesystem messy.

With kind, k3d, etc. the "host" (node as far as kubernetes is concerned) is fully "within" a single container as if it were a VM, and itself runs a "nested" container runtime. kubelet is "not containerized" relative to the pods.

It's not something to use in production, but we use it to test kubernetes itself cheaply, amongst other things.

EDIT: and kubernetes was already using KIND when that mode of operation was made unsupported and the flag deprecated, that's fine because that's not what we're doing. We do not use "containerized kubelet" mode. However everything is in a container.

[iwankgb]

@louiznk I don't think so, it looks hacky to me, especially as it is supposed to support development environments only. I would rather opt for mounting carefully crafted /proc/meminfo when necessary.

[louiznk]

AFAIK there were some attempts for running kubelet inside a container in the past but they were abandoned.

To be clear those efforts were running kubelet within a container relative to the host, as in alongside the workload containers. that was indeed abandoned, in part because this makes everything with the filesystem messy.

With kind, k3d, etc. the "host" (node as far as kubernetes is concerned) is fully "within" a single container as if it were a VM, and itself runs a "nested" container runtime. kubelet is "not containerized" relative to the pods.

It's not something to use in production, but we use it to test kubernetes itself cheaply, amongst other things.

EDIT: and kubernetes was already using KIND when that mode of operation was made unsupported and the flag deprecated, that's fine because that's not what we're doing. We do not use "containerized kubelet" mode. However everything is in a container.

Thanks @BenTheElder

Finally, it's a little confusing to me. As I understood, the case explains by @iwankgb will never happen?
So does this change is a real risk? It's allowing kind, k3d, and others to set and get the memory of a node (even if it's only for dev I like this feature)
It's allowing kind, k3d, and others to set and get the memory of a node

Thank you

[iwankgb]

@louiznk any kubelet running in a container (e.g. one that systemd created) could publish wrong information about resources available. It looks too risky to me.

[louiznk]

@louiznk any kubelet running in a container (e.g. one that systemd created) could publish wrong information about resources available. It looks too risky to me.

Ok, thanks

[louiznk]

@louiznk I don't think so, it looks hacky to me, especially as it is supposed to support development environments only. I would rather opt for mounting carefully crafted /proc/meminfo when necessary.

I will try if it was possible

[huoqifeng]

@louiznk I like the idea to get memory or cpu capacity against docker container so that we can make more use of kind and k3d etc. maybe, a possible approach is to detect container or VM that the kubelet is running against. for example,

• if against a VM, the cat /proc/1/cgroup will show like:

# cat /proc/1/cgroup
11:cpuset:/
6:memory:/

• if against a container, cat /proc/1/cgroup will show like:

# cat /proc/1/cgroup
11:cpuset:/docker/92573e1822d23fe03e73a0c16aee0bfa030b8e4b163a362386010967da23962e
6:memory:/docker/92573e1822d23fe03e73a0c16aee0bfa030b8e4b163a362386010967da23962e/docker/92573e1822d23fe03e73a0c16aee0bfa030b8e4b163a362386010967da23962e/init.scope

[louiznk]

@louiznk I like the idea to get memory or cpu capacity against docker container so that we can make more use of kind and k3d etc. maybe, a possible approach is to detect container or VM that the kubelet is running against. for example,

• if against a VM, the cat /proc/1/cgroup will show like:

# cat /proc/1/cgroup
11:cpuset:/
6:memory:/

• if against a container, cat /proc/1/cgroup will show like:

# cat /proc/1/cgroup
11:cpuset:/docker/92573e1822d23fe03e73a0c16aee0bfa030b8e4b163a362386010967da23962e
6:memory:/docker/92573e1822d23fe03e73a0c16aee0bfa030b8e4b163a362386010967da23962e/docker/92573e1822d23fe03e73a0c16aee0bfa030b8e4b163a362386010967da23962e/init.scope

As I understood the point of @iwankgb the risk is when you have a "classic" k8s with kubelet running in a container (even if this is not common usage) than you cluster will have the wrong node's capacities

[huoqifeng]

@louiznk @iwankgb in that case, could it be detected by checking whether there is sibling dockerd or containerd process?

[iwankgb]

@huoqifeng can you help me understand production use cases for such a scenario?

[huoqifeng]

@iwankgb we're seeking to use kind to deploy CI components in a single VM. It's better if we can restrict the resources in every Kubernetes node (which is container in kind) to avoid over commits. Hope it makes sense.

[iwankgb]

@huoqifeng, do you think this idea could solve your problem? #2699 (comment)

[huoqifeng]

AFAIK there were some attempts for running kubelet inside a container in the past but they were abandoned.

To be clear those efforts were running kubelet within a container relative to the host, as in alongside the workload containers. that was indeed abandoned, in part because this makes everything with the filesystem messy.

With kind, k3d, etc. the "host" (node as far as kubernetes is concerned) is fully "within" a single container as if it were a VM, and itself runs a "nested" container runtime. kubelet is "not containerized" relative to the pods.

It's not something to use in production, but we use it to test kubernetes itself cheaply, amongst other things.

EDIT: and kubernetes was already using KIND when that mode of operation was made unsupported and the flag deprecated, that's fine because that's not what we're doing. We do not use "containerized kubelet" mode. However everything is in a container.

@iwankgb as @BenTheElder mentioned, kind is leveraging container as a whole node but not run kubelet in a container. I think kind have widely usage or potential usage in CI/CD. I think a general and robust cadvisor approach for these cases is really better.

[BenTheElder]

@louiznk I don't think so, it looks hacky to me, especially as it is supposed to support development environments only. I would rather opt for mounting carefully crafted /proc/meminfo when necessary.

Isn't this file full of dynamic values? what would this look like?

EDIT: Within reason I'd be happy to implement faking this in kind and recommend users rely on that instead, but I'm somewhat skeptical of this approach.
We have other faked vfs entries as-is, but they're static info like system IDs

[BenTheElder]

@louiznk any kubelet running in a container (e.g. one that systemd created) could publish wrong information about resources available. It looks too risky to me.

Note that kubelet in a container (relative to the host it is managing) is not a supported configuration in Kubernetes. SIG Node chose to drop this.

With kind we're talking about (somewhat abusively perhaps) pretending a container is the host, with everything about kubernetes "inside" it. Just containerizing your kubelet is not supported upstream as-is. EDIT: and therefore risk to just kubelet in a container seems not to be a big concern.

[I still don't know what the correct answer is here, just attempting to clarify this aspect.]

[louiznk]

Isn't this file full of dynamic values? what would this look like?

Yes, this is dynamic values. I have tried to mount a fake /proc/meminfo and it's work, the difficulty is if you need to update this file

[iwankgb]

@louiznk @BenTheElder do you think that usign --system-reserved flag to limit amount of allocatable memory would be a good enough workaround?

[louiznk]

@louiznk @BenTheElder do you think that usign --system-reserved flag to limit amount of allocatable memory would be a good enough workaround?

Hello @iwankgb
I'm sorry I'm not sure to understood. Do you propose to use this flag for "explain" to cAdvisor to read the /sys/fs/cgroup/memory/memory.limit_in_bytes file (cgroups memory limit) instead of reading the system memory limit in the /proc/meminfo file ?
Thanks

[iwankgb]

I think you can use it to affect amount of memory that is available to pods. As far as I understand, it should help you solve your problem.

[louiznk]

I think you can use it to affect amount of memory that is available to pods. As far as I understand, it should help you solve your problem.

Sorry, I just understood that you speak about the kubelet option. This is an interesting workaround and I think it's would work. I don't find this very elegant because this option was planned for preserving resource for the operating system, not resizing the resource available for kube, but that my point of view

[iwankgb]

@louiznk can this PR and #2698 be closed?

[BenTheElder]

Could we instead have a more direct mechanism to instruct cadvisor what we'd like the resource limits reported as? (So e.g. monitoring tools behave as expected).

I think it would be more generally useful to be able to bypass the detection mechanisms and provide the "real limits". E.g. imagine some bug is encountered, today you have to get all the binaries patched or do something very hacky because there's no override mechanism.

[iwankgb]

I still don't think it is a good idea. The only use case that we have is one related to testing and I don't believe that introducing code aimed at testing project X to project Y is beneficial to project Y as it increases project's complexity and affects, badly, maintainability.
If there is a production use case of such feature then it should be reconsidered. At the moment workaround on side of the project X sounds like reasonable idea to me.

[louiznk]

@louiznk can this PR and #2698 be closed?

Hello, I think this PR could be close if you think this wasn't the good implementation, but I think the issue is still here for projects like kind and k3d, but that a point of view and not a trust.
@BenTheElder do you think #2698 is a real issue for kind ?

[BenTheElder]

The production usecase is bypassing broken detection on the host which I have experienced before. (E.g. the hyperthread snafu).
Edit: and the project being tested here is Kubernetes, which is sort of the primary cadvisor useage, no?

[iwankgb]

@BenTheElder ultimately kubelet would have to inform cAdvisor that it should return memory capacity as specified in cgroup rather than /proc. Information would have to follow this path:

• https://github.com/kubernetes/kubernetes/blob/3321f00ed14e07f774b84d3198ede545c1dee697/pkg/kubelet/kubelet.go#L550.
• https://github.com/kubernetes/kubernetes/blob/3321f00ed14e07f774b84d3198ede545c1dee697/pkg/kubelet/cadvisor/cadvisor_linux.go#L164.

• cadvisor/manager/manager.go

  Line 212 in ac63c56

  newManager.machineInfo = *machineInfo

  
  and

  cadvisor/manager/manager.go

  Line 365 in ac63c56

  m.machineInfo = *info

It would require us to:

• add a flag to kubelet.
• change signature of manager.New() (it looks like backward incompatible change to me).

Instead of it we could alter behavior of kubelet (in https://github.com/kubernetes/kubernetes/blob/3321f00ed14e07f774b84d3198ede545c1dee697/pkg/kubelet/kubelet.go#L550) and force it to cache altered MachineInfo. We still need kubelet flag but we do not need to pass any information to cAdvisor code.

Kubernetes is a large project and important user of cAdvisor, but I still don't think that leaking kubelet testing logic to Cadvisor is a good idea. We should keep in mind that there are companies and people who use cAdvisor to monitor their infrastructure. For them cAdvisor reporting its own memory limit as node memory capacity sounds like a nightmare, I think.

By the way, can you elaborate more on hyperthread snafu? Sounds interesting.

[iwankgb]

Regarding mounting fake /proc/meminfo - yes, values reported their are dynamic indeed but as far as I understand majority of them is irrelevant to this use case. What matters to us are top 3 lines, at most:

➜  ~ cat /proc/meminfo
MemTotal:        1020368 kB
MemFree:           73192 kB
MemAvailable:      63776 kB
Buffers:            9592 kB
Cached:           113976 kB
SwapCached:            0 kB
Active:           827348 kB
Inactive:          54224 kB
Active(anon):     758432 kB
Inactive(anon):    12020 kB
Active(file):      68916 kB
Inactive(file):    42204 kB
Unevictable:           0 kB
Mlocked:               0 kB
SwapTotal:             0 kB
SwapFree:              0 kB
Dirty:                44 kB
Writeback:             0 kB
AnonPages:        758044 kB
Mapped:            31648 kB
Shmem:             12448 kB
Slab:              35808 kB
SReclaimable:      12856 kB
SUnreclaim:        22952 kB
KernelStack:        2492 kB
PageTables:         6576 kB
NFS_Unstable:          0 kB
Bounce:                0 kB
WritebackTmp:          0 kB
CommitLimit:      510184 kB
Committed_AS:    1504032 kB
VmallocTotal:   34359738367 kB
VmallocUsed:           0 kB
VmallocChunk:          0 kB
HardwareCorrupted:     0 kB
AnonHugePages:         0 kB
ShmemHugePages:        0 kB
ShmemPmdMapped:        0 kB
HugePages_Total:       0
HugePages_Free:        0
HugePages_Rsvd:        0
HugePages_Surp:        0
Hugepagesize:       2048 kB
DirectMap4k:      204772 kB
DirectMap2M:      843776 kB
DirectMap1G:           0 kB

[BenTheElder]

@BenTheElder ultimately kubelet would have to inform cAdvisor that it should return memory capacity as specified in cgroup rather than /proc. Information would have to follow this path:

No that's not what I'm asking for. I'm asking to be able to tell cadvisor directly "this is how much memory, CPU, etc. the system has" via config or similar, which would then be helpful whenever trying to work around bugs in production.

Right now if I encounter a bug like the disabled hyperthread CPU count issue previously, I have to get cadvisor patched THEN kubernetes patched and then upgrade kubernetes. If I could instead tell cadvisor "no actually this is what the system resource limits are" this might also be useful in prod, not just to artificially limit them.

We could change something in kubernetes for this similar to system reserved but then the metrics will be incorrect.
The system reserved approach is not good for that use case, because then I need to predict how much the values are incorrect by instead of predicting the correct values and supplying them myself.

#2579, you actually sent the final patch :-)

At one point, cadvisor incorrectly reported offline CPUs for the CPU resource limit (and also didn't work on arm platforms), forcing people to upgrade around it. It took a while to get a patch in. I would have instead specified the correct CPU count externally if that was possible.

---

Regarding mounting fake /proc/meminfo - yes, values reported their are dynamic indeed but as far as I understand majority of them is irrelevant to this use case. What matters to us are top 3 lines, at most:

Sure, but what do we break if all these other values become fixed and inaccurate ..?

[iwankgb]

OK, I understand your reasoning and what you want to achieve but what you are describing is using another tool (a shell script, perhaps) to obtain topology/memory information and feed it to cAdvisor. It looks as if cAdvisor was not needed in the first place. Regarding arm - as far as I understand the architecture has never been supported in cAdvisor and there are at least two open PRs (#2751, #2744) that can help at lest a bit.

@bobbypage, @dashpole - could you weigh in? I think that maintainer decision is needed here.

[dashpole]

I don't think we should use cgroup values as part of MachineInfo. We have APIs for host information, and others for cgroups, and the data they contain is quite different.

If the kubelet wants, it is free to use the values from its own cgroup instead of the values from the host. It already knows which cgroup it is in, so it wouldn't be too hard to implement. If you want to pursue that, I would discuss the idea at Sig-Node, and (if they like the idea) would probably write a KEP...

The idea of overriding host capacity has been brought up before, but i'm afraid of it being abused. IIRC, the use-case last time was to overcommit pods on the node (e.g. if they request 1 core, give them 0.7 cores instead).

[BenTheElder]

The idea of overriding host capacity has been brought up before, but i'm afraid of it being abused. IIRC, the use-case last time was to overcommit pods on the node (e.g. if they request 1 core, give them 0.7 cores instead).

I mean if users really want to abuse this they already can via the VFS route, isn't that their choice?

[BenTheElder]

It looks as if cAdvisor was not needed in the first place.

As a kubelet user I don't get to choose if cAdvisor is used ...?
But yes, I don't think cAdvisor is necessary to get topology / local resource limits, the provisioner for most systems should know these intimately (e.g. in cloud something like GKE will know how big the machines it creates are).

Regarding arm - as far as I understand the architecture has never been supported in cAdvisor and there are at least two open PRs (#2751, #2744) that can help at lest a bit.

It's supported in Kubernetes ... but my point is not specific to arm, only that in those cases in order to fix their usage they must also upgrade to a new Kubernetes version currently because you must use what cAdvisor reports and no layer of the stack provides a way to correct this detected data, even though it would have been trivial for these users to supply the correct topology information.

[iwankgb]

@BenTheElder how about applying the mechanism that you have just described in kubelet?

[BenTheElder]

@BenTheElder how about applying the mechanism that you have just described in kubelet?

That may be a potential avenue, Thought I'm concerned about mismatch from the reported stats, which are again exposed through kubernetes to users direct from cAdvisor IIRC.

In either case I think this PR should probably have long been closed, it seems this approach is not accepted and there's ongoing discussion in slack instead of here (and a PR is likely not the right place to discuss approaches to begin with).

[louiznk]

In either case I think this PR should probably have long been closed, it seems this approach is not accepted and there's ongoing discussion in slack instead of here (and a PR is likely not the right place to discuss approaches to begin with).

I close it