AggregatedStatus in rb status may be deleted

[spiritNO1]

What happened:
1、execution_controller updates member workload fail because of conflict, AppliedFailed is added in work's status.

2、After returns error, execution_controller retries, and updates member workload success.

3、But when setting Applied condition to AppliedSuccessful in function updateAppliedCondition, if the work object in informer cache is not updated, controller will not set AppliedSuccessful.

4、Because of GenerationChangedPredicate event filter, new event will not come if only work's status changes.

5、appliedMessage will be added to rb status, and the status from member workload will be empty.

6、Host workload's status will be empty.

What you expected to happen:
Host workload's status will not be effected.

How to reproduce it (as minimally and precisely as possible):
Hard to reproduce

Anything else we need to know?:
I think maybe GenerationChangedPredicate can be removed from execution_controller?

Environment:

• Karmada version:
• kubectl-karmada or karmadactl version (the result of kubectl-karmada version or karmadactl version):
• Others:

[RainbowMango]

@spiritNO1 Thanks for reporting this. cc @XiShanYongYe-Chang for helping confirm this.

[XiShanYongYe-Chang]

Hi @spiritNO1 Are you referring to the situation where, when updating the work status in step 3, the work get from the cache still has a status condition of "AppliedSuccessful"? Is that correct?

[spiritNO1]

Hi @spiritNO1 Are you referring to the situation where, when updating the work status in step 3, the work get from the cache still has a status condition of "AppliedSuccessful"? Is that correct?

Yes, in this time, cache has not modified applied condition to AppliedFailed

[XiShanYongYe-Chang]

Yes, in this time, cache has not modified applied condition to AppliedFailed

Thanks for response, then I understand that there may not be any problem, because one of the fields is different. The LastTransitionTime will be updated, which will also cause the DeepEqual check to fail, thus allowing the update to continue. How do you think?

[spiritNO1]

Yes, in this time, cache has not modified applied condition to AppliedFailed

Thanks for response, then I understand that there may not be any problem, because one of the fields is different. The LastTransitionTime will be updated, which will also cause the DeepEqual check to fail, thus allowing the update to continue. How do you think?

Many thanks, and I find only status in condition changes can cause LastTransitionTime change.

We encountered this problem in the production environment. From the audit log, we can see that the second update of the condition did not occur. However, we only found the possible location by reading the code, but we don't have any confirmed evidence. I hope you can give me some advice, many thanks.

[XiShanYongYe-Chang]

and I find only status in condition changes can cause LastTransitionTime change.

You're right. This is indeed the issue here. Even if LastTransitionTime changes, since the Status remains the same, it will be considered that there is no change and thus no need for an update.

[XiShanYongYe-Chang]

From this perspective, is there an issue with the implementation of the SetStatusCondition function? It seems inconsistent with its own documented behavior.

[spiritNO1]

It seems inconsistent with its own documented behavior.

It seems inconsistent with its own documented behavior.
Did you mean its code comments?

The code comments is consistent.

And I want to know why GenerationChangedPredicate is added to execution_controller, are there other reasons besides performance issues? If it's just a performance issue, I think I can remove this eventfilter

[XiShanYongYe-Chang]

Did you mean its code comments?

Yes

And I want to know why GenerationChangedPredicate is added to execution_controller, are there other reasons besides performance issues? If it's just a performance issue, I think I can remove this eventfilter

For the execution controller, synchronization is done from top to bottom, so it only needs to be concerned with changes in the work specification, not with changes in the state. If removing GenerationChangedPredicate is merely to solve the current problem, it is unreasonable.

[spiritNO1]

#5318

This issue seems to have encountered a similar problem. We seem to be able to follow their solution and directly get object from the apiserver.

[XiShanYongYe-Chang]

This issue seems to have encountered a similar problem. We seem to be able to follow their solution and directly get object from the apiserver.

Yes, this is also one of the solutions we can consider. This solution can avoid the impact of slow caching, but it comes with the drawback of consuming additional apiserver requests.

[XiShanYongYe-Chang]

It seems inconsistent with its own documented behavior.

It seems inconsistent with its own documented behavior. Did you mean its code comments?

The code comments is consistent.

Another solution I thought of is whether we can maintain our own SetStatusCondition function, add judgment for LastTransitionTime.
What do you think?