Skip to content

Commit

Permalink
Merge pull request #5402 from XiShanYongYe-Chang/automated-cherry-pic…
Browse files Browse the repository at this point in the history 
…k-of-#5227-upstream-release-1.9

Automated cherry pick of #5227: fix error of when cluster status condition update
  • Loading branch information
@karmada-bot
karmada-bot authored Sep 2, 2024
2 parents 5082426 + 09771d4 commit 53b34fd
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 32 deletions.
65 changes: 48 additions & 17 deletions pkg/controllers/status/cluster_status_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,11 @@ func (c *ClusterStatusController) Reconcile(ctx context.Context, req controllerr
return controllerruntime.Result{Requeue: true}, nil
}

return c.syncClusterStatus(cluster)
err := c.syncClusterStatus(cluster)
if err != nil {
return controllerruntime.Result{}, err
}
return controllerruntime.Result{RequeueAfter: c.ClusterStatusUpdateFrequency.Duration}, nil
}

// SetupWithManager creates a controller and register to controller manager.
Expand All @@ -169,7 +173,7 @@ func (c *ClusterStatusController) SetupWithManager(mgr controllerruntime.Manager
}).Complete(c)
}

func (c *ClusterStatusController) syncClusterStatus(cluster *clusterv1alpha1.Cluster) (controllerruntime.Result, error) {
func (c *ClusterStatusController) syncClusterStatus(cluster *clusterv1alpha1.Cluster) error {
start := time.Now()
defer func() {
metrics.RecordClusterStatus(cluster)
Expand All @@ -182,7 +186,7 @@ func (c *ClusterStatusController) syncClusterStatus(cluster *clusterv1alpha1.Clu
clusterClient, err := c.ClusterClientSetFunc(cluster.Name, c.Client, c.ClusterClientOption)
if err != nil {
klog.Errorf("Failed to create a ClusterClient for the given member cluster: %v, err is : %v", cluster.Name, err)
return c.setStatusCollectionFailedCondition(cluster, currentClusterStatus, fmt.Sprintf("failed to create a ClusterClient: %v", err))
return setStatusCollectionFailedCondition(c.Client, cluster, fmt.Sprintf("failed to create a ClusterClient: %v", err))
}

online, healthy := getClusterHealthStatus(clusterClient)
Expand All @@ -193,8 +197,7 @@ func (c *ClusterStatusController) syncClusterStatus(cluster *clusterv1alpha1.Clu
if !online && readyCondition.Status != metav1.ConditionTrue {
klog.V(2).Infof("Cluster(%s) still offline after %s, ensuring offline is set.",
cluster.Name, c.ClusterFailureThreshold.Duration)
meta.SetStatusCondition(&currentClusterStatus.Conditions, *readyCondition)
return c.updateStatusIfNeeded(cluster, currentClusterStatus)
return updateStatusCondition(c.Client, cluster, *readyCondition)
}

// skip collecting cluster status if not ready
Expand All @@ -211,15 +214,13 @@ func (c *ClusterStatusController) syncClusterStatus(cluster *clusterv1alpha1.Clu
// can be safely removed from current controller.
c.initializeGenericInformerManagerForCluster(clusterClient)

err := c.setCurrentClusterStatus(clusterClient, cluster, &currentClusterStatus)
err = c.setCurrentClusterStatus(clusterClient, cluster, &currentClusterStatus)
if err != nil {
return controllerruntime.Result{Requeue: true}, err
return err
}
}

meta.SetStatusCondition(&currentClusterStatus.Conditions, *readyCondition)

return c.updateStatusIfNeeded(cluster, currentClusterStatus)
return c.updateStatusIfNeeded(cluster, currentClusterStatus, *readyCondition)
}

func (c *ClusterStatusController) setCurrentClusterStatus(clusterClient *util.ClusterClient, cluster *clusterv1alpha1.Cluster, currentClusterStatus *clusterv1alpha1.ClusterStatus) error {
Expand Down Expand Up @@ -266,22 +267,26 @@ func (c *ClusterStatusController) setCurrentClusterStatus(clusterClient *util.Cl
return nil
}

func (c *ClusterStatusController) setStatusCollectionFailedCondition(cluster *clusterv1alpha1.Cluster, currentClusterStatus clusterv1alpha1.ClusterStatus, message string) (controllerruntime.Result, error) {
func setStatusCollectionFailedCondition(c client.Client, cluster *clusterv1alpha1.Cluster, message string) error {
readyCondition := util.NewCondition(clusterv1alpha1.ClusterConditionReady, statusCollectionFailed, message, metav1.ConditionFalse)
meta.SetStatusCondition(&currentClusterStatus.Conditions, readyCondition)
return c.updateStatusIfNeeded(cluster, currentClusterStatus)
return updateStatusCondition(c, cluster, readyCondition)
}

// updateStatusIfNeeded calls updateStatus only if the status of the member cluster is not the same as the old status
func (c *ClusterStatusController) updateStatusIfNeeded(cluster *clusterv1alpha1.Cluster, currentClusterStatus clusterv1alpha1.ClusterStatus) (controllerruntime.Result, error) {
func (c *ClusterStatusController) updateStatusIfNeeded(cluster *clusterv1alpha1.Cluster, currentClusterStatus clusterv1alpha1.ClusterStatus, conditions ...metav1.Condition) error {
for _, condition := range conditions {
meta.SetStatusCondition(&currentClusterStatus.Conditions, condition)
}
if !equality.Semantic.DeepEqual(cluster.Status, currentClusterStatus) {
klog.V(4).Infof("Start to update cluster status: %s", cluster.Name)
err := retry.RetryOnConflict(retry.DefaultRetry, func() (err error) {
cluster.Status.KubernetesVersion = currentClusterStatus.KubernetesVersion
cluster.Status.APIEnablements = currentClusterStatus.APIEnablements
cluster.Status.Conditions = currentClusterStatus.Conditions
cluster.Status.NodeSummary = currentClusterStatus.NodeSummary
cluster.Status.ResourceSummary = currentClusterStatus.ResourceSummary
for _, condition := range conditions {
meta.SetStatusCondition(&cluster.Status.Conditions, condition)
}
updateErr := c.Status().Update(context.TODO(), cluster)
if updateErr == nil {
return nil
Expand All @@ -297,11 +302,37 @@ func (c *ClusterStatusController) updateStatusIfNeeded(cluster *clusterv1alpha1.
})
if err != nil {
klog.Errorf("Failed to update health status of the member cluster: %v, err is : %v", cluster.Name, err)
return controllerruntime.Result{Requeue: true}, err
return err
}
}

return controllerruntime.Result{RequeueAfter: c.ClusterStatusUpdateFrequency.Duration}, nil
return nil
}

func updateStatusCondition(c client.Client, cluster *clusterv1alpha1.Cluster, conditions ...metav1.Condition) error {
klog.V(4).Infof("Start to update cluster(%s) status condition", cluster.Name)
err := retry.RetryOnConflict(retry.DefaultRetry, func() (err error) {
for _, condition := range conditions {
meta.SetStatusCondition(&cluster.Status.Conditions, condition)
}
updateErr := c.Status().Update(context.TODO(), cluster)
if updateErr == nil {
return nil
}

updated := &clusterv1alpha1.Cluster{}
if err = c.Get(context.TODO(), client.ObjectKey{Namespace: cluster.Namespace, Name: cluster.Name}, updated); err == nil {
cluster = updated
} else {
klog.Errorf("Failed to get updated cluster %s: %v", cluster.Name, err)
}
return updateErr
})
if err != nil {
klog.Errorf("Failed to update status condition of the member cluster: %v, err is : %v", cluster.Name, err)
return err
}
return nil
}

func (c *ClusterStatusController) initializeGenericInformerManagerForCluster(clusterClient *util.ClusterClient) {
Expand Down
20 changes: 5 additions & 15 deletions pkg/controllers/status/cluster_status_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,9 +228,7 @@ func TestClusterStatusController_syncClusterStatus(t *testing.T) {
if err := c.Client.Create(context.Background(), cluster); err != nil {
t.Fatalf("Failed to create cluster: %v", err)
}
res, err := c.syncClusterStatus(cluster)
expect := controllerruntime.Result{}
assert.Equal(t, expect, res)
err := c.syncClusterStatus(cluster)
assert.Empty(t, err)
})
t.Run("online is false, readyCondition.Status isn't true", func(t *testing.T) {
Expand Down Expand Up @@ -274,10 +272,7 @@ func TestClusterStatusController_syncClusterStatus(t *testing.T) {
if err := c.Client.Create(context.Background(), cluster); err != nil {
t.Fatalf("Failed to create cluster: %v", err)
}

res, err := c.syncClusterStatus(cluster)
expect := controllerruntime.Result{}
assert.Equal(t, expect, res)
err := c.syncClusterStatus(cluster)
assert.Empty(t, err)
})

Expand Down Expand Up @@ -322,9 +317,7 @@ func TestClusterStatusController_syncClusterStatus(t *testing.T) {
if err := c.Client.Create(context.Background(), cluster); err != nil {
t.Fatalf("Failed to create cluster: %v", err)
}
res, err := c.syncClusterStatus(cluster)
expect := controllerruntime.Result{}
assert.Equal(t, expect, res)
err := c.syncClusterStatus(cluster)
assert.Empty(t, err)
})
}
Expand Down Expand Up @@ -913,8 +906,7 @@ func TestClusterStatusController_updateStatusIfNeeded(t *testing.T) {
ClusterClientSetFunc: util.NewClusterClientSet,
}

actual, err := c.updateStatusIfNeeded(cluster, currentClusterStatus)
assert.Equal(t, controllerruntime.Result{}, actual)
err := c.updateStatusIfNeeded(cluster, currentClusterStatus)
assert.Empty(t, err, "updateStatusIfNeeded returns error")
})

Expand Down Expand Up @@ -978,9 +970,7 @@ func TestClusterStatusController_updateStatusIfNeeded(t *testing.T) {
ClusterClientSetFunc: util.NewClusterClientSet,
}

actual, err := c.updateStatusIfNeeded(cluster, currentClusterStatus)
expect := controllerruntime.Result{Requeue: true}
assert.Equal(t, expect, actual)
err := c.updateStatusIfNeeded(cluster, currentClusterStatus)
assert.NotEmpty(t, err, "updateStatusIfNeeded doesn't return error")
})
}
Expand Down

0 comments on commit 53b34fd

Please sign in to comment.