Merge pull request #7820 from norbertcyran/add-taints-to-original-node

Ensure added taints are present on the node in the snapshot

Author: k8s-ci-robot

cluster-autoscaler/core/scaledown/actuation/actuator_test.go

@@ -1302,7 +1302,9 @@ func runStartDeletionTest(t *testing.T, tc startDeletionTestCase, force bool) {
 	// 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)
 	}

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)

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
 	}
 }

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)
+			}
+		})
+	}
+}