Revert "Revert "Merge pull request #3808 from ctripcloud/refactor-execution-workstatus"" #4227

lxtywypc · 2023-11-13T10:36:33Z

This reverts commit 67de65d.

What type of PR is this?
/kind feature

What this PR does / why we need it:
After #4080 , we could bring #3808 back for #3959 (comment).

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

karmada-bot · 2023-11-13T10:36:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign kevin-wangzefeng after the PR has been reviewed.
You can assign the PR to them by writing /assign @kevin-wangzefeng in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

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

…factor-execution-workstatus"" This reverts commit 67de65d. Signed-off-by: lxtywypc <[email protected]>

XiShanYongYe-Chang · 2023-11-14T07:49:25Z

Ask for some comments from @RainbowMango and @chaunceyjiang .

chaunceyjiang · 2023-11-21T11:23:08Z

pkg/controllers/status/work_status_controller.go

+
+func (c *WorkStatusController) onUpdate(old, cur interface{}) {
+	// Still need to compare the whole object because the interpreter is able to take any part of object to reflect status.
+	if !reflect.DeepEqual(old, cur) {


The resourceVersion is always unequal, so it seems like it's always true here, right?

lxtywypc · 2023-11-22T04:14:48Z

Humm...Do you think it is necessary to split this pr into small parts to do a deep review again?
@XiShanYongYe-Chang @chaunceyjiang @RainbowMango

If you think so, I'm glad to take a try this time.

XiShanYongYe-Chang · 2023-11-22T06:13:28Z

Humm...Do you think it is necessary to split this pr into small parts to do a deep review again?

Hi @lxtywypc, I have communicated with @RainbowMango about this PR before. He may be more interested in knowing what benefits we can bring by doing this, besides the current way of modification. Are there any other means to achieve this goal?

Perhaps some additional information can be provided from this perspective.

lxtywypc · 2023-11-22T11:13:17Z

Hi @XiShanYongYe-Chang .

As what I described in #3707, both execution-controller and workstatus-controller could modify the workload in member cluster but only execution-controller could update the condition in work, that would causes wrong work condition sometimes.

The workload might be updated frequentely during releasing, while the status might changes more often. When binding-controller, execution-controller and workstatus-controller all try to modify the work by Update, the conflict occurs easily, and plently.

The conflict would be reduced if we change the way to update status from Update to Patch, but if both controllers could modify the workload and update the condition, the condition might have a flip because of the confilct on modifying the workload in member cluster.

And there might be plenty of useless updating attempts if the two controllers try to update workload at the same time.

So the pr is to make the controllers perform on their own duties, that would make the workflow more clear, and reduce the conflict and useless updating attemts.

XiShanYongYe-Chang · 2023-11-23T01:57:16Z

Thanks @lxtywypc. I understand this point.
Ask some ideas from @RainbowMango and @chaunceyjiang
/cc @RainbowMango @chaunceyjiang

RainbowMango · 2023-11-28T08:53:24Z

@lxtywypc Thanks. I can feel the pain point as I commented at #4282 (comment). I'll look at ASAP. Thanks.

zhzhuang-zju

Hello, thank you for your important commit! Review is ongoing, and my viewpoint just for reference only.

zhzhuang-zju · 2023-12-04T07:49:08Z

pkg/controllers/execution/execution_controller_test.go

@@ -0,0 +1,919 @@
+package execution


You may need to add license at the beginning of the file

zhzhuang-zju · 2023-12-04T07:56:12Z

pkg/controllers/status/work_status_controller.go

-	// in member clusters.
-	needUpdate, err := c.ObjectWatcher.NeedsUpdate(clusterName, desiredObj, observedObj)
-	if err != nil {
+	if !helper.IsResourceApplied(&workObject.Status) {


Is it not necessary to determine needUpdate before refreshing the manifeststatus, and does this lead to data conflicts?

Previously, my idea was to judge if an update is needed before sync work status, and if needUpdate, then not sync work status first. like

if needUpdate { klog.Infof("%s need to be updated, sync work status later", fedKey.String()) return nil }

But now I understand that sync work status will be triggered before and after update, and workQueue will process resource asynchronously, so I think your choice to not judge needUpdate is a little better.

zhzhuang-zju · 2023-12-04T08:04:39Z

pkg/controllers/execution/execution_controller.go

+	// needUpdateCondition judges if the Applied condition needs to update.
+	needUpdateCondition := func() bool {
+		lastWorkAppliedCondition := meta.FindStatusCondition(work.Status.Conditions, workv1alpha1.WorkApplied).DeepCopy()
+


This is to skip unnecessary update operations. Would it be simpler to compare the values before and after line L401 to determine if an update is needed?

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 13, 2023

karmada-bot requested review from Poor12, XiShanYongYe-Chang, chaunceyjiang, ikaven1024 and lonelyCZ November 13, 2023 10:36

karmada-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 13, 2023

Revert "Revert "Merge pull request karmada-io#3808 from ctripcloud/re…

0c79325

…factor-execution-workstatus"" This reverts commit 67de65d. Signed-off-by: lxtywypc <[email protected]>

lxtywypc force-pushed the revert-4016 branch from 4ecd192 to 0c79325 Compare November 13, 2023 10:39

chaunceyjiang reviewed Nov 21, 2023

View reviewed changes

karmada-bot requested review from RainbowMango and chaunceyjiang November 23, 2023 01:57

RainbowMango mentioned this pull request Nov 28, 2023

After a resource is deleted, the resource fails to be delivered again. The work corresponding to the resource is not in the synchronization failure state. #4282

Closed

RainbowMango added this to the v1.9 milestone Nov 28, 2023

zhzhuang-zju reviewed Dec 19, 2023

View reviewed changes

RainbowMango modified the milestones: v1.9, v1.10 Feb 29, 2024

RainbowMango modified the milestones: v1.10, v1.11 May 29, 2024

RainbowMango modified the milestones: v1.11, v1.12 Aug 28, 2024

RainbowMango modified the milestones: v1.12, v1.13 Nov 28, 2024

RainbowMango modified the milestones: v1.13, v1.14 Apr 28, 2025

RainbowMango modified the milestones: v1.14, v1.15 Jul 8, 2025

RainbowMango modified the milestones: v1.15, v1.17 Oct 15, 2025

Revert "Revert "Merge pull request #3808 from ctripcloud/refactor-execution-workstatus"" #4227

Are you sure you want to change the base?

Revert "Revert "Merge pull request #3808 from ctripcloud/refactor-execution-workstatus"" #4227

Uh oh!

Conversation

lxtywypc commented Nov 13, 2023

Uh oh!

karmada-bot commented Nov 13, 2023

Uh oh!

XiShanYongYe-Chang commented Nov 14, 2023

Uh oh!

chaunceyjiang Nov 21, 2023

Choose a reason for hiding this comment

Uh oh!

lxtywypc commented Nov 22, 2023

Uh oh!

XiShanYongYe-Chang commented Nov 22, 2023

Uh oh!

lxtywypc commented Nov 22, 2023

Uh oh!

XiShanYongYe-Chang commented Nov 23, 2023

Uh oh!

RainbowMango commented Nov 28, 2023

Uh oh!

zhzhuang-zju left a comment

Choose a reason for hiding this comment

Uh oh!

zhzhuang-zju Dec 4, 2023

Choose a reason for hiding this comment

Uh oh!

zhzhuang-zju Dec 4, 2023

Choose a reason for hiding this comment

Uh oh!

zhzhuang-zju Dec 19, 2023

Choose a reason for hiding this comment

Uh oh!

zhzhuang-zju Dec 4, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants