Skip to content

Commit b7cd0e1

Browse files
committed
VPA: Revert using containerStatus resources to calculate update priority
Reverts the change that takes containerStatuses resources in to account when calculating update priority. This change, along with along VPA to use containerStatuses when calculating recommendations themselves, should instead be included in a future enhancement and potentially feature-gated. Signed-off-by: Max Cao <[email protected]>
1 parent 6252e92 commit b7cd0e1

File tree

1 file changed

+1
-34
lines changed

1 file changed

+1
-34
lines changed

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

Lines changed: 1 addition & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ func (*defaultPriorityProcessor) GetUpdatePriority(pod *apiv1.Pod, vpa *vpa_type
5252

5353
hasObservedContainers, vpaContainerSet := parseVpaObservedContainers(pod)
5454

55-
for num, podContainer := range pod.Spec.Containers {
55+
for _, podContainer := range pod.Spec.Containers {
5656
if hasObservedContainers && !vpaContainerSet.Has(podContainer.Name) {
5757
klog.V(4).InfoS("Not listed in VPA observed containers label. Skipping container priority calculations", "label", annotations.VpaObservedContainersLabel, "observedContainers", pod.GetAnnotations()[annotations.VpaObservedContainersLabel], "containerName", podContainer.Name, "vpa", klog.KObj(vpa))
5858
continue
@@ -74,36 +74,6 @@ func (*defaultPriorityProcessor) GetUpdatePriority(pod *apiv1.Pod, vpa *vpa_type
7474
(hasUpperBound && request.Cmp(upperBound) > 0) {
7575
outsideRecommendedRange = true
7676
}
77-
78-
// TODO(maxcao13): Can we just ignore the spec, and use status.containerStatus.resources now?
79-
// Apparently: This also means that resources field in the pod spec can no longer be relied upon as an indicator of the pod's actual resources.
80-
// reference: https://kubernetes.io/blog/2023/05/12/in-place-pod-resize-alpha/
81-
// KEP reference: https://github.com/kubernetes/enhancements/pull/5089/files#diff-14542847beb0f0fd767db1aff1316f8569a968385e2bb89567c4cc0af1ae5942R761
82-
// Although this seems like a big API change (wouldn't work for VPA on kubernetes < 1.33 without feature gate applied). I'll leave it up for reviewers.
83-
// IMO, this should probably be implemented for a followup enhancement.
84-
85-
// Statuses can be missing, or status resources can be nil
86-
if len(pod.Status.ContainerStatuses) > num && pod.Status.ContainerStatuses[num].Resources != nil {
87-
if statusRequest, hasStatusRequest := pod.Status.ContainerStatuses[num].Resources.Requests[resourceName]; hasStatusRequest {
88-
// If we're updating, but we still don't have what we asked for, we may still need to act on this pod
89-
if request.MilliValue() > statusRequest.MilliValue() {
90-
scaleUp = true
91-
// It's okay if we're actually still resizing, but if we can't now or we're stuck, make sure the pod
92-
// is still in the list so we can evict it to go live on a fatter node or something
93-
if pod.Status.Resize == apiv1.PodResizeStatusDeferred || pod.Status.Resize == apiv1.PodResizeStatusInfeasible {
94-
klog.V(4).InfoS("Pod looks like it's stuck scaling up, leaving it in for eviction", "pod", klog.KObj(pod), "resizeStatus", pod.Status.Resize)
95-
} else {
96-
klog.V(4).InfoS("Pod is in the process of scaling up, leaving it in so we can see if it's taking too long", "pod", klog.KObj(pod), "resizeStatus", pod.Status.Resize)
97-
}
98-
}
99-
// I guess if it's not outside of compliance, it's probably okay it's stuck here?
100-
if (hasLowerBound && statusRequest.Cmp(lowerBound) < 0) ||
101-
(hasUpperBound && statusRequest.Cmp(upperBound) > 0) {
102-
outsideRecommendedRange = true
103-
}
104-
}
105-
}
106-
10777
} else {
10878
// Note: if the request is not specified, the container will use the
10979
// namespace default request. Currently we ignore it and treat such
@@ -115,9 +85,6 @@ func (*defaultPriorityProcessor) GetUpdatePriority(pod *apiv1.Pod, vpa *vpa_type
11585
}
11686
}
11787

118-
// TODO(jkyros): hmm this gets hairy here because if "status" is what let us into the list,
119-
// we probably need to do these calculations vs the status rather than the spec, because the
120-
// spec is a "lie"
12188
resourceDiff := 0.0
12289
for resource, totalRecommended := range totalRecommendedPerResource {
12390
totalRequest := math.Max(float64(totalRequestPerResource[resource]), 1.0)

0 commit comments

Comments
 (0)