Refactor tests of tainting

Refactor scale down nad deletetaint tests
Speed up deletetaint tests

cluster-autoscaler/core/scale_down_test.go

@@ -49,6 +49,8 @@ import (
 	"k8s.io/klog"
 )
 
+const nothingReturned = "Nothing returned"
+
 func TestFindUnneededNodes(t *testing.T) {
 	p1 := BuildTestPod("p1", 100, 0)
 	p1.Spec.NodeName = "n1"
@@ -408,7 +410,6 @@ func TestFindUnneededNodePool(t *testing.T) {
 
 func TestDeleteNode(t *testing.T) {
 	// common parameters
-	nothingReturned := "Nothing returned"
 	nodeDeleteFailedFunc :=
 		func(string, string) error {
 			return fmt.Errorf("won't remove node")
@@ -918,6 +919,7 @@ func TestScaleDownEmptyMinGroupSizeLimitHit(t *testing.T) {
 	}
 	simpleScaleDownEmpty(t, config)
 }
+
 func simpleScaleDownEmpty(t *testing.T, config *scaleTestConfig) {
 	updatedNodes := make(chan string, 10)
 	deletedNodes := make(chan string, 10)
@@ -1172,7 +1174,7 @@ func getStringFromChan(c chan string) string {
 	case val := <-c:
 		return val
 	case <-time.After(10 * time.Second):
-		return "Nothing returned"
+		return nothingReturned
 	}
 }
 
@@ -1181,7 +1183,7 @@ func getStringFromChanImmediately(c chan string) string {
 	case val := <-c:
 		return val
 	default:
-		return "Nothing returned"
+		return nothingReturned
 	}
 }
 
@@ -1320,58 +1322,22 @@ func TestCheckScaleDownDeltaWithinLimits(t *testing.T) {
 	}
 }
 
-// newFakeInMemoryNodeClient creates fake client that keeps state of cluster nodes in memory
+func getNode(t *testing.T, client kube_client.Interface, name string) *apiv1.Node {
-func newFakeInMemoryNodeClient(nodes []*apiv1.Node) *fake.Clientset {
+	t.Helper()
-	fakeClient := &fake.Clientset{}
-	clusterState := struct {
-		nodes map[string]*apiv1.Node
-	}{
-		make(map[string]*apiv1.Node),
-	}
-
-	for _, node := range nodes {
-		clusterState.nodes[node.Name] = node.DeepCopy()
-	}
-
-	fakeClient.Fake.AddReactor("list", "nodes", func(action core.Action) (bool, runtime.Object, error) {
-		nodes := make([]apiv1.Node, 0, len(clusterState.nodes))
-		for _, node := range clusterState.nodes {
-			nodes = append(nodes, *node.DeepCopy())
-		}
-		return true, &apiv1.NodeList{Items: nodes}, nil
-	})
-	fakeClient.Fake.AddReactor("get", "nodes", func(action core.Action) (bool, runtime.Object, error) {
-		getAction := action.(core.GetAction)
-		node, ok := clusterState.nodes[getAction.GetName()]
-		if !ok {
-			return true, nil, fmt.Errorf("Wrong node: %v", getAction.GetName())
-		}
-		return true, node.DeepCopy(), nil
-	})
-	fakeClient.Fake.AddReactor("update", "nodes", func(action core.Action) (bool, runtime.Object, error) {
-		update := action.(core.UpdateAction)
-		node := update.GetObject().(*apiv1.Node)
-		clusterState.nodes[node.Name] = node.DeepCopy()
-		return true, node.DeepCopy(), nil
-	})
-	fakeClient.Fake.AddReactor("delete", "nodes", func(action core.Action) (bool, runtime.Object, error) {
-		deleteAction := action.(core.DeleteAction)
-		delete(clusterState.nodes, deleteAction.GetName())
-		return true, nil, nil
-	})
-
-	return fakeClient
-}
-
-// Helper functions
-func hasDeletionCandidateTaint(t *testing.T, client kube_client.Interface, name string) bool {
 	node, err := client.CoreV1().Nodes().Get(name, metav1.GetOptions{})
 	if err != nil {
 		t.Fatalf("Failed to retrieve node %v: %v", name, err)
 	}
-	return deletetaint.HasDeletionCandidateTaint(node)
+	return node
 }
+
+func hasDeletionCandidateTaint(t *testing.T, client kube_client.Interface, name string) bool {
+	t.Helper()
+	return deletetaint.HasDeletionCandidateTaint(getNode(t, client, name))
+}
+
 func getAllNodes(t *testing.T, client kube_client.Interface) []*apiv1.Node {
+	t.Helper()
 	nodeList, err := client.CoreV1().Nodes().List(metav1.ListOptions{})
 	if err != nil {
 		t.Fatalf("Failed to retrieve list of nodes: %v", err)
@@ -1382,7 +1348,9 @@ func getAllNodes(t *testing.T, client kube_client.Interface) []*apiv1.Node {
 	}
 	return result
 }
+
 func countDeletionCandidateTaints(t *testing.T, client kube_client.Interface) (total int) {
+	t.Helper()
 	for _, node := range getAllNodes(t, client) {
 		if deletetaint.HasDeletionCandidateTaint(node) {
 			total++
@@ -1411,7 +1379,11 @@ func TestSoftTaint(t *testing.T) {
 	p700.Spec.NodeName = "n1000"
 	p1200.Spec.NodeName = "n2000"
 
-	fakeClient := newFakeInMemoryNodeClient([]*apiv1.Node{n1000, n2000})
+	fakeClient := fake.NewSimpleClientset()
+	_, err := fakeClient.CoreV1().Nodes().Create(n1000)
+	assert.NoError(t, err)
+	_, err = fakeClient.CoreV1().Nodes().Create(n2000)
+	assert.NoError(t, err)
 
 	provider := testprovider.NewTestCloudProvider(nil, func(nodeGroup string, node string) error {
 		t.Fatalf("Unexpected deletion of %s", node)
@@ -1504,11 +1476,22 @@ func TestSoftTaintTimeLimit(t *testing.T) {
 	currentTime := time.Now()
 	updateTime := time.Millisecond
 	maxSoftTaintDuration := 1 * time.Second
+
+	// Replace time tracking function
 	now = func() time.Time {
 		return currentTime
 	}
+	defer func() {
+		now = time.Now
+		return
+	}()
+
+	fakeClient := fake.NewSimpleClientset()
+	_, err := fakeClient.CoreV1().Nodes().Create(n1)
+	assert.NoError(t, err)
+	_, err = fakeClient.CoreV1().Nodes().Create(n2)
+	assert.NoError(t, err)
 
-	fakeClient := newFakeInMemoryNodeClient([]*apiv1.Node{n1, n2})
 	// Move time forward when updating
 	fakeClient.Fake.PrependReactor("update", "nodes", func(action core.Action) (bool, runtime.Object, error) {
 		currentTime = currentTime.Add(updateTime)
@@ -1555,8 +1538,9 @@ func TestSoftTaintTimeLimit(t *testing.T) {
 	assert.False(t, hasDeletionCandidateTaint(t, fakeClient, n1.Name))
 	assert.False(t, hasDeletionCandidateTaint(t, fakeClient, n2.Name))
 
-	// Test duration limit of bulk taint
 	updateTime = maxSoftTaintDuration
+
+	// Test duration limit of bulk taint
 	scaleDown.UpdateUnneededNodes([]*apiv1.Node{n1, n2},
 		[]*apiv1.Node{n1, n2}, []*apiv1.Pod{}, time.Now().Add(-5*time.Minute), nil)
 	errs = scaleDown.SoftTaintUnneededNodes(getAllNodes(t, fakeClient))
@@ -1567,7 +1551,6 @@ func TestSoftTaintTimeLimit(t *testing.T) {
 	assert.Equal(t, 2, countDeletionCandidateTaints(t, fakeClient))
 
 	// Test duration limit of bulk untaint
-	updateTime = maxSoftTaintDuration
 	scaleDown.UpdateUnneededNodes([]*apiv1.Node{n1, n2},
 		[]*apiv1.Node{n1, n2}, []*apiv1.Pod{p1, p2}, time.Now().Add(-5*time.Minute), nil)
 	errs = scaleDown.SoftTaintUnneededNodes(getAllNodes(t, fakeClient))
@@ -1576,7 +1559,4 @@ func TestSoftTaintTimeLimit(t *testing.T) {
 	errs = scaleDown.SoftTaintUnneededNodes(getAllNodes(t, fakeClient))
 	assert.Empty(t, errs)
 	assert.Equal(t, 0, countDeletionCandidateTaints(t, fakeClient))
-
-	// Clean up
-	now = time.Now
 }

cluster-autoscaler/utils/deletetaint/delete.go

@@ -35,9 +35,12 @@ const (
 	ToBeDeletedTaint = "ToBeDeletedByClusterAutoscaler"
 	// DeletionCandidateTaint is a taint used to mark unneeded node as preferably unschedulable.
 	DeletionCandidateTaint = "DeletionCandidateOfClusterAutoscaler"
+)
 
-	maxRetryDeadline      = 5 * time.Second
+// Mutable only in unit tests
-	conflictRetryInterval = 750 * time.Millisecond
+var (
+	maxRetryDeadline      time.Duration = 5 * time.Second
+	conflictRetryInterval time.Duration = 750 * time.Millisecond
 )
 
 // getKeyShortName converts taint key to short name for logging

cluster-autoscaler/utils/deletetaint/delete_test.go

@@ -30,6 +30,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	kube_util "k8s.io/autoscaler/cluster-autoscaler/utils/kubernetes"
+	kube_client "k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/fake"
 	core "k8s.io/client-go/testing"
 
@@ -37,59 +38,59 @@ import (
 )
 
 func TestMarkNodes(t *testing.T) {
+	defer setConflictRetryInterval(setConflictRetryInterval(time.Millisecond))
 	node := BuildTestNode("node", 1000, 1000)
-	fakeClient := buildFakeClient(t, node)
+	fakeClient := buildFakeClientWithConflicts(t, node)
 	err := MarkToBeDeleted(node, fakeClient)
 	assert.NoError(t, err)
 
-	updatedNode, err := fakeClient.Core().Nodes().Get("node", metav1.GetOptions{})
+	updatedNode := getNode(t, fakeClient, "node")
-	assert.NoError(t, err)
 	assert.True(t, HasToBeDeletedTaint(updatedNode))
 	assert.False(t, HasDeletionCandidateTaint(updatedNode))
 }
 
 func TestSoftMarkNodes(t *testing.T) {
+	defer setConflictRetryInterval(setConflictRetryInterval(time.Millisecond))
 	node := BuildTestNode("node", 1000, 1000)
-	fakeClient := buildFakeClient(t, node)
+	fakeClient := buildFakeClientWithConflicts(t, node)
 	err := MarkDeletionCandidate(node, fakeClient)
 	assert.NoError(t, err)
 
-	updatedNode, err := fakeClient.Core().Nodes().Get("node", metav1.GetOptions{})
+	updatedNode := getNode(t, fakeClient, "node")
-	assert.NoError(t, err)
 	assert.False(t, HasToBeDeletedTaint(updatedNode))
 	assert.True(t, HasDeletionCandidateTaint(updatedNode))
 }
 
 func TestCheckNodes(t *testing.T) {
+	defer setConflictRetryInterval(setConflictRetryInterval(time.Millisecond))
 	node := BuildTestNode("node", 1000, 1000)
 	addTaintToSpec(node, ToBeDeletedTaint, apiv1.TaintEffectNoSchedule)
-	fakeClient := buildFakeClient(t, node)
+	fakeClient := buildFakeClientWithConflicts(t, node)
 
-	updatedNode, err := fakeClient.Core().Nodes().Get("node", metav1.GetOptions{})
+	updatedNode := getNode(t, fakeClient, "node")
-	assert.NoError(t, err)
 	assert.True(t, HasToBeDeletedTaint(updatedNode))
 	assert.False(t, HasDeletionCandidateTaint(updatedNode))
 }
 
 func TestSoftCheckNodes(t *testing.T) {
+	defer setConflictRetryInterval(setConflictRetryInterval(time.Millisecond))
 	node := BuildTestNode("node", 1000, 1000)
 	addTaintToSpec(node, DeletionCandidateTaint, apiv1.TaintEffectPreferNoSchedule)
-	fakeClient := buildFakeClient(t, node)
+	fakeClient := buildFakeClientWithConflicts(t, node)
 
-	updatedNode, err := fakeClient.Core().Nodes().Get("node", metav1.GetOptions{})
+	updatedNode := getNode(t, fakeClient, "node")
-	assert.NoError(t, err)
 	assert.False(t, HasToBeDeletedTaint(updatedNode))
 	assert.True(t, HasDeletionCandidateTaint(updatedNode))
 }
 
 func TestQueryNodes(t *testing.T) {
+	defer setConflictRetryInterval(setConflictRetryInterval(time.Millisecond))
 	node := BuildTestNode("node", 1000, 1000)
-	fakeClient := buildFakeClient(t, node)
+	fakeClient := buildFakeClientWithConflicts(t, node)
 	err := MarkToBeDeleted(node, fakeClient)
 	assert.NoError(t, err)
 
-	updatedNode, err := fakeClient.Core().Nodes().Get("node", metav1.GetOptions{})
+	updatedNode := getNode(t, fakeClient, "node")
-	assert.NoError(t, err)
 	assert.True(t, HasToBeDeletedTaint(updatedNode))
 
 	val, err := GetToBeDeletedTime(updatedNode)
@@ -99,13 +100,13 @@ func TestQueryNodes(t *testing.T) {
 }
 
 func TestSoftQueryNodes(t *testing.T) {
+	defer setConflictRetryInterval(setConflictRetryInterval(time.Millisecond))
 	node := BuildTestNode("node", 1000, 1000)
-	fakeClient := buildFakeClient(t, node)
+	fakeClient := buildFakeClientWithConflicts(t, node)
 	err := MarkDeletionCandidate(node, fakeClient)
 	assert.NoError(t, err)
 
-	updatedNode, err := fakeClient.Core().Nodes().Get("node", metav1.GetOptions{})
+	updatedNode := getNode(t, fakeClient, "node")
-	assert.NoError(t, err)
 	assert.True(t, HasDeletionCandidateTaint(updatedNode))
 
 	val, err := GetDeletionCandidateTime(updatedNode)
@@ -115,37 +116,37 @@ func TestSoftQueryNodes(t *testing.T) {
 }
 
 func TestCleanNodes(t *testing.T) {
+	defer setConflictRetryInterval(setConflictRetryInterval(time.Millisecond))
 	node := BuildTestNode("node", 1000, 1000)
 	addTaintToSpec(node, ToBeDeletedTaint, apiv1.TaintEffectNoSchedule)
-	fakeClient := buildFakeClient(t, node)
+	fakeClient := buildFakeClientWithConflicts(t, node)
 
-	updatedNode, err := fakeClient.Core().Nodes().Get("node", metav1.GetOptions{})
+	updatedNode := getNode(t, fakeClient, "node")
-	assert.NoError(t, err)
 	assert.True(t, HasToBeDeletedTaint(updatedNode))
 
 	cleaned, err := CleanToBeDeleted(node, fakeClient)
 	assert.True(t, cleaned)
 	assert.NoError(t, err)
 
-	updatedNode, err = fakeClient.Core().Nodes().Get("node", metav1.GetOptions{})
+	updatedNode = getNode(t, fakeClient, "node")
 	assert.NoError(t, err)
 	assert.False(t, HasToBeDeletedTaint(updatedNode))
 }
 
 func TestSoftCleanNodes(t *testing.T) {
+	defer setConflictRetryInterval(setConflictRetryInterval(time.Millisecond))
 	node := BuildTestNode("node", 1000, 1000)
 	addTaintToSpec(node, DeletionCandidateTaint, apiv1.TaintEffectPreferNoSchedule)
-	fakeClient := buildFakeClient(t, node)
+	fakeClient := buildFakeClientWithConflicts(t, node)
 
-	updatedNode, err := fakeClient.Core().Nodes().Get("node", metav1.GetOptions{})
+	updatedNode := getNode(t, fakeClient, "node")
-	assert.NoError(t, err)
 	assert.True(t, HasDeletionCandidateTaint(updatedNode))
 
 	cleaned, err := CleanDeletionCandidate(node, fakeClient)
 	assert.True(t, cleaned)
 	assert.NoError(t, err)
 
-	updatedNode, err = fakeClient.Core().Nodes().Get("node", metav1.GetOptions{})
+	updatedNode = getNode(t, fakeClient, "node")
 	assert.NoError(t, err)
 	assert.False(t, HasDeletionCandidateTaint(updatedNode))
 }
@@ -155,36 +156,15 @@ func TestCleanAllToBeDeleted(t *testing.T) {
 	n2 := BuildTestNode("n2", 1000, 10)
 	n2.Spec.Taints = []apiv1.Taint{{Key: ToBeDeletedTaint, Value: strconv.FormatInt(time.Now().Unix()-301, 10)}}
 
-	fakeClient := &fake.Clientset{}
+	fakeClient := buildFakeClient(t, n1, n2)
-	fakeClient.Fake.AddReactor("get", "nodes", func(action core.Action) (bool, runtime.Object, error) {
-		getAction := action.(core.GetAction)
-		switch getAction.GetName() {
-		case n1.Name:
-			return true, n1, nil
-		case n2.Name:
-			return true, n2, nil
-		}
-		return true, nil, fmt.Errorf("Wrong node: %v", getAction.GetName())
-	})
-	fakeClient.Fake.AddReactor("update", "nodes", func(action core.Action) (bool, runtime.Object, error) {
-		update := action.(core.UpdateAction)
-		obj := update.GetObject().(*apiv1.Node)
-		switch obj.Name {
-		case n1.Name:
-			n1 = obj
-		case n2.Name:
-			n2 = obj
-		}
-		return true, obj, nil
-	})
 	fakeRecorder := kube_util.CreateEventRecorder(fakeClient)
 
-	assert.Equal(t, 1, len(n2.Spec.Taints))
+	assert.Equal(t, 1, len(getNode(t, fakeClient, "n2").Spec.Taints))
 
 	CleanAllToBeDeleted([]*apiv1.Node{n1, n2}, fakeClient, fakeRecorder)
 
-	assert.Equal(t, 0, len(n1.Spec.Taints))
+	assert.Equal(t, 0, len(getNode(t, fakeClient, "n1").Spec.Taints))
-	assert.Equal(t, 0, len(n2.Spec.Taints))
+	assert.Equal(t, 0, len(getNode(t, fakeClient, "n2").Spec.Taints))
 }
 
 func TestCleanAllDeletionCandidates(t *testing.T) {
@@ -192,43 +172,46 @@ func TestCleanAllDeletionCandidates(t *testing.T) {
 	n2 := BuildTestNode("n2", 1000, 10)
 	n2.Spec.Taints = []apiv1.Taint{{Key: DeletionCandidateTaint, Value: strconv.FormatInt(time.Now().Unix()-301, 10)}}
 
-	fakeClient := &fake.Clientset{}
+	fakeClient := buildFakeClient(t, n1, n2)
-	fakeClient.Fake.AddReactor("get", "nodes", func(action core.Action) (bool, runtime.Object, error) {
-		getAction := action.(core.GetAction)
-		switch getAction.GetName() {
-		case n1.Name:
-			return true, n1, nil
-		case n2.Name:
-			return true, n2, nil
-		}
-		return true, nil, fmt.Errorf("Wrong node: %v", getAction.GetName())
-	})
-	fakeClient.Fake.AddReactor("update", "nodes", func(action core.Action) (bool, runtime.Object, error) {
-		update := action.(core.UpdateAction)
-		obj := update.GetObject().(*apiv1.Node)
-		switch obj.Name {
-		case n1.Name:
-			n1 = obj
-		case n2.Name:
-			n2 = obj
-		}
-		return true, obj, nil
-	})
 	fakeRecorder := kube_util.CreateEventRecorder(fakeClient)
 
-	assert.Equal(t, 1, len(n2.Spec.Taints))
+	assert.Equal(t, 1, len(getNode(t, fakeClient, "n2").Spec.Taints))
 
 	CleanAllDeletionCandidates([]*apiv1.Node{n1, n2}, fakeClient, fakeRecorder)
 
-	assert.Equal(t, 0, len(n1.Spec.Taints))
+	assert.Equal(t, 0, len(getNode(t, fakeClient, "n1").Spec.Taints))
-	assert.Equal(t, 0, len(n2.Spec.Taints))
+	assert.Equal(t, 0, len(getNode(t, fakeClient, "n2").Spec.Taints))
 }
 
-func buildFakeClient(t *testing.T, node *apiv1.Node) *fake.Clientset {
+func setConflictRetryInterval(interval time.Duration) time.Duration {
+	before := conflictRetryInterval
+	conflictRetryInterval = interval
+	return before
+}
+
+func getNode(t *testing.T, client kube_client.Interface, name string) *apiv1.Node {
+	t.Helper()
+	node, err := client.CoreV1().Nodes().Get(name, metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Failed to retrieve node %v: %v", name, err)
+	}
+	return node
+}
+
+func buildFakeClient(t *testing.T, nodes ...*apiv1.Node) *fake.Clientset {
+	t.Helper()
 	fakeClient := fake.NewSimpleClientset()
 
-	_, err := fakeClient.CoreV1().Nodes().Create(node)
+	for _, node := range nodes {
-	assert.NoError(t, err)
+		_, err := fakeClient.CoreV1().Nodes().Create(node)
+		assert.NoError(t, err)
+	}
+
+	return fakeClient
+}
+
+func buildFakeClientWithConflicts(t *testing.T, nodes ...*apiv1.Node) *fake.Clientset {
+	fakeClient := buildFakeClient(t, nodes...)
 
 	// return a 'Conflict' error on the first upadte, then pass it through, then return a Conflict again
 	var returnedConflict int32