Use Patch() when binding-status controller update workload's status #4094

zach593 · 2023-09-30T14:34:08Z

Change-Id: Ia0b5fde0b9d69f73aa6c2e3135e2590a6a503075

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
Discribed in #4093

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

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-controller-manager: use Patch() instead of Update() when updating workload status because error rate is too high.`

codecov-commenter · 2023-09-30T14:51:19Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (557348f) 53.48% compared to head (d423206) 52.86%.
Report is 63 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4094      +/-   ##
==========================================
- Coverage   53.48%   52.86%   -0.62%     
==========================================
  Files         234      239       +5     
  Lines       23304    23592     +288     
==========================================
+ Hits        12465    12473       +8     
- Misses      10157    10442     +285     
+ Partials      682      677       -5

Flag	Coverage Δ
unittests	`52.86% <86.20%> (-0.62%)`	⬇️

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

Files	Coverage Δ
pkg/util/helper/patch.go	`85.71% <100.00%> (+21.00%)`	⬆️
pkg/controllers/status/common.go	`0.00% <0.00%> (ø)`

... and 16 files with indirect coverage changes

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

zach593 · 2023-10-09T07:50:46Z

/retest

karmada-bot · 2023-10-09T07:51:08Z

@zach593: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

RainbowMango · 2023-10-09T08:25:17Z

Hi @zach593 Karmada hasn't enable /retest now, and it is tracked by #3421

The failing CI is due to a CI issue and it was resolved by #4107 yesterday. You might need to rebase this PR and push again.

zach593 · 2023-10-09T10:07:15Z

Cool, thanks for following this.

RainbowMango · 2023-10-12T09:12:51Z

cc @chaunceyjiang @XiShanYongYe-Chang Can you help with this?

XiShanYongYe-Chang · 2023-10-12T11:56:13Z

ok
/assign

XiShanYongYe-Chang · 2023-10-18T03:33:19Z

pkg/controllers/status/common.go

+	if newStatus != nil {
+		j, err := json.Marshal(newStatus)
+		if err != nil {
+			klog.Errorf("Failed to marshal status for resource(%s/%s/%s, Error: %v", gvr, resource.GetNamespace(), resource.GetName(), err)
+			return err
+		}
+		statusPatch = []byte(fmt.Sprintf(`[{"op": "replace", "path": "/status", "value": %s}]`, j))
+	} else {
+		statusPatch = []byte(`[{"op": "remove", "path": "/status"}]`)
+	}


Is it possible to consider using helper.GenMergePatch?

@XiShanYongYe-Chang The reason I used JSONPatchType instead of MergePatchType is that I am worried that some fields may not be deleted when patch status, because we do not understand and cannot modify the definition of resource templates and the logic of related controllers. We even provide a webhook so that users can customize the status.

If you think jsonpatch is acceptable, I can move this part of the construction logic to helper/patch.go (I didn't notice this file existed before); or if you think merge is still a better choice, Then I will update this part.

Obviously I should reply here...😆

I have done some research and conducted some tests, and it seems that MergePatch can completely replace JSONPatch. Therefore, I choose to use MergePatch here. We can monitor if anyone reports any issues in the future.

A reminder from my teammate (@lxtywypc) made me rethink this. What JSONPatch does is completely replace the status field, but MergePatch is content-based. This PR is intended to resolve update conflicts, which means we often keep outdated resource templates. If there are some field changes between this copy and the copy in etcd (which has not been "watched" yet), then patch will set the wrong status into etcd.

So I guess we have to move to JSONPatch... I apologize for my capriciousness.

Never mind. Full discussion and testing gave us a much better result 👍

Updated. kindly ping @XiShanYongYe-Chang to review again.

Thank you for your explanation, I probably understand what you mean, can you help write an example to describe it, so that it is easier to follow?

A simple example could be like this: playground.
If everything works on time but the informer holding an outdated resource template (currentA), using mergePatch may create an inappropriate mergePatchBytes for actual template object (currentB).

Change-Id: I531e40fa99e657af3c060e59c13c47a835356274 Signed-off-by: zach593 <zach_li@outlook.com>

XiShanYongYe-Chang

Thanks @zach593.
I'm sorry for replying late~

pkg/controllers/status/common.go

XiShanYongYe-Chang · 2023-10-27T03:46:36Z

pkg/controllers/status/common.go

 		klog.V(3).Infof("Ignore update resource(%s/%s/%s) status as up to date.", gvr, resource.GetNamespace(), resource.GetName())
 		return nil
 	}

-	if _, err = dynamicClient.Resource(gvr).Namespace(resource.GetNamespace()).UpdateStatus(context.TODO(), newObj, metav1.UpdateOptions{}); err != nil {
-		klog.Errorf("Failed to update resource(%s/%s/%s), Error: %v", gvr, resource.GetNamespace(), resource.GetName(), err)
+	patchBytes, err := helper.GenReplaceFieldJSONPatch("/status", oldStatus, newStatus)


It might be a bit of a duplicate of the question above, for resources without status, can the program execute up to here?

It will return []byte(`[]`), nil, []byte(`[]`) representing an empty jsonpatch, which will be safely passed to apiserver when patching, the objects we patch will always no change. You can test it with kubectl.

Saw that GenMergePatch() returns nil if patchBytes == "{}", so I followed this, returned nil when no patching is required.

Just a question, can we use retry.RetryOnConflict() ?

It could reduce the reconciliation error rate, but can't reduce the HTTP PUT error rate. I feel that it is difficult to convince users that a controller’s HTTP update error rate for a certain type of resource consistently above 30% is within the normal range...

I don't see how to prevent empty patches from being sent to apiserver.

there's a DeepEqual() judge in line 77

okay, thanks for reminding me. can you make the patch more precise?

status: availableReplicas: 1 conditions: - lastTransitionTime: "2023-12-05T09:25:15Z" lastUpdateTime: "2023-12-05T09:25:15Z" message: Deployment has minimum availability. reason: MinimumReplicasAvailable status: "True" type: Available - lastTransitionTime: "2023-11-21T11:43:12Z" lastUpdateTime: "2023-12-05T09:25:15Z" message: ReplicaSet "demo-57755f7c74" has successfully progressed. reason: NewReplicaSetAvailable status: "True" type: Progressing observedGeneration: 6 readyReplicas: 0 replicas: 1 updatedReplicas: 1

if only the field readyReplicas changes from 0 to 1, we'd better use such a patch rather than the entire status:

[{ "op":"replace", "path":"/status/readyReplicas", "value":1 }]

I believe implementing such functionality can indeed make the final patch bytes appear very concise in certain situations (as you illustrated). However, it may introduce other issues: if there's a need to update multiple fields, should a separate patch operation be written for each field? If it's necessary to consolidate certain patch operations into one, what is the appropriate timing for the consolidation?

Implementing it this way seems like using JSONPatch to emulate MergePatch. If the goal is simplicity in patch operations, it might prove to be more trouble than it's worth in many scenarios, and the implementation is likely to be less elegant.

If the concern is that other controllers might modify the status, as mentioned in #4093 (comment), I believe this possibility is low. In the current implementation, .status is returned by the interpreterWebhook, and users have the opportunity to introduce other mechanisms for custom handling.

pkg/util/helper/patch_test.go

pkg/util/helper/patch.go

XiShanYongYe-Chang

Thanks a lot~
Can we then use this logic for ResourceBinding and Work status updates?

/lgtm
/cc @RainbowMango

Change-Id: I68a9b2d293c35e10f287fdc4a625ded3da4848ba Signed-off-by: zach593 <zach_li@outlook.com>

zach593 · 2023-11-06T05:47:41Z

Updated some UT test name.. @XiShanYongYe-Chang

While I feel that a controller that takes ownership of an object should not cause conflict errors when updating an object, and there seems to be no obvious problem with using Patch() to replace Update(), extending the use of Patch() may still require more discussion.

XiShanYongYe-Chang · 2023-11-06T06:09:01Z

I got it, thanks @zach593
/lgtm

RainbowMango

/assign

XiShanYongYe-Chang · 2023-12-02T09:01:01Z

Kindly ping @RainbowMango

RainbowMango · 2023-12-04T03:35:40Z

Invite controller and util owers to help with it:
cc @chaunceyjiang @whitewindmills

chaunceyjiang · 2023-12-05T10:04:14Z

/assign

chaunceyjiang · 2023-12-06T02:56:31Z

Nice!!!

/lgtm

RainbowMango · 2023-12-07T10:07:26Z

Hi @whitewindmills Do you have any comments?

whitewindmills · 2023-12-07T10:18:22Z

/lgtm

RainbowMango

/approve

karmada-bot · 2023-12-09T02:10:11Z

[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:

~~pkg/controllers/OWNERS~~ [RainbowMango]
~~pkg/util/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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Sep 30, 2023

karmada-bot requested review from mrlihanbo and XiShanYongYe-Chang September 30, 2023 14:34

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

zach593 mentioned this pull request Sep 30, 2023

Binding-status controller update workload's status error rate too high #4093

Closed

zach593 force-pushed the fix-binding-status-error-rate branch 3 times, most recently from a338ebc to f4828cb Compare October 12, 2023 07:40

karmada-bot assigned XiShanYongYe-Chang Oct 12, 2023

XiShanYongYe-Chang reviewed Oct 18, 2023

View reviewed changes

zach593 force-pushed the fix-binding-status-error-rate branch 3 times, most recently from 14d6534 to 5aac08a Compare October 20, 2023 02:22

karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 20, 2023

improve update workload status error rate too high

8520227

Change-Id: I531e40fa99e657af3c060e59c13c47a835356274 Signed-off-by: zach593 <zach_li@outlook.com>

zach593 force-pushed the fix-binding-status-error-rate branch from 5aac08a to 8520227 Compare October 20, 2023 02:28

XiShanYongYe-Chang reviewed Oct 27, 2023

View reviewed changes

pkg/util/helper/patch.go Outdated Show resolved Hide resolved

XiShanYongYe-Chang reviewed Oct 27, 2023

View reviewed changes

pkg/util/helper/patch.go Show resolved Hide resolved

zach593 force-pushed the fix-binding-status-error-rate branch 2 times, most recently from a92e5fc to e79f44c Compare November 5, 2023 09:52

XiShanYongYe-Chang reviewed Nov 6, 2023

View reviewed changes

karmada-bot requested a review from RainbowMango November 6, 2023 03:47

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

update details and UT

d423206

Change-Id: I68a9b2d293c35e10f287fdc4a625ded3da4848ba Signed-off-by: zach593 <zach_li@outlook.com>

zach593 force-pushed the fix-binding-status-error-rate branch from e79f44c to d423206 Compare November 6, 2023 05:41

karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 6, 2023

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

RainbowMango reviewed Nov 6, 2023

View reviewed changes

karmada-bot assigned RainbowMango Nov 6, 2023

XiShanYongYe-Chang mentioned this pull request Dec 2, 2023

Fixed occasional errors with clusteraffinities #3518

Closed

karmada-bot assigned chaunceyjiang Dec 5, 2023

karmada-bot assigned whitewindmills Dec 7, 2023

RainbowMango reviewed Dec 9, 2023

View reviewed changes

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

karmada-bot merged commit 33fdf52 into karmada-io:master Dec 9, 2023
12 checks passed

chaosi-zju mentioned this pull request Jul 31, 2024

Expected patch operations may be missed when AggregateStatus #5285

Closed

Use Patch() when binding-status controller update workload's status #4094

Use Patch() when binding-status controller update workload's status #4094

Conversation

zach593 commented Sep 30, 2023 • edited Loading

codecov-commenter commented Sep 30, 2023 • edited Loading

Codecov Report

zach593 commented Oct 9, 2023

karmada-bot commented Oct 9, 2023

RainbowMango commented Oct 9, 2023

zach593 commented Oct 9, 2023

RainbowMango commented Oct 12, 2023

XiShanYongYe-Chang commented Oct 12, 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

zach593 Oct 19, 2023 • edited Loading

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

zach593 Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

zach593 commented Nov 6, 2023

XiShanYongYe-Chang commented Nov 6, 2023

RainbowMango left a comment

Choose a reason for hiding this comment

XiShanYongYe-Chang commented Dec 2, 2023

RainbowMango commented Dec 4, 2023

chaunceyjiang commented Dec 5, 2023

chaunceyjiang commented Dec 6, 2023

RainbowMango commented Dec 7, 2023

whitewindmills commented Dec 7, 2023

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Dec 9, 2023

zach593 commented Sep 30, 2023 •

edited

Loading

codecov-commenter commented Sep 30, 2023 •

edited

Loading

zach593 Oct 19, 2023 •

edited

Loading

zach593 Dec 6, 2023 •

edited

Loading