Fix a bug that ClusterSet status is no longer updated #5338
Conversation
luolanzone
added
action/backport
luolanzone
force-pushed
the
fix-mc-status
branch
2 times, most recently
from
709adb6 to
46e86d0
Compare
|
/test-multicluster-e2e |
|
In commit description:
Change ", revise" -> ". Revise". |
| if err != nil { | ||
| klog.ErrorS(err, "Failed to update ClusterSet's ClusterID", "name", namespacedName) | ||
| } | ||
| // Status will be updated in next interval after the ClusterSet spec is updated. |
There was a problem hiding this comment.
Is Status updated periodically? Why?
Why we cannot update Status right after the spec update?
Do we have a better place to update the spec with ClusterID, like when the controller first sees the ClusterSet?
1. r.Update() will only update Spec but not status. Revert the change back to update status only. 2. Update the ClusterSet's ClusterID in ClusterSet's reconciler. 3. Refine test codes for leader ClusterSet controller. Signed-off-by: Lan Luo <luola@vmware.com>
luolanzone
force-pushed
the
fix-mc-status
branch
from
46e86d0 to
3b9f4f1
Compare
| @@ -199,14 +212,7 @@ func (r *LeaderClusterSetReconciler) updateStatus() { | |||
| status.Conditions = []mcv1alpha2.ClusterSetCondition{overallCondition} | |||
There was a problem hiding this comment.
Just for my understanding, could you still answer my question if updateStatus() is called periodically and why?
There was a problem hiding this comment.
Hi @jianjuns updateStatus() will check the leader's connectivity status in the member cluster's ClusterSet controller, the status update action will be ignored if there is no change. The same function will check the member's connectivity through MemberClusterAnnounce in the leader cluster and the status will also not be updated if there is no change.
I think the main purpose is reflect the connectivity status between leader and member clusters.
|
Hi @jianjuns could you help to move forward? I'd like to back port this to v1.13. Thanks. |
|
/test-multicluster-e2e |
Please make sure all required tests are passed. |
|
@jianjuns sure, thanks for triggering all other tests, I double checked that all required tests are passed now. |
back to update status only.