-
Notifications
You must be signed in to change notification settings - Fork 39.2k
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
client-go memory leak #102718
Comments
/sig api-machinery |
/help |
@fedebongio: Please ensure the request meets the requirements listed here. If this request no longer meets these requirements, the label can be removed In response to this:
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. |
I format code used for reproduction package main
import (
"fmt"
goruntime "runtime"
"time"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
)
func main() {
leak()
}
func unleak() {
store := make(map[string]*v1.Pod)
var index int
for {
plist := generatePod(index)
for _, pod := range plist.Items {
store[pod.Name] = &pod
}
time.Sleep(time.Second * 2)
index++
goruntime.GC()
fmt.Println("==unleak==", index, len(store))
}
}
func leak() {
store := make(map[string]*v1.Pod)
var index int
var items []runtime.Object
for {
items = items[:0]
plist := generatePod(index)
meta.EachListItem(plist, func(obj runtime.Object) error {
items = append(items, obj)
return nil
})
for _, item := range items {
pod := item.(*v1.Pod)
store[pod.Name] = pod
}
time.Sleep(time.Second * 2)
index++
goruntime.GC()
fmt.Println("==leak==", index, len(store))
}
}
func generatePod(num int) *v1.PodList {
var plist v1.PodList
for i := 0; i < 100000-num; i++ {
pod := v1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("pod-%d", i),
},
}
plist.Items = append(plist.Items, pod)
}
return &plist
} |
Re code above - the leak is not coming from the ForEach, but the fact that you're not really releasing all the references here:
Going back to the original comment, I don't think we have a problem with ForEach function:
you should adjust watchcache size - 100 items in 5k-node cluster is certainly enough. There was a function that adjusts it on number of nodes, but it was based on some flag (apiserver-ram-mb or sth like that), which I bet you don't set.
I don't understand this comment. If you're storing the data in cache, then it's expected that those will not be GC-ed. That's the whole point in storing them in cache, right? |
The leak here is caused by the golang slice gc mechanism, and there is no memory leak in the func unleak() {
store := make(map[string]*v1.Pod)
var index int
for {
plist := generatePod(index)
for i := 0; i < len(plist.Items); i++ {
store[plist.Items[i].GetName()] = &plist.Items[i]
//store[pod.Name] = &pod
}
time.Sleep(time.Second * 2)
index++
goruntime.GC()
fmt.Println("==unleak==", index, len(store))
}
} @wojtek-t PTAL |
@sxllwx is right, I add a new test case, and the
|
There is already a PR that is working on a similar issue. The link for this PR discussion |
OK - that makes sense to me. Sorry for confusion, I was looking to much at #102565 (comment) and I still think there is no bug in that PR, because we're not taking an address of slice item there, right? The runtime.Object are already pointers, right? That said, the extractList that is happening before has this problem:
|
After testing, only in two cases will affect the work of GC
The two functions with this kind of operation are
What we need to note is that the corev1.Pod DeepCopyObject method requires a Pointer Receiver. But in the PodList obtained from kube-apiserver, the Items field is a non-pointer type Pod-struct kubernetes/staging/src/k8s.io/api/core/v1/types.go Lines 3721 to 3731 in 0bc75af
So
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/assign |
/remove-lifecycle stale |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closing this issue. In response to this:
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. |
What happened:
We use
client-go informerFactory
to handle some pod information,But we came across memory leak problem recently.
I add some log, and finally find the reason.
Our k8s version is 1.17 with more than 5500 node, but the default-watch-cache-size of
kube-apiserver
is 100 which is too small for our cluster.(The last k8s already use
dynamic size watch-cache
##90058)Reflector.ListAndWatch
Reflector List
function costs more than 35 second, and thenReflector
use the last resourceVersion ofList
to callWatch
function.Reflector Watch
get an errortoo old resource version: 6214379869 (6214383056)
because the last resourceVersion ofList
has been expired in default-watch-cache-size ofkube-apiserver
Reflect.Run
will keep the cycleList
->Watch
-get too old resource version error
Then a memory leak occurred. We use
cache
map to store pod information, and thecache
will contain pods in differentPodList
. This will prevent golang from gc the wholePodList
The code causing memory leak is
meta.EachListItem
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/api/meta/help.go#L115
Replacing
found = append(found, item)
withfound = append(found, item.DeepCopyObject())
can fix the problem.https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/cache/reflector.go#L453
What you expected to happen:
Calling
DeepCopyObject
has some memory overhead,I wonder if there is a better solution for this.
How to reproduce it (as minimally and precisely as possible):
Anything else we need to know?:
Environment:
kubectl version
):cat /etc/os-release
):uname -a
):The text was updated successfully, but these errors were encountered: