Fixing a kubelet memory leak in Kubernetes 1.36(heyoncall.com) |
Fixing a kubelet memory leak in Kubernetes 1.36(heyoncall.com) |
Can't help but feel this is one of the subtle traps hidden beneath the advice that contexts aren't supposed to be stored. I know it's not always that easy, of course.
lostcancel: check cancel func returned by context.WithCancel is called
This is a serious tripping point with Go. There's no way to express: "this is a root context that I _want_ to store and only use to create derived contexts". Goroutines are also a source of problems, you can't easily say "I'm passing the ownership of this context to a goroutine".
The default kubelet `syncFrequency` is 1 minute (per Pod). (There can be additional event-driven ones, but this is a floor.) Back of the envelope: 30 Pods * 1440 * 7 = 300k calls to startPodSync in a week.
I’d guess a lot of production clusters don’t run the latest release of Kubernetes, and 1.36 was only released in late April, less than two months before I reported the issue.
Given that the issue shows up as linear memory growth on the order of maybe 1 GiB/month per node, there really is a time-based discovery process here. (Could be artificially accelerated for a CI test, I imagine!) And any restarted nodes or clusters that upgraded point releases would restart that clock.
The only unusual contributing cause that made the issue more visible to me was trying to run everything on a tiny 2 GiB RAM node!
# cat /proc/$(pgrep kubelet)/oom_score_adj
-999
(from OOMScoreAdjust=-999 in /etc/systemd/system/kubelet.service)
I took a quick look at "containedctx" and it seems like for this case, it would almost be backwards: it would flag the (not-memory-leaking) struct-stored "status.ctx", but wouldn't flag when there is a stored "status.cancelFn" only (which resulted in the memory leak).
Could Go implement a runtime leaked-context detector, like the data race detector? https://go.dev/doc/articles/race_detector
In this case, the cancel() function was preventing the collection. But I think it can be changed to hold a weak reference instead. The overhead is too large to run it normally, but it should be OK for something like the race detector.