From dee89a6cc1ec00ed699a09ed632ca3ed68232a92 Mon Sep 17 00:00:00 2001 From: Reeta Singh Date: Sat, 11 May 2019 17:06:50 -0700 Subject: [PATCH] Refactoring test by make it table driven and adding golangci-lint to the CI pipeline --- .golangci.yml | 15 ++++ .travis.yml | 12 ++- Makefile | 7 ++ cmd/descheduler/app/version.go | 3 +- hack/verify-gofmt.sh | 2 +- pkg/descheduler/evictions/evictions_test.go | 42 ++++++++--- pkg/descheduler/node/node.go | 5 +- pkg/descheduler/strategies/duplicates_test.go | 74 +++++++++++++++---- .../strategies/node_affinity_test.go | 24 +++--- test/e2e/e2e_test.go | 2 - 10 files changed, 137 insertions(+), 49 deletions(-) create mode 100644 .golangci.yml diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 000000000..c4f501b0c --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,15 @@ +run: + deadline: 2m + +linters: + disable-all: true + enable: + - gofmt + - gosimple + - gocyclo + - misspell + - govet + +linters-settings: + goimports: +local-prefixes: github.com/kubernetes-incubator/descheduler diff --git a/.travis.yml b/.travis.yml index 072046f8f..67d508953 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,7 +1,11 @@ +sudo: false + language: go + go: - - 1.9.1 +- "1.10" + script: -- hack/verify-gofmt.sh -- make build -- make test-unit + - make lint + - make build + - make test-unit diff --git a/Makefile b/Makefile index 9e118e90b..e3b2f8b1f 100644 --- a/Makefile +++ b/Makefile @@ -22,6 +22,8 @@ LDFLAG_LOCATION=github.com/kubernetes-incubator/descheduler/cmd/descheduler/app LDFLAGS=-ldflags "-X ${LDFLAG_LOCATION}.version=${VERSION} -X ${LDFLAG_LOCATION}.buildDate=${BUILD} -X ${LDFLAG_LOCATION}.gitCommit=${COMMIT}" +GOLANGCI_VERSION := v1.15.0 +HAS_GOLANGCI := $(shell which golangci-lint) # IMAGE is the image name of descheduler # Should this be changed? @@ -52,3 +54,8 @@ gen: ./hack/update-generated-conversions.sh ./hack/update-generated-deep-copies.sh ./hack/update-generated-defaulters.sh +lint: +ifndef HAS_GOLANGCI + curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $(GOPATH)/bin ${GOLANGCI_VERSION} +endif + golangci-lint run diff --git a/cmd/descheduler/app/version.go b/cmd/descheduler/app/version.go index 020dfd32f..5bf43ac06 100644 --- a/cmd/descheduler/app/version.go +++ b/cmd/descheduler/app/version.go @@ -18,9 +18,10 @@ package app import ( "fmt" - "github.com/spf13/cobra" "runtime" "strings" + + "github.com/spf13/cobra" ) var ( diff --git a/hack/verify-gofmt.sh b/hack/verify-gofmt.sh index c45a0f1f4..365d130d0 100755 --- a/hack/verify-gofmt.sh +++ b/hack/verify-gofmt.sh @@ -24,7 +24,7 @@ DESCHEDULER_ROOT=$(dirname "${BASH_SOURCE}")/.. GO_VERSION=($(go version)) if [[ -z $(echo "${GO_VERSION[2]}" | grep -E 'go1.2|go1.3|go1.4|go1.5|go1.6|go1.7|go1.8|go1.9|go1.10') ]]; then - echo "Unknown go version '${GO_VERSION}', skipping gofmt." + echo "Unknown go version '${GO_VERSION[2]}', skipping gofmt." exit 1 fi diff --git a/pkg/descheduler/evictions/evictions_test.go b/pkg/descheduler/evictions/evictions_test.go index 3cd1ff0b1..7ec711659 100644 --- a/pkg/descheduler/evictions/evictions_test.go +++ b/pkg/descheduler/evictions/evictions_test.go @@ -26,15 +26,39 @@ import ( ) func TestEvictPod(t *testing.T) { - n1 := test.BuildTestNode("node1", 1000, 2000, 9) - p1 := test.BuildTestPod("p1", 400, 0, n1.Name) - fakeClient := &fake.Clientset{} - fakeClient.Fake.AddReactor("list", "pods", func(action core.Action) (bool, runtime.Object, error) { - return true, &v1.PodList{Items: []v1.Pod{*p1}}, nil - }) - evicted, _ := EvictPod(fakeClient, p1, "v1", false) - if !evicted { - t.Errorf("Expected %v pod to be evicted", p1.Name) + node1 := test.BuildTestNode("node1", 1000, 2000, 9) + pod1 := test.BuildTestPod("p1", 400, 0, "node1") + tests := []struct { + description string + node *v1.Node + pod *v1.Pod + pods []v1.Pod + want bool + }{ + { + description: "test pod eviction - pod present", + node: node1, + pod: pod1, + pods: []v1.Pod{*pod1}, + want: true, + }, + { + description: "test pod eviction - pod absent", + node: node1, + pod: pod1, + pods: []v1.Pod{*test.BuildTestPod("p2", 400, 0, "node1"), *test.BuildTestPod("p3", 450, 0, "node1")}, + want: true, + }, } + for _, test := range tests { + fakeClient := &fake.Clientset{} + fakeClient.Fake.AddReactor("list", "pods", func(action core.Action) (bool, runtime.Object, error) { + return true, &v1.PodList{Items: test.pods}, nil + }) + got, _ := EvictPod(fakeClient, test.pod, "v1", false) + if got != test.want { + t.Errorf("Test error for Desc: %s. Expected %v pod eviction to be %v, got %v", test.description, test.pod.Name, test.want, got) + } + } } diff --git a/pkg/descheduler/node/node.go b/pkg/descheduler/node/node.go index 101714864..5b48973d8 100644 --- a/pkg/descheduler/node/node.go +++ b/pkg/descheduler/node/node.go @@ -120,10 +120,7 @@ func IsReady(node *v1.Node) bool { // IsNodeUschedulable checks if the node is unschedulable. This is helper function to check only in case of // underutilized node so that they won't be accounted for. func IsNodeUschedulable(node *v1.Node) bool { - if node.Spec.Unschedulable { - return true - } - return false + return node.Spec.Unschedulable } // PodFitsAnyNode checks if the given pod fits any of the given nodes, based on diff --git a/pkg/descheduler/strategies/duplicates_test.go b/pkg/descheduler/strategies/duplicates_test.go index b53d1da5b..428a1b29f 100644 --- a/pkg/descheduler/strategies/duplicates_test.go +++ b/pkg/descheduler/strategies/duplicates_test.go @@ -27,8 +27,8 @@ import ( core "k8s.io/client-go/testing" ) -//TODO:@ravisantoshgudimetla This could be made table driven. func TestFindDuplicatePods(t *testing.T) { + // first setup pods node := test.BuildTestNode("n1", 2000, 3000, 10) p1 := test.BuildTestPod("p1", 100, 0, node.Name) p1.Namespace = "dev" @@ -86,23 +86,65 @@ func TestFindDuplicatePods(t *testing.T) { // A Critical Pod. p7.Annotations = test.GetCriticalPodAnnotation() - // Setup the fake client. - fakeClient := &fake.Clientset{} - fakeClient.Fake.AddReactor("list", "pods", func(action core.Action) (bool, runtime.Object, error) { - return true, &v1.PodList{Items: []v1.Pod{*p1, *p2, *p3, *p4, *p5, *p6, *p7, *p8, *p9, *p10}}, nil - }) - fakeClient.Fake.AddReactor("get", "nodes", func(action core.Action) (bool, runtime.Object, error) { - return true, node, nil - }) + tests := []struct { + description string + maxPodsToEvict int + pods []v1.Pod + expectedEvictedPodCount int + }{ + { + description: "Three pods in the default Namespace, bound to same ReplicaSet. 2 should get deleted", + maxPodsToEvict: 2, + pods: []v1.Pod{*p1, *p2, *p3}, + expectedEvictedPodCount: 2, + }, + { + description: "Three Pods in the test Namespace, bound to same ReplicaSet. 2 should be evicted", + maxPodsToEvict: 2, + pods: []v1.Pod{*p8, *p9, *p10}, + expectedEvictedPodCount: 2, + }, + { + description: "pods are part of daemonset and local storage - none should get deleted", + maxPodsToEvict: 2, + pods: []v1.Pod{*p4, *p5}, + expectedEvictedPodCount: 0, + }, + { + description: "pods are mirror pod and critical pod - none should get deleted", + maxPodsToEvict: 2, + pods: []v1.Pod{*p6, *p7}, + expectedEvictedPodCount: 0, + }, + { + description: "pods are part of replicaset, daemonset and local storage - all duplicate replicaset pods should get deleted", + maxPodsToEvict: 4, + pods: []v1.Pod{*p1, *p2, *p3, *p8, *p9, *p10, *p4, *p5}, + expectedEvictedPodCount: 4, + }, + { + description: "pods are part of replicaset, daemonset, local storage, mirror pod and critical pod - all duplicate replicaset pods should get deleted", + maxPodsToEvict: 5, + pods: []v1.Pod{*p1, *p2, *p3, *p8, *p9, *p10, *p4, *p5, *p6, *p7}, + expectedEvictedPodCount: 4, + }, + } - expectedEvictedPodCount := 4 - npe := nodePodEvictedCount{} - npe[node] = 0 + for _, test := range tests { - // Start evictions. - podsEvicted := deleteDuplicatePods(fakeClient, "v1", []*v1.Node{node}, false, npe, 10, false) - if podsEvicted != expectedEvictedPodCount { - t.Error("Unexpected number of pods evicted.\nExpected:\t", expectedEvictedPodCount, "\nActual:\t\t", podsEvicted) + npe := nodePodEvictedCount{} + npe[node] = 0 + fakeClient := &fake.Clientset{} + fakeClient.Fake.AddReactor("list", "pods", func(action core.Action) (bool, runtime.Object, error) { + return true, &v1.PodList{Items: test.pods}, nil + }) + fakeClient.Fake.AddReactor("get", "nodes", func(action core.Action) (bool, runtime.Object, error) { + return true, node, nil + }) + podsEvicted := deleteDuplicatePods(fakeClient, "v1", []*v1.Node{node}, false, npe, test.maxPodsToEvict, false) + if podsEvicted != test.expectedEvictedPodCount { + t.Errorf("Test error for Desc: %s. Expected deleted pods count %v , got %v", test.description, test.expectedEvictedPodCount, podsEvicted) + } } } diff --git a/pkg/descheduler/strategies/node_affinity_test.go b/pkg/descheduler/strategies/node_affinity_test.go index bedae1893..5843f9a8e 100644 --- a/pkg/descheduler/strategies/node_affinity_test.go +++ b/pkg/descheduler/strategies/node_affinity_test.go @@ -106,10 +106,10 @@ func TestRemovePodsViolatingNodeAffinity(t *testing.T) { }, }, expectedEvictedPodCount: 0, - pods: addPodsToNode(nodeWithoutLabels), - nodes: []*v1.Node{nodeWithoutLabels, nodeWithLabels}, - npe: nodePodEvictedCount{nodeWithoutLabels: 0, nodeWithLabels: 0}, - maxPodsToEvict: 0, + pods: addPodsToNode(nodeWithoutLabels), + nodes: []*v1.Node{nodeWithoutLabels, nodeWithLabels}, + npe: nodePodEvictedCount{nodeWithoutLabels: 0, nodeWithLabels: 0}, + maxPodsToEvict: 0, }, { description: "Invalid strategy type, should not evict any pods", @@ -122,19 +122,19 @@ func TestRemovePodsViolatingNodeAffinity(t *testing.T) { }, }, expectedEvictedPodCount: 0, - pods: addPodsToNode(nodeWithoutLabels), - nodes: []*v1.Node{nodeWithoutLabels, nodeWithLabels}, - npe: nodePodEvictedCount{nodeWithoutLabels: 0, nodeWithLabels: 0}, - maxPodsToEvict: 0, + pods: addPodsToNode(nodeWithoutLabels), + nodes: []*v1.Node{nodeWithoutLabels, nodeWithLabels}, + npe: nodePodEvictedCount{nodeWithoutLabels: 0, nodeWithLabels: 0}, + maxPodsToEvict: 0, }, { description: "Pod is correctly scheduled on node, no eviction expected", strategy: requiredDuringSchedulingIgnoredDuringExecutionStrategy, expectedEvictedPodCount: 0, - pods: addPodsToNode(nodeWithLabels), - nodes: []*v1.Node{nodeWithLabels}, - npe: nodePodEvictedCount{nodeWithLabels: 0}, - maxPodsToEvict: 0, + pods: addPodsToNode(nodeWithLabels), + nodes: []*v1.Node{nodeWithLabels}, + npe: nodePodEvictedCount{nodeWithLabels: 0}, + maxPodsToEvict: 0, }, { description: "Pod is scheduled on node without matching labels, another schedulable node available, should be evicted", diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 07d29983a..a09f4e14c 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -115,8 +115,6 @@ func startEndToEndForLowNodeUtilization(clientset clientset.Interface) { nodePodCount := strategies.InitializeNodePodCount(nodes) strategies.LowNodeUtilization(ds, lowNodeUtilizationStrategy, evictionPolicyGroupVersion, nodes, nodePodCount) time.Sleep(10 * time.Second) - - return } func TestE2E(t *testing.T) {