karmada-io / karmada

Open, Multi-Cloud, Multi-Cluster Kubernetes Orchestration
https://karmada.io
Apache License 2.0
4.14k stars 811 forks source link

proxy list with rv=0 no longer access member cluster etcd #4896

Open ikaven1024 opened 2 weeks ago

ikaven1024 commented 2 weeks ago

What type of PR is this? /kind bug

What this PR does / why we need it: When client request list with rv=0, proxy fetch objects from cache. It works as expected. While when pagination occurs, a continue will be responsed. This continue will filled with cluster's versions. https://github.com/karmada-io/karmada/blob/02dad486f9628410de07eb4f3156972c8170073d/pkg/search/proxy/store/multi_cluster_cache.go#L295 And the next paging request will access etcd with version in continue. We should make it accessing cache also to improve performance.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-search`: proxy list with rv=0 no longer access member cluster etcd
karmada-bot commented 2 weeks ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: To complete the pull request process, please assign xishanyongye-chang after the PR has been reviewed. You can assign the PR to them by writing /assign @xishanyongye-chang in a comment when ready.

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

Needs approval from an approver in each of these files: - **[pkg/search/OWNERS](https://github.com/karmada-io/karmada/blob/master/pkg/search/OWNERS)** - **[test/OWNERS](https://github.com/karmada-io/karmada/blob/master/test/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
ikaven1024 commented 2 weeks ago

Run karmada-search with --v=4, and request pods with limit=1 and rv=0, get logs:

I0502 22:55:20.201074   20847 multi_cluster_cache.go:233] List member cluster kwok-cluster1 with &internalversion.ListOptions{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, LabelSelector:labels.internalSelector(nil), FieldSelector:fields.andTerm{}, Watch:false, AllowWatchBookmarks:false, ResourceVersion:"0", ResourceVersionMatch:"", TimeoutSeconds:(*int64)(nil), Limit:1, Continue:"", SendInitialEvents:(*bool)(nil)}
I0502 22:55:20.219607   20847 multi_cluster_cache.go:233] List member cluster kwok-cluster2 with &internalversion.ListOptions{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, LabelSelector:labels.internalSelector(nil), FieldSelector:fields.andTerm{}, Watch:false, AllowWatchBookmarks:false, ResourceVersion:"0", ResourceVersionMatch:"", TimeoutSeconds:(*int64)(nil), Limit:1, Continue:"", SendInitialEvents:(*bool)(nil)}

From the logs, we can see that both two member clusters are accessed with ResourceVersion:"0".

codecov-commenter commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 16.66667% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 53.11%. Comparing base (5bc8c54) to head (7015546). Report is 2 commits behind head on master.

Files Patch % Lines
pkg/search/proxy/store/multi_cluster_cache.go 16.66% 4 Missing and 1 partial :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #4896 +/- ## ========================================== - Coverage 53.12% 53.11% -0.02% ========================================== Files 251 251 Lines 20417 20422 +5 ========================================== Hits 10847 10847 - Misses 8856 8860 +4 - Partials 714 715 +1 ``` | [Flag](https://app.codecov.io/gh/karmada-io/karmada/pull/4896/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=karmada-io) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/karmada-io/karmada/pull/4896/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=karmada-io) | `53.11% <16.66%> (-0.02%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=karmada-io#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

RainbowMango commented 1 week ago

Thanks @ikaven1024 , please assign this to @XiShanYongYe-Chang or me once it's ready for review.