refactor the func and event of taint cluster

Signed-off-by: Poor12 <shentiecheng@huawei.com>
Poor12 · Jul 31, 2023 · b2ac1b2 · b2ac1b2
1 parent f160ea7
commit b2ac1b2
Show file tree

Hide file tree

Showing 5 changed files with 81 additions and 59 deletions.
diff --git a/pkg/controllers/cluster/cluster_controller.go b/pkg/controllers/cluster/cluster_controller.go
@@ -3,6 +3,8 @@ package cluster
 import (
 	"context"
 	"fmt"
+	"reflect"
+	"strings"
 	"sync"
 	"time"
 
@@ -212,7 +214,6 @@ func (c *Controller) syncCluster(ctx context.Context, cluster *clusterv1alpha1.C
 	// taint cluster by condition
 	err = c.taintClusterByCondition(ctx, cluster)
 	if err != nil {
-		c.EventRecorder.Event(cluster, corev1.EventTypeWarning, events.EventReasonTaintClusterByConditionFailed, err.Error())
 		return controllerruntime.Result{Requeue: true}, err
 	}
 
@@ -222,8 +223,7 @@ func (c *Controller) syncCluster(ctx context.Context, cluster *clusterv1alpha1.C
 
 func (c *Controller) removeCluster(ctx context.Context, cluster *clusterv1alpha1.Cluster) (controllerruntime.Result, error) {
 	// add terminating taint before cluster is deleted
-	if err := utilhelper.UpdateClusterControllerTaint(ctx, c.Client, []*corev1.Taint{TerminatingTaintTemplate}, nil, cluster); err != nil {
-		c.EventRecorder.Event(cluster, corev1.EventTypeWarning, events.EventReasonRemoveTargetClusterFailed, err.Error())
+	if err := c.updateClusterTaints(ctx, []*corev1.Taint{TerminatingTaintTemplate}, nil, cluster); err != nil {
 		klog.ErrorS(err, "Failed to update terminating taint", "cluster", cluster.Name)
 		return controllerruntime.Result{Requeue: true}, err
 	}
@@ -570,20 +570,20 @@ func (c *Controller) processTaintBaseEviction(ctx context.Context, cluster *clus
 		if features.FeatureGate.Enabled(features.Failover) && decisionTimestamp.After(clusterHealth.readyTransitionTimestamp.Add(c.FailoverEvictionTimeout)) {
 			// We want to update the taint straight away if Cluster is already tainted with the UnreachableTaint
 			taintToAdd := *NotReadyTaintTemplate
-			if err := utilhelper.UpdateClusterControllerTaint(ctx, c.Client, []*corev1.Taint{&taintToAdd}, []*corev1.Taint{UnreachableTaintTemplate}, cluster); err != nil {
+			if err := c.updateClusterTaints(ctx, []*corev1.Taint{&taintToAdd}, []*corev1.Taint{UnreachableTaintTemplate}, cluster); err != nil {
 				klog.ErrorS(err, "Failed to instantly update UnreachableTaint to NotReadyTaint, will try again in the next cycle.", "cluster", cluster.Name)
 			}
 		}
 	case metav1.ConditionUnknown:
 		if features.FeatureGate.Enabled(features.Failover) && decisionTimestamp.After(clusterHealth.probeTimestamp.Add(c.FailoverEvictionTimeout)) {
 			// We want to update the taint straight away if Cluster is already tainted with the UnreachableTaint
 			taintToAdd := *UnreachableTaintTemplate
-			if err := utilhelper.UpdateClusterControllerTaint(ctx, c.Client, []*corev1.Taint{&taintToAdd}, []*corev1.Taint{NotReadyTaintTemplate}, cluster); err != nil {
+			if err := c.updateClusterTaints(ctx, []*corev1.Taint{&taintToAdd}, []*corev1.Taint{NotReadyTaintTemplate}, cluster); err != nil {
 				klog.ErrorS(err, "Failed to instantly swap NotReadyTaint to UnreachableTaint, will try again in the next cycle.", "cluster", cluster.Name)
 			}
 		}
 	case metav1.ConditionTrue:
-		if err := utilhelper.UpdateClusterControllerTaint(ctx, c.Client, nil, []*corev1.Taint{NotReadyTaintTemplate, UnreachableTaintTemplate}, cluster); err != nil {
+		if err := c.updateClusterTaints(ctx, nil, []*corev1.Taint{NotReadyTaintTemplate, UnreachableTaintTemplate}, cluster); err != nil {
 			klog.ErrorS(err, "Failed to remove taints from cluster, will retry in next iteration.", "cluster", cluster.Name)
 		}
 	}
@@ -597,24 +597,71 @@ func (c *Controller) taintClusterByCondition(ctx context.Context, cluster *clust
 		switch currentReadyCondition.Status {
 		case metav1.ConditionFalse:
 			// Add NotReadyTaintTemplateForSched taint immediately.
-			if err = utilhelper.UpdateClusterControllerTaint(ctx, c.Client, []*corev1.Taint{NotReadyTaintTemplateForSched}, []*corev1.Taint{UnreachableTaintTemplateForSched}, cluster); err != nil {
+			if err = c.updateClusterTaints(ctx, []*corev1.Taint{NotReadyTaintTemplateForSched}, []*corev1.Taint{UnreachableTaintTemplateForSched}, cluster); err != nil {
 				klog.ErrorS(err, "Failed to instantly update UnreachableTaintForSched to NotReadyTaintForSched, will try again in the next cycle.", "cluster", cluster.Name)
 			}
 		case metav1.ConditionUnknown:
 			// Add UnreachableTaintTemplateForSched taint immediately.
-			if err = utilhelper.UpdateClusterControllerTaint(ctx, c.Client, []*corev1.Taint{UnreachableTaintTemplateForSched}, []*corev1.Taint{NotReadyTaintTemplateForSched}, cluster); err != nil {
+			if err = c.updateClusterTaints(ctx, []*corev1.Taint{UnreachableTaintTemplateForSched}, []*corev1.Taint{NotReadyTaintTemplateForSched}, cluster); err != nil {
 				klog.ErrorS(err, "Failed to instantly swap NotReadyTaintForSched to UnreachableTaintForSched, will try again in the next cycle.", "cluster", cluster.Name)
 			}
 		case metav1.ConditionTrue:
-			if err = utilhelper.UpdateClusterControllerTaint(ctx, c.Client, nil, []*corev1.Taint{NotReadyTaintTemplateForSched, UnreachableTaintTemplateForSched}, cluster); err != nil {
+			if err = c.updateClusterTaints(ctx, nil, []*corev1.Taint{NotReadyTaintTemplateForSched, UnreachableTaintTemplateForSched}, cluster); err != nil {
 				klog.ErrorS(err, "Failed to remove schedule taints from cluster, will retry in next iteration.", "cluster", cluster.Name)
 			}
 		}
 	} else {
 		// Add NotReadyTaintTemplateForSched taint immediately.
-		if err = utilhelper.UpdateClusterControllerTaint(ctx, c.Client, []*corev1.Taint{NotReadyTaintTemplateForSched}, nil, cluster); err != nil {
+		if err = c.updateClusterTaints(ctx, []*corev1.Taint{NotReadyTaintTemplateForSched}, nil, cluster); err != nil {
 			klog.ErrorS(err, "Failed to add a NotReady taint to the newly added cluster, will try again in the next cycle.", "cluster", cluster.Name)
 		}
 	}
 	return err
 }
+
+func (c *Controller) updateClusterTaints(ctx context.Context, taintsToAdd, taintsToRemove []*corev1.Taint, cluster *clusterv1alpha1.Cluster) error {
+	taints := utilhelper.SetCurrentClusterTaints(taintsToAdd, taintsToRemove, cluster)
+	if reflect.DeepEqual(taints, cluster.Spec.Taints) {
+		return nil
+	}
+
+	cluster = cluster.DeepCopy()
+	cluster.Spec.Taints = taints
+	err := c.Client.Update(ctx, cluster)
+	if err != nil {
+		c.EventRecorder.Event(cluster, corev1.EventTypeWarning, events.EventReasonTaintClusterFailed, err.Error())
+		return err
+	}
+	msg := fmt.Sprintf("Taint cluster succeed: %s.", generateEventMessage(taints))
+	c.EventRecorder.Event(cluster, corev1.EventTypeNormal, events.EventReasonTaintClusterSucceed, msg)
+	return nil
+}
+
+func generateEventMessage(taints []corev1.Taint) string {
+	if len(taints) == 0 {
+		return "cluster now does not have taints"
+	}
+
+	var msg string
+	for i, taint := range taints {
+		if i != 0 {
+			msg += ","
+		}
+		if taint.Value != "" {
+			msg += strings.Join([]string{`{`,
+				`Key:` + fmt.Sprintf("%v", taint.Key) + `,`,
+				`Value:` + fmt.Sprintf("%v", taint.Value) + `,`,
+				`Effect:` + fmt.Sprintf("%v", taint.Effect),
+				`}`,
+			}, "")
+		} else {
+			msg += strings.Join([]string{`{`,
+				`Key:` + fmt.Sprintf("%v", taint.Key) + `,`,
+				`Effect:` + fmt.Sprintf("%v", taint.Effect),
+				`}`,
+			}, "")
+		}
+	}
+
+	return fmt.Sprintf("cluster now has taints([%s])", msg)
+}
diff --git a/pkg/events/events.go b/pkg/events/events.go
@@ -10,10 +10,10 @@ const (
 	EventReasonRemoveExecutionSpaceFailed = "RemoveExecutionSpaceFailed"
 	// EventReasonRemoveExecutionSpaceSucceed indicates that remove execution space succeed.
 	EventReasonRemoveExecutionSpaceSucceed = "RemoveExecutionSpaceSucceed"
-	// EventReasonTaintClusterByConditionFailed indicates that taint cluster by condition failed.
-	EventReasonTaintClusterByConditionFailed = "TaintClusterByConditionFailed"
-	// EventReasonRemoveTargetClusterFailed indicates that failed to remove target cluster from binding.
-	EventReasonRemoveTargetClusterFailed = "RemoveTargetClusterFailed"
+	// EventReasonTaintClusterFailed indicates that taint cluster failed.
+	EventReasonTaintClusterFailed = "TaintClusterFailed"
+	// EventReasonTaintClusterSucceed indicates that taint cluster succeed.
+	EventReasonTaintClusterSucceed = "TaintClusterSucceed"
 	// EventReasonSyncImpersonationConfigSucceed indicates that sync impersonation config succeed.
 	EventReasonSyncImpersonationConfigSucceed = "SyncImpersonationConfigSucceed"
 	// EventReasonSyncImpersonationConfigFailed indicates that sync impersonation config failed.

diff --git a/pkg/util/helper/taint.go b/pkg/util/helper/taint.go
@@ -1,12 +1,10 @@
 package helper
 
 import (
-	"context"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	clusterv1alpha1 "github.com/karmada-io/karmada/pkg/apis/cluster/v1alpha1"
 	policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1"
@@ -32,27 +30,29 @@ func TolerationExists(tolerations []corev1.Toleration, tolerationToFind *corev1.
 	return false
 }
 
-// UpdateClusterControllerTaint add and remove some taints.
-func UpdateClusterControllerTaint(ctx context.Context, client client.Client, taintsToAdd, taintsToRemove []*corev1.Taint, cluster *clusterv1alpha1.Cluster) error {
+// SetCurrentClusterTaints sets current cluster taints which need to be updated.
+func SetCurrentClusterTaints(taintsToAdd, taintsToRemove []*corev1.Taint, cluster *clusterv1alpha1.Cluster) []corev1.Taint {
+	taints := cluster.Spec.Taints
+
 	var clusterTaintsToAdd, clusterTaintsToRemove []corev1.Taint
 	// Find which taints need to be added.
 	for _, taintToAdd := range taintsToAdd {
-		if !TaintExists(cluster.Spec.Taints, taintToAdd) {
+		if !TaintExists(taints, taintToAdd) {
 			clusterTaintsToAdd = append(clusterTaintsToAdd, *taintToAdd)
 		}
 	}
 	// Find which taints need to be removed.
 	for _, taintToRemove := range taintsToRemove {
-		if TaintExists(cluster.Spec.Taints, taintToRemove) {
+		if TaintExists(taints, taintToRemove) {
 			clusterTaintsToRemove = append(clusterTaintsToRemove, *taintToRemove)
 		}
 	}
 	// If no taints need to be added and removed, just return.
 	if len(clusterTaintsToAdd) == 0 && len(clusterTaintsToRemove) == 0 {
-		return nil
+		return taints
 	}
 
-	taints := make([]corev1.Taint, 0, len(cluster.Spec.Taints)+len(clusterTaintsToAdd)-len(clusterTaintsToRemove))
+	taints = make([]corev1.Taint, 0, len(taints)+len(clusterTaintsToAdd)-len(clusterTaintsToRemove))
 	// Remove taints which need to be removed.
 	for i := range cluster.Spec.Taints {
 		if !TaintExists(clusterTaintsToRemove, &cluster.Spec.Taints[i]) {
@@ -67,10 +67,7 @@ func UpdateClusterControllerTaint(ctx context.Context, client client.Client, tai
 		taints = append(taints, taintToAdd)
 	}
 
-	cluster = cluster.DeepCopy()
-	cluster.Spec.Taints = taints
-
-	return client.Update(ctx, cluster)
+	return taints
 }
 
 // AddTolerations add some tolerations if not existed.

diff --git a/pkg/util/helper/taint_test.go b/pkg/util/helper/taint_test.go
@@ -1,7 +1,6 @@
 package helper
 
 import (
-	"context"
 	"fmt"
 	"reflect"
 	"testing"
@@ -10,12 +9,9 @@ import (
 	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"sigs.k8s.io/controller-runtime/pkg/client"
-	fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	clusterv1alpha1 "github.com/karmada-io/karmada/pkg/apis/cluster/v1alpha1"
 	policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1"
-	"github.com/karmada-io/karmada/pkg/util/gclient"
 )
 
 var (
@@ -30,7 +26,7 @@ var (
 	}
 )
 
-func TestUpdateClusterControllerTaint(t *testing.T) {
+func TestSetCurrentClusterTaints(t *testing.T) {
 	type args struct {
 		taints         []corev1.Taint
 		taintsToAdd    []*corev1.Taint
@@ -40,7 +36,6 @@ func TestUpdateClusterControllerTaint(t *testing.T) {
 		name       string
 		args       args
 		wantTaints []corev1.Taint
-		wantErr    bool
 	}{
 		{
 			name: "ready condition from true to false",
@@ -50,7 +45,6 @@ func TestUpdateClusterControllerTaint(t *testing.T) {
 				taintsToRemove: []*corev1.Taint{unreachableTaintTemplate.DeepCopy()},
 			},
 			wantTaints: []corev1.Taint{*notReadyTaintTemplate},
-			wantErr:    false,
 		},
 		{
 			name: "ready condition from true to unknown",
@@ -60,7 +54,6 @@ func TestUpdateClusterControllerTaint(t *testing.T) {
 				taintsToRemove: []*corev1.Taint{notReadyTaintTemplate.DeepCopy()},
 			},
 			wantTaints: []corev1.Taint{*unreachableTaintTemplate},
-			wantErr:    false,
 		},
 		{
 			name: "ready condition from false to unknown",
@@ -70,7 +63,6 @@ func TestUpdateClusterControllerTaint(t *testing.T) {
 				taintsToRemove: []*corev1.Taint{notReadyTaintTemplate.DeepCopy()},
 			},
 			wantTaints: []corev1.Taint{*unreachableTaintTemplate},
-			wantErr:    false,
 		},
 		{
 			name: "ready condition from false to true",
@@ -80,7 +72,6 @@ func TestUpdateClusterControllerTaint(t *testing.T) {
 				taintsToRemove: []*corev1.Taint{notReadyTaintTemplate.DeepCopy(), unreachableTaintTemplate.DeepCopy()},
 			},
 			wantTaints: nil,
-			wantErr:    false,
 		},
 		{
 			name: "ready condition from unknown to true",
@@ -90,7 +81,6 @@ func TestUpdateClusterControllerTaint(t *testing.T) {
 				taintsToRemove: []*corev1.Taint{notReadyTaintTemplate.DeepCopy(), unreachableTaintTemplate.DeepCopy()},
 			},
 			wantTaints: nil,
-			wantErr:    false,
 		},
 		{
 			name: "ready condition from unknown to false",
@@ -100,7 +90,6 @@ func TestUpdateClusterControllerTaint(t *testing.T) {
 				taintsToRemove: []*corev1.Taint{unreachableTaintTemplate.DeepCopy()},
 			},
 			wantTaints: []corev1.Taint{*notReadyTaintTemplate},
-			wantErr:    false,
 		},
 		{
 			name: "clusterTaintsToAdd is nil and clusterTaintsToRemove is nil",
@@ -110,38 +99,26 @@ func TestUpdateClusterControllerTaint(t *testing.T) {
 				taintsToRemove: []*corev1.Taint{notReadyTaintTemplate.DeepCopy()},
 			},
 			wantTaints: []corev1.Taint{*unreachableTaintTemplate},
-			wantErr:    false,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			ctx, cancel := context.WithCancel(context.Background())
-			defer cancel()
-
 			cluster := &clusterv1alpha1.Cluster{
 				ObjectMeta: metav1.ObjectMeta{Name: "member"},
 				Spec: clusterv1alpha1.ClusterSpec{
 					Taints: tt.args.taints,
 				},
 			}
-			c := fakeclient.NewClientBuilder().WithScheme(gclient.NewSchema()).WithObjects(cluster).Build()
-
-			if err := UpdateClusterControllerTaint(ctx, c, tt.args.taintsToAdd, tt.args.taintsToRemove, cluster); (err != nil) != tt.wantErr {
-				t.Errorf("UpdateClusterControllerTaint() error = %v, wantErr %v", err, tt.wantErr)
-			}
-
-			if err := c.Get(ctx, client.ObjectKey{Name: cluster.Name}, cluster); err != nil {
-				t.Fatalf("Failed to get cluster %s: %v", cluster.Name, err)
-			}
 
-			if len(cluster.Spec.Taints) != len(tt.wantTaints) {
-				t.Errorf("Cluster gotTaints = %v, want %v", cluster.Spec.Taints, tt.wantTaints)
+			taints := SetCurrentClusterTaints(tt.args.taintsToAdd, tt.args.taintsToRemove, cluster)
+			if len(taints) != len(tt.wantTaints) {
+				t.Errorf("Cluster gotTaints = %v, want %v", taints, tt.wantTaints)
 			}
-			for i := range cluster.Spec.Taints {
-				if cluster.Spec.Taints[i].Key != tt.wantTaints[i].Key ||
-					cluster.Spec.Taints[i].Value != tt.wantTaints[i].Value ||
-					cluster.Spec.Taints[i].Effect != tt.wantTaints[i].Effect {
-					t.Errorf("Cluster gotTaints = %v, want %v", cluster.Spec.Taints, tt.wantTaints)
+			for i := range taints {
+				if taints[i].Key != tt.wantTaints[i].Key ||
+					taints[i].Value != tt.wantTaints[i].Value ||
+					taints[i].Effect != tt.wantTaints[i].Effect {
+					t.Errorf("Cluster gotTaints = %v, want %v", taints, tt.wantTaints)
 				}
 			}
 		})

diff --git a/test/e2e/failover_test.go b/test/e2e/failover_test.go
@@ -558,7 +558,8 @@ func recoverTaintedCluster(c client.Client, clusterName string, taint corev1.Tai
 			}
 			return false, err
 		}
-		if err := helper.UpdateClusterControllerTaint(context.TODO(), c, nil, []*corev1.Taint{&taint}, clusterObj); err != nil {
+		clusterObj.Spec.Taints = helper.SetCurrentClusterTaints(nil, []*corev1.Taint{&taint}, clusterObj)
+		if err := c.Update(context.TODO(), clusterObj); err != nil {
 			if apierrors.IsConflict(err) {
 				return false, nil
 			}