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

Handle kmem.limit_in_bytes removal #4018

Merged
merged 1 commit into from
Sep 25, 2023
Merged

Conversation

jrife
Copy link
Contributor

@jrife jrife commented Sep 16, 2023

kmem.limit_in_bytes has been removed in upstream linux and this patch is queued to be backported to linux 6.1 stable:

Without this change to libcontainerd, GetStats() will return an error on the latest kernel(s). A downstream effect is that Kubernetes's kubelet does not start up. This fix was tested by ensuring that it unblocks kubelet startup when running on the latest kernel.

@AkihiroSuda
Copy link
Member

This fix was tested by ensuring that it unblocks kubelet startup when running on the latest kernel.

On what distro?
Why is that distro still using cgroup v1 with the latest kernel?

@jrife
Copy link
Contributor Author

jrife commented Sep 17, 2023

This fix was tested by ensuring that it unblocks kubelet startup when running on the latest kernel.

On what distro? Why is that distro still using cgroup v1 with the latest kernel?

I was doing some testing with the latest kernel when I noticed this issue with cgroups v1. The kernel config may have not enabled v2. The base OS was Ubuntu 20.04.2 LTS.

@thaJeztah
Copy link
Member

somewhat orthogonal, but reminds me of this ticket I opened at the time;

@thaJeztah
Copy link
Member

Oh! And I just see the latest comments on that ticket that I somehow missed 🙈 (so ignore my earlier comment)

@kolyshkin
Copy link
Contributor

I need to do some experimenting first, but I think that if the kmem.limit_in_bytes is not set, then the kernel memory accounting is turned off, and thus kmem.{usage_in_bytes,max_usage_in_bytes,failcnt} all return 0 anyway.

Thus, it does not make sense to read those, and we can simply return a bunch of 0 in stats.MemoryStats.KernelUsage.

@jepio
Copy link

jepio commented Sep 20, 2023

This fix was tested by ensuring that it unblocks kubelet startup when running on the latest kernel.

On what distro? Why is that distro still using cgroup v1 with the latest kernel?

Flatcar Container Linux. There is still a large pool of users that want to continue running cgroup v1.

We continue supporting both cgroup v1 and cgroup v2. We are currently updating to kernel v6.1.54, and kubelet doesn't start anymore in the cgroup v1 configuration.

@jepio
Copy link

jepio commented Sep 20, 2023

I need to do some experimenting first, but I think that if the kmem.limit_in_bytes is not set, then the kernel memory accounting is turned off, and thus kmem.{usage_in_bytes,max_usage_in_bytes,failcnt} all return 0 anyway.

Thus, it does not make sense to read those, and we can simply return a bunch of 0 in stats.MemoryStats.KernelUsage.

That's not quite right:

core@localhost /sys/fs/cgroup/memory $ grep . memory.kmem.*
memory.kmem.failcnt:0
memory.kmem.max_usage_in_bytes:23629824
memory.kmem.tcp.failcnt:0
memory.kmem.tcp.limit_in_bytes:9223372036854771712
memory.kmem.tcp.max_usage_in_bytes:0
memory.kmem.tcp.usage_in_bytes:0
memory.kmem.usage_in_bytes:19554304

Kmem accounting is enabled, it's just not possible to limit it and the memory.kmem.limit_in_bytes file has been completely removed.

@kolyshkin kolyshkin added the backport/1.1-todo A PR in main branch which needs to be backported to release-1.1 label Sep 22, 2023
@lifubang
Copy link
Member

The CI error in ci / centos-stream-9 is not releated to your commit, these flaky tests have been fixed recently. Please rebase and sign off your commit.

@jrife
Copy link
Contributor Author

jrife commented Sep 24, 2023

The CI error in ci / centos-stream-9 is not releated to your commit, these flaky tests have been fixed recently. Please rebase and sign off your commit.

Done

@lifubang
Copy link
Member

LGTM, thanks, but please squash your commits.

kmem.limit_in_bytes has been removed in upstream linux and this patch
is queued to be backported to linux 6.1 stable:

- https://lore.kernel.org/linux-mm/[email protected]/T/
- https://www.spinics.net/lists/stable-commits/msg316619.html

Without this change to libcontainerd, GetStats() will return an error
on the latest kernel(s). A downstream effect is that Kubernetes's
kubelet does not start up. This fix was tested by ensuring that it
unblocks kubelet startup when running on the latest kernel.

Signed-off-by: Jordan Rife <[email protected]>
@jrife
Copy link
Contributor Author

jrife commented Sep 24, 2023

LGTM, thanks, but please squash your commits.

Done

@AkihiroSuda AkihiroSuda merged commit a32ad76 into opencontainers:main Sep 25, 2023
36 checks passed
@lifubang
Copy link
Member

@jrife Could you please open a PR to cherry-pick this commit to the branch 1.1?

@jrife
Copy link
Contributor Author

jrife commented Sep 26, 2023

@jrife Could you please open a PR to cherry-pick this commit to the branch 1.1?

Created #4028

@kolyshkin kolyshkin added backport/1.1-done A PR in main branch which has been backported to release-1.1 and removed backport/1.1-todo A PR in main branch which needs to be backported to release-1.1 labels Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cgroupv1 backport/1.1-done A PR in main branch which has been backported to release-1.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants