Merge pull request #8004 from laoj2/fix-container-status

Also consider containerStatus.resources when handling container resources

Author: k8s-ci-robot

vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/resource_updates.go

@@ -26,6 +26,7 @@ import (
 	resource_admission "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource"
 	"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"
+	resourcehelpers "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/resources"
 	vpa_api_util "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/vpa"
 )
 
@@ -76,8 +77,8 @@ func (c *resourcesUpdatesPatchCalculator) CalculatePatches(pod *core.Pod, vpa *v
 func getContainerPatch(pod *core.Pod, i int, annotationsPerContainer vpa_api_util.ContainerToAnnotationsMap, containerResources vpa_api_util.ContainerResources) ([]resource_admission.PatchRecord, string) {
 	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 {
+	requests, limits := resourcehelpers.ContainerRequestsAndLimits(pod.Spec.Containers[i].Name, pod)
+	if limits == nil && requests == nil {
 		patches = append(patches, getPatchInitializingEmptyResources(i))
 	}
 
@@ -86,8 +87,8 @@ func getContainerPatch(pod *core.Pod, i int, annotationsPerContainer vpa_api_uti
 		annotations = make([]string, 0)
 	}
 
-	patches, annotations = appendPatchesAndAnnotations(patches, annotations, pod.Spec.Containers[i].Resources.Requests, i, containerResources.Requests, "requests", "request")
-	patches, annotations = appendPatchesAndAnnotations(patches, annotations, pod.Spec.Containers[i].Resources.Limits, i, containerResources.Limits, "limits", "limit")
+	patches, annotations = appendPatchesAndAnnotations(patches, annotations, requests, i, containerResources.Requests, "requests", "request")
+	patches, annotations = appendPatchesAndAnnotations(patches, annotations, limits, i, containerResources.Limits, "limits", "limit")
 
 	updatesAnnotation := fmt.Sprintf("container %d: ", i) + strings.Join(annotations, ", ")
 	return patches, updatesAnnotation

vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/resource_updates_test.go

@@ -103,7 +103,7 @@ func addAnnotationRequest(updateResources [][]string, kind string) resource_admi
 	return GetAddAnnotationPatch(ResourceUpdatesAnnotation, vpaUpdates)
 }
 
-func TestClalculatePatches_ResourceUpdates(t *testing.T) {
+func TestCalculatePatches_ResourceUpdates(t *testing.T) {
 	tests := []struct {
 		name                 string
 		pod                  *core.Pod
@@ -164,11 +164,32 @@ func TestClalculatePatches_ResourceUpdates(t *testing.T) {
 				addAnnotationRequest([][]string{{cpu}}, request),
 			},
 		},
+		{
+			name: "replacement cpu request recommendation from container status",
+			pod: test.Pod().
+				AddContainer(core.Container{}).
+				AddContainerStatus(test.ContainerStatus().
+					WithCPURequest(resource.MustParse("0")).Get()).Get(),
+			namespace: "default",
+			recommendResources: []vpa_api_util.ContainerResources{
+				{
+					Requests: core.ResourceList{
+						cpu: resource.MustParse("1"),
+					},
+				},
+			},
+			recommendAnnotations: vpa_api_util.ContainerToAnnotationsMap{},
+			expectPatches: []resource_admission.PatchRecord{
+				addResourceRequestPatch(0, cpu, "1"),
+				addAnnotationRequest([][]string{{cpu}}, request),
+			},
+		},
 		{
 			name: "two containers",
 			pod: &core.Pod{
 				Spec: core.PodSpec{
 					Containers: []core.Container{{
+						Name: "container-1",
 						Resources: core.ResourceRequirements{
 							Requests: core.ResourceList{
 								cpu: resource.MustParse("0"),
@@ -249,6 +270,26 @@ func TestClalculatePatches_ResourceUpdates(t *testing.T) {
 				addAnnotationRequest([][]string{{cpu}}, limit),
 			},
 		},
+		{
+			name: "replacement cpu limit from container status",
+			pod: test.Pod().
+				AddContainer(core.Container{}).
+				AddContainerStatus(test.ContainerStatus().
+					WithCPULimit(resource.MustParse("0")).Get()).Get(),
+			namespace: "default",
+			recommendResources: []vpa_api_util.ContainerResources{
+				{
+					Limits: core.ResourceList{
+						cpu: resource.MustParse("1"),
+					},
+				},
+			},
+			recommendAnnotations: vpa_api_util.ContainerToAnnotationsMap{},
+			expectPatches: []resource_admission.PatchRecord{
+				addResourceLimitPatch(0, cpu, "1"),
+				addAnnotationRequest([][]string{{cpu}}, limit),
+			},
+		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {

vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider.go

@@ -24,6 +24,7 @@ import (
 
 	vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1"
 	"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/limitrange"
+	resourcehelpers "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/resources"
 	vpa_api_util "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/vpa"
 )
 
@@ -53,14 +54,15 @@ func GetContainersResources(pod *core.Pod, vpaResourcePolicy *vpa_types.PodResou
 	addAll bool, annotations vpa_api_util.ContainerToAnnotationsMap) []vpa_api_util.ContainerResources {
 	resources := make([]vpa_api_util.ContainerResources, len(pod.Spec.Containers))
 	for i, container := range pod.Spec.Containers {
+		containerRequests, containerLimits := resourcehelpers.ContainerRequestsAndLimits(container.Name, pod)
 		recommendation := vpa_api_util.GetRecommendationForContainer(container.Name, &podRecommendation)
 		if recommendation == nil {
 			if !addAll {
 				klog.V(2).InfoS("No recommendation found for container, skipping", "container", container.Name)
 				continue
 			}
 			klog.V(2).InfoS("No match found for container, using Pod request", "container", container.Name)
-			resources[i].Requests = container.Resources.Requests
+			resources[i].Requests = containerRequests
 		} else {
 			resources[i].Requests = recommendation.Target
 		}
@@ -70,7 +72,7 @@ func GetContainersResources(pod *core.Pod, vpaResourcePolicy *vpa_types.PodResou
 		}
 		containerControlledValues := vpa_api_util.GetContainerControlledValues(container.Name, vpaResourcePolicy)
 		if containerControlledValues == vpa_types.ContainerControlledValuesRequestsAndLimits {
-			proportionalLimits, limitAnnotations := vpa_api_util.GetProportionalLimit(container.Resources.Limits, container.Resources.Requests, resources[i].Requests, defaultLimit)
+			proportionalLimits, limitAnnotations := vpa_api_util.GetProportionalLimit(containerLimits, containerRequests, resources[i].Requests, defaultLimit)
 			if proportionalLimits != nil {
 				resources[i].Limits = proportionalLimits
 				if len(limitAnnotations) > 0 {
@@ -88,19 +90,19 @@ func GetContainersResources(pod *core.Pod, vpaResourcePolicy *vpa_types.PodResou
 				resources[i].Limits = core.ResourceList{}
 			}
 
-			cpuRequest, hasCpuRequest := container.Resources.Requests[core.ResourceCPU]
+			cpuRequest, hasCpuRequest := containerRequests[core.ResourceCPU]
 			if _, ok := resources[i].Requests[core.ResourceCPU]; !ok && hasCpuRequest {
 				resources[i].Requests[core.ResourceCPU] = cpuRequest
 			}
-			memRequest, hasMemRequest := container.Resources.Requests[core.ResourceMemory]
+			memRequest, hasMemRequest := containerRequests[core.ResourceMemory]
 			if _, ok := resources[i].Requests[core.ResourceMemory]; !ok && hasMemRequest {
 				resources[i].Requests[core.ResourceMemory] = memRequest
 			}
-			cpuLimit, hasCpuLimit := container.Resources.Limits[core.ResourceCPU]
+			cpuLimit, hasCpuLimit := containerLimits[core.ResourceCPU]
 			if _, ok := resources[i].Limits[core.ResourceCPU]; !ok && hasCpuLimit {
 				resources[i].Limits[core.ResourceCPU] = cpuLimit
 			}
-			memLimit, hasMemLimit := container.Resources.Limits[core.ResourceMemory]
+			memLimit, hasMemLimit := containerLimits[core.ResourceMemory]
 			if _, ok := resources[i].Limits[core.ResourceMemory]; !ok && hasMemLimit {
 				resources[i].Limits[core.ResourceMemory] = memLimit
 			}

vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider_test.go

@@ -365,6 +365,7 @@ func TestGetContainersResources(t *testing.T) {
 	testCases := []struct {
 		name             string
 		container        apiv1.Container
+		containerStatus  apiv1.ContainerStatus
 		vpa              *vpa_types.VerticalPodAutoscaler
 		expectedCPU      *resource.Quantity
 		expectedMem      *resource.Quantity
@@ -518,11 +519,26 @@ func TestGetContainersResources(t *testing.T) {
 			expectedMemLimit: mustParseResourcePointer("20M"),
 			addAll:           false,
 		},
+		{
+			name:      "CPU and memory recommendation, request and limits only set in containerStatus, addAll true",
+			container: test.Container().WithName("container").Get(),
+			containerStatus: test.ContainerStatus().WithName("container").
+				WithCPURequest(resource.MustParse("1")).
+				WithMemRequest(resource.MustParse("1M")).
+				WithCPULimit(resource.MustParse("10")).
+				WithMemLimit(resource.MustParse("3M")).Get(),
+			vpa:              test.VerticalPodAutoscaler().WithContainer("container").WithTarget("3", "2M").Get(),
+			expectedCPU:      mustParseResourcePointer("3"),
+			expectedMem:      mustParseResourcePointer("2M"),
+			expectedCPULimit: mustParseResourcePointer("30"),
+			expectedMemLimit: mustParseResourcePointer("6M"),
+			addAll:           true,
+		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			pod := test.Pod().WithName("pod").AddContainer(tc.container).Get()
+			pod := test.Pod().WithName("pod").AddContainer(tc.container).AddContainerStatus(tc.containerStatus).Get()
 			resources := GetContainersResources(pod, tc.vpa.Spec.ResourcePolicy, *tc.vpa.Status.Recommendation, nil, tc.addAll, vpa_api_util.ContainerToAnnotationsMap{})
 
 			cpu, cpuPresent := resources[0].Requests[apiv1.ResourceCPU]

vertical-pod-autoscaler/pkg/recommender/input/oom/observer.go

@@ -25,6 +25,7 @@ import (
 	"k8s.io/client-go/tools/cache"
 
 	"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/model"
+	resourcehelpers "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/resources"
 
 	"k8s.io/klog/v2"
 )
@@ -156,7 +157,11 @@ func (o *observer) OnUpdate(oldObj, newObj interface{}) {
 			if oldStatus != nil && containerStatus.RestartCount > oldStatus.RestartCount {
 				oldSpec := findSpec(containerStatus.Name, oldPod.Spec.Containers)
 				if oldSpec != nil {
-					memory := oldSpec.Resources.Requests[apiv1.ResourceMemory]
+					requests, _ := resourcehelpers.ContainerRequestsAndLimits(containerStatus.Name, oldPod)
+					var memory resource.Quantity
+					if requests != nil {
+						memory = requests[apiv1.ResourceMemory]
+					}
 					oomInfo := OomInfo{
 						Timestamp: containerStatus.LastTerminationState.Terminated.FinishedAt.Time.UTC(),
 						Memory:    model.ResourceAmount(memory.Value()),

vertical-pod-autoscaler/pkg/recommender/input/oom/observer_test.go

@@ -100,18 +100,85 @@ func TestOOMReceived(t *testing.T) {
 	assert.NoError(t, err)
 	p2, err := newPod(pod2Yaml)
 	assert.NoError(t, err)
-	observer := NewObserver()
-	go observer.OnUpdate(p1, p2)
-
-	info := <-observer.observedOomsChannel
-	container := info.ContainerID
-	assert.Equal(t, "mockNamespace", container.PodID.Namespace)
-	assert.Equal(t, "Pod1", container.PodID.PodName)
-	assert.Equal(t, "Name11", container.ContainerName)
-	assert.Equal(t, model.ResourceAmount(int64(1024)), info.Memory)
 	timestamp, err := time.Parse(time.RFC3339, "2018-02-23T13:38:48Z")
 	assert.NoError(t, err)
-	assert.Equal(t, timestamp.Unix(), info.Timestamp.Unix())
+
+	testCases := []struct {
+		desc        string
+		oldPod      *v1.Pod
+		newPod      *v1.Pod
+		wantOOMInfo OomInfo
+	}{
+		{
+			desc:   "OK",
+			oldPod: p1,
+			newPod: p2,
+			wantOOMInfo: OomInfo{
+				ContainerID: model.ContainerID{
+					ContainerName: "Name11",
+					PodID: model.PodID{
+						Namespace: "mockNamespace",
+						PodName:   "Pod1",
+					},
+				},
+				Memory:    model.ResourceAmount(int64(1024)),
+				Timestamp: timestamp,
+			},
+		},
+		{
+			desc: "Old pod does not set memory requests",
+			oldPod: func() *v1.Pod {
+				oldPod := p1.DeepCopy()
+				oldPod.Spec.Containers[0].Resources.Requests = nil
+				oldPod.Status.ContainerStatuses[0].Resources = nil
+				return oldPod
+			}(),
+			newPod: p2,
+			wantOOMInfo: OomInfo{
+				ContainerID: model.ContainerID{
+					ContainerName: "Name11",
+					PodID: model.PodID{
+						Namespace: "mockNamespace",
+						PodName:   "Pod1",
+					},
+				},
+				Memory:    model.ResourceAmount(int64(0)),
+				Timestamp: timestamp,
+			},
+		},
+		{
+			desc: "Old pod also set memory request in containerStatus, prefer info from containerStatus",
+			oldPod: func() *v1.Pod {
+				oldPod := p1.DeepCopy()
+				oldPod.Status.ContainerStatuses[0].Resources = &v1.ResourceRequirements{
+					Requests: v1.ResourceList{
+						v1.ResourceMemory: resource.MustParse("2048"),
+					},
+				}
+				return oldPod
+			}(),
+			newPod: p2,
+			wantOOMInfo: OomInfo{
+				ContainerID: model.ContainerID{
+					ContainerName: "Name11",
+					PodID: model.PodID{
+						Namespace: "mockNamespace",
+						PodName:   "Pod1",
+					},
+				},
+				Memory:    model.ResourceAmount(int64(2048)),
+				Timestamp: timestamp,
+			},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			observer := NewObserver()
+			observer.OnUpdate(tc.oldPod, tc.newPod)
+			info := <-observer.observedOomsChannel
+			assert.Equal(t, tc.wantOOMInfo, info)
+		})
+	}
 }
 
 func TestMalformedPodReceived(t *testing.T) {

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

@@ -24,6 +24,7 @@ import (
 
 	vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1"
 	"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/annotations"
+	resourcehelpers "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/resources"
 	vpa_api_util "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/vpa"
 )
 
@@ -65,7 +66,8 @@ func (*defaultPriorityProcessor) GetUpdatePriority(pod *apiv1.Pod, vpa *vpa_type
 			totalRecommendedPerResource[resourceName] += recommended.MilliValue()
 			lowerBound, hasLowerBound := recommendedRequest.LowerBound[resourceName]
 			upperBound, hasUpperBound := recommendedRequest.UpperBound[resourceName]
-			if request, hasRequest := podContainer.Resources.Requests[resourceName]; hasRequest {
+			requests, _ := resourcehelpers.ContainerRequestsAndLimits(podContainer.Name, pod)
+			if request, hasRequest := requests[resourceName]; hasRequest {
 				totalRequestPerResource[resourceName] += request.MilliValue()
 				if recommended.MilliValue() > request.MilliValue() {
 					scaleUp = true

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

@@ -46,6 +46,16 @@ func TestGetUpdatePriority(t *testing.T) {
 				ResourceDiff:            4.0,
 				ScaleUp:                 true,
 			},
+		}, {
+			name: "simple scale up, resources from containerStatus have higher priority",
+			pod: test.Pod().WithName("POD1").AddContainer(test.Container().WithName(containerName).WithCPURequest(resource.MustParse("10000")).Get()).
+				AddContainerStatus(test.ContainerStatus().WithName(containerName).WithCPURequest(resource.MustParse("2")).Get()).Get(),
+			vpa: test.VerticalPodAutoscaler().WithContainer(containerName).WithTarget("10", "").Get(),
+			expectedPrio: PodPriority{
+				OutsideRecommendedRange: false,
+				ResourceDiff:            4.0,
+				ScaleUp:                 true,
+			},
 		}, {
 			name: "simple scale down",
 			pod:  test.Pod().WithName("POD1").AddContainer(test.Container().WithName(containerName).WithCPURequest(resource.MustParse("4")).Get()).Get(),

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

@@ -20,6 +20,7 @@ import (
 	apiv1 "k8s.io/api/core/v1"
 
 	vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1"
+	resourcehelpers "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/resources"
 	vpa_utils "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/vpa"
 )
 
@@ -50,23 +51,24 @@ func (s *scalingDirectionPodEvictionAdmission) Admit(pod *apiv1.Pod, resources *
 		if recommendedResources == nil {
 			continue
 		}
-		if s.admitContainer(container, recommendedResources, podEvictionRequirements) {
+		containerRequests, _ := resourcehelpers.ContainerRequestsAndLimits(container.Name, pod)
+		if s.admitContainer(containerRequests, recommendedResources, podEvictionRequirements) {
 			return true
 		}
 	}
 	return false
 }
 
-func (s *scalingDirectionPodEvictionAdmission) admitContainer(container apiv1.Container, recommendedResources *vpa_types.RecommendedContainerResources, podEvictionRequirements []*vpa_types.EvictionRequirement) bool {
-	_, foundCPURequests := container.Resources.Requests[apiv1.ResourceCPU]
+func (s *scalingDirectionPodEvictionAdmission) admitContainer(containerRequests apiv1.ResourceList, recommendedResources *vpa_types.RecommendedContainerResources, podEvictionRequirements []*vpa_types.EvictionRequirement) bool {
+	_, foundCPURequests := containerRequests[apiv1.ResourceCPU]
 	if !foundCPURequests {
 		return true
 	}
-	_, foundMemoryRequests := container.Resources.Requests[apiv1.ResourceMemory]
+	_, foundMemoryRequests := containerRequests[apiv1.ResourceMemory]
 	if !foundMemoryRequests {
 		return true
 	}
-	return s.checkEvictionRequirementsForContainer(container.Resources.Requests, recommendedResources.Target, podEvictionRequirements)
+	return s.checkEvictionRequirementsForContainer(containerRequests, recommendedResources.Target, podEvictionRequirements)
 }
 
 func (s *scalingDirectionPodEvictionAdmission) checkEvictionRequirementsForContainer(requestedResources apiv1.ResourceList, recommendedResources apiv1.ResourceList, evictionRequirements []*vpa_types.EvictionRequirement) bool {