Skip to content

Commit 0ace054

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#2889 from bskiba/extensible-prio-new
Make PriorityProcessor useful by changing return value to public
2 parents 7af6ee1 + 9c70353 commit 0ace054

File tree

4 files changed

+125
-124
lines changed

4 files changed

+125
-124
lines changed

‎vertical-pod-autoscaler/pkg/updater/priority/priority_processor.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ import (
2929
// PriorityProcessor calculates priority for pod updates.
3030
type PriorityProcessor interface {
3131
GetUpdatePriority(pod *apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler,
32-
recommendation *vpa_types.RecommendedPodResources) podPriority
32+
recommendation *vpa_types.RecommendedPodResources) PodPriority
3333
}
3434

3535
// NewProcessor creates a new default PriorityProcessor.
@@ -41,7 +41,7 @@ type defaultPriorityProcessor struct {
4141
}
4242

4343
func (*defaultPriorityProcessor) GetUpdatePriority(pod *apiv1.Pod, _ *vpa_types.VerticalPodAutoscaler,
44-
recommendation *vpa_types.RecommendedPodResources) podPriority {
44+
recommendation *vpa_types.RecommendedPodResources) PodPriority {
4545
outsideRecommendedRange := false
4646
scaleUp := false
4747
// Sum of requests over all containers, per resource type.
@@ -89,9 +89,9 @@ func (*defaultPriorityProcessor) GetUpdatePriority(pod *apiv1.Pod, _ *vpa_types.
8989
totalRequest := math.Max(float64(totalRequestPerResource[resource]), 1.0)
9090
resourceDiff += math.Abs(totalRequest-float64(totalRecommended)) / totalRequest
9191
}
92-
return podPriority{
93-
outsideRecommendedRange: outsideRecommendedRange,
94-
scaleUp: scaleUp,
95-
resourceDiff: resourceDiff,
92+
return PodPriority{
93+
OutsideRecommendedRange: outsideRecommendedRange,
94+
ScaleUp: scaleUp,
95+
ResourceDiff: resourceDiff,
9696
}
9797
}

‎vertical-pod-autoscaler/pkg/updater/priority/priority_processor_test.go‎

Lines changed: 69 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -34,43 +34,43 @@ func TestGetUpdatePriority(t *testing.T) {
3434
name string
3535
pod *corev1.Pod
3636
vpa *vpa_types.VerticalPodAutoscaler
37-
expectedPrio podPriority
37+
expectedPrio PodPriority
3838
}{
3939
{
4040
name: "simple scale up",
4141
pod: test.Pod().WithName("POD1").AddContainer(test.BuildTestContainer(containerName, "2", "")).Get(),
4242
vpa: test.VerticalPodAutoscaler().WithContainer(containerName).WithTarget("10", "").Get(),
43-
expectedPrio: podPriority{
44-
outsideRecommendedRange: false,
45-
resourceDiff: 4.0,
46-
scaleUp: true,
43+
expectedPrio: PodPriority{
44+
OutsideRecommendedRange: false,
45+
ResourceDiff: 4.0,
46+
ScaleUp: true,
4747
},
4848
}, {
4949
name: "simple scale down",
5050
pod: test.Pod().WithName("POD1").AddContainer(test.BuildTestContainer(containerName, "4", "")).Get(),
5151
vpa: test.VerticalPodAutoscaler().WithContainer(containerName).WithTarget("2", "").Get(),
52-
expectedPrio: podPriority{
53-
outsideRecommendedRange: false,
54-
resourceDiff: 0.5,
55-
scaleUp: false,
52+
expectedPrio: PodPriority{
53+
OutsideRecommendedRange: false,
54+
ResourceDiff: 0.5,
55+
ScaleUp: false,
5656
},
5757
}, {
5858
name: "no resource diff",
5959
pod: test.Pod().WithName("POD1").AddContainer(test.BuildTestContainer(containerName, "2", "")).Get(),
6060
vpa: test.VerticalPodAutoscaler().WithContainer(containerName).WithTarget("2", "").Get(),
61-
expectedPrio: podPriority{
62-
outsideRecommendedRange: false,
63-
resourceDiff: 0.0,
64-
scaleUp: false,
61+
expectedPrio: PodPriority{
62+
OutsideRecommendedRange: false,
63+
ResourceDiff: 0.0,
64+
ScaleUp: false,
6565
},
6666
}, {
6767
name: "scale up on milliquanitites",
6868
pod: test.Pod().WithName("POD1").AddContainer(test.BuildTestContainer(containerName, "10m", "")).Get(),
6969
vpa: test.VerticalPodAutoscaler().WithContainer(containerName).WithTarget("900m", "").Get(),
70-
expectedPrio: podPriority{
71-
outsideRecommendedRange: false,
72-
resourceDiff: 89.0,
73-
scaleUp: true,
70+
expectedPrio: PodPriority{
71+
OutsideRecommendedRange: false,
72+
ResourceDiff: 89.0,
73+
ScaleUp: true,
7474
},
7575
}, {
7676
name: "scale up outside recommended range",
@@ -79,10 +79,10 @@ func TestGetUpdatePriority(t *testing.T) {
7979
WithTarget("10", "").
8080
WithLowerBound("6", "").
8181
WithUpperBound("14", "").Get(),
82-
expectedPrio: podPriority{
83-
outsideRecommendedRange: true,
84-
resourceDiff: 1.5,
85-
scaleUp: true,
82+
expectedPrio: PodPriority{
83+
OutsideRecommendedRange: true,
84+
ResourceDiff: 1.5,
85+
ScaleUp: true,
8686
},
8787
}, {
8888
name: "scale down outside recommended range",
@@ -91,46 +91,46 @@ func TestGetUpdatePriority(t *testing.T) {
9191
WithTarget("2", "").
9292
WithLowerBound("1", "").
9393
WithUpperBound("3", "").Get(),
94-
expectedPrio: podPriority{
95-
outsideRecommendedRange: true,
96-
resourceDiff: 0.75,
97-
scaleUp: false,
94+
expectedPrio: PodPriority{
95+
OutsideRecommendedRange: true,
96+
ResourceDiff: 0.75,
97+
ScaleUp: false,
9898
},
9999
}, {
100100
name: "scale up with multiple quantities",
101101
pod: test.Pod().WithName("POD1").AddContainer(test.BuildTestContainer(containerName, "2", "")).Get(),
102102
vpa: test.VerticalPodAutoscaler().WithContainer(containerName).WithTarget("10", "").Get(),
103-
expectedPrio: podPriority{
104-
outsideRecommendedRange: false,
105-
resourceDiff: 4.0,
106-
scaleUp: true,
103+
expectedPrio: PodPriority{
104+
OutsideRecommendedRange: false,
105+
ResourceDiff: 4.0,
106+
ScaleUp: true,
107107
},
108108
}, {
109109
name: "multiple resources, both scale up",
110110
pod: test.Pod().WithName("POD1").AddContainer(test.BuildTestContainer(containerName, "3", "10M")).Get(),
111111
vpa: test.VerticalPodAutoscaler().WithContainer(containerName).WithTarget("6", "20M").Get(),
112-
expectedPrio: podPriority{
113-
outsideRecommendedRange: false,
114-
resourceDiff: 1.0 + 1.0, // summed relative diffs for resources
115-
scaleUp: true,
112+
expectedPrio: PodPriority{
113+
OutsideRecommendedRange: false,
114+
ResourceDiff: 1.0 + 1.0, // summed relative diffs for resources
115+
ScaleUp: true,
116116
},
117117
}, {
118118
name: "multiple resources, only one scale up",
119119
pod: test.Pod().WithName("POD1").AddContainer(test.BuildTestContainer(containerName, "4", "10M")).Get(),
120120
vpa: test.VerticalPodAutoscaler().WithContainer(containerName).WithTarget("2", "20M").Get(),
121-
expectedPrio: podPriority{
122-
outsideRecommendedRange: false,
123-
resourceDiff: 1.5 + 0.0, // summed relative diffs for resources
124-
scaleUp: true,
121+
expectedPrio: PodPriority{
122+
OutsideRecommendedRange: false,
123+
ResourceDiff: 1.5 + 0.0, // summed relative diffs for resources
124+
ScaleUp: true,
125125
},
126126
}, {
127127
name: "multiple resources, both scale down",
128128
pod: test.Pod().WithName("POD1").AddContainer(test.BuildTestContainer(containerName, "4", "20M")).Get(),
129129
vpa: test.VerticalPodAutoscaler().WithContainer(containerName).WithTarget("2", "10M").Get(),
130-
expectedPrio: podPriority{
131-
outsideRecommendedRange: false,
132-
resourceDiff: 0.5 + 0.5, // summed relative diffs for resources
133-
scaleUp: false,
130+
expectedPrio: PodPriority{
131+
OutsideRecommendedRange: false,
132+
ResourceDiff: 0.5 + 0.5, // summed relative diffs for resources
133+
ScaleUp: false,
134134
},
135135
}, {
136136
name: "multiple resources, one outside recommended range",
@@ -139,10 +139,10 @@ func TestGetUpdatePriority(t *testing.T) {
139139
WithTarget("2", "10M").
140140
WithLowerBound("1", "5M").
141141
WithUpperBound("3", "30M").Get(),
142-
expectedPrio: podPriority{
143-
outsideRecommendedRange: true,
144-
resourceDiff: 0.5 + 0.5, // summed relative diffs for resources
145-
scaleUp: false,
142+
expectedPrio: PodPriority{
143+
OutsideRecommendedRange: true,
144+
ResourceDiff: 0.5 + 0.5, // summed relative diffs for resources
145+
ScaleUp: false,
146146
},
147147
}, {
148148
name: "multiple containers, both scale up",
@@ -153,10 +153,10 @@ func TestGetUpdatePriority(t *testing.T) {
153153
test.Recommendation().
154154
WithContainer("test-container-2").
155155
WithTarget("8", "").GetContainerResources()).Get(),
156-
expectedPrio: podPriority{
157-
outsideRecommendedRange: false,
158-
resourceDiff: 3.0, // relative diff between summed requests and summed recommendations
159-
scaleUp: true,
156+
expectedPrio: PodPriority{
157+
OutsideRecommendedRange: false,
158+
ResourceDiff: 3.0, // relative diff between summed requests and summed recommendations
159+
ScaleUp: true,
160160
},
161161
}, {
162162
name: "multiple containers, both scale down",
@@ -167,10 +167,10 @@ func TestGetUpdatePriority(t *testing.T) {
167167
test.Recommendation().
168168
WithContainer("test-container-2").
169169
WithTarget("2", "").GetContainerResources()).Get(),
170-
expectedPrio: podPriority{
171-
outsideRecommendedRange: false,
172-
resourceDiff: 0.7, // relative diff between summed requests and summed recommendations
173-
scaleUp: false,
170+
expectedPrio: PodPriority{
171+
OutsideRecommendedRange: false,
172+
ResourceDiff: 0.7, // relative diff between summed requests and summed recommendations
173+
ScaleUp: false,
174174
},
175175
}, {
176176
name: "multiple containers, both scale up, one outside range",
@@ -184,10 +184,10 @@ func TestGetUpdatePriority(t *testing.T) {
184184
WithTarget("8", "").
185185
WithLowerBound("3", "").
186186
WithUpperBound("10", "").GetContainerResources()).Get(),
187-
expectedPrio: podPriority{
188-
outsideRecommendedRange: true,
189-
resourceDiff: 3.0, // relative diff between summed requests and summed recommendations
190-
scaleUp: true,
187+
expectedPrio: PodPriority{
188+
OutsideRecommendedRange: true,
189+
ResourceDiff: 3.0, // relative diff between summed requests and summed recommendations
190+
ScaleUp: true,
191191
},
192192
}, {
193193
name: "multiple containers, multiple resources",
@@ -201,11 +201,11 @@ func TestGetUpdatePriority(t *testing.T) {
201201
test.Recommendation().
202202
WithContainer("test-container-2").
203203
WithTarget("7", "30M").GetContainerResources()).Get(),
204-
expectedPrio: podPriority{
205-
outsideRecommendedRange: false,
204+
expectedPrio: PodPriority{
205+
OutsideRecommendedRange: false,
206206
// relative diff between summed requests and summed recommendations, summed over resources
207-
resourceDiff: 0.5 + 0.25,
208-
scaleUp: true,
207+
ResourceDiff: 0.5 + 0.25,
208+
ScaleUp: true,
209209
},
210210
},
211211
}
@@ -281,32 +281,32 @@ func TestGetUpdatePriority_VpaObservedContainers(t *testing.T) {
281281
// and container resource recommendations. Containers not listed
282282
// in an existing vpaObservedContainers annotations shouldn't be taken
283283
// into account during calculations.
284-
assert.InDelta(t, result.resourceDiff, tc.want, 0.0001)
284+
assert.InDelta(t, result.ResourceDiff, tc.want, 0.0001)
285285
})
286286
}
287287
}
288288

289289
type fakePriorityProcessor struct {
290-
priorities map[string]podPriority
290+
priorities map[string]PodPriority
291291
}
292292

293293
// NewFakeProcessor returns a fake processor for testing that can be initialized
294294
// with a map from pod name to priority expected to be returned.
295-
func NewFakeProcessor(priorities map[string]podPriority) PriorityProcessor {
295+
func NewFakeProcessor(priorities map[string]PodPriority) PriorityProcessor {
296296
return &fakePriorityProcessor{
297297
priorities: priorities,
298298
}
299299
}
300300

301301
func (f *fakePriorityProcessor) GetUpdatePriority(pod *corev1.Pod, vpa *vpa_types.VerticalPodAutoscaler,
302-
recommendation *vpa_types.RecommendedPodResources) podPriority {
302+
recommendation *vpa_types.RecommendedPodResources) PodPriority {
303303
prio, ok := f.priorities[pod.Name]
304304
if !ok {
305305
panic(fmt.Sprintf("Unexpected pod name: %v", pod.Name))
306306
}
307-
return podPriority{
308-
scaleUp: prio.scaleUp,
309-
resourceDiff: prio.resourceDiff,
310-
outsideRecommendedRange: prio.outsideRecommendedRange,
307+
return PodPriority{
308+
ScaleUp: prio.ScaleUp,
309+
ResourceDiff: prio.ResourceDiff,
310+
OutsideRecommendedRange: prio.OutsideRecommendedRange,
311311
}
312312
}

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

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ func (calc *UpdatePriorityCalculator) AddPod(pod *apiv1.Pod, now time.Time) {
123123
// - the request is outside the recommended range for some container.
124124
// - the pod lives for at least 24h and the resource diff is >= MinChangePriority.
125125
// - a vpa scaled container OOMed in less than evictAfterOOMThreshold.
126-
if !updatePriority.outsideRecommendedRange && !quickOOM {
126+
if !updatePriority.OutsideRecommendedRange && !quickOOM {
127127
if pod.Status.StartTime == nil {
128128
// TODO: Set proper condition on the VPA.
129129
klog.V(2).Infof("not updating pod %v/%v, missing field pod.Status.StartTime", pod.Namespace, pod.Name)
@@ -133,18 +133,18 @@ func (calc *UpdatePriorityCalculator) AddPod(pod *apiv1.Pod, now time.Time) {
133133
klog.V(2).Infof("not updating a short-lived pod %v/%v, request within recommended range", pod.Namespace, pod.Name)
134134
return
135135
}
136-
if updatePriority.resourceDiff < calc.config.MinChangePriority {
136+
if updatePriority.ResourceDiff < calc.config.MinChangePriority {
137137
klog.V(2).Infof("not updating pod %v/%v, resource diff too low: %v", pod.Namespace, pod.Name, updatePriority)
138138
return
139139
}
140140
}
141141

142142
// If the pod has quick OOMed then evict only if the resources will change
143-
if quickOOM && updatePriority.resourceDiff == 0 {
143+
if quickOOM && updatePriority.ResourceDiff == 0 {
144144
klog.V(2).Infof("not updating pod %v/%v because resource would not change", pod.Namespace, pod.Name)
145145
return
146146
}
147-
klog.V(2).Infof("pod accepted for update %v/%v with priority %v", pod.Namespace, pod.Name, updatePriority.resourceDiff)
147+
klog.V(2).Infof("pod accepted for update %v/%v with priority %v", pod.Namespace, pod.Name, updatePriority.ResourceDiff)
148148
calc.pods = append(calc.pods, prioritizedPod{
149149
pod: pod,
150150
priority: updatePriority,
@@ -183,17 +183,18 @@ func parseVpaObservedContainers(pod *apiv1.Pod) (bool, sets.String) {
183183

184184
type prioritizedPod struct {
185185
pod *apiv1.Pod
186-
priority podPriority
186+
priority PodPriority
187187
recommendation *vpa_types.RecommendedPodResources
188188
}
189189

190-
type podPriority struct {
190+
// PodPriority contains data for a pod update that can be used to prioritize between updates.
191+
type PodPriority struct {
191192
// Is any container outside of the recommended range.
192-
outsideRecommendedRange bool
193+
OutsideRecommendedRange bool
193194
// Does any container want to grow.
194-
scaleUp bool
195+
ScaleUp bool
195196
// Relative difference between the total requested and total recommended resources.
196-
resourceDiff float64
197+
ResourceDiff float64
197198
}
198199

199200
type byPriority []prioritizedPod
@@ -212,9 +213,9 @@ func (list byPriority) Less(i, j int) bool {
212213
// (a) the pod is pending
213214
// (b) there is general resource shortage
214215
// and prioritize scaling up otherwise.
215-
if list[i].priority.scaleUp != list[j].priority.scaleUp {
216-
return list[i].priority.scaleUp
216+
if list[i].priority.ScaleUp != list[j].priority.ScaleUp {
217+
return list[i].priority.ScaleUp
217218
}
218219
// 2. A pod with larger value of resourceDiff takes precedence.
219-
return list[i].priority.resourceDiff > list[j].priority.resourceDiff
220+
return list[i].priority.ResourceDiff > list[j].priority.ResourceDiff
220221
}

0 commit comments

Comments
 (0)