From 7039b6c8aafbad333fa21b3604ba1dae0a7f9b15 Mon Sep 17 00:00:00 2001
From: Sean Malloy <sean.malloy@kohls.com>
Date: Thu, 14 May 2020 00:11:54 -0500
Subject: [PATCH] Refactor function EvictPod to only return an error

The EvictPod function previously returned a bool and an error. The
function now only returns an error. Callers can check for failure by
testing if the returned error is not nil. This aligns the EvictPod
function with idiomatic Go best practices.

Also, the function name has been changed from EvictPod to evictPod
because it is not used outside the evictions package.
---
 pkg/descheduler/evictions/evictions.go      | 31 +++++++++++----------
 pkg/descheduler/evictions/evictions_test.go | 10 +++----
 2 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/pkg/descheduler/evictions/evictions.go b/pkg/descheduler/evictions/evictions.go
index d234c833c..131ed602c 100644
--- a/pkg/descheduler/evictions/evictions.go
+++ b/pkg/descheduler/evictions/evictions.go
@@ -83,25 +83,26 @@ func (pe *PodEvictor) TotalEvicted() int {
 // EvictPod returns non-nil error only when evicting a pod on a node is not
 // possible (due to maxPodsToEvict constraint). Success is true when the pod
 // is evicted on the server side.
-func (pe *PodEvictor) EvictPod(ctx context.Context, pod *v1.Pod, node *v1.Node) (success bool, err error) {
+func (pe *PodEvictor) EvictPod(ctx context.Context, pod *v1.Pod, node *v1.Node) (bool, error) {
 	if pe.maxPodsToEvict > 0 && pe.nodepodCount[node]+1 > pe.maxPodsToEvict {
 		return false, fmt.Errorf("Maximum number %v of evicted pods per %q node reached", pe.maxPodsToEvict, node.Name)
 	}
 
-	success, err = EvictPod(ctx, pe.client, pod, pe.policyGroupVersion, pe.dryRun)
-	if success {
-		pe.nodepodCount[node]++
-		klog.V(1).Infof("Evicted pod: %#v in namespace %#v", pod.Name, pod.Namespace)
-		return success, nil
+	err := evictPod(ctx, pe.client, pod, pe.policyGroupVersion, pe.dryRun)
+	if err != nil {
+		// err is used only for logging purposes
+		klog.Errorf("Error evicting pod: %#v in namespace %#v (%#v)", pod.Name, pod.Namespace, err)
+		return false, nil
 	}
-	// err is used only for logging purposes
-	klog.Errorf("Error evicting pod: %#v in namespace %#v (%#v)", pod.Name, pod.Namespace, err)
-	return false, nil
+
+	pe.nodepodCount[node]++
+	klog.V(1).Infof("Evicted pod: %#v in namespace %#v", pod.Name, pod.Namespace)
+	return true, nil
 }
 
-func EvictPod(ctx context.Context, client clientset.Interface, pod *v1.Pod, policyGroupVersion string, dryRun bool) (bool, error) {
+func evictPod(ctx context.Context, client clientset.Interface, pod *v1.Pod, policyGroupVersion string, dryRun bool) error {
 	if dryRun {
-		return true, nil
+		return nil
 	}
 	deleteOptions := &metav1.DeleteOptions{}
 	// GracePeriodSeconds ?
@@ -124,13 +125,13 @@ func EvictPod(ctx context.Context, client clientset.Interface, pod *v1.Pod, poli
 		eventBroadcaster.StartRecordingToSink(&clientcorev1.EventSinkImpl{Interface: client.CoreV1().Events(pod.Namespace)})
 		r := eventBroadcaster.NewRecorder(scheme.Scheme, v1.EventSource{Component: "sigs.k8s.io.descheduler"})
 		r.Event(pod, v1.EventTypeNormal, "Descheduled", "pod evicted by sigs.k8s.io/descheduler")
-		return true, nil
+		return nil
 	}
 	if apierrors.IsTooManyRequests(err) {
-		return false, fmt.Errorf("error when evicting pod (ignoring) %q: %v", pod.Name, err)
+		return fmt.Errorf("error when evicting pod (ignoring) %q: %v", pod.Name, err)
 	}
 	if apierrors.IsNotFound(err) {
-		return false, fmt.Errorf("pod not found when evicting %q: %v", pod.Name, err)
+		return fmt.Errorf("pod not found when evicting %q: %v", pod.Name, err)
 	}
-	return false, err
+	return err
 }
diff --git a/pkg/descheduler/evictions/evictions_test.go b/pkg/descheduler/evictions/evictions_test.go
index a1e87f8fe..0d6bf1bb8 100644
--- a/pkg/descheduler/evictions/evictions_test.go
+++ b/pkg/descheduler/evictions/evictions_test.go
@@ -20,7 +20,7 @@ import (
 	"context"
 	"testing"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/kubernetes/fake"
 	core "k8s.io/client-go/testing"
@@ -36,21 +36,21 @@ func TestEvictPod(t *testing.T) {
 		node        *v1.Node
 		pod         *v1.Pod
 		pods        []v1.Pod
-		want        bool
+		want        error
 	}{
 		{
 			description: "test pod eviction - pod present",
 			node:        node1,
 			pod:         pod1,
 			pods:        []v1.Pod{*pod1},
-			want:        true,
+			want:        nil,
 		},
 		{
 			description: "test pod eviction - pod absent",
 			node:        node1,
 			pod:         pod1,
 			pods:        []v1.Pod{*test.BuildTestPod("p2", 400, 0, "node1", nil), *test.BuildTestPod("p3", 450, 0, "node1", nil)},
-			want:        true,
+			want:        nil,
 		},
 	}
 
@@ -59,7 +59,7 @@ func TestEvictPod(t *testing.T) {
 		fakeClient.Fake.AddReactor("list", "pods", func(action core.Action) (bool, runtime.Object, error) {
 			return true, &v1.PodList{Items: test.pods}, nil
 		})
-		got, _ := EvictPod(ctx, fakeClient, test.pod, "v1", false)
+		got := evictPod(ctx, 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)
 		}