Fixed an issue where pub webhooks unexpectedly return error when PUB is NOT FOUND

[opencmit2]

…is NOT FOUND

Ⅰ. Describe what this PR does

Fixed an issue #1567 where pub webhooks unexpectedly return error when PUB is NOT FOUND

Ⅱ. Does this pull request fix one issue?

fixes #1567

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

[kruise-bot]

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

[codecov]

Codecov Report

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

Project coverage is 48.52%. Comparing base (0d0031a) to head (0fa5f26).
Report is 19 commits behind head on master.

Files	Patch %	Lines
pkg/control/pubcontrol/pub_control_utils.go	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1579      +/-   ##
==========================================
+ Coverage   47.91%   48.52%   +0.60%     
==========================================
  Files         162      163       +1     
  Lines       23491    18903    -4588     
==========================================
- Hits        11256     9172    -2084     
+ Misses      11014     8510    -2504     
  Partials     1221     1221              

Flag	Coverage Δ
unittests	48.52% <0.00%> (+0.60%)	⬆️

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.

[furykerry]

if errors.IsNotFound(err), code will goes to the else clause, can the code just return so as to avoid unnecessary and potentially problematic code?

[opencmit2]

Do you mean to return nil directly?

[Spground]

We also should check error returned in pub status updating, just ingore error if NotFound error retured.

kruise/pkg/control/pubcontrol/pub_control_utils.go

Line 165 in 1424581

err = kclient.Status().Update(context.TODO(), pubClone)

[opencmit2]

@Spground
Adding this feels a bit redundant since PUB exits without being discovered and does not continue running kclient.Status().Update

If you need to add it, is the following suitable?

                err = kclient.Status().Update(context.TODO(), pubClone)
                costOfUpdate += time.Since(start)
                if err == nil {
                        if err = util.GlobalCache.Add(pubClone); err != nil {
                                klog.ErrorS(err, "Failed to add cache for podUnavailableBudget", "pub", klog.KObj(pub))
                        }
                        return nil
                } else {
                        if errors.IsNotFound(err) {
                                return nil
                        }
                }

[Spground]

@Spground Adding this feels a bit redundant since PUB exits without being discovered and does not continue running kclient.Status().Update

If you need to add it, is the following suitable?

                err = kclient.Status().Update(context.TODO(), pubClone)
                costOfUpdate += time.Since(start)
                if err == nil {
                        if err = util.GlobalCache.Add(pubClone); err != nil {
                                klog.ErrorS(err, "Failed to add cache for podUnavailableBudget", "pub", klog.KObj(pub))
                        }
                        return nil
                } else {
                        if errors.IsNotFound(err) {
                                return nil
                        }
                }

It's not exactly. PUB can be deleted anytime, e.g, before you UPDATE Status and after GET pub.

[opencmit2]

@Spground ok,IsNotFound has been added to both locations. Do I still need get informerCached to make IsNotFound judgment?

[furykerry]

/lgtm

[zmberg]

/approve

[kruise-bot]

[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