Ensure added taints are present on the node in the snapshot

kubernetes · Feb 11, 2025 · 75009db · 75009db
1 parent 08e7250
commit 75009db
Show file tree

Hide file tree

Showing 3 changed files with 129 additions and 1 deletion.
diff --git 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 them 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/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
@@ -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)
+			}
+		})
+	}
+}