address raywainman and omerap12 comments

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

Author: maxcao13

vertical-pod-autoscaler/pkg/updater/inplace/inplace_updated.go

@@ -28,6 +28,8 @@ import (
 
 type inPlaceUpdate struct{}
 
+// CalculatePatches returns a patch that adds a "vpaInPlaceUpdated" annotation
+// to the pod, marking it as having been requested to be updated in-place by VPA.
 func (*inPlaceUpdate) CalculatePatches(pod *core.Pod, _ *vpa_types.VerticalPodAutoscaler) ([]resource_admission.PatchRecord, error) {
 	vpaInPlaceUpdatedValue := annotations.GetVpaInPlaceUpdatedValue()
 	return []resource_admission.PatchRecord{patch.GetAddAnnotationPatch(annotations.VpaInPlaceUpdatedLabel, vpaInPlaceUpdatedValue)}, nil

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

@@ -252,6 +252,7 @@ func (u *updater) RunOnce(ctx context.Context) {
 			podsForInPlace = u.getPodsUpdateOrder(filterNonInPlaceUpdatablePods(livePods, inPlaceLimiter), vpa)
 			inPlaceUpdatablePodsCounter.Add(vpaSize, len(podsForInPlace))
 		} else {
+			// If the feature gate is not enabled but update mode is InPlaceOrRecreate, updater will always fallback to eviction.
 			if updateMode == vpa_types.UpdateModeInPlaceOrRecreate {
 				klog.InfoS("Warning: feature gate is not enabled for this updateMode", "featuregate", features.InPlaceOrRecreate, "updateMode", vpa_types.UpdateModeInPlaceOrRecreate)
 			}
@@ -283,7 +284,7 @@ func (u *updater) RunOnce(ctx context.Context) {
 			err := inPlaceLimiter.InPlaceUpdate(pod, vpa, u.eventRecorder)
 			if err != nil {
 				klog.V(0).InfoS("In-place update failed", "error", err, "pod", klog.KObj(pod))
-				return
+				continue
 			}
 			withInPlaceUpdated = true
 			metrics_updater.AddInPlaceUpdatedPod(vpaSize)

vertical-pod-autoscaler/pkg/updater/restriction/pods_inplace_restriction.go

@@ -174,3 +174,45 @@ func (ip *PodsInPlaceRestrictionImpl) InPlaceUpdate(podToUpdate *apiv1.Pod, vpa
 
 	return nil
 }
+
+// CanEvictInPlacingPod checks if the pod can be evicted while it is currently in the middle of an in-place update.
+func CanEvictInPlacingPod(pod *apiv1.Pod, singleGroupStats singleGroupStats, lastInPlaceAttemptTimeMap map[string]time.Time, clock clock.Clock) bool {
+	if !isInPlaceUpdating(pod) {
+		return false
+	}
+	lastUpdate, exists := lastInPlaceAttemptTimeMap[getPodID(pod)]
+	if !exists {
+		klog.V(4).InfoS("In-place update in progress for pod but no lastUpdateTime found, setting it to now", "pod", klog.KObj(pod))
+		lastUpdate = clock.Now()
+		lastInPlaceAttemptTimeMap[getPodID(pod)] = lastUpdate
+	}
+
+	if singleGroupStats.isPodDisruptable() {
+		// 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 5 minutes has elapsed since the lastInPlaceUpdateTime
+		// 3. .status.resize: InProgress + more than 1 hour has elapsed since the lastInPlaceUpdateTime
+		switch pod.Status.Resize {
+		case apiv1.PodResizeStatusDeferred:
+			if clock.Since(lastUpdate) > DeferredResizeUpdateTimeout {
+				klog.V(4).InfoS(fmt.Sprintf("In-place update deferred for more than %v, falling back to eviction", DeferredResizeUpdateTimeout), "pod", klog.KObj(pod))
+				return true
+			}
+		case apiv1.PodResizeStatusInProgress:
+			if clock.Since(lastUpdate) > InProgressResizeUpdateTimeout {
+				klog.V(4).InfoS(fmt.Sprintf("In-place update in progress for more than %v, falling back to eviction", InProgressResizeUpdateTimeout), "pod", klog.KObj(pod))
+				return true
+			}
+		case apiv1.PodResizeStatusInfeasible:
+			klog.V(4).InfoS("In-place update infeasible, falling back to eviction", "pod", klog.KObj(pod))
+			return true
+		default:
+			klog.V(4).InfoS("In-place update status unknown, falling back to eviction", "pod", klog.KObj(pod))
+			return true
+		}
+		return false
+	}
+	klog.V(4).InfoS("Would be able to evict, but already resizing", "pod", klog.KObj(pod))
+	return false
+}

vertical-pod-autoscaler/pkg/updater/restriction/pods_restriction_factory.go

@@ -350,45 +350,3 @@ func (s *singleGroupStats) isPodDisruptable() bool {
 func isInPlaceUpdating(podToCheck *apiv1.Pod) bool {
 	return podToCheck.Status.Resize != ""
 }
-
-// CanEvictInPlacingPod checks if the pod can be evicted while it is currently in the middle of an in-place update.
-func CanEvictInPlacingPod(pod *apiv1.Pod, singleGroupStats singleGroupStats, lastInPlaceAttemptTimeMap map[string]time.Time, clock clock.Clock) bool {
-	if !isInPlaceUpdating(pod) {
-		return false
-	}
-	lastUpdate, exists := lastInPlaceAttemptTimeMap[getPodID(pod)]
-	if !exists {
-		klog.V(4).InfoS("In-place update in progress for pod but no lastUpdateTime found, setting it to now", "pod", klog.KObj(pod))
-		lastUpdate = clock.Now()
-		lastInPlaceAttemptTimeMap[getPodID(pod)] = lastUpdate
-	}
-
-	if singleGroupStats.isPodDisruptable() {
-		// 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 5 minutes has elapsed since the lastInPlaceUpdateTime
-		// 3. .status.resize: InProgress + more than 1 hour has elapsed since the lastInPlaceUpdateTime
-		switch pod.Status.Resize {
-		case apiv1.PodResizeStatusDeferred:
-			if clock.Since(lastUpdate) > DeferredResizeUpdateTimeout {
-				klog.V(4).InfoS(fmt.Sprintf("In-place update deferred for more than %v, falling back to eviction", DeferredResizeUpdateTimeout), "pod", klog.KObj(pod))
-				return true
-			}
-		case apiv1.PodResizeStatusInProgress:
-			if clock.Since(lastUpdate) > InProgressResizeUpdateTimeout {
-				klog.V(4).InfoS(fmt.Sprintf("In-place update in progress for more than %v, falling back to eviction", InProgressResizeUpdateTimeout), "pod", klog.KObj(pod))
-				return true
-			}
-		case apiv1.PodResizeStatusInfeasible:
-			klog.V(4).InfoS("In-place update infeasible, falling back to eviction", "pod", klog.KObj(pod))
-			return true
-		default:
-			klog.V(4).InfoS("In-place update status unknown, falling back to eviction", "pod", klog.KObj(pod))
-			return true
-		}
-		return false
-	}
-	klog.V(4).InfoS("Would be able to evict, but already resizing", "pod", klog.KObj(pod))
-	return false
-}