feature: Optimizing Pod SidecarSet webhook performance #1547

ls-2018 · 2024-03-28T02:32:17Z

Ⅰ. Describe what this PR does

[feature request] Optimizing Pod SidecarSet webhook performance #1546

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

zmberg · 2024-03-28T02:43:56Z

pkg/util/fieldindex/register.go

+			return nil
+		}
+
+		ns := sets.NewString()


I think there are two types, one is configured spec.namespace and the other is not configured which you can set 'CludsterScope' Namespace, and namespaceSelector also counts as a type of 'ClusterScope'.

zmberg · 2024-03-28T03:16:06Z

pkg/webhook/sidecarset/validating/sidecarset_create_update_handler.go

@@ -93,10 +95,30 @@ func (h *SidecarSetCreateUpdateHandler) validateSidecarSet(obj *appsv1alpha1.Sid
 	}
 	// iterate across all containers in other sidecarsets to avoid duplication of name
 	sidecarSets := &appsv1alpha1.SidecarSetList{}
-	if err := h.Client.List(context.TODO(), sidecarSets, &client.ListOptions{}); err != nil {
-		allErrs = append(allErrs, field.InternalError(field.NewPath(""), fmt.Errorf("query other sidecarsets failed, err: %v", err)))
+	if obj.Spec.Namespace != "" {


Why validate here, didn't understand the scenario.

zmberg · 2024-03-28T03:19:25Z

@ls-2018 There need to be optimized, and only getting the pod.namepsace sidecarSet and the ClusterScope SidecarSet.

zmberg · 2024-03-28T03:23:09Z

pkg/util/fieldindex/register.go

+	IndexNameForController                      = ".metadata.controller"
+	IndexNameForIsActive                        = "isActive"
+	IndexNameForSideCarSetNamespace             = "spec.namespace"
+	IndexNameForSideCarSetClusterScopeNamespace = "spec.namespaceSelector"


Don't build this index of spec.namespaceSelector.

furykerry · 2024-03-28T05:07:18Z

pkg/util/fieldindex/register.go

@@ -152,3 +160,16 @@ func indexImagePullJobActive(c cache.Cache) error {
 		return []string{isActive}
 	})
 }
+
+func indexSideCatSet(c cache.Cache) error {


indexSideCatSet -> indexSidecarSet

furykerry · 2024-03-28T05:14:10Z

pkg/webhook/pod/mutating/sidecarset.go

-	for _, sidecarSet := range sidecarsetList.Items {
+	for _, sidecarSet := range append(sidecarsetList.Items, sidecarsetList2.Items...) {
+		// Prevents sidecarSet from processing twice
+		key := fmt.Sprintf("%s/%s", sidecarSet.Namespace, sidecarSet.Name)


sidecarset is a clusterscope resource, so the name of sidecarset should be unique clusterwise, it is not necessary to add namespace in the key. Actually it seems impossible for a sidecarset to be both cluster scoped and namespace scoped, so is it really possible to precess sidecarset twice here?

furykerry · 2024-03-28T05:16:44Z

pkg/util/fieldindex/register.go

+		if obj.Spec.Namespace != "" {
+			return []string{obj.Spec.Namespace}
+		}
+		return []string{IndexValueSideCarSetClusterScope}


since k8s 1.22, k8s will automatically assign name label in the namespace, so if the sidecarset namespace selector select the built-in label, it should also return the namespace index

furykerry · 2024-03-28T05:21:16Z

pkg/util/fieldindex/register.go

+	IndexNameForController           = ".metadata.controller"
+	IndexNameForIsActive             = "isActive"
+	IndexNameForSideCarSetNamespace  = "namespace"
+	IndexValueSideCarSetClusterScope = "ClusterScope"


IndexNameForSideCarSetNamespace -> IndexNameForSidecarSetNamespace
IndexValueSideCarSetClusterScope -> IndexValueSidecarSetClusterScope

Spground · 2024-04-01T11:55:22Z

Moreover, it will improve a lot if use index in sidecarset controller as same as webhook,

kruise/pkg/controller/sidecarset/sidecarset_processor.go

Line 226 in 1f00e6b

func (p *Processor) listMatchedSidecarSets(pod *corev1.Pod) string {

.

codecov · 2024-04-02T00:24:30Z

Codecov Report

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

Project coverage is 47.78%. Comparing base (1f00e6b) to head (51e60fa).
Report is 3 commits behind head on master.

Files	Patch %	Lines
pkg/controller/sidecarset/sidecarset_processor.go	0.00%	11 Missing ⚠️
pkg/util/fieldindex/register.go	69.23%	8 Missing ⚠️
pkg/webhook/pod/mutating/sidecarset.go	36.36%	4 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1547      +/-   ##
==========================================
- Coverage   47.97%   47.78%   -0.20%     
==========================================
  Files         162      163       +1     
  Lines       23491    23611     +120     
==========================================
+ Hits        11270    11282      +12     
- Misses      11005    11108     +103     
- Partials     1216     1221       +5

Flag	Coverage Δ
unittests	`47.78% <45.83%> (-0.20%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ls-2018 · 2024-04-02T00:35:11Z

/cc @zmberg @furykerry

furykerry · 2024-04-02T01:26:45Z

pkg/util/fieldindex/register.go

@@ -152,3 +161,32 @@ func indexImagePullJobActive(c cache.Cache) error {
 		return []string{isActive}
 	})
 }
+
+func IndexSideCarSet(rawObj client.Object) []string {


plz add ut for this published func

Spground · 2024-04-01T09:01:52Z

pkg/util/fieldindex/register.go

+	IndexNameForController           = ".metadata.controller"
+	IndexNameForIsActive             = "isActive"
+	IndexNameForSidecarSetNamespace  = "namespace"
+	IndexValueSidecarSetClusterScope = "ClusterScope"


How about ClusterScope refactor to be clusterScope , which looks like be same format style.

Spground · 2024-04-01T09:06:26Z

pkg/util/fieldindex/register.go

+	IndexNameForIsActive             = "isActive"
+	IndexNameForSidecarSetNamespace  = "namespace"
+	IndexValueSidecarSetClusterScope = "ClusterScope"
+	LabelMetadataName                = "kubernetes.io/metadata.name"


In my opinion, it's better refs the API label via k8s apis (k8s.io/api/core/v1/well_known_labels.go) rather than we repeat these literal values.

furykerry · 2024-04-02T11:35:03Z

/lgtm

zmberg · 2024-04-07T02:39:11Z

pkg/util/fieldindex/register.go

+	return []string{IndexValueSidecarSetClusterScope}
+}
+
+func indexSideCarSet(c cache.Cache) error {


indexSideCarSet -> indexSidecarSet

Signed-off-by: acejilam <[email protected]>

zmberg · 2024-04-07T06:38:26Z

/lgtm

furykerry · 2024-04-08T03:52:19Z

/lgtm

furykerry · 2024-04-08T03:52:29Z

/approve

Spground · 2024-04-08T03:52:33Z

/lgtm

kruise-bot · 2024-04-08T03:52:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furykerry

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~~ [furykerry]

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

kruise-bot · 2024-04-08T03:52:42Z

@Spground: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kruise-bot requested review from FillZpp and furykerry March 28, 2024 02:32

kruise-bot added the size/M size/M: 30-99 label Mar 28, 2024

ls-2018 force-pushed the 1546 branch 2 times, most recently from 51d4e52 to 983fdd8 Compare March 28, 2024 02:36

zmberg reviewed Mar 28, 2024

View reviewed changes

ls-2018 force-pushed the 1546 branch 3 times, most recently from 8e01623 to 650b7a3 Compare March 28, 2024 02:59

zmberg reviewed Mar 28, 2024

View reviewed changes

ls-2018 force-pushed the 1546 branch from 650b7a3 to 27a49ea Compare March 28, 2024 04:35

furykerry reviewed Mar 28, 2024

View reviewed changes

ls-2018 force-pushed the 1546 branch 2 times, most recently from 4298d8b to d9ee8fd Compare March 28, 2024 12:36

ls-2018 force-pushed the 1546 branch from 0c0f3eb to 5383733 Compare April 1, 2024 23:54

kruise-bot added the do-not-merge/hold label Apr 1, 2024

ls-2018 force-pushed the 1546 branch from 5383733 to 0b4c563 Compare April 2, 2024 00:01

kruise-bot removed the do-not-merge/hold label Apr 2, 2024

ls-2018 force-pushed the 1546 branch from 0b4c563 to 6e30873 Compare April 2, 2024 00:15

kruise-bot added size/L size/L: 100-499 and removed size/M size/M: 30-99 labels Apr 2, 2024

ls-2018 force-pushed the 1546 branch from 6e30873 to 9dce492 Compare April 2, 2024 00:18

ls-2018 requested review from furykerry and zmberg April 2, 2024 00:34

furykerry reviewed Apr 2, 2024

View reviewed changes

ls-2018 force-pushed the 1546 branch from 9dce492 to 125f1dd Compare April 2, 2024 02:55

Spground reviewed Apr 2, 2024

View reviewed changes

kruise-bot assigned furykerry Apr 2, 2024

kruise-bot added the lgtm label Apr 2, 2024

ls-2018 force-pushed the 1546 branch from 125f1dd to d9a7390 Compare April 3, 2024 05:49

kruise-bot removed the lgtm label Apr 3, 2024

zmberg reviewed Apr 7, 2024

View reviewed changes

ls-2018 force-pushed the 1546 branch from d9a7390 to 89ff287 Compare April 7, 2024 02:56

feature: Optimizing Pod SidecarSet webhook performance

51e60fa

Signed-off-by: acejilam <[email protected]>

ls-2018 force-pushed the 1546 branch from 89ff287 to 51e60fa Compare April 7, 2024 02:57

kruise-bot assigned zmberg Apr 7, 2024

kruise-bot added the lgtm label Apr 7, 2024

kruise-bot added the approved label Apr 8, 2024

kruise-bot merged commit 8bb8964 into openkruise:master Apr 8, 2024
32 of 34 checks passed

ls-2018 deleted the 1546 branch April 8, 2024 03:53

ABNER-1 mentioned this pull request Aug 30, 2024

[feature request] Optimizing Kruise-Manager Startup Performance in Large Clusters #1718

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Optimizing Pod SidecarSet webhook performance #1547

feature: Optimizing Pod SidecarSet webhook performance #1547

ls-2018 commented Mar 28, 2024 •

edited

Loading

zmberg Mar 28, 2024

zmberg Mar 28, 2024

zmberg commented Mar 28, 2024

zmberg Mar 28, 2024

furykerry Mar 28, 2024

furykerry Mar 28, 2024

furykerry Mar 28, 2024

furykerry Mar 28, 2024

Spground commented Apr 1, 2024 •

edited

Loading

codecov bot commented Apr 2, 2024 •

edited

Loading

ls-2018 commented Apr 2, 2024

furykerry Apr 2, 2024

Spground Apr 1, 2024

Spground Apr 1, 2024

furykerry commented Apr 2, 2024

zmberg Apr 7, 2024

zmberg commented Apr 7, 2024

furykerry commented Apr 8, 2024

furykerry commented Apr 8, 2024

Spground commented Apr 8, 2024

kruise-bot commented Apr 8, 2024

kruise-bot commented Apr 8, 2024

feature: Optimizing Pod SidecarSet webhook performance #1547

feature: Optimizing Pod SidecarSet webhook performance #1547

Conversation

ls-2018 commented Mar 28, 2024 • edited Loading

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmberg commented Mar 28, 2024

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

Spground commented Apr 1, 2024 • edited Loading

codecov bot commented Apr 2, 2024 • edited Loading

Codecov Report

ls-2018 commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furykerry commented Apr 2, 2024

Choose a reason for hiding this comment

zmberg commented Apr 7, 2024

furykerry commented Apr 8, 2024

furykerry commented Apr 8, 2024

Spground commented Apr 8, 2024

kruise-bot commented Apr 8, 2024

kruise-bot commented Apr 8, 2024

ls-2018 commented Mar 28, 2024 •

edited

Loading

Spground commented Apr 1, 2024 •

edited

Loading

codecov bot commented Apr 2, 2024 •

edited

Loading