Repository: karmada-io/karmada

Issue:
purgeMode not properly set when tolerationTime is 0 in taintManager's syncBindingEviction function
<!-- Please use this template while reporting a bug and provide as much info as possible. Not doing so may result in your bug not being addressed in a timely manner. Thanks!
-->


**What happened**:

There is a logical issue in the syncBindingEviction function of the NoExecuteTaintManager where the purgeMode variable is not properly initialized when tolerationTime is 0.

In the current implementation:

https://github.com/karmada-io/karmada/blob/a5103d07acc781d372a8eb17a1dcce28acc604fd/pkg/controllers/cluster/taint_manager.go#L176-L184

When tolerationTime == 0 but needEviction is false, the purgeMode variable remains uninitialized (zero value), but the eviction still proceeds. This means:
- The purgeMode will be set to the zero value of policyv1alpha1.PurgeMode
- The eviction task may not behave as expected since the purge mode is not explicitly configured
- This could lead to inconsistent eviction behavior

**What you expected to happen**:

The purgeMode should be properly set based on tc.NoExecuteTaintEvictionPurgeMode configuration whenever an eviction is going to be performed.

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

**Anything else we need to know?**:

**Environment**:
- Karmada version: v1.14
- kubectl-karmada or karmadactl version (the result of `kubectl-karmada version` or `karmadactl version`):
- Others:


Hints from issue comments:
@XiShanYongYe-Chang: /assign

Solution patch:
diff --git a/pkg/controllers/cluster/taint_manager.go b/pkg/controllers/cluster/taint_manager.go
index 56956e3ed..37e20acd9 100644
--- a/pkg/controllers/cluster/taint_manager.go
+++ b/pkg/controllers/cluster/taint_manager.go
@@ -173,20 +173,18 @@ func (tc *NoExecuteTaintManager) syncBindingEviction(key util.QueueKey) error {
 		return err
 	}
 
-	var purgeMode policyv1alpha1.PurgeMode
+	// Case 1: Need eviction right now.
+	// Case 2: Need eviction after toleration time.
+	// Case 3: Tolerate forever, we do nothing.
 	if needEviction {
+		var purgeMode policyv1alpha1.PurgeMode
 		switch tc.NoExecuteTaintEvictionPurgeMode {
 		case string(policyv1alpha1.PurgeModeGracefully):
 			purgeMode = policyv1alpha1.PurgeModeGracefully
 		case string(policyv1alpha1.PurgeModeDirectly):
 			purgeMode = policyv1alpha1.PurgeModeDirectly
 		}
-	}
 
-	// Case 1: Need eviction now.
-	// Case 2: Need eviction after toleration time. If time is up, do eviction right now.
-	// Case 3: Tolerate forever, we do nothing.
-	if needEviction || tolerationTime == 0 {
 		// update final result to evict the target cluster
 		binding.Spec.GracefulEvictCluster(cluster, workv1alpha2.NewTaskOptions(
 			workv1alpha2.WithPurgeMode(purgeMode),
@@ -234,20 +232,18 @@ func (tc *NoExecuteTaintManager) syncClusterBindingEviction(key util.QueueKey) e
 		return err
 	}
 
-	var purgeMode policyv1alpha1.PurgeMode
+	// Case 1: Need eviction now.
+	// Case 2: Need eviction after toleration time.
+	// Case 3: Tolerate forever, we do nothing.
 	if needEviction {
+		var purgeMode policyv1alpha1.PurgeMode
 		switch tc.NoExecuteTaintEvictionPurgeMode {
 		case string(policyv1alpha1.PurgeModeGracefully):
 			purgeMode = policyv1alpha1.PurgeModeGracefully
 		case string(policyv1alpha1.PurgeModeDirectly):
 			purgeMode = policyv1alpha1.PurgeModeDirectly
 		}
-	}
 
-	// Case 1: Need eviction now.
-	// Case 2: Need eviction after toleration time. If time is up, do eviction right now.
-	// Case 3: Tolerate forever, we do nothing.
-	if needEviction || tolerationTime == 0 {
 		// update final result to evict the target cluster
 		binding.Spec.GracefulEvictCluster(cluster, workv1alpha2.NewTaskOptions(
 			workv1alpha2.WithPurgeMode(purgeMode),
@@ -267,9 +263,13 @@ func (tc *NoExecuteTaintManager) syncClusterBindingEviction(key util.QueueKey) e
 	return nil
 }
 
-// needEviction returns whether the binding should be evicted from target cluster right now.
-// If a toleration time is found, we return false along with a minimum toleration time as the
-// second return value.
+// needEviction determines if a binding should be evicted from the specified cluster based on taints and tolerations.
+// Returns:
+//   - needEviction: true if the binding should be evicted (when to evict is indicated by the second return value).
+//   - tolerationTime: if needEviction is true, this is the duration to wait before eviction (0 means evict immediately,
+//     >0 means evict after this duration).
+//     If needEviction is false, a value < 0 indicates the binding should be tolerated indefinitely (no eviction).
+//   - error: any error encountered during evaluation.
 func (tc *NoExecuteTaintManager) needEviction(clusterName string, annotations map[string]string) (bool, time.Duration, error) {
 	placement, err := helper.GetAppliedPlacement(annotations)
 	if err != nil {
@@ -302,7 +302,12 @@ func (tc *NoExecuteTaintManager) needEviction(clusterName string, annotations ma
 		return true, 0, nil
 	}
 
-	return false, helper.GetMinTolerationTime(taints, usedTolerations), nil
+	minTolerationTime := helper.GetMinTolerationTime(taints, usedTolerations)
+	if minTolerationTime == 0 {
+		return true, 0, nil
+	}
+
+	return false, minTolerationTime, nil
 }
 
 // SetupWithManager creates a controller and register to controller manager.



Test patch:
diff --git a/pkg/controllers/cluster/taint_manager_test.go b/pkg/controllers/cluster/taint_manager_test.go
index a214e4242..3974e0d48 100644
--- a/pkg/controllers/cluster/taint_manager_test.go
+++ b/pkg/controllers/cluster/taint_manager_test.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"reflect"
 	"testing"
+	"time"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -537,3 +538,303 @@ func TestNoExecuteTaintManager_syncClusterBindingEviction(t *testing.T) {
 		})
 	}
 }
+
+func TestNoExecuteTaintManager_needEviction(t *testing.T) {
+	now := time.Now()
+	oneSecondAgo := metav1.Time{Time: now.Add(-1 * time.Second)}
+	oneSecondLater := metav1.Time{Time: now.Add(1 * time.Second)}
+
+	// Test cases
+	tests := []struct {
+		name                         string
+		clusterName                  string
+		annotations                  map[string]string
+		cluster                      *clusterv1alpha1.Cluster
+		enableNoExecuteTaintEviction bool
+		expectedNeedEviction         bool
+		expectedTolerationTime       time.Duration
+		wantErr                      bool
+	}{
+		{
+			name:        "placement annotation not exist",
+			clusterName: "test-cluster",
+			annotations: map[string]string{},
+			wantErr:     true,
+		},
+		{
+			name:        "placement annotation is invalid JSON",
+			clusterName: "test-cluster",
+			annotations: map[string]string{
+				"policy.karmada.io/applied-placement": "invalid-json",
+			},
+			wantErr: true,
+		},
+		{
+			name:        "placement is nil",
+			clusterName: "test-cluster",
+			annotations: map[string]string{
+				"policy.karmada.io/applied-placement": "",
+			},
+			wantErr: true,
+		},
+		{
+			name:        "cluster not found",
+			clusterName: "non-existent-cluster",
+			annotations: map[string]string{
+				"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]}}`,
+			},
+			expectedNeedEviction:   false,
+			expectedTolerationTime: -1,
+			wantErr:                false,
+		},
+		{
+			name:        "no execute taint eviction disabled",
+			clusterName: "test-cluster",
+			annotations: map[string]string{
+				"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]}}`,
+			},
+			cluster: &clusterv1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-cluster",
+				},
+				Spec: clusterv1alpha1.ClusterSpec{
+					Taints: []corev1.Taint{
+						{
+							Key:    "test-key",
+							Effect: corev1.TaintEffectNoExecute,
+						},
+					},
+				},
+			},
+			enableNoExecuteTaintEviction: false,
+			expectedNeedEviction:         false,
+			expectedTolerationTime:       -1,
+			wantErr:                      false,
+		},
+		{
+			name:        "cluster has no NoExecute taints",
+			clusterName: "test-cluster",
+			annotations: map[string]string{
+				"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]}}`,
+			},
+			cluster: &clusterv1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-cluster",
+				},
+				Spec: clusterv1alpha1.ClusterSpec{
+					Taints: []corev1.Taint{
+						{
+							Key:    "test-key",
+							Effect: corev1.TaintEffectNoSchedule,
+						},
+					},
+				},
+			},
+			enableNoExecuteTaintEviction: true,
+			expectedNeedEviction:         false,
+			expectedTolerationTime:       -1,
+			wantErr:                      false,
+		},
+		{
+			name:        "taint not tolerated - immediate eviction",
+			clusterName: "test-cluster",
+			annotations: map[string]string{
+				"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]},"clusterTolerations":[]}`,
+			},
+			cluster: &clusterv1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-cluster",
+				},
+				Spec: clusterv1alpha1.ClusterSpec{
+					Taints: []corev1.Taint{
+						{
+							Key:       "test-key",
+							Effect:    corev1.TaintEffectNoExecute,
+							TimeAdded: &oneSecondAgo,
+						},
+					},
+				},
+			},
+			enableNoExecuteTaintEviction: true,
+			expectedNeedEviction:         true,
+			expectedTolerationTime:       0,
+			wantErr:                      false,
+		},
+		{
+			name:        "taint tolerated with zero toleration seconds - immediate eviction",
+			clusterName: "test-cluster",
+			annotations: map[string]string{
+				"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]},"clusterTolerations":[{"key":"test-key","operator":"Exists","effect":"NoExecute","tolerationSeconds":0}]}`,
+			},
+			cluster: &clusterv1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-cluster",
+				},
+				Spec: clusterv1alpha1.ClusterSpec{
+					Taints: []corev1.Taint{
+						{
+							Key:       "test-key",
+							Effect:    corev1.TaintEffectNoExecute,
+							TimeAdded: &oneSecondAgo,
+						},
+					},
+				},
+			},
+			enableNoExecuteTaintEviction: true,
+			expectedNeedEviction:         true,
+			expectedTolerationTime:       0,
+			wantErr:                      false,
+		},
+		{
+			name:        "taint tolerated with positive toleration seconds - wait for toleration time",
+			clusterName: "test-cluster",
+			annotations: map[string]string{
+				"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]},"clusterTolerations":[{"key":"test-key","operator":"Exists","effect":"NoExecute","tolerationSeconds":60}]}`,
+			},
+			cluster: &clusterv1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-cluster",
+				},
+				Spec: clusterv1alpha1.ClusterSpec{
+					Taints: []corev1.Taint{
+						{
+							Key:       "test-key",
+							Effect:    corev1.TaintEffectNoExecute,
+							TimeAdded: &oneSecondAgo,
+						},
+					},
+				},
+			},
+			enableNoExecuteTaintEviction: true,
+			expectedNeedEviction:         false,
+			expectedTolerationTime:       59 * time.Second,
+			wantErr:                      false,
+		},
+		{
+			name:        "taint tolerated forever - no eviction",
+			clusterName: "test-cluster",
+			annotations: map[string]string{
+				"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]},"clusterTolerations":[{"key":"test-key","operator":"Exists","effect":"NoExecute"}]}`,
+			},
+			cluster: &clusterv1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-cluster",
+				},
+				Spec: clusterv1alpha1.ClusterSpec{
+					Taints: []corev1.Taint{
+						{
+							Key:       "test-key",
+							Effect:    corev1.TaintEffectNoExecute,
+							TimeAdded: &oneSecondAgo,
+						},
+					},
+				},
+			},
+			enableNoExecuteTaintEviction: true,
+			expectedNeedEviction:         false,
+			expectedTolerationTime:       -1,
+			wantErr:                      false,
+		},
+		{
+			name:        "multiple taints with mixed tolerations",
+			clusterName: "test-cluster",
+			annotations: map[string]string{
+				"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]},"clusterTolerations":[{"key":"tolerated-key","operator":"Exists","effect":"NoExecute","tolerationSeconds":30}]}`,
+			},
+			cluster: &clusterv1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-cluster",
+				},
+				Spec: clusterv1alpha1.ClusterSpec{
+					Taints: []corev1.Taint{
+						{
+							Key:       "tolerated-key",
+							Effect:    corev1.TaintEffectNoExecute,
+							TimeAdded: &oneSecondLater,
+						},
+						{
+							Key:       "not-tolerated-key",
+							Effect:    corev1.TaintEffectNoExecute,
+							TimeAdded: &oneSecondAgo,
+						},
+					},
+				},
+			},
+			enableNoExecuteTaintEviction: true,
+			expectedNeedEviction:         true,
+			expectedTolerationTime:       0,
+			wantErr:                      false,
+		},
+		{
+			name:        "toleration time expired - immediate eviction",
+			clusterName: "test-cluster",
+			annotations: map[string]string{
+				"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]},"clusterTolerations":[{"key":"test-key","operator":"Exists","effect":"NoExecute","tolerationSeconds":1}]}`,
+			},
+			cluster: &clusterv1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-cluster",
+				},
+				Spec: clusterv1alpha1.ClusterSpec{
+					Taints: []corev1.Taint{
+						{
+							Key:       "test-key",
+							Effect:    corev1.TaintEffectNoExecute,
+							TimeAdded: &metav1.Time{Time: now.Add(-2 * time.Second)}, // 2 seconds ago, tolerance is 1 second
+						},
+					},
+				},
+			},
+			enableNoExecuteTaintEviction: true,
+			expectedNeedEviction:         true,
+			expectedTolerationTime:       0,
+			wantErr:                      false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tc := newNoExecuteTaintManager()
+			tc.EnableNoExecuteTaintEviction = tt.enableNoExecuteTaintEviction
+
+			// Create cluster if provided
+			if tt.cluster != nil {
+				if err := tc.Client.Create(context.Background(), tt.cluster); err != nil {
+					t.Fatalf("failed to create cluster: %v", err)
+				}
+			}
+
+			needEviction, tolerationTime, err := tc.needEviction(tt.clusterName, tt.annotations)
+
+			// Check error expectation
+			if (err != nil) != tt.wantErr {
+				t.Errorf("needEviction() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+
+			// If we expect an error, don't check other results
+			if tt.wantErr {
+				return
+			}
+
+			// Check needEviction result
+			if needEviction != tt.expectedNeedEviction {
+				t.Errorf("needEviction() needEviction = %v, want %v", needEviction, tt.expectedNeedEviction)
+			}
+
+			// Check tolerationTime result with tolerance for time-based tests
+			if tt.expectedTolerationTime >= 0 && tolerationTime >= 0 {
+				// For positive time values, allow 1 second tolerance
+				tolerance := 1 * time.Second
+				if tolerationTime < tt.expectedTolerationTime-tolerance || tolerationTime > tt.expectedTolerationTime+tolerance {
+					t.Errorf("needEviction() tolerationTime = %v, want %v (±%v)", tolerationTime, tt.expectedTolerationTime, tolerance)
+				}
+			} else {
+				// For negative values or zero, check exact match
+				if tolerationTime != tt.expectedTolerationTime {
+					t.Errorf("needEviction() tolerationTime = %v, want %v", tolerationTime, tt.expectedTolerationTime)
+				}
+			}
+		})
+	}
+}