fix paging list in karmada search proxy #3402

ikaven1024 · 2023-04-12T15:43:30Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
When paging query via karmada search proxy, if member cluster resources number is larger than page size (limit), the return resourceVersion and continue is not incorrect.

For example, there are two member clusters:

	Cluster1	Cluster2
RV	123	456
Pods	10	10

And client request with:

/apis/search.karmada.io/v1alpha1/proxying/karmada/proxy/api/v1/namespaces/default/pods?limit=5

Incomplete resourceVersion
got:{"cluster1":"123"} (decoded), while want {"cluster1":"123","cluster2":"456"}

Unreasonable continue
There's no resource version in continue, and next page query will got a wrong rv.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-search`: Fixed paging list in karmada search proxy in large-scale member clusters issue.

codecov-commenter · 2023-04-12T16:04:31Z

Codecov Report

Merging #3402 (8924521) into master (ef774d2) will increase coverage by 0.31%.
The diff coverage is 80.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #3402      +/-   ##
==========================================
+ Coverage   51.63%   51.95%   +0.31%     
==========================================
  Files         210      210              
  Lines       18926    19077     +151     
==========================================
+ Hits         9772     9911     +139     
+ Misses       8622     8617       -5     
- Partials      532      549      +17

Flag	Coverage Δ
unittests	`51.95% <80.00%> (+0.31%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/search/proxy/store/util.go	`91.81% <66.66%> (-2.50%)`	⬇️
pkg/search/proxy/store/multi_cluster_cache.go	`76.92% <81.13%> (+2.47%)`	⬆️

... and 10 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ikaven1024 · 2023-04-14T18:40:32Z

Test client:

factory := informers.NewSharedInformerFactoryWithOptions(c, 0, informers.WithTweakListOptions(func(options *metav1.ListOptions) {
	options.Limit = 5
}))

factory.Core().V1().Pods().Informer()

factory.Start(stopCh)
factory.WaitForCacheSync(stopCh)

Logs in search:

I0415 02:35:46.322963   58010 multi_cluster_cache.go:171] Request list /v1, Resource=pods with rv=&store.multiClusterResourceVersion{rvs:map[string]string{}, isZero:true} cont=store.multiClusterContinue{RV:"", Cluster:"", Continue:""}, limit=5
I0415 02:35:46.325858   58010 multi_cluster_cache.go:425] fillMissingClusterResourceVersion gvr=/v1, Resource=pods cluster=kwok-cluster2
I0415 02:35:46.326377   58010 multi_cluster_cache.go:184] Response list /v1, Resource=pods with rv=&store.multiClusterResourceVersion{rvs:map[string]string{"kwok-cluster1":"121450", "kwok-cluster2":"121618"}, isZero:false} continue=store.multiClusterContinue{RV:"eyJrd29rLWNsdXN0ZXIxIjoiMTIxNDUwIiwia3dvay1jbHVzdGVyMiI6IjEyMTYxOCJ9", Cluster:"kwok-cluster2", Continue:""}
I0415 02:35:46.334693   58010 multi_cluster_cache.go:171] Request list /v1, Resource=pods with rv=&store.multiClusterResourceVersion{rvs:map[string]string{}, isZero:false} cont=store.multiClusterContinue{RV:"eyJrd29rLWNsdXN0ZXIxIjoiMTIxNDUwIiwia3dvay1jbHVzdGVyMiI6IjEyMTYxOCJ9", Cluster:"kwok-cluster2", Continue:""}, limit=5
I0415 02:35:46.346554   58010 multi_cluster_cache.go:184] Response list /v1, Resource=pods with rv=&store.multiClusterResourceVersion{rvs:map[string]string{"kwok-cluster1":"121450", "kwok-cluster2":"121618"}, isZero:false} continue=store.multiClusterContinue{RV:"eyJrd29rLWNsdXN0ZXIxIjoiMTIxNDUwIiwia3dvay1jbHVzdGVyMiI6IjEyMTYxOCJ9", Cluster:"kwok-cluster2", Continue:"eyJ2IjoibWV0YS5rOHMuaW8vdjEiLCJydiI6MTIxNjE4LCJzdGFydCI6ImRlZmF1bHQvbmdpbngyLTc4NTk4NjRiNjgtajZiNTZcdTAwMDAifQ"}
I0415 02:35:46.349324   58010 multi_cluster_cache.go:171] Request list /v1, Resource=pods with rv=&store.multiClusterResourceVersion{rvs:map[string]string{}, isZero:false} cont=store.multiClusterContinue{RV:"eyJrd29rLWNsdXN0ZXIxIjoiMTIxNDUwIiwia3dvay1jbHVzdGVyMiI6IjEyMTYxOCJ9", Cluster:"kwok-cluster2", Continue:"eyJ2IjoibWV0YS5rOHMuaW8vdjEiLCJydiI6MTIxNjE4LCJzdGFydCI6ImRlZmF1bHQvbmdpbngyLTc4NTk4NjRiNjgtajZiNTZcdTAwMDAifQ"}, limit=5
I0415 02:35:46.353498   58010 multi_cluster_cache.go:184] Response list /v1, Resource=pods with rv=&store.multiClusterResourceVersion{rvs:map[string]string{"kwok-cluster1":"121450", "kwok-cluster2":"121618"}, isZero:false} continue=store.multiClusterContinue{RV:"eyJrd29rLWNsdXN0ZXIxIjoiMTIxNDUwIiwia3dvay1jbHVzdGVyMiI6IjEyMTYxOCJ9", Cluster:"", Continue:""}
I0415 02:35:46.357867   58010 multi_cluster_cache.go:303] Request watch /v1, Resource=pods with rv=eyJrd29rLWNsdXN0ZXIxIjoiMTIxNDUwIiwia3dvay1jbHVzdGVyMiI6IjEyMTYxOCJ9

ikaven1024 · 2023-04-15T00:02:52Z

/assign @XiShanYongYe-Chang

pkg/search/proxy/store/store.go

pkg/search/proxy/store/multi_cluster_cache.go

nulls-cell

add some comment .

RainbowMango · 2023-04-17T07:31:39Z

Echo test log here before retesting:
https://github.com/karmada-io/karmada/actions/runs/4706573878/jobs/8347964144?pr=3402

Timeline >>
  STEP: Creating ClusterPropagationPolicy(clusterrole-fxvgb) @ 04/15/23 07:35:04.617
  STEP: Creating ClusterRole(clusterrole-fxvgb) @ 04/15/23 07:35:04.639
  [FAILED] in [It] - /home/runner/work/karmada/karmada/test/e2e/framework/rbac.go:158 @ 04/15/23 07:42:14.732
  STEP: Removing ClusterPropagationPolicy(clusterrole-fxvgb) @ 04/15/23 07:42:14.732
  STEP: Remove ClusterRole(clusterrole-fxvgb) @ 04/15/23 07:42:14.746
  << Timeline

  [FAILED] Timed out after 420.000s.
  Expected
      <bool>: false
  to equal
      <bool>: true
  In [It] at: /home/runner/work/karmada/karmada/test/e2e/framework/rbac.go:158 @ 04/15/23 07:42:14.732

  Full Stack Trace
    github.com/karmada-io/karmada/test/e2e/framework.WaitClusterRoleDisappearOnCluster({0x3fd9ec4, 0x7}, {0xc0009d8618, 0x11})
    	/home/runner/work/karmada/karmada/test/e2e/framework/rbac.go:158 +0x556
    github.com/karmada-io/karmada/test/e2e.glob..func2.2.1.4()
    	/home/runner/work/karmada/karmada/test/e2e/clusteraffinities_test.go:257 +0x2ab
------------------------------
[SynchronizedAfterSuite] PASSED [0.009 seconds]
[SynchronizedAfterSuite] 
/home/runner/work/karmada/karmada/test/e2e/suite_test.go:137
------------------------------
• [INTERRUPTED] [0.082 seconds]
Resource interpreter customization testing when Apply single ResourceInterpreterCustomization without DependencyInterpretation operation [JustBeforeEach] InterpreterOperation InterpretStatus testing InterpretStatus

XiShanYongYe-Chang

Thanks~

pkg/search/proxy/store/multi_cluster_cache.go

XiShanYongYe-Chang · 2023-04-18T12:19:52Z

pkg/search/proxy/store/multi_cluster_cache.go

+			if rv == "" {
+				return
+			}


Just a question: when the rv is empty, will an error occur when the whole rv is displayed at the end because the rv of a cluster is empty?

No, the missing cluster is not nested in the multi cluster rv

XiShanYongYe-Chang · 2023-04-18T12:46:19Z

pkg/search/proxy/store/util.go

@@ -125,6 +136,7 @@ func marshalRvs(rvs map[string]string) []byte {
 }

 type multiClusterContinue struct {
+	RV       string `json:"rv"`


Is this change a user-facing change?

User will receive a encoded string. In principle users shall only regard it as a string, and don't care what the playload in it.

Signed-off-by: yingjinhui <yingjinhui@didiglobal.com>

XiShanYongYe-Chang · 2023-04-20T12:14:25Z

Thanks a lot
/approve

Ask again review from @liblikewhen
/cc @liblikewhen

karmada-bot · 2023-04-20T12:14:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: XiShanYongYe-Chang

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:

~~pkg/search/OWNERS~~ [XiShanYongYe-Chang]

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

XiShanYongYe-Chang · 2023-04-23T01:14:48Z

/lgtm

I think we need to rebase it to the previous branch, @ikaven1024 can you help do it?

RainbowMango · 2023-04-23T01:22:01Z

Do you mean cherry-pick?

XiShanYongYe-Chang · 2023-04-23T01:29:48Z

Do you mean cherry-pick?

Yes, it is.

ikaven1024 · 2023-04-23T01:42:40Z

Do you mean cherry-pick?

Yes, it is.

1.4 & 1.5 is ready:
#3449
#3450

1.3: TODO

…02-upstream-release-1.5 Automated cherry pick of #3402: fill cluster rv in proxying list

…02-upstream-release-1.4 Automated cherry pick of #3402: fill cluster rv in proxying list

…02-upstream-release-1.3 Automated cherry pick of #3402: fill cluster rv in proxying list

karmada-bot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 12, 2023

karmada-bot requested review from huntsman-li and XiShanYongYe-Chang April 12, 2023 15:43

karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 12, 2023

ikaven1024 force-pushed the proxy branch from 7cdc168 to 92003e7 Compare April 14, 2023 16:47

karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 14, 2023

ikaven1024 force-pushed the proxy branch from 92003e7 to 1c5e2b5 Compare April 14, 2023 18:02

ikaven1024 changed the title ~~[WIP]fill cluster rv in proxying list~~ fix paging list in karmada search proxy Apr 14, 2023

karmada-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 14, 2023

karmada-bot assigned XiShanYongYe-Chang Apr 15, 2023

nulls-cell reviewed Apr 15, 2023

View reviewed changes

pkg/search/proxy/store/store.go Outdated Show resolved Hide resolved

nulls-cell reviewed Apr 15, 2023

View reviewed changes

pkg/search/proxy/store/multi_cluster_cache.go Outdated Show resolved Hide resolved

nulls-cell reviewed Apr 15, 2023

View reviewed changes

ikaven1024 force-pushed the proxy branch from 1c5e2b5 to 8202490 Compare April 15, 2023 07:04

XiShanYongYe-Chang reviewed Apr 18, 2023

View reviewed changes

ikaven1024 force-pushed the proxy branch from 8202490 to 1bb7325 Compare April 20, 2023 10:57

fill cluster rv in proxying list

8924521

Signed-off-by: yingjinhui <yingjinhui@didiglobal.com>

ikaven1024 force-pushed the proxy branch from 1bb7325 to 8924521 Compare April 20, 2023 10:59

karmada-bot requested a review from nulls-cell April 20, 2023 12:14

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 20, 2023

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2023

karmada-bot merged commit 3812616 into karmada-io:master Apr 23, 2023

This was referenced Apr 23, 2023

Automated cherry pick of #3402: fill cluster rv in proxying list #3449

Merged

Automated cherry pick of #3402: fill cluster rv in proxying list #3450

Merged

ikaven1024 mentioned this pull request Apr 23, 2023

Automated cherry pick of #3402: fill cluster rv in proxying list #3451

Merged

karmada-bot added a commit that referenced this pull request Apr 23, 2023

Merge pull request #3449 from ikaven1024/automated-cherry-pick-of-#34…

874d148

…02-upstream-release-1.5 Automated cherry pick of #3402: fill cluster rv in proxying list

karmada-bot added a commit that referenced this pull request Apr 23, 2023

Merge pull request #3450 from ikaven1024/automated-cherry-pick-of-#34…

2852f2d

…02-upstream-release-1.4 Automated cherry pick of #3402: fill cluster rv in proxying list

karmada-bot added a commit that referenced this pull request Apr 24, 2023

Merge pull request #3451 from ikaven1024/automated-cherry-pick-of-#34…

2420d1a

…02-upstream-release-1.3 Automated cherry pick of #3402: fill cluster rv in proxying list

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix paging list in karmada search proxy #3402

fix paging list in karmada search proxy #3402

ikaven1024 commented Apr 12, 2023 •

edited by RainbowMango

Loading

codecov-commenter commented Apr 12, 2023 •

edited

Loading

ikaven1024 commented Apr 14, 2023

ikaven1024 commented Apr 15, 2023

nulls-cell left a comment •

edited

Loading

RainbowMango commented Apr 17, 2023

XiShanYongYe-Chang left a comment

XiShanYongYe-Chang Apr 18, 2023

ikaven1024 Apr 20, 2023

XiShanYongYe-Chang Apr 18, 2023

ikaven1024 Apr 20, 2023

XiShanYongYe-Chang commented Apr 20, 2023

karmada-bot commented Apr 20, 2023

XiShanYongYe-Chang commented Apr 23, 2023

RainbowMango commented Apr 23, 2023

XiShanYongYe-Chang commented Apr 23, 2023

ikaven1024 commented Apr 23, 2023

fix paging list in karmada search proxy #3402

fix paging list in karmada search proxy #3402

Conversation

ikaven1024 commented Apr 12, 2023 • edited by RainbowMango Loading

codecov-commenter commented Apr 12, 2023 • edited Loading

Codecov Report

ikaven1024 commented Apr 14, 2023

ikaven1024 commented Apr 15, 2023

nulls-cell left a comment • edited Loading

Choose a reason for hiding this comment

RainbowMango commented Apr 17, 2023

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

XiShanYongYe-Chang Apr 18, 2023

Choose a reason for hiding this comment

ikaven1024 Apr 20, 2023

Choose a reason for hiding this comment

XiShanYongYe-Chang Apr 18, 2023

Choose a reason for hiding this comment

ikaven1024 Apr 20, 2023

Choose a reason for hiding this comment

XiShanYongYe-Chang commented Apr 20, 2023

karmada-bot commented Apr 20, 2023

XiShanYongYe-Chang commented Apr 23, 2023

RainbowMango commented Apr 23, 2023

XiShanYongYe-Chang commented Apr 23, 2023

ikaven1024 commented Apr 23, 2023

ikaven1024 commented Apr 12, 2023 •

edited by RainbowMango

Loading

codecov-commenter commented Apr 12, 2023 •

edited

Loading

nulls-cell left a comment •

edited

Loading