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

build cached restmapper based on Kubernetes restmapper #3819

Merged

Conversation

RainbowMango
Copy link
Member

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

The reason why we introduced the cachedRESTMapper is to improve performance, especially when retrieve the GVR as per a GVR, such as here.

We built the cachedRESTMapper based on controller-runtime DynamicRESTMapper, but there is a breaking change happened in v0.15.0, and we can't build the cachedRESTMapper with it. This issue blocks the updation of Kubernetes dependencies.

This PR tries to build the cachedRESTMapper with Kubernetes RESTMapper. This PR is a kind of predecessor task of #3730.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Before this change, the benchmark result:

# go test -bench=.
goos: linux
goarch: amd64
pkg: github.com/karmada-io/karmada/pkg/util/restmapper
cpu: Intel(R) Xeon(R) Gold 6278C CPU @ 2.60GHz
BenchmarkGetGroupVersionResource-4            	   62730	     17698 ns/op
BenchmarkGetGroupVersionResourceWithCache-4   	  924314	      1332 ns/op
PASS
ok  	github.com/karmada-io/karmada/pkg/util/restmapper	2.569s
-bash-5.0# pwd
/root/go/src/github.com/karmada-io/karmada/pkg/util/restmapper

The result shows that cachedRESTMapper has a significant performance advantage.

With this change:

# go test -bench=.
goos: linux
goarch: amd64
pkg: github.com/karmada-io/karmada/pkg/util/restmapper
cpu: Intel(R) Xeon(R) Gold 6278C CPU @ 2.60GHz
BenchmarkGetGroupVersionResource-4               	   59592	     20855 ns/op
BenchmarkGetGroupVersionResourceWithoutCache-4   	   67836	     18764 ns/op
BenchmarkGetGroupVersionResourceWithCache-4      	   41851	     30970 ns/op           // Seems weak, but it's not

The result seems to show that the performance after refactoring is weaker, but in fact, it's not. The performance of cachedRESTMapper appears lower because there is a non-existent resource in the test suites, that cause cachedRESTMapper rebuild the underlying mapper. As a comparison, controller-runtime's restmapper doesn't rebuild so many times because it is limited by the rate limiter.

In Karmada, it is rare to query non-exist resources(unless a new CRD is created). After removing the non-exist resource test from the test suite, the result as follows:

# go test -bench=.
goos: linux
goarch: amd64
pkg: github.com/karmada-io/karmada/pkg/util/restmapper
cpu: Intel(R) Xeon(R) Gold 6278C CPU @ 2.60GHz
BenchmarkGetGroupVersionResource-4               	   72998	     16917 ns/op
BenchmarkGetGroupVersionResourceWithoutCache-4   	   64602	     16650 ns/op
BenchmarkGetGroupVersionResourceWithCache-4      	  823752	      1424 ns/op   

The performance advantage is still significant.

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: RainbowMango <qdurenhongcai@gmail.com>
@karmada-bot karmada-bot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jul 20, 2023
@karmada-bot karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 20, 2023
g.mu.Lock()
g.restMapper = newMapper
g.mu.Unlock()
g.gvkToGVR.Range(func(key, value any) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question.Why not directly use g.gvkToGVR.Delete(key)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here is going to clear the whole cache, not just remove a specific key.
For example, after removing a CRD, the key may remain in the cache(gvkToGVR), so the idea is to clear the cache and then rebuild the cache with the latest loaded resources.

@RainbowMango
Copy link
Member Author

cc @Poor12 @likakuli for comments.

@RainbowMango RainbowMango added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Jul 23, 2023
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@karmada-bot karmada-bot merged commit 822caab into karmada-io:master Jul 23, 2023
@RainbowMango RainbowMango deleted the pr_refactor_cached_restmapper branch December 28, 2023 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants