Skip to content

Commit

Permalink
Merge pull request #5227 from XiShanYongYe-Chang/fix-remedy-ci-failed
Browse files Browse the repository at this point in the history 
fix the error of cluster status old condition update will overwrite the newest condition
  • Loading branch information
@karmada-bot
karmada-bot authored Aug 19, 2024
2 parents c757122 + b59761e commit 7307e2e
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 31 deletions.
57 changes: 40 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(ctx, cluster)
err := c.syncClusterStatus(ctx, 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(ctx context.Context, cluster *clusterv1alpha1.Cluster) (controllerruntime.Result, error) {
func (c *ClusterStatusController) syncClusterStatus(ctx context.Context, cluster *clusterv1alpha1.Cluster) error {
start := time.Now()
defer func() {
metrics.RecordClusterStatus(cluster)
Expand All @@ -182,7 +186,7 @@ func (c *ClusterStatusController) syncClusterStatus(ctx context.Context, cluster
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(ctx, cluster, currentClusterStatus, fmt.Sprintf("failed to create a ClusterClient: %v", err))
return setStatusCollectionFailedCondition(ctx, 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(ctx context.Context, cluster
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(ctx, cluster, currentClusterStatus)
return updateStatusCondition(ctx, c.Client, cluster, *readyCondition)
}

// skip collecting cluster status if not ready
Expand All @@ -211,15 +214,13 @@ func (c *ClusterStatusController) syncClusterStatus(ctx context.Context, cluster
// 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{}, err
return err
}
}

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

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

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

func (c *ClusterStatusController) setStatusCollectionFailedCondition(ctx context.Context, cluster *clusterv1alpha1.Cluster, currentClusterStatus clusterv1alpha1.ClusterStatus, message string) (controllerruntime.Result, error) {
func setStatusCollectionFailedCondition(ctx context.Context, 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(ctx, cluster, currentClusterStatus)
return updateStatusCondition(ctx, 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(ctx context.Context, cluster *clusterv1alpha1.Cluster, currentClusterStatus clusterv1alpha1.ClusterStatus) (controllerruntime.Result, error) {
func (c *ClusterStatusController) updateStatusIfNeeded(ctx context.Context, 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) {
_, err = helper.UpdateStatus(ctx, c.Client, cluster, func() 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)
}
return nil
})
return err
})
if err != nil {
klog.Errorf("Failed to update health status of the member cluster: %v, err is : %v", cluster.Name, err)
return controllerruntime.Result{}, err
return err
}
}

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

func updateStatusCondition(ctx context.Context, 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) {
_, err = helper.UpdateStatus(ctx, c, cluster, func() error {
for _, condition := range conditions {
meta.SetStatusCondition(&cluster.Status.Conditions, condition)
}
return nil
})
return err
})
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
19 changes: 5 additions & 14 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(context.Background(), cluster)
expect := controllerruntime.Result{}
assert.Equal(t, expect, res)
err := c.syncClusterStatus(context.Background(), cluster)
assert.Empty(t, err)
})
t.Run("online is false, readyCondition.Status isn't true", func(t *testing.T) {
Expand Down Expand Up @@ -275,9 +273,7 @@ func TestClusterStatusController_syncClusterStatus(t *testing.T) {
t.Fatalf("Failed to create cluster: %v", err)
}

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

Expand Down Expand Up @@ -322,9 +318,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(context.Background(), cluster)
expect := controllerruntime.Result{}
assert.Equal(t, expect, res)
err := c.syncClusterStatus(context.Background(), cluster)
assert.Empty(t, err)
})
}
Expand Down Expand Up @@ -913,8 +907,7 @@ func TestClusterStatusController_updateStatusIfNeeded(t *testing.T) {
ClusterClientSetFunc: util.NewClusterClientSet,
}

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

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

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

0 comments on commit 7307e2e

Please sign in to comment.