Merge pull request kubernetes#83558 from hprateek43/sortable_list_pac…

…kage Move Sortable List into its own package
liufangwai · Oct 12, 2019 · aee99ce · aee99ce
2 parents 457fa6b + 5896561
commit aee99ce
Show file tree

Hide file tree

Showing 5 changed files with 16 additions and 74 deletions.
diff --git a/pkg/scheduler/core/extender_test.go b/pkg/scheduler/core/extender_test.go
@@ -19,6 +19,7 @@ package core
 import (
 	"fmt"
 	"reflect"
+	"sort"
 	"testing"
 	"time"
 
@@ -202,7 +203,7 @@ func (f *FakeExtender) selectVictimsOnNodeByExtender(
 	// and get cached node info by given node name.
 	nodeInfoCopy := f.cachedNodeNameToInfo[node.GetName()].Clone()
 
-	potentialVictims := util.SortableList{CompFunc: util.MoreImportantPod}
+	var potentialVictims []*v1.Pod
 
 	removePod := func(rp *v1.Pod) {
 		nodeInfoCopy.RemovePod(rp)
@@ -215,11 +216,11 @@ func (f *FakeExtender) selectVictimsOnNodeByExtender(
 	podPriority := podutil.GetPodPriority(pod)
 	for _, p := range nodeInfoCopy.Pods() {
 		if podutil.GetPodPriority(p) < podPriority {
-			potentialVictims.Items = append(potentialVictims.Items, p)
+			potentialVictims = append(potentialVictims, p)
 			removePod(p)
 		}
 	}
-	potentialVictims.Sort()
+	sort.Slice(potentialVictims, func(i, j int) bool { return util.MoreImportantPod(potentialVictims[i], potentialVictims[j]) })
 
 	// If the new pod does not fit after removing all the lower priority pods,
 	// we are almost done and this node is not suitable for preemption.
@@ -248,8 +249,8 @@ func (f *FakeExtender) selectVictimsOnNodeByExtender(
 
 	// For now, assume all potential victims to be non-violating.
 	// Now we try to reprieve non-violating victims.
-	for _, p := range potentialVictims.Items {
-		reprievePod(p.(*v1.Pod))
+	for _, p := range potentialVictims {
+		reprievePod(p)
 	}
 
 	return victims, numViolatingVictim, true, nil

diff --git a/pkg/scheduler/core/generic_scheduler.go b/pkg/scheduler/core/generic_scheduler.go
@@ -1052,9 +1052,9 @@ func (g *genericScheduler) selectNodesForPreemption(
 // preempted.
 // This function is stable and does not change the order of received pods. So, if it
 // receives a sorted list, grouping will preserve the order of the input list.
-func filterPodsWithPDBViolation(pods []interface{}, pdbs []*policy.PodDisruptionBudget) (violatingPods, nonViolatingPods []*v1.Pod) {
+func filterPodsWithPDBViolation(pods []*v1.Pod, pdbs []*policy.PodDisruptionBudget) (violatingPods, nonViolatingPods []*v1.Pod) {
 	for _, obj := range pods {
-		pod := obj.(*v1.Pod)
+		pod := obj
 		pdbForPodIsViolated := false
 		// A pod with no labels will not match any PDB. So, no need to check.
 		if len(pod.Labels) != 0 {
@@ -1110,7 +1110,7 @@ func (g *genericScheduler) selectVictimsOnNode(
 	queue internalqueue.SchedulingQueue,
 	pdbs []*policy.PodDisruptionBudget,
 ) ([]*v1.Pod, int, bool) {
-	potentialVictims := util.SortableList{CompFunc: util.MoreImportantPod}
+	var potentialVictims []*v1.Pod
 
 	removePod := func(rp *v1.Pod) error {
 		if err := nodeInfo.RemovePod(rp); err != nil {
@@ -1145,7 +1145,7 @@ func (g *genericScheduler) selectVictimsOnNode(
 	podPriority := podutil.GetPodPriority(pod)
 	for _, p := range nodeInfo.Pods() {
 		if podutil.GetPodPriority(p) < podPriority {
-			potentialVictims.Items = append(potentialVictims.Items, p)
+			potentialVictims = append(potentialVictims, p)
 			if err := removePod(p); err != nil {
 				return nil, 0, false
 			}
@@ -1166,11 +1166,11 @@ func (g *genericScheduler) selectVictimsOnNode(
 	}
 	var victims []*v1.Pod
 	numViolatingVictim := 0
-	potentialVictims.Sort()
+	sort.Slice(potentialVictims, func(i, j int) bool { return util.MoreImportantPod(potentialVictims[i], potentialVictims[j]) })
 	// Try to reprieve as many pods as possible. We first try to reprieve the PDB
 	// violating victims and then other non-violating ones. In both cases, we start
 	// from the highest priority victims.
-	violatingVictims, nonViolatingVictims := filterPodsWithPDBViolation(potentialVictims.Items, pdbs)
+	violatingVictims, nonViolatingVictims := filterPodsWithPDBViolation(potentialVictims, pdbs)
 	reprievePod := func(p *v1.Pod) (bool, error) {
 		if err := addPod(p); err != nil {
 			return false, err

diff --git a/pkg/scheduler/util/BUILD b/pkg/scheduler/util/BUILD
@@ -14,7 +14,6 @@ go_test(
     ],
     embed = [":go_default_library"],
     deps = [
-        "//pkg/api/v1/pod:go_default_library",
         "//pkg/scheduler/apis/extender/v1:go_default_library",
         "//staging/src/k8s.io/api/core/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",

diff --git a/pkg/scheduler/util/utils.go b/pkg/scheduler/util/utils.go
@@ -17,7 +17,6 @@ limitations under the License.
 package util
 
 import (
-	"sort"
 	"time"
 
 	v1 "k8s.io/api/core/v1"
@@ -92,39 +91,15 @@ func GetEarliestPodStartTime(victims *extenderv1.Victims) *metav1.Time {
 	return earliestPodStartTime
 }
 
-// SortableList is a list that implements sort.Interface.
-type SortableList struct {
-	Items    []interface{}
-	CompFunc lessFunc
-}
-
-var _ = sort.Interface(&SortableList{})
-
-func (l *SortableList) Len() int { return len(l.Items) }
-
-func (l *SortableList) Less(i, j int) bool {
-	return l.CompFunc(l.Items[i], l.Items[j])
-}
-
-func (l *SortableList) Swap(i, j int) {
-	l.Items[i], l.Items[j] = l.Items[j], l.Items[i]
-}
-
-// Sort sorts the items in the list using the given CompFunc. Item1 is placed
-// before Item2 when CompFunc(Item1, Item2) returns true.
-func (l *SortableList) Sort() {
-	sort.Sort(l)
-}
-
 // MoreImportantPod return true when priority of the first pod is higher than
 // the second one. If two pods' priorities are equal, compare their StartTime.
 // It takes arguments of the type "interface{}" to be used with SortableList,
 // but expects those arguments to be *v1.Pod.
-func MoreImportantPod(pod1, pod2 interface{}) bool {
-	p1 := podutil.GetPodPriority(pod1.(*v1.Pod))
-	p2 := podutil.GetPodPriority(pod2.(*v1.Pod))
+func MoreImportantPod(pod1, pod2 *v1.Pod) bool {
+	p1 := podutil.GetPodPriority(pod1)
+	p2 := podutil.GetPodPriority(pod2)
 	if p1 != p2 {
 		return p1 > p2
 	}
-	return GetPodStartTime(pod1.(*v1.Pod)).Before(GetPodStartTime(pod2.(*v1.Pod)))
+	return GetPodStartTime(pod1).Before(GetPodStartTime(pod2))
 }
diff --git a/pkg/scheduler/util/utils_test.go b/pkg/scheduler/util/utils_test.go
@@ -25,44 +25,11 @@ import (
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/diff"
-	"k8s.io/kubernetes/pkg/api/v1/pod"
 	extenderv1 "k8s.io/kubernetes/pkg/scheduler/apis/extender/v1"
 )
 
 // TestSortableList tests SortableList by storing pods in the list and sorting
 // them by their priority.
-func TestSortableList(t *testing.T) {
-	higherPriority := func(pod1, pod2 interface{}) bool {
-		return pod.GetPodPriority(pod1.(*v1.Pod)) > pod.GetPodPriority(pod2.(*v1.Pod))
-	}
-	podList := SortableList{CompFunc: higherPriority}
-	// Add a few Pods with different priorities from lowest to highest priority.
-	for i := 0; i < 10; i++ {
-		var p = int32(i)
-		pod := &v1.Pod{
-			Spec: v1.PodSpec{
-				Containers: []v1.Container{
-					{
-						Name:  "container",
-						Image: "image",
-					},
-				},
-				Priority: &p,
-			},
-		}
-		podList.Items = append(podList.Items, pod)
-	}
-	podList.Sort()
-	if len(podList.Items) != 10 {
-		t.Errorf("expected length of list was 10, got: %v", len(podList.Items))
-	}
-	var prevPriority = int32(10)
-	for _, p := range podList.Items {
-		if *p.(*v1.Pod).Spec.Priority >= prevPriority {
-			t.Errorf("Pods are not soreted. Current pod pririty is %v, while previous one was %v.", *p.(*v1.Pod).Spec.Priority, prevPriority)
-		}
-	}
-}
 
 func TestGetContainerPorts(t *testing.T) {
 	tests := []struct {