refactor search reconciling logic #3670

tedli · 2023-06-14T10:12:12Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

clusters change didn't trigger registries reconcile.
a cluster may has more than one cluster manager informer.

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

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

whitewindmills

Kindly add e2e case to cover this scenario.

codecov-commenter · 2023-06-15T11:55:52Z

Codecov Report

Merging #3670 (8fddeea) into master (c921acd) will increase coverage by 0.01%.
The diff coverage is 57.89%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #3670      +/-   ##
==========================================
+ Coverage   55.68%   55.70%   +0.01%     
==========================================
  Files         222      223       +1     
  Lines       21195    21233      +38     
==========================================
+ Hits        11803    11828      +25     
- Misses       8765     8774       +9     
- Partials      627      631       +4

Flag	Coverage Δ
unittests	`55.70% <57.89%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/util/common.go	`57.89% <57.89%> (ø)`

... and 2 files with indirect coverage changes

pkg/search/backendstore/store.go

pkg/search/controller.go

pkg/util/fedinformer/genericmanager/multi-cluster-manager.go

XiShanYongYe-Chang · 2023-06-16T09:20:06Z

Thanks @tedli @whitewindmills
I'll take a review ASAP.

pkg/search/controller.go

pkg/util/common.go

pkg/search/controller.go

XiShanYongYe-Chang

Thanks~
Still reviewing.

XiShanYongYe-Chang · 2023-06-21T07:57:39Z

pkg/search/controller.go

-
-		c.queue.Add(cls)
+	clusterSet := sets.New[string]()
+	for _, registry := range []*searchv1alpha1.ResourceRegistry{newRR, oldRR} {


This processing may result in some redundancy. For certain clusters in rr, there have been no changes and we can focus only on the ones with differences.

This controller reconcile the relationship between clusters and registries, what's more the key of the controller's queue is cluster name.
At this point, we don't know cluster changed or not, the only thing we know is if reflect.DeepEqual(oldRR.Spec, newRR.Spec) not true, the registry must changed.
So it's need to do some reconcile. But, as I mentioned above, the key of the controller queue is cluster name, this leave us only 2 choices, 1 is to queue the cluster name, do reconcile at cluster level, that's what I did; 2 is to introduce another worker, which the key of queue is registry name, then we can reconcile at registry level.
However, if we decice to do using method 2, then we also have to retrieve clusters of old registry spec, and clusters of new registry spec, to find out what changed, then reconcile changed clusters, because finally reconciling multi cluster informer, is what we want.
It seemd not possible to reduce the redunancy, since if cluster-registry relationship not changed, the resources may also be changed, like previously we want to cache Pod, but currently we no longer want caching Pod, the cluster-registry relation not changed, but the resource did. That's why I use diff logic.

XiShanYongYe-Chang · 2023-06-21T09:52:09Z

pkg/search/controller.go

+	defer func() {
+		updated = cr
+	}()


cr a pointer type, do we need to handle this step separately?

No, we not have to do this.

But by doing this we have 2 benefits, 1 is that the input cr arg could be nil; 2 is that at the callee level we know the cr may be changed by this function, which is more expressive.

That's some like append, a = append(a, obj).

pkg/search/controller.go

XiShanYongYe-Chang · 2023-06-21T10:04:32Z

pkg/search/controller.go

+	for _, name := range added {
+		cr.registries[name] = struct{}{}
+		addedResources = c.getRegistryAddedResources(registries[name], cr, addedResources)
+	}
+	for _, name := range removed {
+		delete(cr.registries, name)
+		for resource, previousRegistries := range cr.resources {
+			delete(previousRegistries, name)
+			if len(previousRegistries) == 0 {
+				removedResources = append(removedResources, resource)
+			}
+		}
+		for _, resource := range removedResources {
+			delete(cr.resources, resource)
+		}
+	}


Do we need to handle deletions first before handling additions, because the added elements may contain deleted ones?

I didn't got you, did you mean it's better to handle removed (L215) before added (L211)?

Either is ok.

Yes, what I mean is to delete first and then add, so as to prevent the elements from being cleared when deleting and adding elements that contain the same content.

XiShanYongYe-Chang · 2023-06-21T10:06:17Z

pkg/search/controller.go

+	for _, registry := range matched {
+		addedResources = c.getRegistryAddedResources(registry, cr, addedResources)
+	}


This processing step feels redundant. All the matched RRs should have already been processed.

No, there may be added ones, and removed ones. But still remaining ones may have resources changed.
Like, registry target cluster is aaa=bbb, resources, Pod, Node, cluster label changed, and resources also changed, like adding Event.
So we not only need to handle registries added or removed, but also need to handle resources changes.

XiShanYongYe-Chang · 2023-06-21T10:09:30Z

pkg/search/controller.go

+		cr = &crv
+		addedRegistries, removedRegistries = util.DiffKey(cr.registries, matchedRegistries)
+	}
+	if len(addedRegistries) > 0 {


Can we omit these judgments to improve the code readability and reduce cyclomatic complexity? Same as below.

Yes, at first I thought it's not need to stop the informer, just remove uneeded resources. So I did diff to find out what are added, what are removed.

But it's not work, we have to stop a informer if something changed, to init a new one. Unless we do a huge refactor of informer.

XiShanYongYe-Chang · 2023-06-21T10:31:37Z

pkg/search/controller.go

+	if resourcesChanged {
+		klog.Infof("Create new informer for cluster %s", cluster)
+		c.InformerManager.Stop(cluster)
+	}


Do we need to stop the informer before making changes to the resources, or can we directly modify the current informer?

No, like the comment above, I firstly came the same idea as you, so I did diff to find out what are added and what are removed. But it just not work.

You can find the first commit of this PR, I did the exact same thing, modifing the current informer.

karmada/pkg/util/fedinformer/genericmanager/single-cluster-manager.go

Lines 19 to 22 in a9df865

type SingleClusterInformerManager interface {

// UnwatchResource removes all existing event handlers from the informer of the given

// resource.

UnwatchResource(resource schema.GroupVersionResource)

Here, the first commit of this PR, I tried to add a UnwatchResource(resource schema.GroupVersionResource), without success.

XiShanYongYe-Chang · 2023-06-25T07:42:11Z

pkg/util/common_test.go

+	}
+)
+
+func TestDiffKey(t *testing.T) {


Can this test achieve condition coverage? Consider the boundary conditions.

XiShanYongYe-Chang

Thank you. Everything has been reviewed once, and only some testing issues remain.

XiShanYongYe-Chang · 2023-06-25T07:52:58Z

test/e2e/search_test.go

+					var proxyList *corev1.NodeList
+					gomega.Eventually(func(g gomega.Gomega) {
+						var err error
+						proxyList, err = proxyClient.CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{})


The current modifications made in PR should not involve any changes in the search/proxy functionality. This test example may have been placed in the wrong location and can be moved to this side of line 344.

XiShanYongYe-Chang · 2023-06-25T07:55:52Z

test/e2e/search_test.go

@@ -710,5 +710,72 @@ var _ = ginkgo.Describe("[karmada-search] karmada search testing", ginkgo.Ordere
 				})
 			})
 		})
+		ginkgo.Describe("update cluster testing", ginkgo.Ordered, func() {
+			ginkgo.Context("cluster labeled after registry created", ginkgo.Ordered, func() {


This line seems redundant.

I didn't touch this test case, it's provided by other commiter previously. It seemed that the diff algo of github get confused, it marks this line green treated as modification of this pr, but it's not.

XiShanYongYe-Chang · 2023-06-27T08:00:15Z

pkg/util/common_test.go

+			t.Errorf("removed = %v, want []string{\"kubefed\"}", removed)
+		}
+	})
+	return


This line can be removed.

echo lint errors here:

Installing golangci-lint v1.[5](https://github.com/karmada-io/karmada/actions/runs/5387138616/jobs/9778049938?pr=3670#step:5:6)2.2 golangci/golangci-lint info checking GitHub for tag 'v1.52.2' golangci/golangci-lint info found version: 1.52.2 for v1.52.2/linux/amd[6](https://github.com/karmada-io/karmada/actions/runs/5387138616/jobs/9778049938?pr=3670#step:5:7)4 golangci/golangci-lint info installed /home/runner/go/bin/golangci-lint Error: pkg/util/common_test.go:[8](https://github.com/karmada-io/karmada/actions/runs/5387138616/jobs/9778049938?pr=3670#step:5:9)6:2: S1023: redundant `return` statement (gosimple) return ^ Please review the above warnings. If the above warnings do not make sense, feel free to file an issue. Error: Process completed with exit code 1.

XiShanYongYe-Chang · 2023-06-27T08:16:18Z

test/e2e/search_test.go

@@ -710,5 +756,42 @@ var _ = ginkgo.Describe("[karmada-search] karmada search testing", ginkgo.Ordere
 				})
 			})
 		})
+		ginkgo.Describe("update cluster testing", ginkgo.Ordered, func() {


This new add test block can be removed.

test/e2e/search_test.go

XiShanYongYe-Chang · 2023-06-28T09:02:32Z

test/e2e/search_test.go

+			m1Dynamic = framework.GetClusterDynamicClient(member1)
+			gomega.Expect(m1Dynamic).ShouldNot(gomega.BeNil())
+			proxyConfig := *restConfig
+			proxyConfig.Host += "/apis/search.karmada.io/v1alpha1/proxying/karmada/proxy"


Hi @tedli, this e2e might not be written correctly. The test coverage does not include the modified content. In this revision, we have added handling for cluster modification events, but this only applies to search cache and not search proxy capability.

Have you tested the new processing logic locally? If it passes the local test, please share the test results.
And then we can submit an E2E test for the modification in a subsequent PR to promote the integration of the current refactoring code.

Hi reminder that @tedli might not be familiar with E2E tests. Can we go with a simple PR first? If you think we need an E2E test, open an issue to track.

I'm saying that because this PR has been blocked for weeks.

Yes, I think we can start by removing the E2E part and adding a separate PR to include e2e testing. This test can be completed by anyone who is interested. I create Issue #3731 to track it. How do you think @tedli

Hi all,
Sorry for late reply. I will remove e2e test case, and I'm glad to take a deeper dive with karmada e2e test, and then finish #3731

Thanks @tedli

XiShanYongYe-Chang · 2023-06-29T09:05:08Z

I think we can merge now :)
Before merging it, can you help squash those commits into one commit?

Signed-off-by: lizhen6 <lizhen6@360.cn>

XiShanYongYe-Chang · 2023-06-30T02:59:20Z

/lgtm
Ask an approve from @RainbowMango
/cc @RainbowMango

RainbowMango

/approve

karmada-bot · 2023-06-30T14:59:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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:

~~OWNERS~~ [RainbowMango]

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

karmada-bot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 14, 2023

karmada-bot requested review from huntsman-li and RainbowMango June 14, 2023 10:12

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 14, 2023

whitewindmills reviewed Jun 15, 2023

View reviewed changes

tedli marked this pull request as draft June 15, 2023 06:05

karmada-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 15, 2023

tedli marked this pull request as ready for review June 15, 2023 11:40

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 Jun 15, 2023

whitewindmills reviewed Jun 16, 2023

View reviewed changes

XiShanYongYe-Chang reviewed Jun 20, 2023

View reviewed changes

pkg/search/controller.go Outdated Show resolved Hide resolved

pkg/util/common.go Show resolved Hide resolved

pkg/search/controller.go Outdated Show resolved Hide resolved

XiShanYongYe-Chang reviewed Jun 21, 2023

View reviewed changes

XiShanYongYe-Chang reviewed Jun 25, 2023

View reviewed changes

XiShanYongYe-Chang reviewed Jun 27, 2023

View reviewed changes

XiShanYongYe-Chang reviewed Jun 28, 2023

View reviewed changes

XiShanYongYe-Chang mentioned this pull request Jun 28, 2023

Add E2E for #3670: when cluster update, karmada need to reconcile search cache #3731

Closed

refactor search reconciling logic

8fddeea

Signed-off-by: lizhen6 <lizhen6@360.cn>

tedli force-pushed the refactor-search branch from 7903dc5 to 8fddeea Compare June 30, 2023 01:53

karmada-bot assigned XiShanYongYe-Chang Jun 30, 2023

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

RainbowMango approved these changes Jun 30, 2023

View reviewed changes

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

karmada-bot merged commit dfd12ef into karmada-io:master Jun 30, 2023
12 checks passed

	type SingleClusterInformerManager interface {
	// UnwatchResource removes all existing event handlers from the informer of the given
	// resource.
	UnwatchResource(resource schema.GroupVersionResource)

refactor search reconciling logic #3670

refactor search reconciling logic #3670

Conversation

tedli commented Jun 14, 2023

whitewindmills left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 15, 2023 • edited Loading

Codecov Report

XiShanYongYe-Chang commented Jun 16, 2023

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiShanYongYe-Chang commented Jun 29, 2023

XiShanYongYe-Chang commented Jun 30, 2023

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Jun 30, 2023

codecov-commenter commented Jun 15, 2023 •

edited

Loading