Skip to content

Commit ad9cae7

Browse files
XiShanYongYe-ChangXiShanYongYe-Chang
committed
purgeMode not properly set when tolerationTime is 0 when cluster failover occurs
Signed-off-by: changzhen <[email protected]>
1 parent 0b929ac commit ad9cae7

File tree

2 files changed

+322
-16
lines changed

2 files changed

+322
-16
lines changed

‎pkg/controllers/cluster/taint_manager.go‎

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -173,20 +173,18 @@ func (tc *NoExecuteTaintManager) syncBindingEviction(key util.QueueKey) error {
173173
return err
174174
}
175175

176-
var purgeMode policyv1alpha1.PurgeMode
176+
// Case 1: Need eviction right now.
177+
// Case 2: Need eviction after toleration time.
178+
// Case 3: Tolerate forever, we do nothing.
177179
if needEviction {
180+
var purgeMode policyv1alpha1.PurgeMode
178181
switch tc.NoExecuteTaintEvictionPurgeMode {
179182
case string(policyv1alpha1.PurgeModeGracefully):
180183
purgeMode = policyv1alpha1.PurgeModeGracefully
181184
case string(policyv1alpha1.PurgeModeDirectly):
182185
purgeMode = policyv1alpha1.PurgeModeDirectly
183186
}
184-
}
185187

186-
// Case 1: Need eviction now.
187-
// Case 2: Need eviction after toleration time. If time is up, do eviction right now.
188-
// Case 3: Tolerate forever, we do nothing.
189-
if needEviction || tolerationTime == 0 {
190188
// update final result to evict the target cluster
191189
binding.Spec.GracefulEvictCluster(cluster, workv1alpha2.NewTaskOptions(
192190
workv1alpha2.WithPurgeMode(purgeMode),
@@ -234,20 +232,18 @@ func (tc *NoExecuteTaintManager) syncClusterBindingEviction(key util.QueueKey) e
234232
return err
235233
}
236234

237-
var purgeMode policyv1alpha1.PurgeMode
235+
// Case 1: Need eviction now.
236+
// Case 2: Need eviction after toleration time.
237+
// Case 3: Tolerate forever, we do nothing.
238238
if needEviction {
239+
var purgeMode policyv1alpha1.PurgeMode
239240
switch tc.NoExecuteTaintEvictionPurgeMode {
240241
case string(policyv1alpha1.PurgeModeGracefully):
241242
purgeMode = policyv1alpha1.PurgeModeGracefully
242243
case string(policyv1alpha1.PurgeModeDirectly):
243244
purgeMode = policyv1alpha1.PurgeModeDirectly
244245
}
245-
}
246246

247-
// Case 1: Need eviction now.
248-
// Case 2: Need eviction after toleration time. If time is up, do eviction right now.
249-
// Case 3: Tolerate forever, we do nothing.
250-
if needEviction || tolerationTime == 0 {
251247
// update final result to evict the target cluster
252248
binding.Spec.GracefulEvictCluster(cluster, workv1alpha2.NewTaskOptions(
253249
workv1alpha2.WithPurgeMode(purgeMode),
@@ -267,9 +263,13 @@ func (tc *NoExecuteTaintManager) syncClusterBindingEviction(key util.QueueKey) e
267263
return nil
268264
}
269265

270-
// needEviction returns whether the binding should be evicted from target cluster right now.
271-
// If a toleration time is found, we return false along with a minimum toleration time as the
272-
// second return value.
266+
// needEviction determines if a binding should be evicted from the specified cluster based on taints and tolerations.
267+
// Returns:
268+
// - needEviction: true if the binding should be evicted (when to evict is indicated by the second return value).
269+
// - tolerationTime: if needEviction is true, this is the duration to wait before eviction (0 means evict immediately,
270+
// >0 means evict after this duration).
271+
// If needEviction is false, a value < 0 indicates the binding should be tolerated indefinitely (no eviction).
272+
// - error: any error encountered during evaluation.
273273
func (tc *NoExecuteTaintManager) needEviction(clusterName string, annotations map[string]string) (bool, time.Duration, error) {
274274
placement, err := helper.GetAppliedPlacement(annotations)
275275
if err != nil {
@@ -302,7 +302,12 @@ func (tc *NoExecuteTaintManager) needEviction(clusterName string, annotations ma
302302
return true, 0, nil
303303
}
304304

305-
return false, helper.GetMinTolerationTime(taints, usedTolerations), nil
305+
minTolerationTime := helper.GetMinTolerationTime(taints, usedTolerations)
306+
if minTolerationTime == 0 {
307+
return true, 0, nil
308+
}
309+
310+
return false, minTolerationTime, nil
306311
}
307312

308313
// SetupWithManager creates a controller and register to controller manager.

‎pkg/controllers/cluster/taint_manager_test.go‎

Lines changed: 301 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"context"
2121
"reflect"
2222
"testing"
23+
"time"
2324

2425
corev1 "k8s.io/api/core/v1"
2526
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -537,3 +538,303 @@ func TestNoExecuteTaintManager_syncClusterBindingEviction(t *testing.T) {
537538
})
538539
}
539540
}
541+
542+
func TestNoExecuteTaintManager_needEviction(t *testing.T) {
543+
now := time.Now()
544+
oneSecondAgo := metav1.Time{Time: now.Add(-1 * time.Second)}
545+
oneSecondLater := metav1.Time{Time: now.Add(1 * time.Second)}
546+
547+
// Test cases
548+
tests := []struct {
549+
name string
550+
clusterName string
551+
annotations map[string]string
552+
cluster *clusterv1alpha1.Cluster
553+
enableNoExecuteTaintEviction bool
554+
expectedNeedEviction bool
555+
expectedTolerationTime time.Duration
556+
wantErr bool
557+
}{
558+
{
559+
name: "placement annotation not exist",
560+
clusterName: "test-cluster",
561+
annotations: map[string]string{},
562+
wantErr: true,
563+
},
564+
{
565+
name: "placement annotation is invalid JSON",
566+
clusterName: "test-cluster",
567+
annotations: map[string]string{
568+
"policy.karmada.io/applied-placement": "invalid-json",
569+
},
570+
wantErr: true,
571+
},
572+
{
573+
name: "placement is nil",
574+
clusterName: "test-cluster",
575+
annotations: map[string]string{
576+
"policy.karmada.io/applied-placement": "",
577+
},
578+
wantErr: true,
579+
},
580+
{
581+
name: "cluster not found",
582+
clusterName: "non-existent-cluster",
583+
annotations: map[string]string{
584+
"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]}}`,
585+
},
586+
expectedNeedEviction: false,
587+
expectedTolerationTime: -1,
588+
wantErr: false,
589+
},
590+
{
591+
name: "no execute taint eviction disabled",
592+
clusterName: "test-cluster",
593+
annotations: map[string]string{
594+
"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]}}`,
595+
},
596+
cluster: &clusterv1alpha1.Cluster{
597+
ObjectMeta: metav1.ObjectMeta{
598+
Name: "test-cluster",
599+
},
600+
Spec: clusterv1alpha1.ClusterSpec{
601+
Taints: []corev1.Taint{
602+
{
603+
Key: "test-key",
604+
Effect: corev1.TaintEffectNoExecute,
605+
},
606+
},
607+
},
608+
},
609+
enableNoExecuteTaintEviction: false,
610+
expectedNeedEviction: false,
611+
expectedTolerationTime: -1,
612+
wantErr: false,
613+
},
614+
{
615+
name: "cluster has no NoExecute taints",
616+
clusterName: "test-cluster",
617+
annotations: map[string]string{
618+
"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]}}`,
619+
},
620+
cluster: &clusterv1alpha1.Cluster{
621+
ObjectMeta: metav1.ObjectMeta{
622+
Name: "test-cluster",
623+
},
624+
Spec: clusterv1alpha1.ClusterSpec{
625+
Taints: []corev1.Taint{
626+
{
627+
Key: "test-key",
628+
Effect: corev1.TaintEffectNoSchedule,
629+
},
630+
},
631+
},
632+
},
633+
enableNoExecuteTaintEviction: true,
634+
expectedNeedEviction: false,
635+
expectedTolerationTime: -1,
636+
wantErr: false,
637+
},
638+
{
639+
name: "taint not tolerated - immediate eviction",
640+
clusterName: "test-cluster",
641+
annotations: map[string]string{
642+
"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]},"clusterTolerations":[]}`,
643+
},
644+
cluster: &clusterv1alpha1.Cluster{
645+
ObjectMeta: metav1.ObjectMeta{
646+
Name: "test-cluster",
647+
},
648+
Spec: clusterv1alpha1.ClusterSpec{
649+
Taints: []corev1.Taint{
650+
{
651+
Key: "test-key",
652+
Effect: corev1.TaintEffectNoExecute,
653+
TimeAdded: &oneSecondAgo,
654+
},
655+
},
656+
},
657+
},
658+
enableNoExecuteTaintEviction: true,
659+
expectedNeedEviction: true,
660+
expectedTolerationTime: 0,
661+
wantErr: false,
662+
},
663+
{
664+
name: "taint tolerated with zero toleration seconds - immediate eviction",
665+
clusterName: "test-cluster",
666+
annotations: map[string]string{
667+
"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]},"clusterTolerations":[{"key":"test-key","operator":"Exists","effect":"NoExecute","tolerationSeconds":0}]}`,
668+
},
669+
cluster: &clusterv1alpha1.Cluster{
670+
ObjectMeta: metav1.ObjectMeta{
671+
Name: "test-cluster",
672+
},
673+
Spec: clusterv1alpha1.ClusterSpec{
674+
Taints: []corev1.Taint{
675+
{
676+
Key: "test-key",
677+
Effect: corev1.TaintEffectNoExecute,
678+
TimeAdded: &oneSecondAgo,
679+
},
680+
},
681+
},
682+
},
683+
enableNoExecuteTaintEviction: true,
684+
expectedNeedEviction: true,
685+
expectedTolerationTime: 0,
686+
wantErr: false,
687+
},
688+
{
689+
name: "taint tolerated with positive toleration seconds - wait for toleration time",
690+
clusterName: "test-cluster",
691+
annotations: map[string]string{
692+
"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]},"clusterTolerations":[{"key":"test-key","operator":"Exists","effect":"NoExecute","tolerationSeconds":60}]}`,
693+
},
694+
cluster: &clusterv1alpha1.Cluster{
695+
ObjectMeta: metav1.ObjectMeta{
696+
Name: "test-cluster",
697+
},
698+
Spec: clusterv1alpha1.ClusterSpec{
699+
Taints: []corev1.Taint{
700+
{
701+
Key: "test-key",
702+
Effect: corev1.TaintEffectNoExecute,
703+
TimeAdded: &oneSecondAgo,
704+
},
705+
},
706+
},
707+
},
708+
enableNoExecuteTaintEviction: true,
709+
expectedNeedEviction: false,
710+
expectedTolerationTime: 59 * time.Second,
711+
wantErr: false,
712+
},
713+
{
714+
name: "taint tolerated forever - no eviction",
715+
clusterName: "test-cluster",
716+
annotations: map[string]string{
717+
"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]},"clusterTolerations":[{"key":"test-key","operator":"Exists","effect":"NoExecute"}]}`,
718+
},
719+
cluster: &clusterv1alpha1.Cluster{
720+
ObjectMeta: metav1.ObjectMeta{
721+
Name: "test-cluster",
722+
},
723+
Spec: clusterv1alpha1.ClusterSpec{
724+
Taints: []corev1.Taint{
725+
{
726+
Key: "test-key",
727+
Effect: corev1.TaintEffectNoExecute,
728+
TimeAdded: &oneSecondAgo,
729+
},
730+
},
731+
},
732+
},
733+
enableNoExecuteTaintEviction: true,
734+
expectedNeedEviction: false,
735+
expectedTolerationTime: -1,
736+
wantErr: false,
737+
},
738+
{
739+
name: "multiple taints with mixed tolerations",
740+
clusterName: "test-cluster",
741+
annotations: map[string]string{
742+
"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]},"clusterTolerations":[{"key":"tolerated-key","operator":"Exists","effect":"NoExecute","tolerationSeconds":30}]}`,
743+
},
744+
cluster: &clusterv1alpha1.Cluster{
745+
ObjectMeta: metav1.ObjectMeta{
746+
Name: "test-cluster",
747+
},
748+
Spec: clusterv1alpha1.ClusterSpec{
749+
Taints: []corev1.Taint{
750+
{
751+
Key: "tolerated-key",
752+
Effect: corev1.TaintEffectNoExecute,
753+
TimeAdded: &oneSecondLater,
754+
},
755+
{
756+
Key: "not-tolerated-key",
757+
Effect: corev1.TaintEffectNoExecute,
758+
TimeAdded: &oneSecondAgo,
759+
},
760+
},
761+
},
762+
},
763+
enableNoExecuteTaintEviction: true,
764+
expectedNeedEviction: true,
765+
expectedTolerationTime: 0,
766+
wantErr: false,
767+
},
768+
{
769+
name: "toleration time expired - immediate eviction",
770+
clusterName: "test-cluster",
771+
annotations: map[string]string{
772+
"policy.karmada.io/applied-placement": `{"clusterAffinity":{"clusterNames":["test-cluster"]},"clusterTolerations":[{"key":"test-key","operator":"Exists","effect":"NoExecute","tolerationSeconds":1}]}`,
773+
},
774+
cluster: &clusterv1alpha1.Cluster{
775+
ObjectMeta: metav1.ObjectMeta{
776+
Name: "test-cluster",
777+
},
778+
Spec: clusterv1alpha1.ClusterSpec{
779+
Taints: []corev1.Taint{
780+
{
781+
Key: "test-key",
782+
Effect: corev1.TaintEffectNoExecute,
783+
TimeAdded: &metav1.Time{Time: now.Add(-2 * time.Second)}, // 2 seconds ago, tolerance is 1 second
784+
},
785+
},
786+
},
787+
},
788+
enableNoExecuteTaintEviction: true,
789+
expectedNeedEviction: true,
790+
expectedTolerationTime: 0,
791+
wantErr: false,
792+
},
793+
}
794+
795+
for _, tt := range tests {
796+
t.Run(tt.name, func(t *testing.T) {
797+
tc := newNoExecuteTaintManager()
798+
tc.EnableNoExecuteTaintEviction = tt.enableNoExecuteTaintEviction
799+
800+
// Create cluster if provided
801+
if tt.cluster != nil {
802+
if err := tc.Client.Create(context.Background(), tt.cluster); err != nil {
803+
t.Fatalf("failed to create cluster: %v", err)
804+
}
805+
}
806+
807+
needEviction, tolerationTime, err := tc.needEviction(tt.clusterName, tt.annotations)
808+
809+
// Check error expectation
810+
if (err != nil) != tt.wantErr {
811+
t.Errorf("needEviction() error = %v, wantErr %v", err, tt.wantErr)
812+
return
813+
}
814+
815+
// If we expect an error, don't check other results
816+
if tt.wantErr {
817+
return
818+
}
819+
820+
// Check needEviction result
821+
if needEviction != tt.expectedNeedEviction {
822+
t.Errorf("needEviction() needEviction = %v, want %v", needEviction, tt.expectedNeedEviction)
823+
}
824+
825+
// Check tolerationTime result with tolerance for time-based tests
826+
if tt.expectedTolerationTime >= 0 && tolerationTime >= 0 {
827+
// For positive time values, allow 1 second tolerance
828+
tolerance := 1 * time.Second
829+
if tolerationTime < tt.expectedTolerationTime-tolerance || tolerationTime > tt.expectedTolerationTime+tolerance {
830+
t.Errorf("needEviction() tolerationTime = %v, want %v (±%v)", tolerationTime, tt.expectedTolerationTime, tolerance)
831+
}
832+
} else {
833+
// For negative values or zero, check exact match
834+
if tolerationTime != tt.expectedTolerationTime {
835+
t.Errorf("needEviction() tolerationTime = %v, want %v", tolerationTime, tt.expectedTolerationTime)
836+
}
837+
}
838+
})
839+
}
840+
}

0 commit comments

Comments
 (0)