karmada-search: fix the problem that ResourceVersion base64 encrypted repeatedly when starting multiple informer to watch resource #3376

niuyueyang1996 · 2023-04-10T09:31:49Z

if start many watcher to watch pod.
pod resourceVersion will base64 duplicate.
soo deepCopy event.

What type of PR is this?
/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #3377

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-search`: Fixed the problem that ResourceVersion base64 encrypted repeatedly when starting multiple informers to watch resource.

karmada-bot · 2023-04-10T09:32:00Z

Welcome @niuyueyang1996! It looks like this is your first PR to karmada-io/karmada 🎉

niuyueyang1996 · 2023-04-10T09:44:23Z

/assign @huntsman-li

niuyueyang1996 · 2023-04-10T09:47:26Z

pkg/search/proxy/store/multi_cluster_cache.go

@@ -315,7 +315,6 @@ func (c *MultiClusterCache) Watch(ctx context.Context, gvr schema.GroupVersionRe
 		}

 		mux.AddSource(w, func(e watch.Event) {
-			// We can safely modify data because it is deepCopied in cacheWatcher.convertToWatchEvent


the note is wrong.
if object is *cachingObject,will not DeepCopy.

cachingObject will be deep-copied lazily, but our usage violate its condition:

karmada/vendor/k8s.io/apiserver/pkg/storage/cacher/caching_object.go

Lines 71 to 75 in 681b946

// The lazy deep-copy make is useful, as effectively the only

// case when we are setting some fields are ResourceVersion for

// DELETE events, so in all other cases we can effectively avoid

// performing any deep copies.

deepCopied bool

ikaven1024 · 2023-04-10T10:11:21Z

Thanks, I will check it.
/assign

codecov-commenter · 2023-04-10T11:30:14Z

Codecov Report

Merging #3376 (37f58dd) into master (a7ae11c) will increase coverage by 0.02%.
The diff coverage is 100.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    #3376      +/-   ##
==========================================
+ Coverage   51.62%   51.64%   +0.02%     
==========================================
  Files         210      210              
  Lines       18928    18927       -1     
==========================================
+ Hits         9771     9775       +4     
+ Misses       8631     8626       -5     
  Partials      526      526

Flag	Coverage Δ
unittests	`51.64% <100.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/search/proxy/store/multi_cluster_cache.go	`74.44% <ø> (-0.10%)`	⬇️
pkg/search/proxy/store/util.go	`94.31% <100.00%> (+1.42%)`	⬆️

... and 1 file 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-10T11:33:16Z

LGTM

@niuyueyang1996 Could you add message to the second commit, or squash them, thanks.

pod resourceVersion will base64 duplicate. soo deepCopy event. Signed-off-by: niuyueyang <719415781@qq.com>

niuyueyang1996 · 2023-04-10T12:06:10Z

i already squash them.

XiShanYongYe-Chang · 2023-04-10T12:34:39Z

Hi @niuyueyang1996 @ikaven1024, thanks to all of you!
Do we need to cherry-pick this patch to the previous branch?

XiShanYongYe-Chang · 2023-04-10T12:35:11Z

Ask @RainbowMango to help run the CI test.
/cc @RainbowMango

ikaven1024 · 2023-04-10T13:08:32Z

Do we need to cherry-pick this patch to the previous branch?

I think so.

niuyueyang1996 · 2023-04-11T01:03:50Z

i have another question in #3377.
why list func, response resourceVersion not use base64 encode?

XiShanYongYe-Chang · 2023-04-11T01:48:54Z

why list func, response resourceVersion not use base64 encode?

Do you mean that #3377 raises two questions, but the current pr only solves one of them? Ask @ikaven1024 to help take a look again~

niuyueyang1996 · 2023-04-11T02:01:44Z

yes,the current pr only fix the bug.
the other questions i am not sure it is a bug or expected.

RainbowMango

Can you guys help to propose a release-notes? @XiShanYongYe-Chang @ikaven1024

ikaven1024 · 2023-04-11T03:18:40Z

why list func, response resourceVersion not use base64 encode?

I have reply it in #3377.

XiShanYongYe-Chang · 2023-04-11T03:41:51Z

@niuyueyang1996 @ikaven1024 How about update the release-note like this:

karmada-search: fix the problem that ResourceVersion base64 encrypted repeatedly when starting multiple informer to watch resource

ikaven1024 · 2023-04-11T03:44:48Z

@niuyueyang1996 @ikaven1024 How about update the release-note like this:
karmada-search: fix the problem that ResourceVersion base64 encrypted repeatedly when starting multiple informer to watch resource

LGTM

niuyueyang1996 · 2023-04-11T06:37:03Z

@niuyueyang1996 @ikaven1024 How about update the release-note like this:
karmada-search: fix the problem that ResourceVersion base64 encrypted repeatedly when starting multiple informer to watch resource

i agree with the release-note.

XiShanYongYe-Chang · 2023-04-11T07:01:56Z

i agree with the release-note.

Thanks~ @niuyueyang1996, can you help update the release-note and title.

/approve

Leave lgtm to @ikaven1024

karmada-bot · 2023-04-11T07:02:03Z

[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

ikaven1024 · 2023-04-11T07:24:28Z

/lgtm

…-#3376-upstream-release-1.5 Automated cherry pick of #3376: if start many watcher to watch pod. pod resourceVersion will

…-#3376-upstream-release-1.3 Automated cherry pick of #3376: if start many watcher to watch pod. pod resourceVersion will

…-#3376-upstream-release-1.4 Automated cherry pick of #3376: if start many watcher to watch pod. pod resourceVersion will

karmada-bot requested review from ikaven1024 and XiShanYongYe-Chang April 10, 2023 09:31

karmada-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 10, 2023

karmada-bot assigned huntsman-li Apr 10, 2023

niuyueyang1996 commented Apr 10, 2023

View reviewed changes

karmada-bot assigned ikaven1024 Apr 10, 2023

karmada-bot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Apr 10, 2023

if start many watcher to watch pod.

daeac00

pod resourceVersion will base64 duplicate. soo deepCopy event. Signed-off-by: niuyueyang <719415781@qq.com>

karmada-bot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Apr 10, 2023

XiShanYongYe-Chang mentioned this pull request Apr 10, 2023

start many informer,pod resourceVersion will duplicate base64。 #3377

Closed

karmada-bot requested a review from RainbowMango April 10, 2023 12:35

RainbowMango reviewed Apr 11, 2023

View reviewed changes

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

niuyueyang1996 changed the title ~~watch event deepcopy~~ karmada-search: fix the problem that ResourceVersion base64 encrypted repeatedly when starting multiple informer to watch resource Apr 11, 2023

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

karmada-bot merged commit b273800 into karmada-io:master Apr 11, 2023
12 checks passed

XiShanYongYe-Chang mentioned this pull request Apr 11, 2023

cherry-pick #3376 to the previous branch #3385

Closed

3 tasks

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

Merge pull request #3387 from niuyueyang1996/automated-cherry-pick-of…

ed1813d

…-#3376-upstream-release-1.5 Automated cherry pick of #3376: if start many watcher to watch pod. pod resourceVersion will

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

Merge pull request #3391 from niuyueyang1996/automated-cherry-pick-of…

2f367d8

…-#3376-upstream-release-1.3 Automated cherry pick of #3376: if start many watcher to watch pod. pod resourceVersion will

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

Merge pull request #3388 from niuyueyang1996/automated-cherry-pick-of…

6824165

…-#3376-upstream-release-1.4 Automated cherry pick of #3376: if start many watcher to watch pod. pod resourceVersion will

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

karmada-search: fix the problem that ResourceVersion base64 encrypted repeatedly when starting multiple informer to watch resource #3376

karmada-search: fix the problem that ResourceVersion base64 encrypted repeatedly when starting multiple informer to watch resource #3376

niuyueyang1996 commented Apr 10, 2023 •

edited by RainbowMango

karmada-bot commented Apr 10, 2023

niuyueyang1996 commented Apr 10, 2023

niuyueyang1996 Apr 10, 2023

ikaven1024 Apr 10, 2023

ikaven1024 commented Apr 10, 2023

codecov-commenter commented Apr 10, 2023

ikaven1024 commented Apr 10, 2023

niuyueyang1996 commented Apr 10, 2023

XiShanYongYe-Chang commented Apr 10, 2023

XiShanYongYe-Chang commented Apr 10, 2023

ikaven1024 commented Apr 10, 2023

niuyueyang1996 commented Apr 11, 2023

XiShanYongYe-Chang commented Apr 11, 2023

niuyueyang1996 commented Apr 11, 2023

RainbowMango left a comment

ikaven1024 commented Apr 11, 2023

XiShanYongYe-Chang commented Apr 11, 2023

ikaven1024 commented Apr 11, 2023

niuyueyang1996 commented Apr 11, 2023

XiShanYongYe-Chang commented Apr 11, 2023

karmada-bot commented Apr 11, 2023

ikaven1024 commented Apr 11, 2023

	// The lazy deep-copy make is useful, as effectively the only
	// case when we are setting some fields are ResourceVersion for
	// DELETE events, so in all other cases we can effectively avoid
	// performing any deep copies.
	deepCopied bool

karmada-search: fix the problem that ResourceVersion base64 encrypted repeatedly when starting multiple informer to watch resource #3376

karmada-search: fix the problem that ResourceVersion base64 encrypted repeatedly when starting multiple informer to watch resource #3376

Conversation

niuyueyang1996 commented Apr 10, 2023 • edited by RainbowMango

karmada-bot commented Apr 10, 2023

niuyueyang1996 commented Apr 10, 2023

niuyueyang1996 Apr 10, 2023

Choose a reason for hiding this comment

ikaven1024 Apr 10, 2023

Choose a reason for hiding this comment

ikaven1024 commented Apr 10, 2023

codecov-commenter commented Apr 10, 2023

Codecov Report

ikaven1024 commented Apr 10, 2023

niuyueyang1996 commented Apr 10, 2023

XiShanYongYe-Chang commented Apr 10, 2023

XiShanYongYe-Chang commented Apr 10, 2023

ikaven1024 commented Apr 10, 2023

niuyueyang1996 commented Apr 11, 2023

XiShanYongYe-Chang commented Apr 11, 2023

niuyueyang1996 commented Apr 11, 2023

RainbowMango left a comment

Choose a reason for hiding this comment

ikaven1024 commented Apr 11, 2023

XiShanYongYe-Chang commented Apr 11, 2023

ikaven1024 commented Apr 11, 2023

niuyueyang1996 commented Apr 11, 2023

XiShanYongYe-Chang commented Apr 11, 2023

karmada-bot commented Apr 11, 2023

ikaven1024 commented Apr 11, 2023

niuyueyang1996 commented Apr 10, 2023 •

edited by RainbowMango