diff --git a/cluster-autoscaler/core/scaledown/actuation/actuator_test.go b/cluster-autoscaler/core/scaledown/actuation/actuator_test.go index 2749628c6b7f..1f7e024941ae 100644 --- a/cluster-autoscaler/core/scaledown/actuation/actuator_test.go +++ b/cluster-autoscaler/core/scaledown/actuation/actuator_test.go @@ -1223,7 +1223,9 @@ func TestStartDeletion(t *testing.T) { // Verify ScaleDownNodes looks as expected. ignoreSdNodeOrder := cmpopts.SortSlices(func(a, b *status.ScaleDownNode) bool { return a.Node.Name < b.Node.Name }) cmpNg := cmp.Comparer(func(a, b *testprovider.TestNodeGroup) bool { return a.Id() == b.Id() }) - statusCmpOpts := cmp.Options{ignoreSdNodeOrder, cmpNg, cmpopts.EquateEmpty()} + // Deletion taint may be lifted by goroutine, ignore taints to avoid race condition + ignoreTaints := cmpopts.IgnoreFields(apiv1.NodeSpec{}, "Taints") + statusCmpOpts := cmp.Options{ignoreSdNodeOrder, cmpNg, cmpopts.EquateEmpty(), ignoreTaints} if diff := cmp.Diff(wantScaleDownNodes, gotScaleDownNodes, statusCmpOpts); diff != "" { t.Errorf("StartDeletion scaled down nodes diff (-want +got):\n%s", diff) } diff --git a/cluster-autoscaler/core/static_autoscaler_test.go b/cluster-autoscaler/core/static_autoscaler_test.go index aabb4cde56f8..b05c914c113e 100644 --- a/cluster-autoscaler/core/static_autoscaler_test.go +++ b/cluster-autoscaler/core/static_autoscaler_test.go @@ -504,7 +504,7 @@ func TestStaticAutoscalerRunOnce(t *testing.T) { // Remove unregistered nodes. readyNodeLister.SetNodes([]*apiv1.Node{n1, n2}) allNodeLister.SetNodes([]*apiv1.Node{n1, n2}) - allPodListerMock.On("List").Return([]*apiv1.Pod{p1, p2}, nil).Once() + allPodListerMock.On("List").Return([]*apiv1.Pod{p1}, nil).Once() daemonSetListerMock.On("List", labels.Everything()).Return([]*appsv1.DaemonSet{}, nil).Once() onScaleDownMock.On("ScaleDown", "ng2", "n3").Return(nil).Once() podDisruptionBudgetListerMock.On("List").Return([]*policyv1.PodDisruptionBudget{}, nil).Once() @@ -536,11 +536,6 @@ func TestStaticAutoscalerRunOnceWithScaleDownDelayPerNG(t *testing.T) { onScaleDownMock := &onScaleDownMock{} deleteFinished := make(chan bool, 1) - n1 := BuildTestNode("n1", 1000, 1000) - SetNodeReadyState(n1, true, time.Now()) - n2 := BuildTestNode("n2", 1000, 1000) - SetNodeReadyState(n2, true, time.Now()) - tn := BuildTestNode("tn", 1000, 1000) tni := framework.NewTestNodeInfo(tn) @@ -710,6 +705,11 @@ func TestStaticAutoscalerRunOnceWithScaleDownDelayPerNG(t *testing.T) { tc.beforeTest(processors) + n1 := BuildTestNode("n1", 1000, 1000) + SetNodeReadyState(n1, true, time.Now()) + n2 := BuildTestNode("n2", 1000, 1000) + SetNodeReadyState(n2, true, time.Now()) + provider.AddNode("ng1", n1) provider.AddNode("ng2", n2) ng1.SetTargetSize(1) diff --git a/cluster-autoscaler/utils/taints/taints.go b/cluster-autoscaler/utils/taints/taints.go index 267a4a62872b..148f323099a7 100644 --- a/cluster-autoscaler/utils/taints/taints.go +++ b/cluster-autoscaler/utils/taints/taints.go @@ -215,6 +215,7 @@ func AddTaints(node *apiv1.Node, client kube_client.Interface, taints []apiv1.Ta return err } klog.V(1).Infof("Successfully added %v on node %v", strings.Join(taintKeys(taints), ","), node.Name) + node.Spec.Taints = append([]apiv1.Taint{}, freshNode.Spec.Taints...) return nil } } @@ -350,6 +351,7 @@ func CleanTaints(node *apiv1.Node, client kube_client.Interface, taintKeys []str return false, err } klog.V(1).Infof("Successfully released %v on node %v", strings.Join(taintKeys, ","), node.Name) + node.Spec.Taints = append([]apiv1.Taint{}, freshNode.Spec.Taints...) return true, nil } } diff --git a/cluster-autoscaler/utils/taints/taints_test.go b/cluster-autoscaler/utils/taints/taints_test.go index fd4bc315eaee..15db998767b6 100644 --- a/cluster-autoscaler/utils/taints/taints_test.go +++ b/cluster-autoscaler/utils/taints/taints_test.go @@ -723,3 +723,127 @@ func TestCountNodeTaints(t *testing.T) { got := CountNodeTaints([]*apiv1.Node{node, node2}, taintConfig) assert.Equal(t, want, got) } + +func TestAddTaints(t *testing.T) { + testCases := []struct { + name string + existingTaints []string + newTaints []string + wantTaints []string + }{ + { + name: "no existing taints", + newTaints: []string{"t1", "t2"}, + wantTaints: []string{"t1", "t2"}, + }, + { + name: "existing taints - no overlap", + existingTaints: []string{"t1"}, + newTaints: []string{"t2", "t3"}, + wantTaints: []string{"t1", "t2", "t3"}, + }, + { + name: "existing taints - duplicates", + existingTaints: []string{"t1"}, + newTaints: []string{"t1", "t2"}, + wantTaints: []string{"t1", "t2"}, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + n := BuildTestNode("node", 1000, 1000) + existingTaints := make([]apiv1.Taint, len(tc.existingTaints)) + for i, t := range tc.existingTaints { + existingTaints[i] = apiv1.Taint{ + Key: t, + Effect: apiv1.TaintEffectNoSchedule, + } + } + n.Spec.Taints = append([]apiv1.Taint{}, existingTaints...) + fakeClient := buildFakeClient(t, n) + newTaints := make([]apiv1.Taint, len(tc.newTaints)) + for i, t := range tc.newTaints { + newTaints[i] = apiv1.Taint{ + Key: t, + Effect: apiv1.TaintEffectNoSchedule, + } + } + err := AddTaints(n, fakeClient, newTaints, false) + assert.NoError(t, err) + apiNode := getNode(t, fakeClient, "node") + for _, want := range tc.wantTaints { + assert.True(t, HasTaint(n, want)) + assert.True(t, HasTaint(apiNode, want)) + } + }) + } +} + +func TestCleanTaints(t *testing.T) { + testCases := []struct { + name string + existingTaints []string + taintsToRemove []string + wantTaints []string + wantModified bool + }{ + { + name: "no existing taints", + taintsToRemove: []string{"t1", "t2"}, + wantTaints: []string{}, + wantModified: false, + }, + { + name: "existing taints - no overlap", + existingTaints: []string{"t1"}, + taintsToRemove: []string{"t2", "t3"}, + wantTaints: []string{"t1"}, + wantModified: false, + }, + { + name: "existing taints - remove one", + existingTaints: []string{"t1", "t2"}, + taintsToRemove: []string{"t1"}, + wantTaints: []string{"t2"}, + wantModified: true, + }, + { + name: "existing taints - remove all", + existingTaints: []string{"t1", "t2"}, + taintsToRemove: []string{"t1", "t2"}, + wantTaints: []string{}, + wantModified: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + n := BuildTestNode("node", 1000, 1000) + existingTaints := make([]apiv1.Taint, len(tc.existingTaints)) + for i, taintKey := range tc.existingTaints { + existingTaints[i] = apiv1.Taint{ + Key: taintKey, + Effect: apiv1.TaintEffectNoSchedule, + } + } + n.Spec.Taints = append([]apiv1.Taint{}, existingTaints...) + fakeClient := buildFakeClient(t, n) + + modified, err := CleanTaints(n, fakeClient, tc.taintsToRemove, false) + assert.NoError(t, err) + assert.Equal(t, tc.wantModified, modified) + + apiNode := getNode(t, fakeClient, "node") + + for _, want := range tc.wantTaints { + assert.True(t, HasTaint(apiNode, want)) + assert.True(t, HasTaint(n, want)) + } + + for _, removed := range tc.taintsToRemove { + assert.False(t, HasTaint(apiNode, removed)) + assert.False(t, HasTaint(n, removed), "Taint %s should have been removed from local node object", removed) + } + }) + } +}