From fae4f69d3633c41c781fd5dbe6bfc56453dbcec6 Mon Sep 17 00:00:00 2001 From: Jonathan Basseri Date: Fri, 14 Dec 2018 16:31:08 -0800 Subject: [PATCH 1/2] 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 d34a186ce8..f5e0a6fd26 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 cc0a057fdf..60c0d7b30d 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) } From d27d28a44e6a20b94dfbd6981b25e4a31f51285f Mon Sep 17 00:00:00 2001 From: Jonathan Basseri Date: Fri, 14 Dec 2018 16:33:12 -0800 Subject: [PATCH 2/2] Flatten nominated pod logic in PriorityQueue. This replaces deeply nested ifs & fors with early returns & continues. --- .../internal/queue/scheduling_queue.go | 37 ++++++++++--------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/pkg/scheduler/internal/queue/scheduling_queue.go b/pkg/scheduler/internal/queue/scheduling_queue.go index f5e0a6fd26..cb185fbf0b 100644 --- a/pkg/scheduler/internal/queue/scheduling_queue.go +++ b/pkg/scheduler/internal/queue/scheduling_queue.go @@ -276,31 +276,34 @@ func (p *PriorityQueue) run() { // already exist in the map. Adding an existing pod is not going to update the pod. func (p *PriorityQueue) addNominatedPodIfNeeded(pod *v1.Pod) { nnn := NominatedNodeName(pod) - if len(nnn) > 0 { - for _, np := range p.nominatedPods[nnn] { - if np.UID == pod.UID { - klog.V(4).Infof("Pod %v/%v already exists in the nominated map!", pod.Namespace, pod.Name) - return - } - } - p.nominatedPods[nnn] = append(p.nominatedPods[nnn], pod) + if len(nnn) <= 0 { + return } + for _, np := range p.nominatedPods[nnn] { + if np.UID == pod.UID { + klog.V(4).Infof("Pod %v/%v already exists in the nominated map!", pod.Namespace, pod.Name) + return + } + } + p.nominatedPods[nnn] = append(p.nominatedPods[nnn], pod) } // deleteNominatedPodIfExists deletes a pod from the nominatedPods. // NOTE: this function assumes lock has been acquired in caller. func (p *PriorityQueue) deleteNominatedPodIfExists(pod *v1.Pod) { nnn := NominatedNodeName(pod) - if len(nnn) > 0 { - for i, np := range p.nominatedPods[nnn] { - if np.UID == pod.UID { - p.nominatedPods[nnn] = append(p.nominatedPods[nnn][:i], p.nominatedPods[nnn][i+1:]...) - if len(p.nominatedPods[nnn]) == 0 { - delete(p.nominatedPods, nnn) - } - break - } + if len(nnn) <= 0 { + return + } + for i, np := range p.nominatedPods[nnn] { + if np.UID != pod.UID { + continue } + p.nominatedPods[nnn] = append(p.nominatedPods[nnn][:i], p.nominatedPods[nnn][i+1:]...) + if len(p.nominatedPods[nnn]) == 0 { + delete(p.nominatedPods, nnn) + } + break } }