diff --git a/cluster-autoscaler/core/scale_down_test.go b/cluster-autoscaler/core/scale_down_test.go index 4496fe2839..45f80a1024 100644 --- a/cluster-autoscaler/core/scale_down_test.go +++ b/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 newFakeInMemoryNodeClient(nodes []*apiv1.Node) *fake.Clientset { - 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 { +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 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 } diff --git a/cluster-autoscaler/utils/deletetaint/delete.go b/cluster-autoscaler/utils/deletetaint/delete.go index 46cf9be650..a08aeed15f 100644 --- a/cluster-autoscaler/utils/deletetaint/delete.go +++ b/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 - conflictRetryInterval = 750 * time.Millisecond +// Mutable only in unit tests +var ( + maxRetryDeadline time.Duration = 5 * time.Second + conflictRetryInterval time.Duration = 750 * time.Millisecond ) // getKeyShortName converts taint key to short name for logging diff --git a/cluster-autoscaler/utils/deletetaint/delete_test.go b/cluster-autoscaler/utils/deletetaint/delete_test.go index 5a42d0cc93..afda2201e2 100644 --- a/cluster-autoscaler/utils/deletetaint/delete_test.go +++ b/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{}) - assert.NoError(t, err) + updatedNode := getNode(t, fakeClient, "node") 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{}) - assert.NoError(t, err) + updatedNode := getNode(t, fakeClient, "node") 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{}) - assert.NoError(t, err) + updatedNode := getNode(t, fakeClient, "node") 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{}) - assert.NoError(t, err) + updatedNode := getNode(t, fakeClient, "node") 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{}) - assert.NoError(t, err) + updatedNode := getNode(t, fakeClient, "node") 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{}) - assert.NoError(t, err) + updatedNode := getNode(t, fakeClient, "node") 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{}) - assert.NoError(t, err) + updatedNode := getNode(t, fakeClient, "node") 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{}) - assert.NoError(t, err) + updatedNode := getNode(t, fakeClient, "node") 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.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 - }) + fakeClient := buildFakeClient(t, n1, n2) 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(n2.Spec.Taints)) + assert.Equal(t, 0, len(getNode(t, fakeClient, "n1").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.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 - }) + fakeClient := buildFakeClient(t, n1, n2) 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(n2.Spec.Taints)) + assert.Equal(t, 0, len(getNode(t, fakeClient, "n1").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) - assert.NoError(t, err) + for _, node := range nodes { + _, 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