From a11ca9dc24ca2cfe5c82f820ab064b06c897d30f Mon Sep 17 00:00:00 2001 From: Max Cao Date: Tue, 3 Dec 2024 13:27:29 -0800 Subject: [PATCH] VPA: Slightly improve runtime on splitting aggregates Signed-off-by: Max Cao --- .../pkg/recommender/model/cluster.go | 37 ++++++++----------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/recommender/model/cluster.go b/vertical-pod-autoscaler/pkg/recommender/model/cluster.go index 09cce9e9c25..71fea092f5d 100644 --- a/vertical-pod-autoscaler/pkg/recommender/model/cluster.go +++ b/vertical-pod-autoscaler/pkg/recommender/model/cluster.go @@ -130,22 +130,24 @@ func (cluster *ClusterState) AddOrUpdatePod(podID PodID, newLabels labels.Set, p } newlabelSetKey := cluster.getLabelSetKey(newLabels) - if podExists && pod.labelSetKey != newlabelSetKey { - // This Pod is already counted in the old VPA, remove the link. - cluster.removePodFromItsVpa(pod) - } - if !podExists || pod.labelSetKey != newlabelSetKey { + if pod.labelSetKey != newlabelSetKey { + if podExists { + // This Pod is already counted in the old VPA, remove the link. + cluster.removePodFromItsVpa(pod) + } pod.labelSetKey = newlabelSetKey // Set the links between the containers and aggregations based on the current pod labels. for containerName, container := range pod.Containers { containerID := ContainerID{PodID: podID, ContainerName: containerName} container.aggregator = cluster.findOrCreateAggregateContainerState(containerID) } - - cluster.addPodToItsVpa(pod) + cluster.setVPAContainersPerPod(pod, true) + } else if !podExists { + cluster.setVPAContainersPerPod(pod, true) + } else if len(pod.Containers) > 1 { + // Tally the number of containers for later when we're averaging the recommendations if there's more than one container + cluster.setVPAContainersPerPod(pod, false) } - // Tally the number of containers for later when we're averaging the recommendations - cluster.setVPAContainersPerPod(pod) pod.Phase = phase } @@ -153,7 +155,8 @@ func (cluster *ClusterState) AddOrUpdatePod(podID PodID, newLabels labels.Set, p // so that later when we're splitting the minimum recommendations over containers, we're splitting them over // the correct number and not just the number of aggregates that have *ever* been present. (We don't want minimum resources // to erroneously shrink, either) -func (cluster *ClusterState) setVPAContainersPerPod(pod *PodState) { +// If addPodToItsVpa is true, it also increments the pod count for the VPA. +func (cluster *ClusterState) setVPAContainersPerPod(pod *PodState, addPodToItsVpa bool) { for _, vpa := range cluster.Vpas { if vpa_utils.PodLabelsMatchVPA(pod.ID.Namespace, cluster.labelSetMap[pod.labelSetKey], vpa.ID.Namespace, vpa.PodSelector) { // We want the "high water mark" of the most containers in the pod in the event @@ -161,17 +164,9 @@ func (cluster *ClusterState) setVPAContainersPerPod(pod *PodState) { if len(pod.Containers) > vpa.ContainersPerPod { vpa.ContainersPerPod = len(pod.Containers) } - } - } - -} - -// addPodToItsVpa increases the count of Pods associated with a VPA object. -// Does a scan similar to findOrCreateAggregateContainerState so could be optimized if needed. -func (cluster *ClusterState) addPodToItsVpa(pod *PodState) { - for _, vpa := range cluster.Vpas { - if vpa_utils.PodLabelsMatchVPA(pod.ID.Namespace, cluster.labelSetMap[pod.labelSetKey], vpa.ID.Namespace, vpa.PodSelector) { - vpa.PodCount++ + if addPodToItsVpa { + vpa.PodCount++ + } } } }