Merge pull request #149 from reetasingh/master

Adding golang ci lint support and Refactoring test by make it table driven

.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

.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

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

cmd/descheduler/app/version.go

@@ -18,9 +18,10 @@ package app
 
 import (
 	"fmt"
-	"github.com/spf13/cobra"
 	"runtime"
 	"strings"
+
+	"github.com/spf13/cobra"
 )
 
 var (

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
 

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

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

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

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",

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) {