fix: SidecarSet Expectations Leakage Bug #1301

wangwu50 · 2023-05-29T06:37:22Z

fix sidecarSet expectations leakage bug when delete pod alfter update sidecarset

kruise-bot · 2023-05-29T06:37:37Z

Welcome @wangwu50! It looks like this is your first PR to openkruise/kruise 🎉

wangwu50 · 2023-05-29T06:38:32Z

zmberg · 2023-05-29T06:42:11Z

pkg/controller/sidecarset/sidecarset_controller.go

@@ -115,9 +112,8 @@ var _ reconcile.Reconciler = &ReconcileSidecarSet{}
 // ReconcileSidecarSet reconciles a SidecarSet object
 type ReconcileSidecarSet struct {
 	client.Client
-	scheme             *runtime.Scheme
-	updateExpectations expectations.UpdateExpectations


Why did you remove this filed? My understanding of the previous bug had nothing to do with this field being defined here, right?

Only updateExpectations were removed because they were initialized in utils

A utils is added so that the sidecarset handler can access it, similar to how cloneset handles it

you can move the function to file https://github.com/openkruise/kruise/blob/master/pkg/control/sidecarcontrol/util.go.

OK, I've moved it

codecov-commenter · 2023-05-29T06:48:47Z

Codecov Report

Merging #1301 (dbb13ee) into master (95e42f3) will decrease coverage by 0.03%.
The diff coverage is 59.09%.

❗ 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    #1301      +/-   ##
==========================================
- Coverage   48.60%   48.57%   -0.03%     
==========================================
  Files         151      151              
  Lines       21005    21042      +37     
==========================================
+ Hits        10210    10222      +12     
- Misses       9680     9702      +22     
- Partials     1115     1118       +3

Flag	Coverage Δ
unittests	`48.57% <59.09%> (-0.03%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/control/sidecarcontrol/util.go	`53.48% <ø> (ø)`
pkg/controller/sidecarset/sidecarset_controller.go	`15.38% <0.00%> (+0.29%)`	⬆️
...troller/sidecarset/sidecarset_pod_event_handler.go	`63.63% <53.33%> (-1.46%)`	⬇️
pkg/controller/sidecarset/sidecarset_processor.go	`68.67% <83.33%> (+0.06%)`	⬆️

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

zmberg · 2023-05-30T02:02:51Z

pkg/controller/sidecarset/sidecarset_pod_event_handler.go

@@ -82,6 +102,9 @@ func (p *enqueueRequestForPod) updatePod(q workqueue.RateLimitingInterface, old,
 		if sidecarSet.Spec.UpdateStrategy.Type == appsv1alpha1.NotUpdateSidecarSetStrategyType {
 			continue
 		}
+		if newPod.DeletionTimestamp != nil {


I think you should not judge the deletionTimestamp, and use the ObserveUpdated function.

ObserveUpdated does not determine if the pod is deleted if the pod is pending deletion

If this pod is an older version, there may be problems

zmberg · 2023-05-30T02:39:22Z

pkg/controller/sidecarset/sidecarset_pod_event_handler.go

@@ -57,6 +74,9 @@ func (p *enqueueRequestForPod) addPod(q workqueue.RateLimitingInterface, obj run
 	}

 	for _, sidecarSet := range sidecarSets {
+		if pod.DeletionTimestamp != nil {


I think it is not necessary to execute the expectation function when add pod.

on a restart of the controller manager, it's possible a new pod shows up in a state that is already pending deletion. Prevent the pod from being a creation observation.

Signed-off-by: wangwenchao7 <wangwenchao7@xiaomi.com>

zmberg · 2023-05-30T08:36:07Z

/lgtm

furykerry

/lgtm

pkg/controller/sidecarset/sidecarset_pod_event_handler.go

Signed-off-by: wangwenchao7 <wangwenchao7@xiaomi.com>

veophi · 2023-05-30T09:34:05Z

/lgtm

zmberg · 2023-06-01T08:21:40Z

/approve

kruise-bot · 2023-06-01T08:21:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zmberg

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

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

* fix: SidecarSet Expectations Leakage Bug Signed-off-by: wangwenchao7 <wangwenchao7@xiaomi.com> * fix: Modify the code as suggested Signed-off-by: wangwenchao7 <wangwenchao7@xiaomi.com> * fix: use ObserveUpdated func when update Signed-off-by: wangwenchao7 <wangwenchao7@xiaomi.com> * fix: observeUpdated before type check Signed-off-by: wangwenchao7 <wangwenchao7@xiaomi.com> --------- Signed-off-by: wangwenchao7 <wangwenchao7@xiaomi.com> Co-authored-by: wangwenchao7 <wangwenchao7@xiaomi.com>

* fix: SidecarSet Expectations Leakage Bug * fix: Modify the code as suggested * fix: use ObserveUpdated func when update * fix: observeUpdated before type check --------- Co-authored-by: wangwenchao7 <wangwenchao7@xiaomi.com>

kruise-bot requested review from Fei-Guo and zmberg May 29, 2023 06:37

kruise-bot added the size/L size/L: 100-499 label May 29, 2023

zmberg reviewed May 29, 2023

View reviewed changes

zmberg reviewed May 30, 2023

View reviewed changes

wangwu50 force-pushed the fix/sidecarset_expectations branch from 5f868e5 to 503a149 Compare May 30, 2023 04:03

wangwu50 and others added 3 commits May 30, 2023 16:03

fix: SidecarSet Expectations Leakage Bug

3dc47d3

Signed-off-by: wangwenchao7 <wangwenchao7@xiaomi.com>

fix: Modify the code as suggested

a8301db

Signed-off-by: wangwenchao7 <wangwenchao7@xiaomi.com>

fix: use ObserveUpdated func when update

79bd334

Signed-off-by: wangwenchao7 <wangwenchao7@xiaomi.com>

wangwu50 force-pushed the fix/sidecarset_expectations branch from 7a9a37e to 79bd334 Compare May 30, 2023 08:03

kruise-bot assigned zmberg May 30, 2023

kruise-bot added the lgtm label May 30, 2023

furykerry reviewed May 30, 2023

View reviewed changes

kruise-bot assigned furykerry May 30, 2023

veophi reviewed May 30, 2023

View reviewed changes

pkg/controller/sidecarset/sidecarset_pod_event_handler.go Outdated Show resolved Hide resolved

fix: observeUpdated before type check

dbb13ee

Signed-off-by: wangwenchao7 <wangwenchao7@xiaomi.com>

kruise-bot removed the lgtm label May 30, 2023

kruise-bot assigned veophi May 30, 2023

kruise-bot added the lgtm label May 30, 2023

kruise-bot added the approved label Jun 1, 2023

kruise-bot merged commit b7977a7 into openkruise:master Jun 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: SidecarSet Expectations Leakage Bug #1301

fix: SidecarSet Expectations Leakage Bug #1301

wangwu50 commented May 29, 2023

kruise-bot commented May 29, 2023

wangwu50 commented May 29, 2023

zmberg May 29, 2023

wangwu50 May 29, 2023

wangwu50 May 29, 2023

zmberg May 29, 2023

wangwu50 May 29, 2023

codecov-commenter commented May 29, 2023 •

edited

Loading

zmberg May 30, 2023

wangwu50 May 30, 2023

wangwu50 May 30, 2023

zmberg May 30, 2023

wangwu50 May 30, 2023

zmberg commented May 30, 2023

furykerry left a comment

veophi commented May 30, 2023

zmberg commented Jun 1, 2023

kruise-bot commented Jun 1, 2023

fix: SidecarSet Expectations Leakage Bug #1301

fix: SidecarSet Expectations Leakage Bug #1301

Conversation

wangwu50 commented May 29, 2023

kruise-bot commented May 29, 2023

wangwu50 commented May 29, 2023

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

codecov-commenter commented May 29, 2023 • edited Loading

Codecov Report

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

zmberg commented May 30, 2023

furykerry left a comment

Choose a reason for hiding this comment

veophi commented May 30, 2023

zmberg commented Jun 1, 2023

kruise-bot commented Jun 1, 2023

codecov-commenter commented May 29, 2023 •

edited

Loading