Merge pull request kubernetes#2897 from bskiba/fix_less

Correct Less function for priority.

Author: k8s-ci-robot

vertical-pod-autoscaler/pkg/updater/priority/update_priority_calculator.go

@@ -153,7 +153,7 @@ func (calc *UpdatePriorityCalculator) AddPod(pod *apiv1.Pod, now time.Time) {
 
 // GetSortedPods returns a list of pods ordered by update priority (highest update priority first)
 func (calc *UpdatePriorityCalculator) GetSortedPods(admission PodEvictionAdmission) []*apiv1.Pod {
-	sort.Sort(byPriority(calc.pods))
+	sort.Sort(byPriorityDesc(calc.pods))
 
 	result := []*apiv1.Pod{}
 	for _, podPrio := range calc.pods {
@@ -197,30 +197,31 @@ type PodPriority struct {
 	ResourceDiff float64
 }
 
-type byPriority []prioritizedPod
+type byPriorityDesc []prioritizedPod
 
-func (list byPriority) Len() int {
+func (list byPriorityDesc) Len() int {
 	return len(list)
 }
-func (list byPriority) Swap(i, j int) {
+func (list byPriorityDesc) Swap(i, j int) {
 	list[i], list[j] = list[j], list[i]
 }
 
 // Less implements reverse ordering by priority (highest priority first).
-func (list byPriority) Less(i, j int) bool {
-	return list[i].priority.Less(list[j].priority)
+// This means we return true if priority at index j is lower than at index i.
+func (list byPriorityDesc) Less(i, j int) bool {
+	return list[j].priority.Less(list[i].priority)
 }
 
-// Less compares pod update priorities.
+// Less returns true if p is lower than other.
 func (p PodPriority) Less(other PodPriority) bool {
 	// 1. If any container wants to grow, the pod takes precedence.
 	// TODO: A better policy would be to prioritize scaling down when
 	// (a) the pod is pending
 	// (b) there is general resource shortage
 	// and prioritize scaling up otherwise.
 	if p.ScaleUp != other.ScaleUp {
-		return p.ScaleUp
+		return other.ScaleUp
 	}
 	// 2. A pod with larger value of resourceDiff takes precedence.
-	return p.ResourceDiff > other.ResourceDiff
+	return p.ResourceDiff < other.ResourceDiff
 }

vertical-pod-autoscaler/pkg/updater/priority/update_priority_calculator_test.go

@@ -499,3 +499,58 @@ func TestAdmission(t *testing.T) {
 	result := calculator.GetSortedPods(&pod1Admission{})
 	assert.Exactly(t, []*apiv1.Pod{pod1}, result, "Wrong priority order")
 }
+
+func TestLessPodPriority(t *testing.T) {
+	testCases := []struct {
+		name        string
+		prio, other PodPriority
+		isLess      bool
+	}{
+		{
+			name: "scale down more than empty",
+			prio: PodPriority{
+				ScaleUp:      false,
+				ResourceDiff: 0.1,
+			},
+			other:  PodPriority{},
+			isLess: false,
+		}, {
+			name: "scale up more than empty",
+			prio: PodPriority{
+				ScaleUp:      true,
+				ResourceDiff: 0.1,
+			},
+			other:  PodPriority{},
+			isLess: false,
+		}, {
+			name: "two scale ups",
+			prio: PodPriority{
+				ScaleUp:      true,
+				ResourceDiff: 0.1,
+			},
+			other: PodPriority{
+				ScaleUp:      true,
+				ResourceDiff: 1.0,
+			},
+			isLess: true,
+		}, {
+			name: "two scale downs",
+			prio: PodPriority{
+				ScaleUp:      false,
+				ResourceDiff: 0.9,
+			},
+			other: PodPriority{
+				ScaleUp:      false,
+				ResourceDiff: 0.1,
+			},
+			isLess: false,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.isLess, tc.prio.Less(tc.other))
+			assert.Equal(t, !tc.isLess, tc.other.Less(tc.prio))
+		})
+	}
+
+}