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
performance improvement for karmada-controller-manager to use DeferredDiscoveryRESTMapper
#2105
Comments
/assign |
Thanks for pointing it out. I guess you have evaluated them and I wonder why you finally chose |
There is a similar scene in kube-controller-manager's GC logic where By the way, there is a performance problem in the kube-controller-manager‘s GC controller and i have commit a pr to fix it. Refer to kubernetes/kubernetes#110888 |
I just looked at the implementation of DynamicRESTMapper, seems it will get the Can you help confirm that? By the way, how do you observe this issue? I don't know how to test it and evaluate the effect of the improvement made by #2106. |
The CPU time is mostly occupied by Anyway, do you have the profiling data after #2106? I wonder how much performance has improved by this PR. |
I closed the previous pr #2106 for this reason #2106 (comment). |
Can you explain why the first method works better than the current one? I mean by theoretically. |
benchmark:
BenchmarkGetGroupVersionResource1: use Via CPU Profiling, I see that there are some In this case we only write a few times and read more times from gvk to gvr cache, so cache the result directly and return fast will improve the read performance obviously。 |
You are using a |
Yes, put a real kubeconfig file in same folder |
Personality, I don't think a benchmark should rely on a real system. People may get different results when they run against different systems.
Thanks in advance, but we need to provide more evidence for that. Actually, I'm looking into it now. |
I investigate the implementation of DynamicRESTMapper, did find some performance issue: DynamicRESTMapper uses |
Wow, this is one of the main cost indeed.
I make a test with fake data without remote request. When there are only one api resource with one gvk exist, the first one even 10 times faster than the second one. fake data: groupResources := []*restmapper.APIGroupResources{
&restmapper.APIGroupResources{
Group: metav1.APIGroup{
Name: "apps",
Versions: []metav1.GroupVersionForDiscovery{
metav1.GroupVersionForDiscovery{
GroupVersion: "apps/v1",
Version: "v1",
},
},
PreferredVersion: metav1.GroupVersionForDiscovery{
GroupVersion: "apps/v1",
Version: "v1",
},
},
VersionedResources: map[string][]metav1.APIResource{
"v1":[]metav1.APIResource{
metav1.APIResource{
Name: "deployments",
Namespaced: true,
Kind: "Deployment",
ShortNames: []string{"deploy"},
Categories: []string{"all"},
StorageVersionHash: "8aSe+NMegvE=",
},
},
},
},
} For test, I just modified the code in vendor(here) and use the fake data above instead of the real data from remote. |
ok |
What would you like to be added:
use
DeferredDiscoveryRESTMapper
instead ofDynamicRESTMapper
to improve karmada-controller-manager performanceWhy is this needed:
karmada/pkg/detector/detector.go
Lines 772 to 773 in c1b5aef
karmada/pkg/detector/detector.go
Lines 591 to 593 in c1b5aef
DynamicRESTMapper
will visit cluster every time to get specific gvr by gvk. it's a waste of performance when there are many object but with less gvk.The text was updated successfully, but these errors were encountered: