diff --git a/vertical-pod-autoscaler/deploy/vpa-rbac.yaml b/vertical-pod-autoscaler/deploy/vpa-rbac.yaml index 0ff58467b8b..fda8cec53b5 100644 --- a/vertical-pod-autoscaler/deploy/vpa-rbac.yaml +++ b/vertical-pod-autoscaler/deploy/vpa-rbac.yaml @@ -121,6 +121,32 @@ rules: - create --- apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: system:vpa-updater-in-place +rules: + - apiGroups: + - "" + resources: + - pods/resize + - pods # required for patching vpaInPlaceUpdated annotations onto the pod + verbs: + - patch +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: system:vpa-updater-in-place-binding +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: system:vpa-updater-in-place +subjects: + - kind: ServiceAccount + name: vpa-updater + namespace: kube-system +--- +apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding metadata: name: system:metrics-reader diff --git a/vertical-pod-autoscaler/hack/vpa-process-yaml.sh b/vertical-pod-autoscaler/hack/vpa-process-yaml.sh index 47e0b805f7e..8e70bce1800 100755 --- a/vertical-pod-autoscaler/hack/vpa-process-yaml.sh +++ b/vertical-pod-autoscaler/hack/vpa-process-yaml.sh @@ -24,8 +24,8 @@ function print_help { echo "separator and substituting REGISTRY and TAG for pod images" } -# Requires input from stdin, otherwise hangs. Checks for "admission-controller", "updater", or "recommender", and -# applies the respective kubectl patch command to add the feature gates specified in the FEATURE_GATES environment variable. +# Requires input from stdin, otherwise hangs. If the input is a Deployment manifest, +# apply kubectl patch to add feature gates specified in the FEATURE_GATES environment variable. # e.g. cat file.yaml | apply_feature_gate function apply_feature_gate() { local input="" @@ -33,11 +33,13 @@ function apply_feature_gate() { input+="$line"$'\n' done - if [ -n "${FEATURE_GATES}" ]; then - if echo "$input" | grep -q "admission-controller"; then - echo "$input" | kubectl patch --type=json --local -p='[{"op": "add", "path": "/spec/template/spec/containers/0/args/-", "value": "--feature-gates='"${FEATURE_GATES}"'"}]' -o yaml -f - - elif echo "$input" | grep -q "updater" || echo "$input" | grep -q "recommender"; then - echo "$input" | kubectl patch --type=json --local -p='[{"op": "add", "path": "/spec/template/spec/containers/0/args", "value": ["--feature-gates='"${FEATURE_GATES}"'"]}]' -o yaml -f - + # matching precisely "kind: Deployment" to avoid matching "kind: DeploymentConfig" or a line with extra whitespace + if echo "$input" | grep -qE '^kind: Deployment$'; then + if [ -n "${FEATURE_GATES}" ]; then + if ! echo "$input" | kubectl patch --type=json --local -p='[{"op": "add", "path": "/spec/template/spec/containers/0/args/-", "value": "--feature-gates='"${FEATURE_GATES}"'"}]' -o yaml -f - 2>/dev/null; then + # If it fails, there was no args field, so we need to add it + echo "$input" | kubectl patch --type=json --local -p='[{"op": "add", "path": "/spec/template/spec/containers/0/args", "value": ["--feature-gates='"${FEATURE_GATES}"'"]}]' -o yaml -f - + fi else echo "$input" fi diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/handler_test.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/handler_test.go index aa8c5aa7aba..6477eeb33d9 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/handler_test.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/handler_test.go @@ -54,6 +54,10 @@ type fakePatchCalculator struct { err error } +func (*fakePatchCalculator) PatchResourceTarget() patch.PatchResourceTarget { + return patch.Pod +} + func (c *fakePatchCalculator) CalculatePatches(_ *apiv1.Pod, _ *vpa_types.VerticalPodAutoscaler) ( []resource_admission.PatchRecord, error) { return c.patches, c.err diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/calculator.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/calculator.go index 8b92d2204bb..0cd12571390 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/calculator.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/calculator.go @@ -23,7 +23,22 @@ import ( vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" ) +// PatchResourceTarget is the type of resource that can be patched. +type PatchResourceTarget string + +const ( + // Pod refers to the pod resource itself. + Pod PatchResourceTarget = "Pod" + // Resize refers to the resize subresource of the pod. + Resize PatchResourceTarget = "Resize" + + // Future subresources can be added here. + // e.g. Status PatchResourceTarget = "Status" +) + // Calculator is capable of calculating required patches for pod. type Calculator interface { CalculatePatches(pod *core.Pod, vpa *vpa_types.VerticalPodAutoscaler) ([]resource.PatchRecord, error) + // PatchResourceTarget returns the resource this calculator should calculate patches for. + PatchResourceTarget() PatchResourceTarget } diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/observed_containers.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/observed_containers.go index 98e7262e5ad..f80b79bf954 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/observed_containers.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/observed_containers.go @@ -31,6 +31,10 @@ func (*observedContainers) CalculatePatches(pod *core.Pod, _ *vpa_types.Vertical return []resource_admission.PatchRecord{GetAddAnnotationPatch(annotations.VpaObservedContainersLabel, vpaObservedContainersValue)}, nil } +func (*observedContainers) PatchResourceTarget() PatchResourceTarget { + return Pod +} + // NewObservedContainersCalculator returns calculator for // observed containers patches. func NewObservedContainersCalculator() Calculator { diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/resource_updates.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/resource_updates.go index 9be03f1c00d..963dfa29c07 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/resource_updates.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/resource_updates.go @@ -21,7 +21,6 @@ import ( "strings" core "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/resource" resource_admission "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation" @@ -47,6 +46,10 @@ func NewResourceUpdatesCalculator(recommendationProvider recommendation.Provider } } +func (*resourcesUpdatesPatchCalculator) PatchResourceTarget() PatchResourceTarget { + return Pod +} + func (c *resourcesUpdatesPatchCalculator) CalculatePatches(pod *core.Pod, vpa *vpa_types.VerticalPodAutoscaler) ([]resource_admission.PatchRecord, error) { result := []resource_admission.PatchRecord{} @@ -78,7 +81,7 @@ func getContainerPatch(pod *core.Pod, i int, annotationsPerContainer vpa_api_uti // Add empty resources object if missing. if pod.Spec.Containers[i].Resources.Limits == nil && pod.Spec.Containers[i].Resources.Requests == nil { - patches = append(patches, getPatchInitializingEmptyResources(i)) + patches = append(patches, GetPatchInitializingEmptyResources(i)) } annotations, found := annotationsPerContainer[pod.Spec.Containers[i].Name] @@ -96,34 +99,11 @@ func getContainerPatch(pod *core.Pod, i int, annotationsPerContainer vpa_api_uti func appendPatchesAndAnnotations(patches []resource_admission.PatchRecord, annotations []string, current core.ResourceList, containerIndex int, resources core.ResourceList, fieldName, resourceName string) ([]resource_admission.PatchRecord, []string) { // Add empty object if it's missing and we're about to fill it. if current == nil && len(resources) > 0 { - patches = append(patches, getPatchInitializingEmptyResourcesSubfield(containerIndex, fieldName)) + patches = append(patches, GetPatchInitializingEmptyResourcesSubfield(containerIndex, fieldName)) } for resource, request := range resources { - patches = append(patches, getAddResourceRequirementValuePatch(containerIndex, fieldName, resource, request)) + patches = append(patches, GetAddResourceRequirementValuePatch(containerIndex, fieldName, resource, request)) annotations = append(annotations, fmt.Sprintf("%s %s", resource, resourceName)) } return patches, annotations } - -func getAddResourceRequirementValuePatch(i int, kind string, resource core.ResourceName, quantity resource.Quantity) resource_admission.PatchRecord { - return resource_admission.PatchRecord{ - Op: "add", - Path: fmt.Sprintf("/spec/containers/%d/resources/%s/%s", i, kind, resource), - Value: quantity.String()} -} - -func getPatchInitializingEmptyResources(i int) resource_admission.PatchRecord { - return resource_admission.PatchRecord{ - Op: "add", - Path: fmt.Sprintf("/spec/containers/%d/resources", i), - Value: core.ResourceRequirements{}, - } -} - -func getPatchInitializingEmptyResourcesSubfield(i int, kind string) resource_admission.PatchRecord { - return resource_admission.PatchRecord{ - Op: "add", - Path: fmt.Sprintf("/spec/containers/%d/resources/%s", i, kind), - Value: core.ResourceList{}, - } -} diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/util.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/util.go index 7bdea7988ab..0c68ab6cd55 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/util.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/util.go @@ -19,6 +19,9 @@ package patch import ( "fmt" + core "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + resource_admission "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource" ) @@ -39,3 +42,30 @@ func GetAddAnnotationPatch(annotationName, annotationValue string) resource_admi Value: annotationValue, } } + +// GetAddResourceRequirementValuePatch returns a patch record to add resource requirements to a container. +func GetAddResourceRequirementValuePatch(i int, kind string, resource core.ResourceName, quantity resource.Quantity) resource_admission.PatchRecord { + return resource_admission.PatchRecord{ + Op: "add", + Path: fmt.Sprintf("/spec/containers/%d/resources/%s/%s", i, kind, resource), + Value: quantity.String()} +} + +// GetPatchInitializingEmptyResources returns a patch record to initialize an empty resources object for a container. +func GetPatchInitializingEmptyResources(i int) resource_admission.PatchRecord { + return resource_admission.PatchRecord{ + Op: "add", + Path: fmt.Sprintf("/spec/containers/%d/resources", i), + Value: core.ResourceRequirements{}, + } +} + +// GetPatchInitializingEmptyResourcesSubfield returns a patch record to initialize an empty subfield +// (e.g., "requests" or "limits") within a container's resources object. +func GetPatchInitializingEmptyResourcesSubfield(i int, kind string) resource_admission.PatchRecord { + return resource_admission.PatchRecord{ + Op: "add", + Path: fmt.Sprintf("/spec/containers/%d/resources/%s", i, kind), + Value: core.ResourceList{}, + } +} diff --git a/vertical-pod-autoscaler/pkg/updater/inplace/inplace_updated.go b/vertical-pod-autoscaler/pkg/updater/inplace/inplace_updated.go new file mode 100644 index 00000000000..31ea45a0990 --- /dev/null +++ b/vertical-pod-autoscaler/pkg/updater/inplace/inplace_updated.go @@ -0,0 +1,46 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package inplace + +import ( + core "k8s.io/api/core/v1" + + resource_admission "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch" + + vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/annotations" +) + +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 +} + +func (*inPlaceUpdate) PatchResourceTarget() patch.PatchResourceTarget { + return patch.Pod +} + +// NewInPlaceUpdatedCalculator returns calculator for +// observed containers patches. +func NewInPlaceUpdatedCalculator() patch.Calculator { + return &inPlaceUpdate{} +} diff --git a/vertical-pod-autoscaler/pkg/updater/inplace/resource_updates.go b/vertical-pod-autoscaler/pkg/updater/inplace/resource_updates.go new file mode 100644 index 00000000000..44d9ea1ace1 --- /dev/null +++ b/vertical-pod-autoscaler/pkg/updater/inplace/resource_updates.go @@ -0,0 +1,88 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package inplace + +import ( + "fmt" + + core "k8s.io/api/core/v1" + + resource_admission "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation" + vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" + vpa_api_util "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/vpa" +) + +type resourcesInplaceUpdatesPatchCalculator struct { + recommendationProvider recommendation.Provider +} + +// NewResourceInPlaceUpdatesCalculator returns a calculator for +// in-place resource update patches. +func NewResourceInPlaceUpdatesCalculator(recommendationProvider recommendation.Provider) patch.Calculator { + return &resourcesInplaceUpdatesPatchCalculator{ + recommendationProvider: recommendationProvider, + } +} + +// PatchResourceTarget returns the resize subresource to apply calculator patches. +func (*resourcesInplaceUpdatesPatchCalculator) PatchResourceTarget() patch.PatchResourceTarget { + return patch.Resize +} + +// CalculatePatches calculates a JSON patch from a VPA's recommendation to send to the pod "resize" subresource as an in-place resize. +func (c *resourcesInplaceUpdatesPatchCalculator) CalculatePatches(pod *core.Pod, vpa *vpa_types.VerticalPodAutoscaler) ([]resource_admission.PatchRecord, error) { + result := []resource_admission.PatchRecord{} + + containersResources, _, err := c.recommendationProvider.GetContainersResourcesForPod(pod, vpa) + if err != nil { + return []resource_admission.PatchRecord{}, fmt.Errorf("Failed to calculate resource patch for pod %s/%s: %v", pod.Namespace, pod.Name, err) + } + + for i, containerResources := range containersResources { + newPatches := getContainerPatch(pod, i, containerResources) + result = append(result, newPatches...) + } + + return result, nil +} + +func getContainerPatch(pod *core.Pod, i int, containerResources vpa_api_util.ContainerResources) []resource_admission.PatchRecord { + var patches []resource_admission.PatchRecord + // Add empty resources object if missing. + if pod.Spec.Containers[i].Resources.Limits == nil && + pod.Spec.Containers[i].Resources.Requests == nil { + patches = append(patches, patch.GetPatchInitializingEmptyResources(i)) + } + + patches = appendPatches(patches, pod.Spec.Containers[i].Resources.Requests, i, containerResources.Requests, "requests") + patches = appendPatches(patches, pod.Spec.Containers[i].Resources.Limits, i, containerResources.Limits, "limits") + + return patches +} + +func appendPatches(patches []resource_admission.PatchRecord, current core.ResourceList, containerIndex int, resources core.ResourceList, fieldName string) []resource_admission.PatchRecord { + // Add empty object if it's missing and we're about to fill it. + if current == nil && len(resources) > 0 { + patches = append(patches, patch.GetPatchInitializingEmptyResourcesSubfield(containerIndex, fieldName)) + } + for resource, request := range resources { + patches = append(patches, patch.GetAddResourceRequirementValuePatch(containerIndex, fieldName, resource, request)) + } + return patches +} diff --git a/vertical-pod-autoscaler/pkg/updater/logic/updater.go b/vertical-pod-autoscaler/pkg/updater/logic/updater.go index b0029315372..7e7d0f1b21b 100644 --- a/vertical-pod-autoscaler/pkg/updater/logic/updater.go +++ b/vertical-pod-autoscaler/pkg/updater/logic/updater.go @@ -31,6 +31,8 @@ import ( kube_client "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" + utils "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/updater/utils" + corescheme "k8s.io/client-go/kubernetes/scheme" clientv1 "k8s.io/client-go/kubernetes/typed/core/v1" v1lister "k8s.io/client-go/listers/core/v1" @@ -38,14 +40,16 @@ import ( "k8s.io/client-go/tools/record" "k8s.io/klog/v2" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch" vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" vpa_clientset "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/clientset/versioned" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/clientset/versioned/scheme" vpa_lister "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/listers/autoscaling.k8s.io/v1" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/features" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target" controllerfetcher "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target/controller_fetcher" - "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/updater/eviction" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/updater/priority" + restriction "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/updater/restriction" metrics_updater "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/metrics/updater" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/status" vpa_api_util "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/vpa" @@ -61,11 +65,12 @@ type updater struct { vpaLister vpa_lister.VerticalPodAutoscalerLister podLister v1lister.PodLister eventRecorder record.EventRecorder - evictionFactory eviction.PodsEvictionRestrictionFactory + restrictionFactory restriction.PodsRestrictionFactory recommendationProcessor vpa_api_util.RecommendationProcessor evictionAdmission priority.PodEvictionAdmission priorityProcessor priority.PriorityProcessor evictionRateLimiter *rate.Limiter + inPlaceRateLimiter *rate.Limiter selectorFetcher target.VpaTargetSelectorFetcher useAdmissionControllerStatus bool statusValidator status.Validator @@ -77,7 +82,7 @@ type updater struct { func NewUpdater( kubeClient kube_client.Interface, vpaClient *vpa_clientset.Clientset, - minReplicasForEvicition int, + minReplicasForEviction int, evictionRateLimit float64, evictionRateBurst int, evictionToleranceFraction float64, @@ -90,19 +95,29 @@ func NewUpdater( priorityProcessor priority.PriorityProcessor, namespace string, ignoredNamespaces []string, + patchCalculators []patch.Calculator, ) (Updater, error) { evictionRateLimiter := getRateLimiter(evictionRateLimit, evictionRateBurst) - factory, err := eviction.NewPodsEvictionRestrictionFactory(kubeClient, minReplicasForEvicition, evictionToleranceFraction) + // TODO: Create in-place rate limits for the in-place rate limiter + inPlaceRateLimiter := getRateLimiter(evictionRateLimit, evictionRateBurst) + factory, err := restriction.NewPodsRestrictionFactory( + kubeClient, + minReplicasForEviction, + evictionToleranceFraction, + patchCalculators, + ) if err != nil { - return nil, fmt.Errorf("Failed to create eviction restriction factory: %v", err) + return nil, fmt.Errorf("Failed to create restriction factory: %v", err) } + return &updater{ vpaLister: vpa_api_util.NewVpasLister(vpaClient, make(chan struct{}), namespace), podLister: newPodLister(kubeClient, namespace), eventRecorder: newEventRecorder(kubeClient), - evictionFactory: factory, + restrictionFactory: factory, recommendationProcessor: recommendationProcessor, evictionRateLimiter: evictionRateLimiter, + inPlaceRateLimiter: inPlaceRateLimiter, evictionAdmission: evictionAdmission, priorityProcessor: priorityProcessor, selectorFetcher: selectorFetcher, @@ -149,8 +164,8 @@ func (u *updater) RunOnce(ctx context.Context) { continue } if vpa_api_util.GetUpdateMode(vpa) != vpa_types.UpdateModeRecreate && - vpa_api_util.GetUpdateMode(vpa) != vpa_types.UpdateModeAuto { - klog.V(3).InfoS("Skipping VPA object because its mode is not \"Recreate\" or \"Auto\"", "vpa", klog.KObj(vpa)) + vpa_api_util.GetUpdateMode(vpa) != vpa_types.UpdateModeAuto && vpa_api_util.GetUpdateMode(vpa) != vpa_types.UpdateModeInPlaceOrRecreate { + klog.V(3).InfoS("Skipping VPA object because its mode is not \"InPlaceOrRecreate\", \"Recreate\" or \"Auto\"", "vpa", klog.KObj(vpa)) continue } selector, err := u.selectorFetcher.Fetch(ctx, vpa) @@ -198,32 +213,89 @@ func (u *updater) RunOnce(ctx context.Context) { // wrappers for metrics which are computed every loop run controlledPodsCounter := metrics_updater.NewControlledPodsCounter() evictablePodsCounter := metrics_updater.NewEvictablePodsCounter() + inPlaceUpdatablePodsCounter := metrics_updater.NewInPlaceUpdtateablePodsCounter() vpasWithEvictablePodsCounter := metrics_updater.NewVpasWithEvictablePodsCounter() vpasWithEvictedPodsCounter := metrics_updater.NewVpasWithEvictedPodsCounter() + vpasWithInPlaceUpdatablePodsCounter := metrics_updater.NewVpasWithInPlaceUpdtateablePodsCounter() + vpasWithInPlaceUpdatedPodsCounter := metrics_updater.NewVpasWithInPlaceUpdtatedPodsCounter() + // using defer to protect against 'return' after evictionRateLimiter.Wait defer controlledPodsCounter.Observe() defer evictablePodsCounter.Observe() defer vpasWithEvictablePodsCounter.Observe() defer vpasWithEvictedPodsCounter.Observe() + // separate counters for in-place + defer inPlaceUpdatablePodsCounter.Observe() + defer vpasWithInPlaceUpdatablePodsCounter.Observe() + defer vpasWithInPlaceUpdatedPodsCounter.Observe() // NOTE: this loop assumes that controlledPods are filtered - // to contain only Pods controlled by a VPA in auto or recreate mode + // to contain only Pods controlled by a VPA in auto, recreate, or inPlaceOrRecreate mode for vpa, livePods := range controlledPods { vpaSize := len(livePods) controlledPodsCounter.Add(vpaSize, vpaSize) - evictionLimiter := u.evictionFactory.NewPodsEvictionRestriction(livePods, vpa) - podsForUpdate := u.getPodsUpdateOrder(filterNonEvictablePods(livePods, evictionLimiter), vpa) - evictablePodsCounter.Add(vpaSize, len(podsForUpdate)) + creatorToSingleGroupStatsMap, podToReplicaCreatorMap, err := u.restrictionFactory.GetCreatorMaps(livePods, vpa) + if err != nil { + klog.ErrorS(err, "Failed to get creator maps") + continue + } + + evictionLimiter := u.restrictionFactory.NewPodsEvictionRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) + inPlaceLimiter := u.restrictionFactory.NewPodsInPlaceRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) + + podsForInPlace := make([]*apiv1.Pod, 0) + podsForEviction := make([]*apiv1.Pod, 0) + updateMode := vpa_api_util.GetUpdateMode(vpa) + + if updateMode == vpa_types.UpdateModeInPlaceOrRecreate && features.Enabled(features.InPlaceOrRecreate) { + 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) + } + podsForEviction = u.getPodsUpdateOrder(filterNonEvictablePods(livePods, evictionLimiter), vpa) + evictablePodsCounter.Add(vpaSize, len(podsForEviction)) + } + withInPlaceUpdatable := false + withInPlaceUpdated := false withEvictable := false withEvicted := false - for _, pod := range podsForUpdate { + + for _, pod := range podsForInPlace { + withInPlaceUpdatable = true + decision := inPlaceLimiter.CanInPlaceUpdate(pod) + + if decision == utils.InPlaceDeferred { + klog.V(0).InfoS("In-place update deferred", "pod", klog.KObj(pod)) + continue + } else if decision == utils.InPlaceEvict { + podsForEviction = append(podsForEviction, pod) + continue + } + err = u.inPlaceRateLimiter.Wait(ctx) + if err != nil { + klog.V(0).InfoS("In-place rate limiter wait failed for in-place resize", "error", err) + return + } + err := inPlaceLimiter.InPlaceUpdate(pod, vpa, u.eventRecorder) + if err != nil { + klog.V(0).InfoS("In-place update failed", "error", err, "pod", klog.KObj(pod)) + continue + } + withInPlaceUpdated = true + metrics_updater.AddInPlaceUpdatedPod(vpaSize) + } + + for _, pod := range podsForEviction { withEvictable = true if !evictionLimiter.CanEvict(pod) { continue } - err := u.evictionRateLimiter.Wait(ctx) + err = u.evictionRateLimiter.Wait(ctx) if err != nil { klog.V(0).InfoS("Eviction rate limiter wait failed", "error", err) return @@ -238,6 +310,12 @@ func (u *updater) RunOnce(ctx context.Context) { } } + if withInPlaceUpdatable { + vpasWithInPlaceUpdatablePodsCounter.Add(vpaSize, 1) + } + if withInPlaceUpdated { + vpasWithInPlaceUpdatedPodsCounter.Add(vpaSize, 1) + } if withEvictable { vpasWithEvictablePodsCounter.Add(vpaSize, 1) } @@ -248,17 +326,17 @@ func (u *updater) RunOnce(ctx context.Context) { timer.ObserveStep("EvictPods") } -func getRateLimiter(evictionRateLimit float64, evictionRateLimitBurst int) *rate.Limiter { - var evictionRateLimiter *rate.Limiter - if evictionRateLimit <= 0 { +func getRateLimiter(rateLimit float64, rateLimitBurst int) *rate.Limiter { + var rateLimiter *rate.Limiter + if rateLimit <= 0 { // As a special case if the rate is set to rate.Inf, the burst rate is ignored // see https://github.com/golang/time/blob/master/rate/rate.go#L37 - evictionRateLimiter = rate.NewLimiter(rate.Inf, 0) + rateLimiter = rate.NewLimiter(rate.Inf, 0) klog.V(1).InfoS("Rate limit disabled") } else { - evictionRateLimiter = rate.NewLimiter(rate.Limit(evictionRateLimit), evictionRateLimitBurst) + rateLimiter = rate.NewLimiter(rate.Limit(rateLimit), rateLimitBurst) } - return evictionRateLimiter + return rateLimiter } // getPodsUpdateOrder returns list of pods that should be updated ordered by update priority @@ -276,24 +354,30 @@ func (u *updater) getPodsUpdateOrder(pods []*apiv1.Pod, vpa *vpa_types.VerticalP return priorityCalculator.GetSortedPods(u.evictionAdmission) } -func filterNonEvictablePods(pods []*apiv1.Pod, evictionRestriction eviction.PodsEvictionRestriction) []*apiv1.Pod { +func filterPods(pods []*apiv1.Pod, predicate func(*apiv1.Pod) bool) []*apiv1.Pod { result := make([]*apiv1.Pod, 0) for _, pod := range pods { - if evictionRestriction.CanEvict(pod) { + if predicate(pod) { result = append(result, pod) } } return result } +func filterNonInPlaceUpdatablePods(pods []*apiv1.Pod, inplaceRestriction restriction.PodsInPlaceRestriction) []*apiv1.Pod { + return filterPods(pods, func(pod *apiv1.Pod) bool { + return inplaceRestriction.CanInPlaceUpdate(pod) != utils.InPlaceDeferred + }) +} + +func filterNonEvictablePods(pods []*apiv1.Pod, evictionRestriction restriction.PodsEvictionRestriction) []*apiv1.Pod { + return filterPods(pods, evictionRestriction.CanEvict) +} + func filterDeletedPods(pods []*apiv1.Pod) []*apiv1.Pod { - result := make([]*apiv1.Pod, 0) - for _, pod := range pods { - if pod.DeletionTimestamp == nil { - result = append(result, pod) - } - } - return result + return filterPods(pods, func(pod *apiv1.Pod) bool { + return pod.DeletionTimestamp == nil + }) } func newPodLister(kubeClient kube_client.Interface, namespace string) v1lister.PodLister { diff --git a/vertical-pod-autoscaler/pkg/updater/logic/updater_test.go b/vertical-pod-autoscaler/pkg/updater/logic/updater_test.go index 64357d4924e..8586ea77cbd 100644 --- a/vertical-pod-autoscaler/pkg/updater/logic/updater_test.go +++ b/vertical-pod-autoscaler/pkg/updater/logic/updater_test.go @@ -22,6 +22,9 @@ import ( "testing" "time" + restriction "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/updater/restriction" + utils "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/updater/utils" + "golang.org/x/time/rate" v1 "k8s.io/api/autoscaling/v1" @@ -33,11 +36,12 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes/fake" + featuregatetesting "k8s.io/component-base/featuregate/testing" vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/features" controllerfetcher "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target/controller_fetcher" target_mock "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target/mock" - "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/updater/eviction" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/updater/priority" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/status" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/test" @@ -55,24 +59,63 @@ func TestRunOnce_Mode(t *testing.T) { updateMode vpa_types.UpdateMode expectFetchCalls bool expectedEvictionCount int + expectedInPlacedCount int + canEvict bool + canInPlaceUpdate utils.InPlaceDecision }{ { name: "with Auto mode", updateMode: vpa_types.UpdateModeAuto, expectFetchCalls: true, expectedEvictionCount: 5, + expectedInPlacedCount: 0, + canEvict: true, + canInPlaceUpdate: utils.InPlaceApproved, }, { name: "with Initial mode", updateMode: vpa_types.UpdateModeInitial, expectFetchCalls: false, expectedEvictionCount: 0, + expectedInPlacedCount: 0, + canEvict: true, + canInPlaceUpdate: utils.InPlaceApproved, }, { name: "with Off mode", updateMode: vpa_types.UpdateModeOff, expectFetchCalls: false, expectedEvictionCount: 0, + expectedInPlacedCount: 0, + canEvict: true, + canInPlaceUpdate: utils.InPlaceApproved, + }, + { + name: "with InPlaceOrRecreate mode expecting in-place updates", + updateMode: vpa_types.UpdateModeInPlaceOrRecreate, + expectFetchCalls: true, + expectedEvictionCount: 0, + expectedInPlacedCount: 5, + canEvict: true, + canInPlaceUpdate: utils.InPlaceApproved, + }, + { + name: "with InPlaceOrRecreate mode expecting fallback to evictions", + updateMode: vpa_types.UpdateModeInPlaceOrRecreate, + expectFetchCalls: true, + expectedEvictionCount: 5, + expectedInPlacedCount: 0, + canEvict: true, + canInPlaceUpdate: utils.InPlaceEvict, + }, + { + name: "with InPlaceOrRecreate mode expecting no evictions or in-place", + updateMode: vpa_types.UpdateModeInPlaceOrRecreate, + expectFetchCalls: true, + expectedEvictionCount: 0, + expectedInPlacedCount: 0, + canEvict: false, + canInPlaceUpdate: utils.InPlaceDeferred, }, } for _, tc := range tests { @@ -83,6 +126,8 @@ func TestRunOnce_Mode(t *testing.T) { newFakeValidator(true), tc.expectFetchCalls, tc.expectedEvictionCount, + tc.expectedInPlacedCount, + tc.canInPlaceUpdate, ) }) } @@ -94,18 +139,21 @@ func TestRunOnce_Status(t *testing.T) { statusValidator status.Validator expectFetchCalls bool expectedEvictionCount int + expectedInPlacedCount int }{ { name: "with valid status", statusValidator: newFakeValidator(true), expectFetchCalls: true, expectedEvictionCount: 5, + expectedInPlacedCount: 0, }, { name: "with invalid status", statusValidator: newFakeValidator(false), expectFetchCalls: false, expectedEvictionCount: 0, + expectedInPlacedCount: 0, }, } for _, tc := range tests { @@ -116,6 +164,8 @@ func TestRunOnce_Status(t *testing.T) { tc.statusValidator, tc.expectFetchCalls, tc.expectedEvictionCount, + tc.expectedInPlacedCount, + utils.InPlaceApproved, ) }) } @@ -127,7 +177,10 @@ func testRunOnceBase( statusValidator status.Validator, expectFetchCalls bool, expectedEvictionCount int, + expectedInPlacedCount int, + canInPlaceUpdate utils.InPlaceDecision, ) { + featuregatetesting.SetFeatureGateDuringTest(t, features.MutableFeatureGate, features.InPlaceOrRecreate, true) ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -151,6 +204,7 @@ func testRunOnceBase( } pods := make([]*apiv1.Pod, livePods) eviction := &test.PodsEvictionRestrictionMock{} + inplace := &test.PodsInPlaceRestrictionMock{} for i := range pods { pods[i] = test.Pod().WithName("test_"+strconv.Itoa(i)). @@ -159,11 +213,18 @@ func testRunOnceBase( Get() pods[i].Labels = labels + + inplace.On("CanInPlaceUpdate", pods[i]).Return(canInPlaceUpdate) + inplace.On("InPlaceUpdate", pods[i], nil).Return(nil) + eviction.On("CanEvict", pods[i]).Return(true) eviction.On("Evict", pods[i], nil).Return(nil) } - factory := &fakeEvictFactory{eviction} + factory := &restriction.FakePodsRestrictionFactory{ + Eviction: eviction, + InPlace: inplace, + } vpaLister := &test.VerticalPodAutoscalerListerMock{} podLister := &test.PodListerMock{} @@ -173,12 +234,14 @@ func testRunOnceBase( Name: rc.Name, APIVersion: rc.APIVersion, } + vpaObj := test.VerticalPodAutoscaler(). WithContainer(containerName). WithTarget("2", "200M"). WithMinAllowed(containerName, "1", "100M"). WithMaxAllowed(containerName, "3", "1G"). - WithTargetRef(targetRef).Get() + WithTargetRef(targetRef). + Get() vpaObj.Spec.UpdatePolicy = &vpa_types.PodUpdatePolicy{UpdateMode: &updateMode} vpaLister.On("List").Return([]*vpa_types.VerticalPodAutoscaler{vpaObj}, nil).Once() @@ -188,8 +251,9 @@ func testRunOnceBase( updater := &updater{ vpaLister: vpaLister, podLister: podLister, - evictionFactory: factory, + restrictionFactory: factory, evictionRateLimiter: rate.NewLimiter(rate.Inf, 0), + inPlaceRateLimiter: rate.NewLimiter(rate.Inf, 0), evictionAdmission: priority.NewDefaultPodEvictionAdmission(), recommendationProcessor: &test.FakeRecommendationProcessor{}, selectorFetcher: mockSelectorFetcher, @@ -204,11 +268,16 @@ func testRunOnceBase( } updater.RunOnce(context.Background()) eviction.AssertNumberOfCalls(t, "Evict", expectedEvictionCount) + inplace.AssertNumberOfCalls(t, "InPlaceUpdate", expectedInPlacedCount) } func TestRunOnceNotingToProcess(t *testing.T) { eviction := &test.PodsEvictionRestrictionMock{} - factory := &fakeEvictFactory{eviction} + inplace := &test.PodsInPlaceRestrictionMock{} + factory := &restriction.FakePodsRestrictionFactory{ + Eviction: eviction, + InPlace: inplace, + } vpaLister := &test.VerticalPodAutoscalerListerMock{} podLister := &test.PodListerMock{} vpaLister.On("List").Return(nil, nil).Once() @@ -216,8 +285,9 @@ func TestRunOnceNotingToProcess(t *testing.T) { updater := &updater{ vpaLister: vpaLister, podLister: podLister, - evictionFactory: factory, + restrictionFactory: factory, evictionRateLimiter: rate.NewLimiter(rate.Inf, 0), + inPlaceRateLimiter: rate.NewLimiter(rate.Inf, 0), evictionAdmission: priority.NewDefaultPodEvictionAdmission(), recommendationProcessor: &test.FakeRecommendationProcessor{}, useAdmissionControllerStatus: true, @@ -243,14 +313,6 @@ func TestGetRateLimiter(t *testing.T) { } } -type fakeEvictFactory struct { - evict eviction.PodsEvictionRestriction -} - -func (f fakeEvictFactory) NewPodsEvictionRestriction(pods []*apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler) eviction.PodsEvictionRestriction { - return f.evict -} - type fakeValidator struct { isValid bool } @@ -288,7 +350,7 @@ func TestRunOnceIgnoreNamespaceMatchingPods(t *testing.T) { } pods := make([]*apiv1.Pod, livePods) eviction := &test.PodsEvictionRestrictionMock{} - + inplace := &test.PodsInPlaceRestrictionMock{} for i := range pods { pods[i] = test.Pod().WithName("test_"+strconv.Itoa(i)). AddContainer(test.Container().WithName(containerName).WithCPURequest(resource.MustParse("1")).WithMemRequest(resource.MustParse("100M")).Get()). @@ -300,7 +362,10 @@ func TestRunOnceIgnoreNamespaceMatchingPods(t *testing.T) { eviction.On("Evict", pods[i], nil).Return(nil) } - factory := &fakeEvictFactory{eviction} + factory := &restriction.FakePodsRestrictionFactory{ + Eviction: eviction, + InPlace: inplace, + } vpaLister := &test.VerticalPodAutoscalerListerMock{} podLister := &test.PodListerMock{} @@ -310,13 +375,15 @@ func TestRunOnceIgnoreNamespaceMatchingPods(t *testing.T) { Name: rc.Name, APIVersion: rc.APIVersion, } + vpaObj := test.VerticalPodAutoscaler(). WithNamespace("default"). WithContainer(containerName). WithTarget("2", "200M"). WithMinAllowed(containerName, "1", "100M"). WithMaxAllowed(containerName, "3", "1G"). - WithTargetRef(targetRef).Get() + WithTargetRef(targetRef). + Get() vpaLister.On("List").Return([]*vpa_types.VerticalPodAutoscaler{vpaObj}, nil).Once() @@ -326,8 +393,9 @@ func TestRunOnceIgnoreNamespaceMatchingPods(t *testing.T) { updater := &updater{ vpaLister: vpaLister, podLister: podLister, - evictionFactory: factory, + restrictionFactory: factory, evictionRateLimiter: rate.NewLimiter(rate.Inf, 0), + inPlaceRateLimiter: rate.NewLimiter(rate.Inf, 0), evictionAdmission: priority.NewDefaultPodEvictionAdmission(), recommendationProcessor: &test.FakeRecommendationProcessor{}, selectorFetcher: mockSelectorFetcher, @@ -358,6 +426,7 @@ func TestRunOnceIgnoreNamespaceMatching(t *testing.T) { updater.RunOnce(context.Background()) eviction.AssertNumberOfCalls(t, "Evict", 0) + eviction.AssertNumberOfCalls(t, "InPlaceUpdate", 0) } func TestNewEventRecorder(t *testing.T) { diff --git a/vertical-pod-autoscaler/pkg/updater/main.go b/vertical-pod-autoscaler/pkg/updater/main.go index d6ba42b0d58..a384aff9dca 100644 --- a/vertical-pod-autoscaler/pkg/updater/main.go +++ b/vertical-pod-autoscaler/pkg/updater/main.go @@ -36,10 +36,13 @@ import ( "k8s.io/klog/v2" "k8s.io/autoscaler/vertical-pod-autoscaler/common" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation" vpa_clientset "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/clientset/versioned" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/features" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target" controllerfetcher "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target/controller_fetcher" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/updater/inplace" updater "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/updater/logic" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/updater/priority" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/limitrange" @@ -188,6 +191,10 @@ func run(healthCheck *metrics.HealthCheck, commonFlag *common.CommonFlags) { ignoredNamespaces := strings.Split(commonFlag.IgnoredVpaObjectNamespaces, ",") + recommendationProvider := recommendation.NewProvider(limitRangeCalculator, vpa_api_util.NewCappingRecommendationProcessor(limitRangeCalculator)) + + calculators := []patch.Calculator{inplace.NewResourceInPlaceUpdatesCalculator(recommendationProvider), inplace.NewInPlaceUpdatedCalculator()} + // TODO: use SharedInformerFactory in updater updater, err := updater.NewUpdater( kubeClient, @@ -205,6 +212,7 @@ func run(healthCheck *metrics.HealthCheck, commonFlag *common.CommonFlags) { priority.NewProcessor(), commonFlag.VpaObjectNamespace, ignoredNamespaces, + calculators, ) if err != nil { klog.ErrorS(err, "Failed to create updater") diff --git a/vertical-pod-autoscaler/pkg/updater/restriction/fake_pods_restriction.go b/vertical-pod-autoscaler/pkg/updater/restriction/fake_pods_restriction.go new file mode 100644 index 00000000000..4637f0b2cc7 --- /dev/null +++ b/vertical-pod-autoscaler/pkg/updater/restriction/fake_pods_restriction.go @@ -0,0 +1,46 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package restriction + +import ( + apiv1 "k8s.io/api/core/v1" + + vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" +) + +// FakePodsRestrictionFactory is a fake implementation of the PodsRestrictionFactory interface. +type FakePodsRestrictionFactory struct { + // Eviction is the fake eviction restriction. + Eviction PodsEvictionRestriction + // InPlace is the fake in-place restriction. + InPlace PodsInPlaceRestriction +} + +// NewPodsEvictionRestriction returns the fake eviction restriction. +func (f *FakePodsRestrictionFactory) NewPodsEvictionRestriction(creatorToSingleGroupStatsMap map[podReplicaCreator]singleGroupStats, podToReplicaCreatorMap map[string]podReplicaCreator) PodsEvictionRestriction { + return f.Eviction +} + +// NewPodsInPlaceRestriction returns the fake in-place restriction. +func (f *FakePodsRestrictionFactory) NewPodsInPlaceRestriction(creatorToSingleGroupStatsMap map[podReplicaCreator]singleGroupStats, podToReplicaCreatorMap map[string]podReplicaCreator) PodsInPlaceRestriction { + return f.InPlace +} + +// GetCreatorMaps returns nil maps. +func (f *FakePodsRestrictionFactory) GetCreatorMaps(pods []*apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler) (map[podReplicaCreator]singleGroupStats, map[string]podReplicaCreator, error) { + return nil, nil, nil +} diff --git a/vertical-pod-autoscaler/pkg/updater/restriction/pods_eviction_restriction.go b/vertical-pod-autoscaler/pkg/updater/restriction/pods_eviction_restriction.go new file mode 100644 index 00000000000..4a423781e33 --- /dev/null +++ b/vertical-pod-autoscaler/pkg/updater/restriction/pods_eviction_restriction.go @@ -0,0 +1,112 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package restriction + +import ( + "context" + "fmt" + "time" + + apiv1 "k8s.io/api/core/v1" + policyv1 "k8s.io/api/policy/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + kube_client "k8s.io/client-go/kubernetes" + "k8s.io/client-go/tools/record" + "k8s.io/klog/v2" + "k8s.io/utils/clock" + + vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" +) + +// PodsEvictionRestriction controls pods evictions. It ensures that we will not evict too +// many pods from one replica set. For replica set will allow to evict one pod or more if +// evictionToleranceFraction is configured. +type PodsEvictionRestriction interface { + // Evict sends eviction instruction to the api client. + // Returns error if pod cannot be evicted or if client returned error. + Evict(pod *apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler, eventRecorder record.EventRecorder) error + // CanEvict checks if pod can be safely evicted + CanEvict(pod *apiv1.Pod) bool +} + +// PodsEvictionRestrictionImpl is the implementation of the PodsEvictionRestriction interface. +type PodsEvictionRestrictionImpl struct { + client kube_client.Interface + podToReplicaCreatorMap map[string]podReplicaCreator + creatorToSingleGroupStatsMap map[podReplicaCreator]singleGroupStats + clock clock.Clock + lastInPlaceAttemptTimeMap map[string]time.Time +} + +// CanEvict checks if pod can be safely evicted +func (e *PodsEvictionRestrictionImpl) CanEvict(pod *apiv1.Pod) bool { + cr, present := e.podToReplicaCreatorMap[getPodID(pod)] + if present { + singleGroupStats, present := e.creatorToSingleGroupStatsMap[cr] + if pod.Status.Phase == apiv1.PodPending { + return true + } + if present { + if isInPlaceUpdating(pod) { + return CanEvictInPlacingPod(pod, singleGroupStats, e.lastInPlaceAttemptTimeMap, e.clock) + } + return singleGroupStats.isPodDisruptable() + } + } + return false +} + +// Evict sends eviction instruction to api client. Returns error if pod cannot be evicted or if client returned error +// Does not check if pod was actually evicted after eviction grace period. +func (e *PodsEvictionRestrictionImpl) Evict(podToEvict *apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler, eventRecorder record.EventRecorder) error { + cr, present := e.podToReplicaCreatorMap[getPodID(podToEvict)] + if !present { + return fmt.Errorf("pod not suitable for eviction %s/%s: not in replicated pods map", podToEvict.Namespace, podToEvict.Name) + } + + if !e.CanEvict(podToEvict) { + return fmt.Errorf("cannot evict pod %s/%s: eviction budget exceeded", podToEvict.Namespace, podToEvict.Name) + } + + eviction := &policyv1.Eviction{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: podToEvict.Namespace, + Name: podToEvict.Name, + }, + } + err := e.client.CoreV1().Pods(podToEvict.Namespace).EvictV1(context.TODO(), eviction) + if err != nil { + klog.ErrorS(err, "Failed to evict pod", "pod", klog.KObj(podToEvict)) + return err + } + eventRecorder.Event(podToEvict, apiv1.EventTypeNormal, "EvictedByVPA", + "Pod was evicted by VPA Updater to apply resource recommendation.") + + eventRecorder.Event(vpa, apiv1.EventTypeNormal, "EvictedPod", + "VPA Updater evicted Pod "+podToEvict.Name+" to apply resource recommendation.") + + if podToEvict.Status.Phase != apiv1.PodPending { + singleGroupStats, present := e.creatorToSingleGroupStatsMap[cr] + if !present { + return fmt.Errorf("Internal error - cannot find stats for replication group %v", cr) + } + singleGroupStats.evicted = singleGroupStats.evicted + 1 + e.creatorToSingleGroupStatsMap[cr] = singleGroupStats + } + + return nil +} diff --git a/vertical-pod-autoscaler/pkg/updater/restriction/pods_eviction_restriction_test.go b/vertical-pod-autoscaler/pkg/updater/restriction/pods_eviction_restriction_test.go new file mode 100644 index 00000000000..4bb4b032762 --- /dev/null +++ b/vertical-pod-autoscaler/pkg/updater/restriction/pods_eviction_restriction_test.go @@ -0,0 +1,259 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package restriction + +import ( + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + apiv1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + baseclocktest "k8s.io/utils/clock/testing" + + vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/test" +) + +func TestEvictTooFewReplicas(t *testing.T) { + replicas := int32(5) + livePods := 5 + + rc := apiv1.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rc", + Namespace: "default", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "ReplicationController", + }, + Spec: apiv1.ReplicationControllerSpec{ + Replicas: &replicas, + }, + } + + pods := make([]*apiv1.Pod, livePods) + for i := range pods { + pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta).Get() + } + + basicVpa := getBasicVpa() + factory, err := getRestrictionFactory(&rc, nil, nil, nil, 10, 0.5, nil, nil, nil) + assert.NoError(t, err) + creatorToSingleGroupStatsMap, podToReplicaCreatorMap, err := factory.GetCreatorMaps(pods, basicVpa) + assert.NoError(t, err) + eviction := factory.NewPodsEvictionRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) + + for _, pod := range pods { + assert.False(t, eviction.CanEvict(pod)) + } + + for _, pod := range pods { + err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder()) + assert.Error(t, err, "Error expected") + } +} + +func TestEvictionTolerance(t *testing.T) { + replicas := int32(5) + livePods := 5 + tolerance := 0.8 + + rc := apiv1.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rc", + Namespace: "default", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "ReplicationController", + }, + Spec: apiv1.ReplicationControllerSpec{ + Replicas: &replicas, + }, + } + + pods := make([]*apiv1.Pod, livePods) + for i := range pods { + pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta).Get() + } + + basicVpa := getBasicVpa() + factory, err := getRestrictionFactory(&rc, nil, nil, nil, 2 /*minReplicas*/, tolerance, nil, nil, nil) + assert.NoError(t, err) + creatorToSingleGroupStatsMap, podToReplicaCreatorMap, err := factory.GetCreatorMaps(pods, basicVpa) + assert.NoError(t, err) + eviction := factory.NewPodsEvictionRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) + + for _, pod := range pods { + assert.True(t, eviction.CanEvict(pod)) + } + + for _, pod := range pods[:4] { + err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder()) + assert.Nil(t, err, "Should evict with no error") + } + for _, pod := range pods[4:] { + err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder()) + assert.Error(t, err, "Error expected") + } +} + +func TestEvictAtLeastOne(t *testing.T) { + replicas := int32(5) + livePods := 5 + tolerance := 0.1 + + rc := apiv1.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rc", + Namespace: "default", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "ReplicationController", + }, + Spec: apiv1.ReplicationControllerSpec{ + Replicas: &replicas, + }, + } + + pods := make([]*apiv1.Pod, livePods) + for i := range pods { + pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta).Get() + } + + basicVpa := getBasicVpa() + factory, err := getRestrictionFactory(&rc, nil, nil, nil, 2, tolerance, nil, nil, nil) + assert.NoError(t, err) + creatorToSingleGroupStatsMap, podToReplicaCreatorMap, err := factory.GetCreatorMaps(pods, basicVpa) + assert.NoError(t, err) + eviction := factory.NewPodsEvictionRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) + + for _, pod := range pods { + assert.True(t, eviction.CanEvict(pod)) + } + + for _, pod := range pods[:1] { + err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder()) + assert.Nil(t, err, "Should evict with no error") + } + for _, pod := range pods[1:] { + err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder()) + assert.Error(t, err, "Error expected") + } +} + +func TestEvictEmitEvent(t *testing.T) { + rc := apiv1.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rc", + Namespace: "default", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "ReplicationController", + }, + } + + index := 0 + generatePod := func() test.PodBuilder { + index++ + return test.Pod().WithName(fmt.Sprintf("test-%v", index)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta) + } + + basicVpa := getBasicVpa() + + testCases := []struct { + name string + replicas int32 + evictionTolerance float64 + vpa *vpa_types.VerticalPodAutoscaler + pods []podWithExpectations + errorExpected bool + }{ + { + name: "Pods that can be evicted", + replicas: 4, + evictionTolerance: 0.5, + vpa: basicVpa, + pods: []podWithExpectations{ + { + pod: generatePod().WithPhase(apiv1.PodPending).Get(), + canEvict: true, + evictionSuccess: true, + }, + { + pod: generatePod().WithPhase(apiv1.PodPending).Get(), + canEvict: true, + evictionSuccess: true, + }, + }, + errorExpected: false, + }, + { + name: "Pod that can not be evicted", + replicas: 4, + evictionTolerance: 0.5, + vpa: basicVpa, + pods: []podWithExpectations{ + + { + pod: generatePod().Get(), + canEvict: false, + evictionSuccess: false, + }, + }, + errorExpected: true, + }, + } + + for _, testCase := range testCases { + rc.Spec = apiv1.ReplicationControllerSpec{ + Replicas: &testCase.replicas, + } + pods := make([]*apiv1.Pod, 0, len(testCase.pods)) + for _, p := range testCase.pods { + pods = append(pods, p.pod) + } + clock := baseclocktest.NewFakeClock(time.Time{}) + factory, err := getRestrictionFactory(&rc, nil, nil, nil, 2, testCase.evictionTolerance, clock, map[string]time.Time{}, nil) + assert.NoError(t, err) + creatorToSingleGroupStatsMap, podToReplicaCreatorMap, err := factory.GetCreatorMaps(pods, testCase.vpa) + assert.NoError(t, err) + eviction := factory.NewPodsEvictionRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) + + for _, p := range testCase.pods { + mockRecorder := test.MockEventRecorder() + mockRecorder.On("Event", mock.Anything, apiv1.EventTypeNormal, "EvictedByVPA", mock.Anything).Return() + mockRecorder.On("Event", mock.Anything, apiv1.EventTypeNormal, "EvictedPod", mock.Anything).Return() + + errGot := eviction.Evict(p.pod, testCase.vpa, mockRecorder) + if testCase.errorExpected { + assert.Error(t, errGot) + } else { + assert.NoError(t, errGot) + } + + if p.canEvict { + mockRecorder.AssertNumberOfCalls(t, "Event", 2) + + } else { + mockRecorder.AssertNumberOfCalls(t, "Event", 0) + } + } + } +} diff --git a/vertical-pod-autoscaler/pkg/updater/restriction/pods_inplace_restriction.go b/vertical-pod-autoscaler/pkg/updater/restriction/pods_inplace_restriction.go new file mode 100644 index 00000000000..8a86cef1b57 --- /dev/null +++ b/vertical-pod-autoscaler/pkg/updater/restriction/pods_inplace_restriction.go @@ -0,0 +1,218 @@ +/* +Copyright 2015 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package restriction + +import ( + "context" + "fmt" + "time" + + apiv1 "k8s.io/api/core/v1" + kube_client "k8s.io/client-go/kubernetes" + "k8s.io/client-go/tools/record" + "k8s.io/klog/v2" + "k8s.io/utils/clock" + + vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/features" + + "encoding/json" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8stypes "k8s.io/apimachinery/pkg/types" + + utils "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/updater/utils" + + resource_updates "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch" +) + +// TODO: Make these configurable by flags +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 = 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. + InProgressResizeUpdateTimeout = 1 * time.Hour +) + +// PodsInPlaceRestriction controls pods in-place updates. It ensures that we will not update too +// many pods from one replica set. For replica set will allow to update one pod or more if +// inPlaceToleranceFraction is configured. +type PodsInPlaceRestriction interface { + // InPlaceUpdate attempts to actuate the in-place resize. + // Returns error if client returned error. + InPlaceUpdate(pod *apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler, eventRecorder record.EventRecorder) error + // CanInPlaceUpdate checks if pod can be safely updated in-place. If not, it will return a decision to potentially evict the pod. + CanInPlaceUpdate(pod *apiv1.Pod) utils.InPlaceDecision +} + +// PodsInPlaceRestrictionImpl is the implementation of the PodsInPlaceRestriction interface. +type PodsInPlaceRestrictionImpl struct { + client kube_client.Interface + podToReplicaCreatorMap map[string]podReplicaCreator + creatorToSingleGroupStatsMap map[podReplicaCreator]singleGroupStats + patchCalculators []patch.Calculator + clock clock.Clock + lastInPlaceAttemptTimeMap map[string]time.Time +} + +// CanInPlaceUpdate checks if pod can be safely updated +func (ip *PodsInPlaceRestrictionImpl) CanInPlaceUpdate(pod *apiv1.Pod) utils.InPlaceDecision { + if !features.Enabled(features.InPlaceOrRecreate) { + return utils.InPlaceEvict + } + + cr, present := ip.podToReplicaCreatorMap[getPodID(pod)] + if present { + singleGroupStats, present := ip.creatorToSingleGroupStatsMap[cr] + if pod.Status.Phase == apiv1.PodPending { + return utils.InPlaceDeferred + } + if present { + if isInPlaceUpdating(pod) { + canEvict := CanEvictInPlacingPod(pod, singleGroupStats, ip.lastInPlaceAttemptTimeMap, ip.clock) + if canEvict { + return utils.InPlaceEvict + } + return utils.InPlaceDeferred + } + if singleGroupStats.isPodDisruptable() { + return utils.InPlaceApproved + } + } + } + klog.V(4).InfoS("Can't in-place update pod, but not falling back to eviction. Waiting for next loop", "pod", klog.KObj(pod)) + return utils.InPlaceDeferred +} + +// InPlaceUpdate sends calculates patches and sends resize request to api client. Returns error if pod cannot be in-place updated or if client returned error. +// Does not check if pod was actually in-place updated after grace period. +func (ip *PodsInPlaceRestrictionImpl) InPlaceUpdate(podToUpdate *apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler, eventRecorder record.EventRecorder) error { + cr, present := ip.podToReplicaCreatorMap[getPodID(podToUpdate)] + if !present { + return fmt.Errorf("pod not suitable for in-place update %v: not in replicated pods map", podToUpdate.Name) + } + + if ip.CanInPlaceUpdate(podToUpdate) != utils.InPlaceApproved { + return fmt.Errorf("cannot in-place update pod %s", klog.KObj(podToUpdate)) + } + + // separate patches since we have to patch resize and spec separately + resizePatches := []resource_updates.PatchRecord{} + annotationPatches := []resource_updates.PatchRecord{} + if podToUpdate.Annotations == nil { + annotationPatches = append(annotationPatches, patch.GetAddEmptyAnnotationsPatch()) + } + for _, calculator := range ip.patchCalculators { + p, err := calculator.CalculatePatches(podToUpdate, vpa) + if err != nil { + return err + } + klog.V(4).InfoS("Calculated patches for pod", "pod", klog.KObj(podToUpdate), "patches", p) + if calculator.PatchResourceTarget() == patch.Resize { + resizePatches = append(resizePatches, p...) + } else { + annotationPatches = append(annotationPatches, p...) + } + } + if len(resizePatches) > 0 { + patch, err := json.Marshal(resizePatches) + if err != nil { + return err + } + + res, err := ip.client.CoreV1().Pods(podToUpdate.Namespace).Patch(context.TODO(), podToUpdate.Name, k8stypes.JSONPatchType, patch, metav1.PatchOptions{}, "resize") + if err != nil { + return err + } + klog.V(4).InfoS("In-place patched pod /resize subresource using patches", "pod", klog.KObj(res), "patches", string(patch)) + + if len(annotationPatches) > 0 { + patch, err := json.Marshal(annotationPatches) + if err != nil { + return err + } + res, err = ip.client.CoreV1().Pods(podToUpdate.Namespace).Patch(context.TODO(), podToUpdate.Name, k8stypes.JSONPatchType, patch, metav1.PatchOptions{}) + if err != nil { + return err + } + klog.V(4).InfoS("Patched pod annotations", "pod", klog.KObj(res), "patches", string(patch)) + } + } else { + return fmt.Errorf("no resource patches were calculated to apply") + } + + // TODO(maxcao13): If this keeps getting called on the same object with the same reason, it is considered a patch request. + // And we fail to have the corresponding rbac for it. So figure out if we need this later. + // Do we even need to emit an event? The node might reject the resize request. If so, should we rename this to InPlaceResizeAttempted? + // eventRecorder.Event(podToUpdate, apiv1.EventTypeNormal, "InPlaceResizedByVPA", "Pod was resized in place by VPA Updater.") + + singleGroupStats, present := ip.creatorToSingleGroupStatsMap[cr] + if !present { + klog.InfoS("Internal error - cannot find stats for replication group", "pod", klog.KObj(podToUpdate), "podReplicaCreator", cr) + } else { + singleGroupStats.inPlaceUpdateInitiated = singleGroupStats.inPlaceUpdateInitiated + 1 + ip.creatorToSingleGroupStatsMap[cr] = singleGroupStats + } + + 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 +} diff --git a/vertical-pod-autoscaler/pkg/updater/restriction/pods_inplace_restriction_test.go b/vertical-pod-autoscaler/pkg/updater/restriction/pods_inplace_restriction_test.go new file mode 100644 index 00000000000..1c08f427fb1 --- /dev/null +++ b/vertical-pod-autoscaler/pkg/updater/restriction/pods_inplace_restriction_test.go @@ -0,0 +1,360 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package restriction + +import ( + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" + + apiv1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + featuregatetesting "k8s.io/component-base/featuregate/testing" + baseclocktest "k8s.io/utils/clock/testing" + + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/features" + utils "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/updater/utils" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/test" +) + +type CanInPlaceUpdateTestParams struct { + name string + pods []*apiv1.Pod + replicas int32 + evictionTolerance float64 + lastInPlaceAttempt time.Time + expectedInPlaceDecision utils.InPlaceDecision +} + +func TestCanInPlaceUpdate(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, features.MutableFeatureGate, features.InPlaceOrRecreate, true) + + rc := apiv1.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rc", + Namespace: "default", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "ReplicationController", + }, + } + + index := 0 + generatePod := func() test.PodBuilder { + index++ + return test.Pod().WithName(fmt.Sprintf("test-%v", index)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta) + } + // NOTE: the pod we are checking for CanInPlaceUpdate will always be the first one for these tests + whichPodIdxForCanInPlaceUpdate := 0 + + testCases := []CanInPlaceUpdateTestParams{ + { + name: "CanInPlaceUpdate=InPlaceApproved - (half of 3)", + pods: []*apiv1.Pod{ + generatePod().Get(), + generatePod().Get(), + generatePod().Get(), + }, + replicas: 3, + evictionTolerance: 0.5, + lastInPlaceAttempt: time.Time{}, + expectedInPlaceDecision: utils.InPlaceApproved, + }, + { + name: "CanInPlaceUpdate=InPlaceDeferred - no pods can be in-placed, one missing", + pods: []*apiv1.Pod{ + generatePod().Get(), + generatePod().Get(), + }, + replicas: 3, + evictionTolerance: 0.5, + lastInPlaceAttempt: time.Time{}, + expectedInPlaceDecision: utils.InPlaceDeferred, + }, + { + name: "CanInPlaceUpdate=InPlaceApproved - small tolerance, all running", + pods: []*apiv1.Pod{ + generatePod().Get(), + generatePod().Get(), + generatePod().Get(), + }, + replicas: 3, + evictionTolerance: 0.1, + lastInPlaceAttempt: time.Time{}, + expectedInPlaceDecision: utils.InPlaceApproved, + }, + { + name: "CanInPlaceUpdate=InPlaceApproved - small tolerance, one missing", + pods: []*apiv1.Pod{ + generatePod().Get(), + generatePod().Get(), + }, + replicas: 3, + evictionTolerance: 0.5, + lastInPlaceAttempt: time.Time{}, + expectedInPlaceDecision: utils.InPlaceDeferred, + }, + { + name: "CanInPlaceUpdate=InPlaceDeferred - resize Deferred, conditions not met to fallback", + pods: []*apiv1.Pod{ + generatePod().WithResizeStatus(apiv1.PodResizeStatusDeferred).Get(), + generatePod().Get(), + generatePod().Get(), + }, + replicas: 3, + evictionTolerance: 0.5, + lastInPlaceAttempt: time.UnixMilli(3600000), // 1 hour from epoch + expectedInPlaceDecision: utils.InPlaceDeferred, + }, + { + name: ("CanInPlaceUpdate=InPlaceEvict - resize inProgress for more too long"), + pods: []*apiv1.Pod{ + generatePod().WithResizeStatus(apiv1.PodResizeStatusInProgress).Get(), + generatePod().Get(), + generatePod().Get(), + }, + replicas: 3, + evictionTolerance: 0.5, + lastInPlaceAttempt: time.UnixMilli(0), // epoch (too long ago...) + expectedInPlaceDecision: utils.InPlaceEvict, + }, + { + name: "CanInPlaceUpdate=InPlaceDeferred - resize InProgress, conditions not met to fallback", + pods: []*apiv1.Pod{ + generatePod().WithResizeStatus(apiv1.PodResizeStatusInProgress).Get(), + generatePod().Get(), + generatePod().Get(), + }, + replicas: 3, + evictionTolerance: 0.5, + lastInPlaceAttempt: time.UnixMilli(3600000), // 1 hour from epoch + expectedInPlaceDecision: utils.InPlaceDeferred, + }, + { + name: "CanInPlaceUpdate=InPlaceEvict - infeasible", + pods: []*apiv1.Pod{ + generatePod().WithResizeStatus(apiv1.PodResizeStatusInfeasible).Get(), + generatePod().Get(), + generatePod().Get(), + }, + replicas: 3, + evictionTolerance: 0.5, + lastInPlaceAttempt: time.Time{}, + expectedInPlaceDecision: utils.InPlaceEvict, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + rc.Spec = apiv1.ReplicationControllerSpec{ + Replicas: &tc.replicas, + } + + selectedPod := tc.pods[whichPodIdxForCanInPlaceUpdate] + + clock := baseclocktest.NewFakeClock(time.UnixMilli(3600001)) // 1 hour from epoch + 1 millis + lipatm := map[string]time.Time{getPodID(selectedPod): tc.lastInPlaceAttempt} + + factory, err := getRestrictionFactory(&rc, nil, nil, nil, 2, tc.evictionTolerance, clock, lipatm, GetFakeCalculatorsWithFakeResourceCalc()) + assert.NoError(t, err) + creatorToSingleGroupStatsMap, podToReplicaCreatorMap, err := factory.GetCreatorMaps(tc.pods, getIPORVpa()) + assert.NoError(t, err) + inPlace := factory.NewPodsInPlaceRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) + + result := inPlace.CanInPlaceUpdate(selectedPod) + assert.Equal(t, tc.expectedInPlaceDecision, result) + }) + } +} + +func TestInPlaceDisabledFeatureGate(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, features.MutableFeatureGate, features.InPlaceOrRecreate, false) + + replicas := int32(5) + livePods := 5 + tolerance := 1.0 + + rc := apiv1.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rc", + Namespace: "default", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "ReplicationController", + }, + Spec: apiv1.ReplicationControllerSpec{ + Replicas: &replicas, + }, + } + + pods := make([]*apiv1.Pod, livePods) + for i := range pods { + pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta).Get() + } + + basicVpa := getBasicVpa() + factory, err := getRestrictionFactory(&rc, nil, nil, nil, 2, tolerance, nil, nil, GetFakeCalculatorsWithFakeResourceCalc()) + assert.NoError(t, err) + creatorToSingleGroupStatsMap, podToReplicaCreatorMap, err := factory.GetCreatorMaps(pods, basicVpa) + assert.NoError(t, err) + inplace := factory.NewPodsInPlaceRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) + + for _, pod := range pods { + assert.Equal(t, utils.InPlaceEvict, inplace.CanInPlaceUpdate(pod)) + } +} + +func TestInPlaceTooFewReplicas(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, features.MutableFeatureGate, features.InPlaceOrRecreate, true) + + replicas := int32(5) + livePods := 5 + tolerance := 0.5 + + rc := apiv1.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rc", + Namespace: "default", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "ReplicationController", + }, + Spec: apiv1.ReplicationControllerSpec{ + Replicas: &replicas, + }, + } + + pods := make([]*apiv1.Pod, livePods) + for i := range pods { + pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta).Get() + } + + clock := baseclocktest.NewFakeClock(time.Time{}) + lipatm := map[string]time.Time{} + + basicVpa := getIPORVpa() + factory, err := getRestrictionFactory(&rc, nil, nil, nil, 10 /*minReplicas*/, tolerance, clock, lipatm, GetFakeCalculatorsWithFakeResourceCalc()) + assert.NoError(t, err) + creatorToSingleGroupStatsMap, podToReplicaCreatorMap, err := factory.GetCreatorMaps(pods, basicVpa) + assert.NoError(t, err) + inplace := factory.NewPodsInPlaceRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) + + for _, pod := range pods { + assert.Equal(t, utils.InPlaceDeferred, inplace.CanInPlaceUpdate(pod)) + } + + for _, pod := range pods { + err := inplace.InPlaceUpdate(pod, basicVpa, test.FakeEventRecorder()) + assert.Error(t, err, "Error expected") + } +} + +func TestEvictionToleranceForInPlace(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, features.MutableFeatureGate, features.InPlaceOrRecreate, true) + + replicas := int32(5) + livePods := 5 + tolerance := 0.8 + + rc := apiv1.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rc", + Namespace: "default", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "ReplicationController", + }, + Spec: apiv1.ReplicationControllerSpec{ + Replicas: &replicas, + }, + } + + pods := make([]*apiv1.Pod, livePods) + for i := range pods { + pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta).Get() + } + + clock := baseclocktest.NewFakeClock(time.Time{}) + lipatm := map[string]time.Time{} + + basicVpa := getIPORVpa() + factory, err := getRestrictionFactory(&rc, nil, nil, nil, 2 /*minReplicas*/, tolerance, clock, lipatm, GetFakeCalculatorsWithFakeResourceCalc()) + assert.NoError(t, err) + creatorToSingleGroupStatsMap, podToReplicaCreatorMap, err := factory.GetCreatorMaps(pods, basicVpa) + assert.NoError(t, err) + inplace := factory.NewPodsInPlaceRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) + + for _, pod := range pods { + assert.Equal(t, utils.InPlaceApproved, inplace.CanInPlaceUpdate(pod)) + } + + for _, pod := range pods[:4] { + err := inplace.InPlaceUpdate(pod, basicVpa, test.FakeEventRecorder()) + assert.Nil(t, err, "Should evict with no error") + } + for _, pod := range pods[4:] { + err := inplace.InPlaceUpdate(pod, basicVpa, test.FakeEventRecorder()) + assert.Error(t, err, "Error expected") + } +} + +func TestInPlaceAtLeastOne(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, features.MutableFeatureGate, features.InPlaceOrRecreate, true) + + replicas := int32(5) + livePods := 5 + tolerance := 0.1 + + rc := apiv1.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rc", + Namespace: "default", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "ReplicationController", + }, + Spec: apiv1.ReplicationControllerSpec{ + Replicas: &replicas, + }, + } + + pods := make([]*apiv1.Pod, livePods) + for i := range pods { + pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta).Get() + } + + basicVpa := getBasicVpa() + factory, err := getRestrictionFactory(&rc, nil, nil, nil, 2, tolerance, nil, nil, GetFakeCalculatorsWithFakeResourceCalc()) + assert.NoError(t, err) + creatorToSingleGroupStatsMap, podToReplicaCreatorMap, err := factory.GetCreatorMaps(pods, basicVpa) + assert.NoError(t, err) + inplace := factory.NewPodsInPlaceRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) + + for _, pod := range pods { + assert.Equal(t, utils.InPlaceApproved, inplace.CanInPlaceUpdate(pod)) + } + + for _, pod := range pods[:1] { + err := inplace.InPlaceUpdate(pod, basicVpa, test.FakeEventRecorder()) + assert.Nil(t, err, "Should in-place update with no error") + } + for _, pod := range pods[1:] { + err := inplace.InPlaceUpdate(pod, basicVpa, test.FakeEventRecorder()) + assert.Error(t, err, "Error expected") + } +} diff --git a/vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction.go b/vertical-pod-autoscaler/pkg/updater/restriction/pods_restriction_factory.go similarity index 63% rename from vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction.go rename to vertical-pod-autoscaler/pkg/updater/restriction/pods_restriction_factory.go index ca6452d8e16..9dc99fa5b6e 100644 --- a/vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction.go +++ b/vertical-pod-autoscaler/pkg/updater/restriction/pods_restriction_factory.go @@ -1,5 +1,5 @@ /* -Copyright 2017 The Kubernetes Authors. +Copyright 2025 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -14,24 +14,23 @@ See the License for the specific language governing permissions and limitations under the License. */ -package eviction +package restriction import ( - "context" "fmt" "time" appsv1 "k8s.io/api/apps/v1" apiv1 "k8s.io/api/core/v1" - policyv1 "k8s.io/api/policy/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" appsinformer "k8s.io/client-go/informers/apps/v1" coreinformer "k8s.io/client-go/informers/core/v1" kube_client "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/cache" - "k8s.io/client-go/tools/record" "k8s.io/klog/v2" + "k8s.io/utils/clock" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch" vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" ) @@ -39,48 +38,7 @@ const ( resyncPeriod time.Duration = 1 * time.Minute ) -// PodsEvictionRestriction controls pods evictions. It ensures that we will not evict too -// many pods from one replica set. For replica set will allow to evict one pod or more if -// evictionToleranceFraction is configured. -type PodsEvictionRestriction interface { - // Evict sends eviction instruction to the api client. - // Returns error if pod cannot be evicted or if client returned error. - Evict(pod *apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler, eventRecorder record.EventRecorder) error - // CanEvict checks if pod can be safely evicted - CanEvict(pod *apiv1.Pod) bool -} - -type podsEvictionRestrictionImpl struct { - client kube_client.Interface - podToReplicaCreatorMap map[string]podReplicaCreator - creatorToSingleGroupStatsMap map[podReplicaCreator]singleGroupStats -} - -type singleGroupStats struct { - configured int - pending int - running int - evictionTolerance int - evicted int -} - -// PodsEvictionRestrictionFactory creates PodsEvictionRestriction -type PodsEvictionRestrictionFactory interface { - // NewPodsEvictionRestriction creates PodsEvictionRestriction for given set of pods, - // controlled by a single VPA object. - NewPodsEvictionRestriction(pods []*apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler) PodsEvictionRestriction -} - -type podsEvictionRestrictionFactoryImpl struct { - client kube_client.Interface - rcInformer cache.SharedIndexInformer // informer for Replication Controllers - ssInformer cache.SharedIndexInformer // informer for Stateful Sets - rsInformer cache.SharedIndexInformer // informer for Replica Sets - dsInformer cache.SharedIndexInformer // informer for Daemon Sets - minReplicas int - evictionToleranceFraction float64 -} - +// ControllerKind is the type of controller that can manage a pod. type controllerKind string const ( @@ -97,107 +55,132 @@ type podReplicaCreator struct { Kind controllerKind } -// CanEvict checks if pod can be safely evicted -func (e *podsEvictionRestrictionImpl) CanEvict(pod *apiv1.Pod) bool { - cr, present := e.podToReplicaCreatorMap[getPodID(pod)] - if present { - singleGroupStats, present := e.creatorToSingleGroupStatsMap[cr] - if pod.Status.Phase == apiv1.PodPending { - return true - } - if present { - shouldBeAlive := singleGroupStats.configured - singleGroupStats.evictionTolerance - if singleGroupStats.running-singleGroupStats.evicted > shouldBeAlive { - return true - } - // If all pods are running and eviction tolerance is small evict 1 pod. - if singleGroupStats.running == singleGroupStats.configured && - singleGroupStats.evictionTolerance == 0 && - singleGroupStats.evicted == 0 { - return true - } - } - } - return false +// PodsRestrictionFactory is a factory for creating PodsEvictionRestriction and PodsInPlaceRestriction. +type PodsRestrictionFactory interface { + GetCreatorMaps(pods []*apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler) (map[podReplicaCreator]singleGroupStats, map[string]podReplicaCreator, error) + NewPodsEvictionRestriction(creatorToSingleGroupStatsMap map[podReplicaCreator]singleGroupStats, podToReplicaCreatorMap map[string]podReplicaCreator) PodsEvictionRestriction + NewPodsInPlaceRestriction(creatorToSingleGroupStatsMap map[podReplicaCreator]singleGroupStats, podToReplicaCreatorMap map[string]podReplicaCreator) PodsInPlaceRestriction } -// Evict sends eviction instruction to api client. Returns error if pod cannot be evicted or if client returned error -// Does not check if pod was actually evicted after eviction grace period. -func (e *podsEvictionRestrictionImpl) Evict(podToEvict *apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler, eventRecorder record.EventRecorder) error { - cr, present := e.podToReplicaCreatorMap[getPodID(podToEvict)] - if !present { - return fmt.Errorf("pod not suitable for eviction %s/%s: not in replicated pods map", podToEvict.Namespace, podToEvict.Name) - } - - if !e.CanEvict(podToEvict) { - return fmt.Errorf("cannot evict pod %s/%s: eviction budget exceeded", podToEvict.Namespace, podToEvict.Name) - } - - eviction := &policyv1.Eviction{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: podToEvict.Namespace, - Name: podToEvict.Name, - }, - } - err := e.client.CoreV1().Pods(podToEvict.Namespace).EvictV1(context.TODO(), eviction) - if err != nil { - klog.ErrorS(err, "Failed to evict pod", "pod", klog.KObj(podToEvict)) - return err - } - eventRecorder.Event(podToEvict, apiv1.EventTypeNormal, "EvictedByVPA", - "Pod was evicted by VPA Updater to apply resource recommendation.") - - eventRecorder.Event(vpa, apiv1.EventTypeNormal, "EvictedPod", - "VPA Updater evicted Pod "+podToEvict.Name+" to apply resource recommendation.") - - if podToEvict.Status.Phase != apiv1.PodPending { - singleGroupStats, present := e.creatorToSingleGroupStatsMap[cr] - if !present { - return fmt.Errorf("Internal error - cannot find stats for replication group %v", cr) - } - singleGroupStats.evicted = singleGroupStats.evicted + 1 - e.creatorToSingleGroupStatsMap[cr] = singleGroupStats - } - - return nil +// PodsRestrictionFactoryImpl is the implementation of the PodsRestrictionFactory interface. +type PodsRestrictionFactoryImpl struct { + client kube_client.Interface + rcInformer cache.SharedIndexInformer // informer for Replication Controllers + ssInformer cache.SharedIndexInformer // informer for Stateful Sets + rsInformer cache.SharedIndexInformer // informer for Replica Sets + dsInformer cache.SharedIndexInformer // informer for Daemon Sets + minReplicas int + evictionToleranceFraction float64 + clock clock.Clock + lastInPlaceAttemptTimeMap map[string]time.Time + patchCalculators []patch.Calculator } -// NewPodsEvictionRestrictionFactory creates PodsEvictionRestrictionFactory -func NewPodsEvictionRestrictionFactory(client kube_client.Interface, minReplicas int, - evictionToleranceFraction float64) (PodsEvictionRestrictionFactory, error) { - rcInformer, err := setUpInformer(client, replicationController) +// NewPodsRestrictionFactory creates a new PodsRestrictionFactory. +func NewPodsRestrictionFactory(client kube_client.Interface, minReplicas int, evictionToleranceFraction float64, patchCalculators []patch.Calculator) (PodsRestrictionFactory, error) { + rcInformer, err := setupInformer(client, replicationController) if err != nil { return nil, fmt.Errorf("Failed to create rcInformer: %v", err) } - ssInformer, err := setUpInformer(client, statefulSet) + ssInformer, err := setupInformer(client, statefulSet) if err != nil { return nil, fmt.Errorf("Failed to create ssInformer: %v", err) } - rsInformer, err := setUpInformer(client, replicaSet) + rsInformer, err := setupInformer(client, replicaSet) if err != nil { return nil, fmt.Errorf("Failed to create rsInformer: %v", err) } - dsInformer, err := setUpInformer(client, daemonSet) + dsInformer, err := setupInformer(client, daemonSet) if err != nil { return nil, fmt.Errorf("Failed to create dsInformer: %v", err) } - return &podsEvictionRestrictionFactoryImpl{ + return &PodsRestrictionFactoryImpl{ client: client, rcInformer: rcInformer, // informer for Replication Controllers - ssInformer: ssInformer, // informer for Replica Sets - rsInformer: rsInformer, // informer for Stateful Sets + ssInformer: ssInformer, // informer for Stateful Sets + rsInformer: rsInformer, // informer for Replica Sets dsInformer: dsInformer, // informer for Daemon Sets minReplicas: minReplicas, - evictionToleranceFraction: evictionToleranceFraction}, nil + evictionToleranceFraction: evictionToleranceFraction, + clock: &clock.RealClock{}, + lastInPlaceAttemptTimeMap: make(map[string]time.Time), + patchCalculators: patchCalculators, + }, nil } -// NewPodsEvictionRestriction creates PodsEvictionRestriction for a given set of pods, -// controlled by a single VPA object. -func (f *podsEvictionRestrictionFactoryImpl) NewPodsEvictionRestriction(pods []*apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler) PodsEvictionRestriction { - // We can evict pod only if it is a part of replica set - // For each replica set we can evict only a fraction of pods. - // Evictions may be later limited by pod disruption budget if configured. +func (f *PodsRestrictionFactoryImpl) getReplicaCount(creator podReplicaCreator) (int, error) { + switch creator.Kind { + case replicationController: + rcObj, exists, err := f.rcInformer.GetStore().GetByKey(creator.Namespace + "/" + creator.Name) + if err != nil { + return 0, fmt.Errorf("replication controller %s/%s is not available, err: %v", creator.Namespace, creator.Name, err) + } + if !exists { + return 0, fmt.Errorf("replication controller %s/%s does not exist", creator.Namespace, creator.Name) + } + rc, ok := rcObj.(*apiv1.ReplicationController) + if !ok { + return 0, fmt.Errorf("Failed to parse Replication Controller") + } + if rc.Spec.Replicas == nil || *rc.Spec.Replicas == 0 { + return 0, fmt.Errorf("replication controller %s/%s has no replicas config", creator.Namespace, creator.Name) + } + return int(*rc.Spec.Replicas), nil + case replicaSet: + rsObj, exists, err := f.rsInformer.GetStore().GetByKey(creator.Namespace + "/" + creator.Name) + if err != nil { + return 0, fmt.Errorf("replica set %s/%s is not available, err: %v", creator.Namespace, creator.Name, err) + } + if !exists { + return 0, fmt.Errorf("replica set %s/%s does not exist", creator.Namespace, creator.Name) + } + rs, ok := rsObj.(*appsv1.ReplicaSet) + if !ok { + return 0, fmt.Errorf("Failed to parse Replicaset") + } + if rs.Spec.Replicas == nil || *rs.Spec.Replicas == 0 { + return 0, fmt.Errorf("replica set %s/%s has no replicas config", creator.Namespace, creator.Name) + } + return int(*rs.Spec.Replicas), nil + case statefulSet: + ssObj, exists, err := f.ssInformer.GetStore().GetByKey(creator.Namespace + "/" + creator.Name) + if err != nil { + return 0, fmt.Errorf("stateful set %s/%s is not available, err: %v", creator.Namespace, creator.Name, err) + } + if !exists { + return 0, fmt.Errorf("stateful set %s/%s does not exist", creator.Namespace, creator.Name) + } + ss, ok := ssObj.(*appsv1.StatefulSet) + if !ok { + return 0, fmt.Errorf("Failed to parse StatefulSet") + } + if ss.Spec.Replicas == nil || *ss.Spec.Replicas == 0 { + return 0, fmt.Errorf("stateful set %s/%s has no replicas config", creator.Namespace, creator.Name) + } + return int(*ss.Spec.Replicas), nil + case daemonSet: + dsObj, exists, err := f.dsInformer.GetStore().GetByKey(creator.Namespace + "/" + creator.Name) + if err != nil { + return 0, fmt.Errorf("daemon set %s/%s is not available, err: %v", creator.Namespace, creator.Name, err) + } + if !exists { + return 0, fmt.Errorf("daemon set %s/%s does not exist", creator.Namespace, creator.Name) + } + ds, ok := dsObj.(*appsv1.DaemonSet) + if !ok { + return 0, fmt.Errorf("Failed to parse DaemonSet") + } + if ds.Status.NumberReady == 0 { + return 0, fmt.Errorf("daemon set %s/%s has no number ready pods", creator.Namespace, creator.Name) + } + return int(ds.Status.NumberReady), nil + } + return 0, nil +} +// GetCreatorMaps is a helper function that returns a map of pod replica creators to their single group stats +// and a map of pod ids to pod replica creator from a list of pods and it's corresponding VPA. +func (f *PodsRestrictionFactoryImpl) GetCreatorMaps(pods []*apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler) (map[podReplicaCreator]singleGroupStats, map[string]podReplicaCreator, error) { livePods := make(map[podReplicaCreator][]*apiv1.Pod) for _, pod := range pods { @@ -245,33 +228,44 @@ func (f *podsEvictionRestrictionFactoryImpl) NewPodsEvictionRestriction(pods []* singleGroup := singleGroupStats{} singleGroup.configured = configured - singleGroup.evictionTolerance = int(float64(configured) * f.evictionToleranceFraction) + singleGroup.evictionTolerance = int(float64(configured) * f.evictionToleranceFraction) // truncated for _, pod := range replicas { podToReplicaCreatorMap[getPodID(pod)] = creator if pod.Status.Phase == apiv1.PodPending { singleGroup.pending = singleGroup.pending + 1 } + if isInPlaceUpdating(pod) { + singleGroup.inPlaceUpdateOngoing = singleGroup.inPlaceUpdateOngoing + 1 + } } singleGroup.running = len(replicas) - singleGroup.pending creatorToSingleGroupStatsMap[creator] = singleGroup + } - return &podsEvictionRestrictionImpl{ + return creatorToSingleGroupStatsMap, podToReplicaCreatorMap, nil +} + +// NewPodsEvictionRestriction creates a new PodsEvictionRestriction. +func (f *PodsRestrictionFactoryImpl) NewPodsEvictionRestriction(creatorToSingleGroupStatsMap map[podReplicaCreator]singleGroupStats, podToReplicaCreatorMap map[string]podReplicaCreator) PodsEvictionRestriction { + return &PodsEvictionRestrictionImpl{ client: f.client, podToReplicaCreatorMap: podToReplicaCreatorMap, - creatorToSingleGroupStatsMap: creatorToSingleGroupStatsMap} + creatorToSingleGroupStatsMap: creatorToSingleGroupStatsMap, + clock: f.clock, + lastInPlaceAttemptTimeMap: f.lastInPlaceAttemptTimeMap, + } } -func getPodReplicaCreator(pod *apiv1.Pod) (*podReplicaCreator, error) { - creator := managingControllerRef(pod) - if creator == nil { - return nil, nil - } - podReplicaCreator := &podReplicaCreator{ - Namespace: pod.Namespace, - Name: creator.Name, - Kind: controllerKind(creator.Kind), +// NewPodsInPlaceRestriction creates a new PodsInPlaceRestriction. +func (f *PodsRestrictionFactoryImpl) NewPodsInPlaceRestriction(creatorToSingleGroupStatsMap map[podReplicaCreator]singleGroupStats, podToReplicaCreatorMap map[string]podReplicaCreator) PodsInPlaceRestriction { + return &PodsInPlaceRestrictionImpl{ + client: f.client, + podToReplicaCreatorMap: podToReplicaCreatorMap, + creatorToSingleGroupStatsMap: creatorToSingleGroupStatsMap, + clock: f.clock, + lastInPlaceAttemptTimeMap: f.lastInPlaceAttemptTimeMap, + patchCalculators: f.patchCalculators, } - return podReplicaCreator, nil } func getPodID(pod *apiv1.Pod) string { @@ -281,78 +275,17 @@ func getPodID(pod *apiv1.Pod) string { return pod.Namespace + "/" + pod.Name } -func (f *podsEvictionRestrictionFactoryImpl) getReplicaCount(creator podReplicaCreator) (int, error) { - switch creator.Kind { - case replicationController: - rcObj, exists, err := f.rcInformer.GetStore().GetByKey(creator.Namespace + "/" + creator.Name) - if err != nil { - return 0, fmt.Errorf("replication controller %s/%s is not available, err: %v", creator.Namespace, creator.Name, err) - } - if !exists { - return 0, fmt.Errorf("replication controller %s/%s does not exist", creator.Namespace, creator.Name) - } - rc, ok := rcObj.(*apiv1.ReplicationController) - if !ok { - return 0, fmt.Errorf("Failed to parse Replication Controller") - } - if rc.Spec.Replicas == nil || *rc.Spec.Replicas == 0 { - return 0, fmt.Errorf("replication controller %s/%s has no replicas config", creator.Namespace, creator.Name) - } - return int(*rc.Spec.Replicas), nil - - case replicaSet: - rsObj, exists, err := f.rsInformer.GetStore().GetByKey(creator.Namespace + "/" + creator.Name) - if err != nil { - return 0, fmt.Errorf("replica set %s/%s is not available, err: %v", creator.Namespace, creator.Name, err) - } - if !exists { - return 0, fmt.Errorf("replica set %s/%s does not exist", creator.Namespace, creator.Name) - } - rs, ok := rsObj.(*appsv1.ReplicaSet) - if !ok { - return 0, fmt.Errorf("Failed to parse Replicaset") - } - if rs.Spec.Replicas == nil || *rs.Spec.Replicas == 0 { - return 0, fmt.Errorf("replica set %s/%s has no replicas config", creator.Namespace, creator.Name) - } - return int(*rs.Spec.Replicas), nil - - case statefulSet: - ssObj, exists, err := f.ssInformer.GetStore().GetByKey(creator.Namespace + "/" + creator.Name) - if err != nil { - return 0, fmt.Errorf("stateful set %s/%s is not available, err: %v", creator.Namespace, creator.Name, err) - } - if !exists { - return 0, fmt.Errorf("stateful set %s/%s does not exist", creator.Namespace, creator.Name) - } - ss, ok := ssObj.(*appsv1.StatefulSet) - if !ok { - return 0, fmt.Errorf("Failed to parse StatefulSet") - } - if ss.Spec.Replicas == nil || *ss.Spec.Replicas == 0 { - return 0, fmt.Errorf("stateful set %s/%s has no replicas config", creator.Namespace, creator.Name) - } - return int(*ss.Spec.Replicas), nil - - case daemonSet: - dsObj, exists, err := f.dsInformer.GetStore().GetByKey(creator.Namespace + "/" + creator.Name) - if err != nil { - return 0, fmt.Errorf("daemon set %s/%s is not available, err: %v", creator.Namespace, creator.Name, err) - } - if !exists { - return 0, fmt.Errorf("daemon set %s/%s does not exist", creator.Namespace, creator.Name) - } - ds, ok := dsObj.(*appsv1.DaemonSet) - if !ok { - return 0, fmt.Errorf("Failed to parse DaemonSet") - } - if ds.Status.NumberReady == 0 { - return 0, fmt.Errorf("daemon set %s/%s has no number ready pods", creator.Namespace, creator.Name) - } - return int(ds.Status.NumberReady), nil +func getPodReplicaCreator(pod *apiv1.Pod) (*podReplicaCreator, error) { + creator := managingControllerRef(pod) + if creator == nil { + return nil, nil } - - return 0, nil + podReplicaCreator := &podReplicaCreator{ + Namespace: pod.Namespace, + Name: creator.Name, + Kind: controllerKind(creator.Kind), + } + return podReplicaCreator, nil } func managingControllerRef(pod *apiv1.Pod) *metav1.OwnerReference { @@ -366,7 +299,7 @@ func managingControllerRef(pod *apiv1.Pod) *metav1.OwnerReference { return &managingController } -func setUpInformer(kubeClient kube_client.Interface, kind controllerKind) (cache.SharedIndexInformer, error) { +func setupInformer(kubeClient kube_client.Interface, kind controllerKind) (cache.SharedIndexInformer, error) { var informer cache.SharedIndexInformer switch kind { case replicationController: @@ -392,3 +325,28 @@ func setUpInformer(kubeClient kube_client.Interface, kind controllerKind) (cache } return informer, nil } + +type singleGroupStats struct { + configured int + pending int + running int + evictionTolerance int + evicted int + inPlaceUpdateOngoing int // number of pods from last loop that are still in-place updating + inPlaceUpdateInitiated int // number of pods from the current loop that have newly requested in-place resize +} + +// isPodDisruptable checks if all pods are running and eviction tolerance is small, we can +// disrupt the current pod. +func (s *singleGroupStats) isPodDisruptable() bool { + shouldBeAlive := s.configured - s.evictionTolerance + actuallyAlive := s.running - (s.evicted + s.inPlaceUpdateInitiated) + return actuallyAlive > shouldBeAlive || + (s.configured == s.running && s.evictionTolerance == 0 && s.evicted == 0 && s.inPlaceUpdateInitiated == 0) + // we don't want to block pods from being considered for eviction if tolerance is small and some pods are potentially stuck resizing +} + +// isInPlaceUpdating checks whether or not the given pod is currently in the middle of an in-place update +func isInPlaceUpdating(podToCheck *apiv1.Pod) bool { + return podToCheck.Status.Resize != "" +} diff --git a/vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction_test.go b/vertical-pod-autoscaler/pkg/updater/restriction/pods_restriction_factory_test.go similarity index 55% rename from vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction_test.go rename to vertical-pod-autoscaler/pkg/updater/restriction/pods_restriction_factory_test.go index 855ced5f2ca..a2e557e2099 100644 --- a/vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction_test.go +++ b/vertical-pod-autoscaler/pkg/updater/restriction/pods_restriction_factory_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package eviction +package restriction import ( "fmt" @@ -22,7 +22,8 @@ import ( "time" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" + + resource_admission "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource" appsv1 "k8s.io/api/apps/v1" batchv1 "k8s.io/api/batch/v1" @@ -32,22 +33,42 @@ import ( coreinformer "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/tools/cache" + featuregatetesting "k8s.io/component-base/featuregate/testing" + "k8s.io/utils/clock" + baseclocktest "k8s.io/utils/clock/testing" + "k8s.io/utils/ptr" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch" vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/features" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/updater/utils" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/test" + vpa_api_util "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/vpa" ) type podWithExpectations struct { - pod *apiv1.Pod - canEvict bool - evictionSuccess bool + pod *apiv1.Pod + canEvict bool + evictionSuccess bool + canInPlaceUpdate utils.InPlaceDecision + inPlaceUpdateSuccess bool } func getBasicVpa() *vpa_types.VerticalPodAutoscaler { return test.VerticalPodAutoscaler().WithContainer("any").Get() } -func TestEvictReplicatedByController(t *testing.T) { +func getIPORVpa() *vpa_types.VerticalPodAutoscaler { + vpa := getBasicVpa() + vpa.Spec.UpdatePolicy = &vpa_types.PodUpdatePolicy{ + UpdateMode: ptr.To(vpa_types.UpdateModeInPlaceOrRecreate), + } + return vpa +} + +func TestDisruptReplicatedByController(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, features.MutableFeatureGate, features.InPlaceOrRecreate, true) + rc := apiv1.ReplicationController{ ObjectMeta: metav1.ObjectMeta{ Name: "rc", @@ -268,29 +289,217 @@ func TestEvictReplicatedByController(t *testing.T) { }, }, }, + { + name: "In-place update only first pod (half of 3).", + replicas: 3, + evictionTolerance: 0.5, + vpa: getIPORVpa(), + pods: []podWithExpectations{ + { + pod: generatePod().Get(), + canInPlaceUpdate: utils.InPlaceApproved, + inPlaceUpdateSuccess: true, + }, + { + pod: generatePod().Get(), + canInPlaceUpdate: utils.InPlaceApproved, + inPlaceUpdateSuccess: false, + }, + { + pod: generatePod().Get(), + canInPlaceUpdate: utils.InPlaceApproved, + inPlaceUpdateSuccess: false, + }, + }, + }, + { + name: "For small eviction tolerance at least one pod is in-place resized.", + replicas: 3, + evictionTolerance: 0.1, + vpa: getIPORVpa(), + pods: []podWithExpectations{ + { + pod: generatePod().Get(), + canInPlaceUpdate: utils.InPlaceApproved, + inPlaceUpdateSuccess: true, + }, + { + pod: generatePod().Get(), + canInPlaceUpdate: utils.InPlaceApproved, + inPlaceUpdateSuccess: false, + }, + { + pod: generatePod().Get(), + canInPlaceUpdate: utils.InPlaceApproved, + inPlaceUpdateSuccess: false, + }, + }, + }, + { + name: "Ongoing in-placing pods will not get resized again, but may be considered for eviction or deferred.", + replicas: 3, + evictionTolerance: 0.1, + vpa: getIPORVpa(), + pods: []podWithExpectations{ + { + pod: generatePod().WithResizeStatus(apiv1.PodResizeStatusInfeasible).Get(), + canInPlaceUpdate: utils.InPlaceEvict, + inPlaceUpdateSuccess: false, + }, + { + pod: generatePod().WithResizeStatus(apiv1.PodResizeStatusInProgress).Get(), + canInPlaceUpdate: utils.InPlaceDeferred, + inPlaceUpdateSuccess: false, + }, + { + pod: generatePod().Get(), + canInPlaceUpdate: utils.InPlaceApproved, + inPlaceUpdateSuccess: true, + }, + }, + }, + { + name: "Cannot in-place a single Pod under default settings.", + replicas: 1, + evictionTolerance: 0.5, + vpa: getIPORVpa(), + pods: []podWithExpectations{ + { + pod: generatePod().Get(), + canInPlaceUpdate: utils.InPlaceDeferred, + inPlaceUpdateSuccess: false, + }, + }, + }, + { + name: "Can in-place even a single Pod using PodUpdatePolicy.MinReplicas.", + replicas: 1, + evictionTolerance: 0.5, + vpa: func() *vpa_types.VerticalPodAutoscaler { + vpa := getIPORVpa() + vpa.Spec.UpdatePolicy.MinReplicas = ptr.To(int32(1)) + return vpa + }(), + pods: []podWithExpectations{ + { + pod: generatePod().Get(), + canInPlaceUpdate: utils.InPlaceApproved, + inPlaceUpdateSuccess: true, + }, + }, + }, + { + name: "First pod can be evicted without violation of tolerance, even if other evictable pods have ongoing resizes.", + replicas: 3, + evictionTolerance: 0.5, + vpa: getBasicVpa(), + pods: []podWithExpectations{ + { + pod: generatePod().Get(), + canEvict: true, + evictionSuccess: true, + }, + { + pod: generatePod().WithResizeStatus(apiv1.PodResizeStatusInfeasible).Get(), + canEvict: true, + evictionSuccess: false, + }, + { + pod: generatePod().WithResizeStatus(apiv1.PodResizeStatusInfeasible).Get(), + canEvict: true, + evictionSuccess: false, + }, + }, + }, + { + name: "No pods are evictable even if some pods are stuck resizing, but some are missing and eviction tolerance is small.", + replicas: 4, + evictionTolerance: 0.1, + vpa: getBasicVpa(), + pods: []podWithExpectations{ + { + pod: generatePod().Get(), + canEvict: false, + evictionSuccess: false, + }, + { + pod: generatePod().WithResizeStatus(apiv1.PodResizeStatusInfeasible).Get(), + canEvict: false, + evictionSuccess: false, + }, + { + pod: generatePod().Get(), + canEvict: false, + evictionSuccess: false, + }, + }, + }, + { + name: "All pods, including resizing pods, are evictable due to large tolerance.", + replicas: 3, + evictionTolerance: 1, + vpa: getBasicVpa(), + pods: []podWithExpectations{ + { + pod: generatePod().Get(), + canEvict: true, + evictionSuccess: true, + }, + { + pod: generatePod().WithResizeStatus(apiv1.PodResizeStatusInfeasible).Get(), + canEvict: true, + evictionSuccess: true, + }, + { + pod: generatePod().Get(), + canEvict: true, + evictionSuccess: true, + }, + }, + }, } for _, testCase := range testCases { - rc.Spec = apiv1.ReplicationControllerSpec{ - Replicas: &testCase.replicas, - } - pods := make([]*apiv1.Pod, 0, len(testCase.pods)) - for _, p := range testCase.pods { - pods = append(pods, p.pod) - } - factory, _ := getEvictionRestrictionFactory(&rc, nil, nil, nil, 2, testCase.evictionTolerance) - eviction := factory.NewPodsEvictionRestriction(pods, testCase.vpa) - for i, p := range testCase.pods { - assert.Equalf(t, p.canEvict, eviction.CanEvict(p.pod), "TC %v - unexpected CanEvict result for pod-%v %#v", testCase.name, i, p.pod) - } - for i, p := range testCase.pods { - err := eviction.Evict(p.pod, testCase.vpa, test.FakeEventRecorder()) - if p.evictionSuccess { - assert.NoErrorf(t, err, "TC %v - unexpected Evict result for pod-%v %#v", testCase.name, i, p.pod) - } else { - assert.Errorf(t, err, "TC %v - unexpected Evict result for pod-%v %#v", testCase.name, i, p.pod) + t.Run(testCase.name, func(t *testing.T) { + rc.Spec = apiv1.ReplicationControllerSpec{ + Replicas: &testCase.replicas, } - } + pods := make([]*apiv1.Pod, 0, len(testCase.pods)) + for _, p := range testCase.pods { + pods = append(pods, p.pod) + } + factory, err := getRestrictionFactory(&rc, nil, nil, nil, 2, testCase.evictionTolerance, baseclocktest.NewFakeClock(time.Time{}), make(map[string]time.Time), GetFakeCalculatorsWithFakeResourceCalc()) + assert.NoError(t, err) + creatorToSingleGroupStatsMap, podToReplicaCreatorMap, err := factory.GetCreatorMaps(pods, testCase.vpa) + assert.NoError(t, err) + eviction := factory.NewPodsEvictionRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) + inplace := factory.NewPodsInPlaceRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) + updateMode := vpa_api_util.GetUpdateMode(testCase.vpa) + for i, p := range testCase.pods { + if updateMode == vpa_types.UpdateModeInPlaceOrRecreate { + assert.Equalf(t, p.canInPlaceUpdate, inplace.CanInPlaceUpdate(p.pod), "TC %v - unexpected CanInPlaceUpdate result for pod-%v %#v", testCase.name, i, p.pod) + } else { + assert.Equalf(t, p.canEvict, eviction.CanEvict(p.pod), "TC %v - unexpected CanEvict result for pod-%v %#v", testCase.name, i, p.pod) + } + } + for i, p := range testCase.pods { + if updateMode == vpa_types.UpdateModeInPlaceOrRecreate { + err := inplace.InPlaceUpdate(p.pod, testCase.vpa, test.FakeEventRecorder()) + if p.inPlaceUpdateSuccess { + assert.NoErrorf(t, err, "TC %v - unexpected InPlaceUpdate result for pod-%v %#v", testCase.name, i, p.pod) + } else { + assert.Errorf(t, err, "TC %v - unexpected InPlaceUpdate result for pod-%v %#v", testCase.name, i, p.pod) + } + } else { + err := eviction.Evict(p.pod, testCase.vpa, test.FakeEventRecorder()) + if p.evictionSuccess { + assert.NoErrorf(t, err, "TC %v - unexpected Evict result for pod-%v %#v", testCase.name, i, p.pod) + } else { + assert.Errorf(t, err, "TC %v - unexpected Evict result for pod-%v %#v", testCase.name, i, p.pod) + } + } + } + }) } } @@ -317,8 +526,11 @@ func TestEvictReplicatedByReplicaSet(t *testing.T) { } basicVpa := getBasicVpa() - factory, _ := getEvictionRestrictionFactory(nil, &rs, nil, nil, 2, 0.5) - eviction := factory.NewPodsEvictionRestriction(pods, basicVpa) + factory, err := getRestrictionFactory(nil, &rs, nil, nil, 2, 0.5, nil, nil, nil) + assert.NoError(t, err) + creatorToSingleGroupStatsMap, podToReplicaCreatorMap, err := factory.GetCreatorMaps(pods, basicVpa) + assert.NoError(t, err) + eviction := factory.NewPodsEvictionRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) for _, pod := range pods { assert.True(t, eviction.CanEvict(pod)) @@ -357,8 +569,11 @@ func TestEvictReplicatedByStatefulSet(t *testing.T) { } basicVpa := getBasicVpa() - factory, _ := getEvictionRestrictionFactory(nil, nil, &ss, nil, 2, 0.5) - eviction := factory.NewPodsEvictionRestriction(pods, basicVpa) + factory, err := getRestrictionFactory(nil, nil, &ss, nil, 2, 0.5, nil, nil, nil) + assert.NoError(t, err) + creatorToSingleGroupStatsMap, podToReplicaCreatorMap, err := factory.GetCreatorMaps(pods, basicVpa) + assert.NoError(t, err) + eviction := factory.NewPodsEvictionRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) for _, pod := range pods { assert.True(t, eviction.CanEvict(pod)) @@ -396,8 +611,11 @@ func TestEvictReplicatedByDaemonSet(t *testing.T) { } basicVpa := getBasicVpa() - factory, _ := getEvictionRestrictionFactory(nil, nil, nil, &ds, 2, 0.5) - eviction := factory.NewPodsEvictionRestriction(pods, basicVpa) + factory, err := getRestrictionFactory(nil, nil, nil, &ds, 2, 0.5, nil, nil, nil) + assert.NoError(t, err) + creatorToSingleGroupStatsMap, podToReplicaCreatorMap, err := factory.GetCreatorMaps(pods, basicVpa) + assert.NoError(t, err) + eviction := factory.NewPodsEvictionRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) for _, pod := range pods { assert.True(t, eviction.CanEvict(pod)) @@ -432,8 +650,11 @@ func TestEvictReplicatedByJob(t *testing.T) { } basicVpa := getBasicVpa() - factory, _ := getEvictionRestrictionFactory(nil, nil, nil, nil, 2, 0.5) - eviction := factory.NewPodsEvictionRestriction(pods, basicVpa) + factory, err := getRestrictionFactory(nil, nil, nil, nil, 2, 0.5, nil, nil, nil) + assert.NoError(t, err) + creatorToSingleGroupStatsMap, podToReplicaCreatorMap, err := factory.GetCreatorMaps(pods, basicVpa) + assert.NoError(t, err) + eviction := factory.NewPodsEvictionRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap) for _, pod := range pods { assert.True(t, eviction.CanEvict(pod)) @@ -449,223 +670,9 @@ func TestEvictReplicatedByJob(t *testing.T) { } } -func TestEvictTooFewReplicas(t *testing.T) { - replicas := int32(5) - livePods := 5 - - rc := apiv1.ReplicationController{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rc", - Namespace: "default", - }, - TypeMeta: metav1.TypeMeta{ - Kind: "ReplicationController", - }, - Spec: apiv1.ReplicationControllerSpec{ - Replicas: &replicas, - }, - } - - pods := make([]*apiv1.Pod, livePods) - for i := range pods { - pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta).Get() - } - - basicVpa := getBasicVpa() - factory, _ := getEvictionRestrictionFactory(&rc, nil, nil, nil, 10, 0.5) - eviction := factory.NewPodsEvictionRestriction(pods, basicVpa) - - for _, pod := range pods { - assert.False(t, eviction.CanEvict(pod)) - } - - for _, pod := range pods { - err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder()) - assert.Error(t, err, "Error expected") - } -} - -func TestEvictionTolerance(t *testing.T) { - replicas := int32(5) - livePods := 5 - tolerance := 0.8 - - rc := apiv1.ReplicationController{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rc", - Namespace: "default", - }, - TypeMeta: metav1.TypeMeta{ - Kind: "ReplicationController", - }, - Spec: apiv1.ReplicationControllerSpec{ - Replicas: &replicas, - }, - } - - pods := make([]*apiv1.Pod, livePods) - for i := range pods { - pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta).Get() - } - - basicVpa := getBasicVpa() - factory, _ := getEvictionRestrictionFactory(&rc, nil, nil, nil, 2 /*minReplicas*/, tolerance) - eviction := factory.NewPodsEvictionRestriction(pods, basicVpa) - - for _, pod := range pods { - assert.True(t, eviction.CanEvict(pod)) - } - - for _, pod := range pods[:4] { - err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder()) - assert.Nil(t, err, "Should evict with no error") - } - for _, pod := range pods[4:] { - err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder()) - assert.Error(t, err, "Error expected") - } -} - -func TestEvictAtLeastOne(t *testing.T) { - replicas := int32(5) - livePods := 5 - tolerance := 0.1 - - rc := apiv1.ReplicationController{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rc", - Namespace: "default", - }, - TypeMeta: metav1.TypeMeta{ - Kind: "ReplicationController", - }, - Spec: apiv1.ReplicationControllerSpec{ - Replicas: &replicas, - }, - } - - pods := make([]*apiv1.Pod, livePods) - for i := range pods { - pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta).Get() - } - - basicVpa := getBasicVpa() - factory, _ := getEvictionRestrictionFactory(&rc, nil, nil, nil, 2, tolerance) - eviction := factory.NewPodsEvictionRestriction(pods, basicVpa) - - for _, pod := range pods { - assert.True(t, eviction.CanEvict(pod)) - } - - for _, pod := range pods[:1] { - err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder()) - assert.Nil(t, err, "Should evict with no error") - } - for _, pod := range pods[1:] { - err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder()) - assert.Error(t, err, "Error expected") - } -} - -func TestEvictEmitEvent(t *testing.T) { - rc := apiv1.ReplicationController{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rc", - Namespace: "default", - }, - TypeMeta: metav1.TypeMeta{ - Kind: "ReplicationController", - }, - } - - index := 0 - generatePod := func() test.PodBuilder { - index++ - return test.Pod().WithName(fmt.Sprintf("test-%v", index)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta) - } - - basicVpa := getBasicVpa() - - testCases := []struct { - name string - replicas int32 - evictionTolerance float64 - vpa *vpa_types.VerticalPodAutoscaler - pods []podWithExpectations - errorExpected bool - }{ - { - name: "Pods that can be evicted", - replicas: 4, - evictionTolerance: 0.5, - vpa: basicVpa, - pods: []podWithExpectations{ - { - pod: generatePod().WithPhase(apiv1.PodPending).Get(), - canEvict: true, - evictionSuccess: true, - }, - { - pod: generatePod().WithPhase(apiv1.PodPending).Get(), - canEvict: true, - evictionSuccess: true, - }, - }, - errorExpected: false, - }, - { - name: "Pod that can not be evicted", - replicas: 4, - evictionTolerance: 0.5, - vpa: basicVpa, - pods: []podWithExpectations{ - - { - pod: generatePod().Get(), - canEvict: false, - evictionSuccess: false, - }, - }, - errorExpected: true, - }, - } - - for _, testCase := range testCases { - rc.Spec = apiv1.ReplicationControllerSpec{ - Replicas: &testCase.replicas, - } - pods := make([]*apiv1.Pod, 0, len(testCase.pods)) - for _, p := range testCase.pods { - pods = append(pods, p.pod) - } - factory, _ := getEvictionRestrictionFactory(&rc, nil, nil, nil, 2, testCase.evictionTolerance) - eviction := factory.NewPodsEvictionRestriction(pods, testCase.vpa) - - for _, p := range testCase.pods { - mockRecorder := test.MockEventRecorder() - mockRecorder.On("Event", mock.Anything, apiv1.EventTypeNormal, "EvictedByVPA", mock.Anything).Return() - mockRecorder.On("Event", mock.Anything, apiv1.EventTypeNormal, "EvictedPod", mock.Anything).Return() - - errGot := eviction.Evict(p.pod, testCase.vpa, mockRecorder) - if testCase.errorExpected { - assert.Error(t, errGot) - } else { - assert.NoError(t, errGot) - } - - if p.canEvict { - mockRecorder.AssertNumberOfCalls(t, "Event", 2) - - } else { - mockRecorder.AssertNumberOfCalls(t, "Event", 0) - } - } - } -} - -func getEvictionRestrictionFactory(rc *apiv1.ReplicationController, rs *appsv1.ReplicaSet, +func getRestrictionFactory(rc *apiv1.ReplicationController, rs *appsv1.ReplicaSet, ss *appsv1.StatefulSet, ds *appsv1.DaemonSet, minReplicas int, - evictionToleranceFraction float64) (PodsEvictionRestrictionFactory, error) { + evictionToleranceFraction float64, clock clock.Clock, lipuatm map[string]time.Time, patchCalculators []patch.Calculator) (PodsRestrictionFactory, error) { kubeClient := &fake.Clientset{} rcInformer := coreinformer.NewReplicationControllerInformer(kubeClient, apiv1.NamespaceAll, 0*time.Second, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) @@ -700,17 +707,52 @@ func getEvictionRestrictionFactory(rc *apiv1.ReplicationController, rs *appsv1.R } } - return &podsEvictionRestrictionFactoryImpl{ + return &PodsRestrictionFactoryImpl{ client: kubeClient, - rsInformer: rsInformer, rcInformer: rcInformer, ssInformer: ssInformer, + rsInformer: rsInformer, dsInformer: dsInformer, minReplicas: minReplicas, evictionToleranceFraction: evictionToleranceFraction, + clock: clock, + lastInPlaceAttemptTimeMap: lipuatm, + patchCalculators: patchCalculators, }, nil } func getTestPodName(index int) string { return fmt.Sprintf("test-%v", index) } + +type fakeResizePatchCalculator struct { + patches []resource_admission.PatchRecord + err error +} + +func (c *fakeResizePatchCalculator) CalculatePatches(_ *apiv1.Pod, _ *vpa_types.VerticalPodAutoscaler) ( + []resource_admission.PatchRecord, error) { + return c.patches, c.err +} + +func (c *fakeResizePatchCalculator) PatchResourceTarget() patch.PatchResourceTarget { + return patch.Resize +} + +func NewFakeCalculatorWithInPlacePatches() patch.Calculator { + return &fakeResizePatchCalculator{ + patches: []resource_admission.PatchRecord{ + { + Op: "fakeop", + Path: "fakepath", + Value: apiv1.ResourceList{}, + }, + }, + } +} + +func GetFakeCalculatorsWithFakeResourceCalc() []patch.Calculator { + return []patch.Calculator{ + NewFakeCalculatorWithInPlacePatches(), + } +} diff --git a/vertical-pod-autoscaler/pkg/updater/utils/types.go b/vertical-pod-autoscaler/pkg/updater/utils/types.go new file mode 100644 index 00000000000..8ddb85a0eb8 --- /dev/null +++ b/vertical-pod-autoscaler/pkg/updater/utils/types.go @@ -0,0 +1,29 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package utils + +// InPlaceDecision is the type of decision that can be made for a pod. +type InPlaceDecision string + +const ( + // InPlaceApproved means we can in-place update the pod. + InPlaceApproved InPlaceDecision = "InPlaceApproved" + // InPlaceDeferred means we can't in-place update the pod right now, but we will wait for the next loop to check for in-placeability again + InPlaceDeferred InPlaceDecision = "InPlaceDeferred" + // InPlaceEvict means we will attempt to evict the pod. + InPlaceEvict InPlaceDecision = "InPlaceEvict" +) diff --git a/vertical-pod-autoscaler/pkg/utils/annotations/vpa_inplace_update.go b/vertical-pod-autoscaler/pkg/utils/annotations/vpa_inplace_update.go new file mode 100644 index 00000000000..1083b657455 --- /dev/null +++ b/vertical-pod-autoscaler/pkg/utils/annotations/vpa_inplace_update.go @@ -0,0 +1,27 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package annotations + +const ( + // VpaInPlaceUpdatedLabel is a label used by the vpa inplace updated annotation. + VpaInPlaceUpdatedLabel = "vpaInPlaceUpdated" +) + +// GetVpaInPlaceUpdatedValue creates an annotation value for a given pod. +func GetVpaInPlaceUpdatedValue() string { + return "true" +} diff --git a/vertical-pod-autoscaler/pkg/utils/metrics/updater/updater.go b/vertical-pod-autoscaler/pkg/utils/metrics/updater/updater.go index ae3d6f89dde..a0ec4a9ca45 100644 --- a/vertical-pod-autoscaler/pkg/utils/metrics/updater/updater.go +++ b/vertical-pod-autoscaler/pkg/utils/metrics/updater/updater.go @@ -76,13 +76,47 @@ var ( }, []string{"vpa_size_log2"}, ) + inPlaceUpdatableCount = prometheus.NewGaugeVec( + prometheus.GaugeOpts{ + Namespace: metricsNamespace, + Name: "in_place_Updatable_pods_total", + Help: "Number of Pods matching in place update criteria.", + }, []string{"vpa_size_log2"}, + ) + + inPlaceUpdatedCount = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Namespace: metricsNamespace, + Name: "in_place_updated_pods_total", + Help: "Number of Pods updated in-place by Updater to apply a new recommendation.", + }, []string{"vpa_size_log2"}, + ) + + vpasWithInPlaceUpdatablePodsCount = prometheus.NewGaugeVec( + prometheus.GaugeOpts{ + Namespace: metricsNamespace, + Name: "vpas_with_in_place_Updatable_pods_total", + Help: "Number of VPA objects with at least one Pod matching in place update criteria.", + }, []string{"vpa_size_log2"}, + ) + + vpasWithInPlaceUpdatedPodsCount = prometheus.NewGaugeVec( + prometheus.GaugeOpts{ + Namespace: metricsNamespace, + Name: "vpas_with_in_place_updated_pods_total", + Help: "Number of VPA objects with at least one in-place updated Pod.", + }, []string{"vpa_size_log2"}, + ) + + // TODO: Add metrics for failed in-place update attempts + functionLatency = metrics.CreateExecutionTimeMetric(metricsNamespace, "Time spent in various parts of VPA Updater main loop.") ) // Register initializes all metrics for VPA Updater func Register() { - prometheus.MustRegister(controlledCount, evictableCount, evictedCount, vpasWithEvictablePodsCount, vpasWithEvictedPodsCount, functionLatency) + prometheus.MustRegister(controlledCount, evictableCount, evictedCount, vpasWithEvictablePodsCount, vpasWithEvictedPodsCount, inPlaceUpdatableCount, inPlaceUpdatedCount, vpasWithInPlaceUpdatablePodsCount, vpasWithInPlaceUpdatedPodsCount, functionLatency) } // NewExecutionTimer provides a timer for Updater's RunOnce execution @@ -124,6 +158,27 @@ func AddEvictedPod(vpaSize int) { evictedCount.WithLabelValues(strconv.Itoa(log2)).Inc() } +// NewInPlaceUpdtateablePodsCounter returns a wrapper for counting Pods which are matching in-place update criteria +func NewInPlaceUpdtateablePodsCounter() *SizeBasedGauge { + return newSizeBasedGauge(evictableCount) +} + +// NewVpasWithInPlaceUpdtateablePodsCounter returns a wrapper for counting VPA objects with Pods matching in-place update criteria +func NewVpasWithInPlaceUpdtateablePodsCounter() *SizeBasedGauge { + return newSizeBasedGauge(vpasWithEvictablePodsCount) +} + +// NewVpasWithInPlaceUpdtatedPodsCounter returns a wrapper for counting VPA objects with evicted Pods +func NewVpasWithInPlaceUpdtatedPodsCounter() *SizeBasedGauge { + return newSizeBasedGauge(vpasWithEvictedPodsCount) +} + +// AddInPlaceUpdatedPod increases the counter of pods updated in place by Updater, by given VPA size +func AddInPlaceUpdatedPod(vpaSize int) { + log2 := metrics.GetVpaSizeLog2(vpaSize) + inPlaceUpdatedCount.WithLabelValues(strconv.Itoa(log2)).Inc() +} + // Add increases the counter for the given VPA size func (g *SizeBasedGauge) Add(vpaSize int, value int) { log2 := metrics.GetVpaSizeLog2(vpaSize) diff --git a/vertical-pod-autoscaler/pkg/utils/test/test_pod.go b/vertical-pod-autoscaler/pkg/utils/test/test_pod.go index e5f66e81a56..047c9673e34 100644 --- a/vertical-pod-autoscaler/pkg/utils/test/test_pod.go +++ b/vertical-pod-autoscaler/pkg/utils/test/test_pod.go @@ -29,6 +29,8 @@ type PodBuilder interface { WithLabels(labels map[string]string) PodBuilder WithAnnotations(annotations map[string]string) PodBuilder WithPhase(phase apiv1.PodPhase) PodBuilder + WithQOSClass(class apiv1.PodQOSClass) PodBuilder + WithResizeStatus(resizeStatus apiv1.PodResizeStatus) PodBuilder Get() *apiv1.Pod } @@ -47,6 +49,8 @@ type podBuilderImpl struct { labels map[string]string annotations map[string]string phase apiv1.PodPhase + qosClass apiv1.PodQOSClass + resizeStatus apiv1.PodResizeStatus } func (pb *podBuilderImpl) WithLabels(labels map[string]string) PodBuilder { @@ -86,6 +90,18 @@ func (pb *podBuilderImpl) WithPhase(phase apiv1.PodPhase) PodBuilder { return &r } +func (pb *podBuilderImpl) WithQOSClass(class apiv1.PodQOSClass) PodBuilder { + r := *pb + r.qosClass = class + return &r +} + +func (pb *podBuilderImpl) WithResizeStatus(resizeStatus apiv1.PodResizeStatus) PodBuilder { + r := *pb + r.resizeStatus = resizeStatus + return &r +} + func (pb *podBuilderImpl) Get() *apiv1.Pod { startTime := metav1.Time{ Time: testTimestamp, @@ -126,6 +142,12 @@ func (pb *podBuilderImpl) Get() *apiv1.Pod { if pb.phase != "" { pod.Status.Phase = pb.phase } + if pb.qosClass != "" { + pod.Status.QOSClass = pb.qosClass + } + if pb.resizeStatus != "" { + pod.Status.Resize = pb.resizeStatus + } return pod } diff --git a/vertical-pod-autoscaler/pkg/utils/test/test_utils.go b/vertical-pod-autoscaler/pkg/utils/test/test_utils.go index 257f613504f..33beb7dadd4 100644 --- a/vertical-pod-autoscaler/pkg/utils/test/test_utils.go +++ b/vertical-pod-autoscaler/pkg/utils/test/test_utils.go @@ -33,6 +33,7 @@ import ( vpa_types_v1beta1 "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1beta1" vpa_lister "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/listers/autoscaling.k8s.io/v1" vpa_lister_v1beta1 "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/listers/autoscaling.k8s.io/v1beta1" + utils "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/updater/utils" ) var ( @@ -121,6 +122,23 @@ func (m *PodsEvictionRestrictionMock) CanEvict(pod *apiv1.Pod) bool { return args.Bool(0) } +// PodsInPlaceRestrictionMock is a mock of PodsInPlaceRestriction +type PodsInPlaceRestrictionMock struct { + mock.Mock +} + +// InPlaceUpdate is a mock implementation of PodsInPlaceRestriction.InPlaceUpdate +func (m *PodsInPlaceRestrictionMock) InPlaceUpdate(pod *apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler, eventRecorder record.EventRecorder) error { + args := m.Called(pod, eventRecorder) + return args.Error(0) +} + +// CanInPlaceUpdate is a mock implementation of PodsInPlaceRestriction.CanInPlaceUpdate +func (m *PodsInPlaceRestrictionMock) CanInPlaceUpdate(pod *apiv1.Pod) utils.InPlaceDecision { + args := m.Called(pod) + return args.Get(0).(utils.InPlaceDecision) +} + // PodListerMock is a mock of PodLister type PodListerMock struct { mock.Mock