From fae4f69d3633c41c781fd5dbe6bfc56453dbcec6 Mon Sep 17 00:00:00 2001
From: Jonathan Basseri <misterikkit@google.com>
Date: Fri, 14 Dec 2018 16:31:08 -0800
Subject: [PATCH] Fix return value of PriorityQueue.Add.

This function was returning a non-nil error for the common, non-failure
case. The fix is to properly scope local error values and add early
returns.
---
 .../internal/queue/scheduling_queue.go        | 30 +++++++++----------
 .../internal/queue/scheduling_queue_test.go   | 20 +++++++++----
 2 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/pkg/scheduler/internal/queue/scheduling_queue.go b/pkg/scheduler/internal/queue/scheduling_queue.go
index d34a186ce8e..f5e0a6fd265 100644
--- a/pkg/scheduler/internal/queue/scheduling_queue.go
+++ b/pkg/scheduler/internal/queue/scheduling_queue.go
@@ -317,23 +317,23 @@ func (p *PriorityQueue) updateNominatedPod(oldPod, newPod *v1.Pod) {
 func (p *PriorityQueue) Add(pod *v1.Pod) error {
 	p.lock.Lock()
 	defer p.lock.Unlock()
-	err := p.activeQ.Add(pod)
-	if err != nil {
+	if err := p.activeQ.Add(pod); err != nil {
 		klog.Errorf("Error adding pod %v/%v to the scheduling queue: %v", pod.Namespace, pod.Name, err)
-	} else {
-		if p.unschedulableQ.get(pod) != nil {
-			klog.Errorf("Error: pod %v/%v is already in the unschedulable queue.", pod.Namespace, pod.Name)
-			p.deleteNominatedPodIfExists(pod)
-			p.unschedulableQ.delete(pod)
-		}
-		// Delete pod from backoffQ if it is backing off
-		if err = p.podBackoffQ.Delete(pod); err == nil {
-			klog.Errorf("Error: pod %v/%v is already in the podBackoff queue.", pod.Namespace, pod.Name)
-		}
-		p.addNominatedPodIfNeeded(pod)
-		p.cond.Broadcast()
+		return err
 	}
-	return err
+	if p.unschedulableQ.get(pod) != nil {
+		klog.Errorf("Error: pod %v/%v is already in the unschedulable queue.", pod.Namespace, pod.Name)
+		p.deleteNominatedPodIfExists(pod)
+		p.unschedulableQ.delete(pod)
+	}
+	// Delete pod from backoffQ if it is backing off
+	if err := p.podBackoffQ.Delete(pod); err == nil {
+		klog.Errorf("Error: pod %v/%v is already in the podBackoff queue.", pod.Namespace, pod.Name)
+	}
+	p.addNominatedPodIfNeeded(pod)
+	p.cond.Broadcast()
+
+	return nil
 }
 
 // AddIfNotPresent adds a pod to the active queue if it is not present in any of
diff --git a/pkg/scheduler/internal/queue/scheduling_queue_test.go b/pkg/scheduler/internal/queue/scheduling_queue_test.go
index cc0a057fdf9..60c0d7b30d6 100644
--- a/pkg/scheduler/internal/queue/scheduling_queue_test.go
+++ b/pkg/scheduler/internal/queue/scheduling_queue_test.go
@@ -96,9 +96,15 @@ var highPriorityPod, highPriNominatedPod, medPriorityPod, unschedulablePod = v1.
 
 func TestPriorityQueue_Add(t *testing.T) {
 	q := NewPriorityQueue(nil)
-	q.Add(&medPriorityPod)
-	q.Add(&unschedulablePod)
-	q.Add(&highPriorityPod)
+	if err := q.Add(&medPriorityPod); err != nil {
+		t.Errorf("add failed: %v", err)
+	}
+	if err := q.Add(&unschedulablePod); err != nil {
+		t.Errorf("add failed: %v", err)
+	}
+	if err := q.Add(&highPriorityPod); err != nil {
+		t.Errorf("add failed: %v", err)
+	}
 	expectedNominatedPods := map[string][]*v1.Pod{
 		"node1": {&medPriorityPod, &unschedulablePod},
 	}
@@ -228,7 +234,9 @@ func TestPriorityQueue_Delete(t *testing.T) {
 	q := NewPriorityQueue(nil)
 	q.Update(&highPriorityPod, &highPriNominatedPod)
 	q.Add(&unschedulablePod)
-	q.Delete(&highPriNominatedPod)
+	if err := q.Delete(&highPriNominatedPod); err != nil {
+		t.Errorf("delete failed: %v", err)
+	}
 	if _, exists, _ := q.activeQ.Get(&unschedulablePod); !exists {
 		t.Errorf("Expected %v to be in activeQ.", unschedulablePod.Name)
 	}
@@ -238,7 +246,9 @@ func TestPriorityQueue_Delete(t *testing.T) {
 	if len(q.nominatedPods) != 1 {
 		t.Errorf("Expected nomindatePods to have only 'unschedulablePod': %v", q.nominatedPods)
 	}
-	q.Delete(&unschedulablePod)
+	if err := q.Delete(&unschedulablePod); err != nil {
+		t.Errorf("delete failed: %v", err)
+	}
 	if len(q.nominatedPods) != 0 {
 		t.Errorf("Expected nomindatePods to be empty: %v", q.nominatedPods)
 	}