VPA: Revert changes that related to disruption/disruptionless changes

Because of kubernetes#7813, this commit reverts a lot of the changes that introducted logic that involved actuating in-place updates based on the containerResizePolicy.

Signed-off-by: Max Cao <[email protected]>

Author: maxcao13

vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction.go

@@ -444,31 +444,14 @@ func (e *podsEvictionRestrictionImpl) CanInPlaceUpdate(pod *apiv1.Pod) bool {
 			return false
 		}
 
-		noRestartPoliciesPopulated := true
-		isPodRestartPolicyNever := pod.Spec.RestartPolicy == apiv1.RestartPolicyNever
-
 		for _, container := range pod.Spec.Containers {
 			// If some of these are populated, we know it at least understands resizing
-			if len(container.ResizePolicy) > 0 {
-				noRestartPoliciesPopulated = false
-			}
-
-			for _, policy := range container.ResizePolicy {
-				if policy.RestartPolicy != apiv1.NotRequired {
-					klog.V(4).InfoS("in-place resize of pod will cause container disruption, because of container resize policy", "pod", klog.KObj(pod), "container", container.Name, "containerResizeRestartPolicy", policy.RestartPolicy)
-					if isPodRestartPolicyNever {
-						klog.InfoS("in-place resize of pod not possible, container resize policy and pod restartPolicy conflict", "pod", klog.KObj(pod), "container", container.Name, "containerResizeRestartPolicy", policy.RestartPolicy, "podRestartPolicy", pod.Spec.RestartPolicy)
-						return false
-					}
-				}
+			if container.ResizePolicy == nil {
+				klog.InfoS("Can't resize pod, container resize policy does not exist; is InPlacePodVerticalScaling enabled?", "pod", klog.KObj(pod))
+				return false
 			}
 		}
 
-		// If none of the policies are populated, our feature is probably not enabled, so we can't in-place regardless
-		if noRestartPoliciesPopulated {
-			klog.InfoS("impossible to resize pod in-place, container resize policies are not populated", "pod", klog.KObj(pod))
-		}
-
 		singleGroupStats, present := e.creatorToSingleGroupStatsMap[cr]
 		// If we're pending, we can't in-place resize
 		// TODO(jkyros): are we sure we can't? Should I just set this to "if running"?
@@ -483,7 +466,7 @@ func (e *podsEvictionRestrictionImpl) CanInPlaceUpdate(pod *apiv1.Pod) bool {
 			// number of pods that are actually running
 			actuallyAlive := singleGroupStats.running - (singleGroupStats.evicted + singleGroupStats.inPlaceUpdating)
 			klog.V(4).InfoS("Checking pod disruption tolerance",
-				"podName", pod.Name,
+				"pod", klog.KObj(pod),
 				"configuredPods", singleGroupStats.configured,
 				"runningPods", singleGroupStats.running,
 				"evictedPods", singleGroupStats.evicted,
@@ -493,15 +476,15 @@ func (e *podsEvictionRestrictionImpl) CanInPlaceUpdate(pod *apiv1.Pod) bool {
 				"actuallyAlive", actuallyAlive,
 			)
 			if actuallyAlive > shouldBeAlive {
-				klog.V(4).InfoS("Pod can be resized in-place; more pods are running than required", "podName", pod.Name, "shouldBeAlive", shouldBeAlive, "actuallyAlive", actuallyAlive)
+				klog.V(4).InfoS("Pod can be resized in-place; more pods are running than required", "pod", klog.KObj(pod), "shouldBeAlive", shouldBeAlive, "actuallyAlive", actuallyAlive)
 				return true
 			}
 
 			// If all pods are running, no pods are being evicted or updated, and eviction tolerance is small, we can resize in-place
 			if singleGroupStats.running == singleGroupStats.configured &&
 				singleGroupStats.evictionTolerance == 0 &&
 				singleGroupStats.evicted == 0 && singleGroupStats.inPlaceUpdating == 0 {
-				klog.V(4).InfoS("Pod can be resized in-place; all pods are running and eviction tolerance is 0", "podName", pod.Name)
+				klog.V(4).InfoS("Pod can be resized in-place; all pods are running and eviction tolerance is 0", "pod", klog.KObj(pod))
 				return true
 			}
 		}
@@ -584,6 +567,8 @@ func (e *podsEvictionRestrictionImpl) InPlaceUpdate(podToUpdate *apiv1.Pod, vpa
 	return nil
 }
 
+// TODO(maxcao13): Switch to conditions after 1.33 is released: https://github.com/kubernetes/enhancements/pull/5089
+
 // IsInPlaceUpdating checks whether or not the given pod is currently in the middle of an in-place update
 func IsInPlaceUpdating(podToCheck *apiv1.Pod) (isUpdating bool) {
 	return podToCheck.Status.Resize != ""

vertical-pod-autoscaler/pkg/updater/logic/updater.go

@@ -55,7 +55,7 @@ import (
 const (
 	// DeferredResizeUpdateTimeout defines the duration during which an in-place resize request
 	// is considered deferred. If the resize is not completed within this time, it falls back to eviction.
-	DeferredResizeUpdateTimeout = 1 * time.Minute
+	DeferredResizeUpdateTimeout = 5 * time.Minute
 
 	// InProgressResizeUpdateTimeout defines the duration during which an in-place resize request
 	// is considered in progress. If the resize is not completed within this time, it falls back to eviction.
@@ -243,8 +243,7 @@ func (u *updater) RunOnce(ctx context.Context) {
 		withEvictable := false
 		withEvicted := false
 
-		for _, updatablePod := range podsForUpdate {
-			pod := updatablePod.Pod()
+		for _, pod := range podsForUpdate {
 			if vpa_api_util.GetUpdateMode(vpa) == vpa_types.UpdateModeInPlaceOrRecreate {
 				withInPlaceUpdatable = true
 				fallBackToEviction, err := u.AttemptInPlaceUpdate(ctx, vpa, pod, evictionLimiter)
@@ -253,14 +252,7 @@ func (u *updater) RunOnce(ctx context.Context) {
 					return
 				}
 				if fallBackToEviction {
-					// TODO(jkyros): this needs to be cleaner, but we absolutely need to make sure a disruptionless update doesn't "sneak through"
-					// if the pod is disruptionless, there's no need to evict, just wait until the next updater loop
-					if updatablePod.IsDisruptionless() {
-						klog.InfoS("Not falling back to eviction, pod was supposed to be disruptionless", "pod", klog.KObj(pod))
-						continue
-					} else {
-						klog.V(4).InfoS("Falling back to eviction for pod", "pod", klog.KObj(pod))
-					}
+					klog.V(4).InfoS("Falling back to eviction for pod", "pod", klog.KObj(pod))
 				} else {
 					withInPlaceUpdated = true
 					metrics_updater.AddInPlaceUpdatedPod(vpaSize)
@@ -303,20 +295,6 @@ func (u *updater) RunOnce(ctx context.Context) {
 	timer.ObserveStep("EvictPods")
 }
 
-// VpaRecommendationProvided checks the VPA status to see if it has provided a recommendation yet. Used
-// to make sure we don't get bogus values for in-place scaling
-// TODO(jkyros):  take this out when you find the proper place to gate this
-func VpaRecommendationProvided(vpa *vpa_types.VerticalPodAutoscaler) bool {
-	// for _, condition := range vpa.Status.Conditions {
-	// 	if condition.Type == vpa_types.RecommendationProvided && condition.Status == apiv1.ConditionTrue {
-	// 		return true
-	// 	}
-	// }
-	// TODO(maxcao13): The above condition doesn't work in tests because sometimes there is no recommender to set this status
-	// so we should check the recommendation field directly. Or we can set the above condition manually in tests.
-	return vpa.Status.Recommendation != nil
-}
-
 func getRateLimiter(evictionRateLimit float64, evictionRateLimitBurst int) *rate.Limiter {
 	var evictionRateLimiter *rate.Limiter
 	if evictionRateLimit <= 0 {
@@ -331,7 +309,7 @@ func getRateLimiter(evictionRateLimit float64, evictionRateLimitBurst int) *rate
 }
 
 // getPodsUpdateOrder returns list of pods that should be updated ordered by update priority
-func (u *updater) getPodsUpdateOrder(pods []*apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler) []*priority.PrioritizedPod {
+func (u *updater) getPodsUpdateOrder(pods []*apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler) []*apiv1.Pod {
 	priorityCalculator := priority.NewUpdatePriorityCalculator(
 		vpa,
 		nil,
@@ -342,10 +320,9 @@ func (u *updater) getPodsUpdateOrder(pods []*apiv1.Pod, vpa *vpa_types.VerticalP
 		priorityCalculator.AddPod(pod, time.Now())
 	}
 
-	return priorityCalculator.GetSortedPrioritizedPods(u.evictionAdmission)
+	return priorityCalculator.GetSortedPods(u.evictionAdmission)
 }
 
-// TODO(maxcao13): Deprecated in favour of filterNonUpdatablePods
 func filterNonEvictablePods(pods []*apiv1.Pod, evictionRestriction eviction.PodsEvictionRestriction) []*apiv1.Pod {
 	result := make([]*apiv1.Pod, 0)
 	for _, pod := range pods {
@@ -411,6 +388,7 @@ func (u *updater) AttemptInPlaceUpdate(ctx context.Context, vpa *vpa_types.Verti
 				lastInPlaceUpdateTime = time.Now()
 				u.lastInPlaceUpdateAttemptTimeMap[eviction.GetPodID(pod)] = lastInPlaceUpdateTime
 			}
+			// TODO(maxcao13): fix this after 1.33 KEP changes
 			// if currently inPlaceUpdating, we should only fallback to eviction if the update has failed. i.e: one of the following conditions:
 			// 1. .status.resize: Infeasible
 			// 2. .status.resize: Deferred + more than 1 minute has elapsed since the lastInPlaceUpdateTime
@@ -446,7 +424,6 @@ func (u *updater) AttemptInPlaceUpdate(ctx context.Context, vpa *vpa_types.Verti
 	// TODO(jkyros): need our own rate limiter or can we freeload off the eviction one?
 	err = u.evictionRateLimiter.Wait(ctx)
 	if err != nil {
-		// TODO(jkyros): whether or not we fall back to eviction here probably depends on *why* we failed
 		klog.ErrorS(err, "Eviction rate limiter wait failed for in-place resize", "pod", klog.KObj(pod))
 		return false, err
 	}

vertical-pod-autoscaler/pkg/updater/main.go

@@ -58,7 +58,6 @@ var (
 	minReplicas = flag.Int("min-replicas", 2,
 		`Minimum number of replicas to perform update`)
 
-	// TODO(maxcao13): Should this be combined into disruption tolerance, or should we have a separate flag for that, or we just don't rename?
 	evictionToleranceFraction = flag.Float64("eviction-tolerance", 0.5,
 		`Fraction of replica count that can be evicted for update, if more than one pod can be evicted.`)
 

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

@@ -75,16 +75,12 @@ func (*defaultPriorityProcessor) GetUpdatePriority(pod *apiv1.Pod, vpa *vpa_type
 					outsideRecommendedRange = true
 				}
 
-				// TODO(jkyros): I think we're picking up early zeroes here from the VPA when it has no recommendation, I think that's why I have to wait
-				// for the recommendation later before I try to scale in-place
-				// TODO(jkyros): For in place VPA, this might be gross, but we need this pod to be in the eviction list because it doesn't actually have
-				// the resources it asked for even if the spec is right, and we might need to fall back to evicting it
-				// TODO(jkyros): Can we have empty container status at this point for real? It's at least failing the tests if we don't check, but
-				// we could just populate the status in the tests
 				// TODO(maxcao13): Can we just ignore the spec, and use status.containerStatus.resources now?
 				// 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.
 				// reference: https://kubernetes.io/blog/2023/05/12/in-place-pod-resize-alpha/
 				// KEP reference: https://github.com/kubernetes/enhancements/pull/5089/files#diff-14542847beb0f0fd767db1aff1316f8569a968385e2bb89567c4cc0af1ae5942R761
+				// 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.
+				// IMO, this should probably be implemented for a followup enhancement.
 
 				// Statuses can be missing, or status resources can be nil
 				if len(pod.Status.ContainerStatuses) > num && pod.Status.ContainerStatuses[num].Resources != nil {

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

@@ -49,7 +49,7 @@ var (
 // than pod with 100M current memory and 150M recommendation (100% increase vs 50% increase)
 type UpdatePriorityCalculator struct {
 	vpa                     *vpa_types.VerticalPodAutoscaler
-	pods                    []PrioritizedPod
+	pods                    []prioritizedPod
 	config                  *UpdateConfig
 	recommendationProcessor vpa_api_util.RecommendationProcessor
 	priorityProcessor       PriorityProcessor
@@ -117,8 +117,6 @@ func (calc *UpdatePriorityCalculator) AddPod(pod *apiv1.Pod, now time.Time) {
 		}
 	}
 
-	disruptionlessRecommendation := calc.CalculateDisruptionFreeActions(pod, processedRecommendation)
-
 	// The update is allowed in following cases:
 	// - the request is outside the recommended range for some container.
 	// - the pod lives for at least 24h and the resource diff is >= MinChangePriority.
@@ -129,31 +127,12 @@ func (calc *UpdatePriorityCalculator) AddPod(pod *apiv1.Pod, now time.Time) {
 			klog.V(4).InfoS("Not updating pod, missing field pod.Status.StartTime", "pod", klog.KObj(pod))
 			return
 		}
-		// TODO(maxcao13): hopefully this doesn't break anything but we switch the order so that significant change is checked first before lifetime
-		// this way we don't in-place scale it for insignificant change, else we would mark it disruptionless and still have an in-place update
-		if updatePriority.ResourceDiff < calc.config.MinChangePriority {
-			klog.V(4).InfoS("Not updating pod, resource diff too low", "pod", klog.KObj(pod), "updatePriority", updatePriority)
+		if now.Before(pod.Status.StartTime.Add(*podLifetimeUpdateThreshold)) {
+			klog.V(4).InfoS("Not updating a short-lived pod, request within recommended range", "pod", klog.KObj(pod))
 			return
 		}
-		if now.Before(pod.Status.StartTime.Add(*podLifetimeUpdateThreshold)) {
-			// TODO(jkyros): do we need an in-place update threshold arg ?
-			// If our recommendations are disruptionless, we can bypass the threshold limit
-			if len(disruptionlessRecommendation.ContainerRecommendations) > 0 {
-				klog.V(2).InfoS("Short-lived, but pod still accepted for disruptionless in-place update",
-					"pod", klog.KObj(pod),
-					"numContainers", len(pod.Spec.Containers),
-					"resourceDiff", updatePriority.ResourceDiff,
-					"fractionOfDisruptionlessRecommendations", len(disruptionlessRecommendation.ContainerRecommendations)/len(processedRecommendation.ContainerRecommendations),
-				)
-				updatePriority.Disruptionless = true
-				calc.pods = append(calc.pods, PrioritizedPod{
-					pod:            pod,
-					priority:       updatePriority,
-					recommendation: disruptionlessRecommendation})
-			} else {
-				// we cannot perform this update disruption-free, so do not update this pod's resources
-				klog.V(4).InfoS("Not updating a short-lived pod, request within recommended range", "pod", klog.KObj(pod))
-			}
+		if updatePriority.ResourceDiff < calc.config.MinChangePriority {
+			klog.V(4).InfoS("Not updating pod, resource diff too low", "pod", klog.KObj(pod), "updatePriority", updatePriority)
 			return
 		}
 	}
@@ -164,30 +143,12 @@ func (calc *UpdatePriorityCalculator) AddPod(pod *apiv1.Pod, now time.Time) {
 		return
 	}
 	klog.V(2).InfoS("Pod accepted for update", "pod", klog.KObj(pod), "updatePriority", updatePriority.ResourceDiff, "processedRecommendations", calc.GetProcessedRecommendationTargets(processedRecommendation))
-	calc.pods = append(calc.pods, PrioritizedPod{
+	calc.pods = append(calc.pods, prioritizedPod{
 		pod:            pod,
 		priority:       updatePriority,
 		recommendation: processedRecommendation})
 }
 
-// GetSortedPrioritizedPods returns a list of prioritized pods ordered by update priority (highest update priority first). Used instead
-// of GetSortedPods when we need access to the priority information
-func (calc *UpdatePriorityCalculator) GetSortedPrioritizedPods(admission PodEvictionAdmission) []*PrioritizedPod {
-	sort.Sort(byPriorityDesc(calc.pods))
-
-	//result := []*apiv1.Pod{}
-	result := []*PrioritizedPod{}
-	for num, podPrio := range calc.pods {
-		if admission.Admit(podPrio.pod, podPrio.recommendation) {
-			result = append(result, &calc.pods[num])
-		} else {
-			klog.V(2).InfoS("Pod removed from update queue by PodEvictionAdmission", "pod", klog.KObj(podPrio.Pod()))
-		}
-	}
-
-	return result
-}
-
 // GetSortedPods returns a list of pods ordered by update priority (highest update priority first)
 func (calc *UpdatePriorityCalculator) GetSortedPods(admission PodEvictionAdmission) []*apiv1.Pod {
 	sort.Sort(byPriorityDesc(calc.pods))
@@ -251,25 +212,12 @@ func parseVpaObservedContainers(pod *apiv1.Pod) (bool, sets.Set[string]) {
 	return hasObservedContainers, vpaContainerSet
 }
 
-// PrioritizedPod contains the priority and recommendation details for a pod.
-// TODO(jkyros): I made this public, but there may be a cleaner way
-type PrioritizedPod struct {
+type prioritizedPod struct {
 	pod            *apiv1.Pod
 	priority       PodPriority
 	recommendation *vpa_types.RecommendedPodResources
 }
 
-// IsDisruptionless returns the disruptionless status of the underlying pod priority
-// TODO(jkyros): scope issues, maybe not the best place to put Disruptionless
-func (p PrioritizedPod) IsDisruptionless() bool {
-	return p.priority.Disruptionless
-}
-
-// Pod returns the underlying private pod
-func (p PrioritizedPod) Pod() *apiv1.Pod {
-	return p.pod
-}
-
 // PodPriority contains data for a pod update that can be used to prioritize between updates.
 type PodPriority struct {
 	// Is any container outside of the recommended range.
@@ -278,11 +226,9 @@ type PodPriority struct {
 	ScaleUp bool
 	// Relative difference between the total requested and total recommended resources.
 	ResourceDiff float64
-	// Is this update disruptionless
-	Disruptionless bool
 }
 
-type byPriorityDesc []PrioritizedPod
+type byPriorityDesc []prioritizedPod
 
 func (list byPriorityDesc) Len() int {
 	return len(list)
@@ -310,67 +256,3 @@ func (p PodPriority) Less(other PodPriority) bool {
 	// 2. A pod with larger value of resourceDiff takes precedence.
 	return p.ResourceDiff < other.ResourceDiff
 }
-
-// CalculateDisruptionFreeActions calculates the set of actions we think we can perform without disruption based on the pod/container resize/restart
-// policies and returns that set of actions.
-func (calc *UpdatePriorityCalculator) CalculateDisruptionFreeActions(pod *apiv1.Pod, recommendation *vpa_types.RecommendedPodResources) *vpa_types.RecommendedPodResources {
-
-	var disruptionlessRecommendation = &vpa_types.RecommendedPodResources{}
-
-	for _, container := range pod.Spec.Containers {
-		// If we don't have a resize policy, we can't check it
-		if len(container.ResizePolicy) == 0 {
-			continue
-		}
-
-		// So we get whatever the recommendation was for this container
-		resourceRec := getRecommendationForContainerName(container.Name, recommendation)
-		// If we didn't find a recommendation for this container, we don't have anything to do
-		if resourceRec == nil {
-			continue
-		}
-		// Then we go through all the resource recommendations it has
-		for resource := range resourceRec.Target {
-			// And we look up what the restart policy is for those resources
-			resourceRestartPolicy := getRestartPolicyForResource(resource, container.ResizePolicy)
-			// If we don't have one, that's probably bad
-			if resourceRestartPolicy == nil {
-				continue
-			}
-			// If we do have one, and it's disruptive, then we know this won't work
-			if *resourceRestartPolicy != apiv1.NotRequired {
-				continue
-			}
-
-		}
-
-		// And if we made it here, we should theoretically be able to do this without disruption
-		disruptionlessRecommendation.ContainerRecommendations = append(disruptionlessRecommendation.ContainerRecommendations, *resourceRec)
-
-	}
-
-	return disruptionlessRecommendation
-}
-
-// getRecommendationForContainerName searches through the list of ContainerRecommendations until it finds one matching the named container. Used
-// to match up containers with their recommendations (we have container, we want resource recommendation)
-func getRecommendationForContainerName(name string, recommendation *vpa_types.RecommendedPodResources) *vpa_types.RecommendedContainerResources {
-	for _, recommendationContainer := range recommendation.ContainerRecommendations {
-		if recommendationContainer.ContainerName == name {
-			return &recommendationContainer
-		}
-	}
-	return nil
-}
-
-// getRestartPolicyForResource searches through the list of resources in the resize policy until it finds the one matching the named resource. Used
-// to match up restart policies with our resource recommendations (we have resource, we want policy).
-func getRestartPolicyForResource(resourceName apiv1.ResourceName, policy []apiv1.ContainerResizePolicy) *apiv1.ResourceResizeRestartPolicy {
-	// TODO(jkyros): can there be duplicate policies for resources? we just take the first one now
-	for _, resizePolicy := range policy {
-		if resizePolicy.ResourceName == resourceName {
-			return &resizePolicy.RestartPolicy
-		}
-	}
-	return nil
-}