Merge pull request #8062 from laoj2/fix-spec-client

Read container resources from containerStatus in the spec client

vertical-pod-autoscaler/pkg/recommender/input/spec/spec_client.go

@@ -22,6 +22,7 @@ import (
 	v1lister "k8s.io/client-go/listers/core/v1"
 
 	"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/model"
+	resourcehelpers "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/resources"
 )
 
 // BasicPodSpec contains basic information defining a pod and its containers.
@@ -79,16 +80,13 @@ func (client *specClient) GetPodSpecs() ([]*BasicPodSpec, error) {
 	}
 	return podSpecs, nil
 }
+
 func newBasicPodSpec(pod *v1.Pod) *BasicPodSpec {
-	podId := model.PodID{
+	containerSpecs := newContainerSpecs(pod, pod.Spec.Containers, false /* isInitContainer */)
-		PodName:   pod.Name,
+	initContainerSpecs := newContainerSpecs(pod, pod.Spec.InitContainers, true /* isInitContainer */)
-		Namespace: pod.Namespace,
-	}
-	containerSpecs := newContainerSpecs(podId, pod.Spec.Containers)
-	initContainerSpecs := newContainerSpecs(podId, pod.Spec.InitContainers)
 
 	basicPodSpec := &BasicPodSpec{
-		ID:             podId,
+		ID:             podID(pod),
 		PodLabels:      pod.Labels,
 		Containers:     containerSpecs,
 		InitContainers: initContainerSpecs,
@@ -97,34 +95,38 @@ func newBasicPodSpec(pod *v1.Pod) *BasicPodSpec {
 	return basicPodSpec
 }
 
-func newContainerSpecs(podID model.PodID, containers []v1.Container) []BasicContainerSpec {
+func newContainerSpecs(pod *v1.Pod, containers []v1.Container, isInitContainer bool) []BasicContainerSpec {
 	var containerSpecs []BasicContainerSpec
-
 	for _, container := range containers {
-		containerSpec := newContainerSpec(podID, container)
+		containerSpec := newContainerSpec(pod, container, isInitContainer)
 		containerSpecs = append(containerSpecs, containerSpec)
 	}
-
 	return containerSpecs
 }
 
-func newContainerSpec(podID model.PodID, container v1.Container) BasicContainerSpec {
+func newContainerSpec(pod *v1.Pod, container v1.Container, isInitContainer bool) BasicContainerSpec {
 	containerSpec := BasicContainerSpec{
 		ID: model.ContainerID{
-			PodID:         podID,
+			PodID:         podID(pod),
 			ContainerName: container.Name,
 		},
 		Image:   container.Image,
-		Request: calculateRequestedResources(container),
+		Request: calculateRequestedResources(pod, container, isInitContainer),
 	}
 	return containerSpec
 }
 
-func calculateRequestedResources(container v1.Container) model.Resources {
+func calculateRequestedResources(pod *v1.Pod, container v1.Container, isInitContainer bool) model.Resources {
-	cpuQuantity := container.Resources.Requests[v1.ResourceCPU]
+	requestsAndLimitsFn := resourcehelpers.ContainerRequestsAndLimits
+	if isInitContainer {
+		requestsAndLimitsFn = resourcehelpers.InitContainerRequestsAndLimits
+	}
+	requests, _ := requestsAndLimitsFn(container.Name, pod)
+
+	cpuQuantity := requests[v1.ResourceCPU]
 	cpuMillicores := cpuQuantity.MilliValue()
 
-	memoryQuantity := container.Resources.Requests[v1.ResourceMemory]
+	memoryQuantity := requests[v1.ResourceMemory]
 	memoryBytes := memoryQuantity.Value()
 
 	return model.Resources{
@@ -133,3 +135,10 @@ func calculateRequestedResources(container v1.Container) model.Resources {
 	}
 
 }
+
+func podID(pod *v1.Pod) model.PodID {
+	return model.PodID{
+		PodName:   pod.Name,
+		Namespace: pod.Namespace,
+	}
+}

vertical-pod-autoscaler/pkg/recommender/input/spec/spec_client_test.go

@@ -47,6 +47,6 @@ func TestGetPodSpecsReturnsSpecs(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, len(tc.podSpecs), len(podSpecs), "SpecClient returned different number of results then expected")
 	for _, podSpec := range podSpecs {
-		assert.Contains(t, tc.podSpecs, podSpec, "One of returned BasicPodSpcec is different than expected")
+		assert.Contains(t, tc.podSpecs, podSpec, "One of returned BasicPodSpec is different than expected")
 	}
 }

vertical-pod-autoscaler/pkg/recommender/input/spec/spec_client_test_util.go

@@ -68,6 +68,19 @@ metadata:
   name: Pod2
   labels:
     Pod2LabelKey: Pod2LabelValue
+status:
+  containerStatuses:
+  - name: Name23
+    resources:
+      requests:
+        memory: "250Mi"
+        cpu: "30m"
+  initContainerStatuses:
+  - name: Name22-init
+    resources:
+      requests:
+        memory: "350Mi"
+        cpu: "40m"
 spec:
   containers:
   - name: Name21
@@ -82,6 +95,14 @@ spec:
       requests:
         memory: "4096Mi"
         cpu: "4000m"
+  - name: Name23
+    image: Name23Image
+    resources:
+      # Requests below will be ignored because
+      # requests are also defined in containerStatus.
+      requests:
+        memory: "1Mi"
+        cpu: "1m"
   initContainers:
   - name: Name21-init
     image: Name21-initImage
@@ -89,6 +110,14 @@ spec:
       requests:
         memory: "128Mi"
         cpu: "40m"
+  - name: Name22-init
+    image: Name22-initImage
+    resources:
+      requests:
+        # Requests below will be ignored because
+        # requests are also defined in initContainerStatus.
+        memory: "1Mi"
+        cpu: "1m"
 `
 
 type podListerMock struct {
@@ -122,11 +151,13 @@ func newSpecClientTestCase() *specClientTestCase {
 	containerSpec12 := newTestContainerSpec(podID1, "Name12", 1000, 1024*1024*1024)
 	containerSpec21 := newTestContainerSpec(podID2, "Name21", 2000, 2048*1024*1024)
 	containerSpec22 := newTestContainerSpec(podID2, "Name22", 4000, 4096*1024*1024)
+	containerSpec23 := newTestContainerSpec(podID2, "Name23", 30, 250*1024*1024)
 
 	initContainerSpec21 := newTestContainerSpec(podID2, "Name21-init", 40, 128*1024*1024)
+	initContainerSpec22 := newTestContainerSpec(podID2, "Name22-init", 40, 350*1024*1024)
 
 	podSpec1 := newTestPodSpec(podID1, []BasicContainerSpec{containerSpec11, containerSpec12}, nil)
-	podSpec2 := newTestPodSpec(podID2, []BasicContainerSpec{containerSpec21, containerSpec22}, []BasicContainerSpec{initContainerSpec21})
+	podSpec2 := newTestPodSpec(podID2, []BasicContainerSpec{containerSpec21, containerSpec22, containerSpec23}, []BasicContainerSpec{initContainerSpec21, initContainerSpec22})
 
 	return &specClientTestCase{
 		podSpecs: []*BasicPodSpec{podSpec1, podSpec2},

vertical-pod-autoscaler/pkg/utils/resources/resourcehelpers.go

@@ -30,7 +30,7 @@ import (
 //
 // [1] https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/1287-in-place-update-pod-resources
 func ContainerRequestsAndLimits(containerName string, pod *v1.Pod) (v1.ResourceList, v1.ResourceList) {
-	cs := containerStatusForContainer(containerName, pod)
+	cs := containerStatusFor(containerName, pod)
 	if cs != nil && cs.Resources != nil {
 		return cs.Resources.Requests.DeepCopy(), cs.Resources.Limits.DeepCopy()
 	}
@@ -44,6 +44,29 @@ func ContainerRequestsAndLimits(containerName string, pod *v1.Pod) (v1.ResourceL
 	return nil, nil
 }
 
+// InitContainerRequestsAndLimits returns a copy of the actual resource requests
+// and limits of a given initContainer:
+//
+//   - If in-place pod updates feature [1] is enabled, the actual resource requests
+//     are stored in the initContainer status field.
+//   - Otherwise, fallback to the resource requests defined in the pod spec.
+//
+// [1] https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/1287-in-place-update-pod-resources
+func InitContainerRequestsAndLimits(initContainerName string, pod *v1.Pod) (v1.ResourceList, v1.ResourceList) {
+	cs := initContainerStatusFor(initContainerName, pod)
+	if cs != nil && cs.Resources != nil {
+		return cs.Resources.Requests.DeepCopy(), cs.Resources.Limits.DeepCopy()
+	}
+
+	klog.V(6).InfoS("initContainer resources not found in initContainerStatus for initContainer. Falling back to resources defined in the pod spec. This is expected for clusters with in-place pod updates feature disabled.", "initContainer", initContainerName, "initContainerStatus", cs)
+	initContainer := findInitContainer(initContainerName, pod)
+	if initContainer != nil {
+		return initContainer.Resources.Requests.DeepCopy(), initContainer.Resources.Limits.DeepCopy()
+	}
+
+	return nil, nil
+}
+
 func findContainer(containerName string, pod *v1.Pod) *v1.Container {
 	for i, container := range pod.Spec.Containers {
 		if container.Name == containerName {
@@ -53,7 +76,16 @@ func findContainer(containerName string, pod *v1.Pod) *v1.Container {
 	return nil
 }
 
-func containerStatusForContainer(containerName string, pod *v1.Pod) *v1.ContainerStatus {
+func findInitContainer(initContainerName string, pod *v1.Pod) *v1.Container {
+	for i, initContainer := range pod.Spec.InitContainers {
+		if initContainer.Name == initContainerName {
+			return &pod.Spec.InitContainers[i]
+		}
+	}
+	return nil
+}
+
+func containerStatusFor(containerName string, pod *v1.Pod) *v1.ContainerStatus {
 	for i, containerStatus := range pod.Status.ContainerStatuses {
 		if containerStatus.Name == containerName {
 			return &pod.Status.ContainerStatuses[i]
@@ -61,3 +93,12 @@ func containerStatusForContainer(containerName string, pod *v1.Pod) *v1.Containe
 	}
 	return nil
 }
+
+func initContainerStatusFor(initContainerName string, pod *v1.Pod) *v1.ContainerStatus {
+	for i, initContainerStatus := range pod.Status.InitContainerStatuses {
+		if initContainerStatus.Name == initContainerName {
+			return &pod.Status.InitContainerStatuses[i]
+		}
+	}
+	return nil
+}

vertical-pod-autoscaler/pkg/utils/resources/resourcehelpers_test.go

@@ -107,11 +107,36 @@ func TestContainerRequestsAndLimits(t *testing.T) {
 			wantLimits:   nil,
 		},
 		{
-			desc:          "Container with no requests or limits",
+			desc:          "Init container with the same name as the container is ignored",
-			containerName: "inexistent-container",
+			containerName: "container-1",
+			pod: test.Pod().AddInitContainer(
+				test.Container().WithName("container-1").
+					WithCPURequest(resource.MustParse("1")).
+					WithMemRequest(resource.MustParse("10Mi")).
+					WithCPULimit(resource.MustParse("2")).
+					WithMemLimit(resource.MustParse("20Mi")).Get()).
+				AddContainer(
+					test.Container().WithName("container-1").
+						WithCPURequest(resource.MustParse("4")).
+						WithMemRequest(resource.MustParse("40Mi")).
+						WithCPULimit(resource.MustParse("5")).
+						WithMemLimit(resource.MustParse("50Mi")).Get()).
+				Get(),
+			wantRequests: apiv1.ResourceList{
+				apiv1.ResourceCPU:    resource.MustParse("4"),
+				apiv1.ResourceMemory: resource.MustParse("40Mi"),
+			},
+			wantLimits: apiv1.ResourceList{
+				apiv1.ResourceCPU:    resource.MustParse("5"),
+				apiv1.ResourceMemory: resource.MustParse("50Mi"),
+			},
+		},
+		{
+			desc:          "Container with no requests or limits returns non-nil resources",
+			containerName: "container",
 			pod:           test.Pod().AddContainer(test.Container().WithName("container").Get()).Get(),
-			wantRequests:  nil,
+			wantRequests:  apiv1.ResourceList{},
-			wantLimits:    nil,
+			wantLimits:    apiv1.ResourceList{},
 		},
 		{
 			desc:          "2 containers",
@@ -159,3 +184,162 @@ func TestContainerRequestsAndLimits(t *testing.T) {
 		})
 	}
 }
+
+func TestInitContainerRequestsAndLimits(t *testing.T) {
+	testCases := []struct {
+		desc              string
+		initContainerName string
+		pod               *apiv1.Pod
+		wantRequests      apiv1.ResourceList
+		wantLimits        apiv1.ResourceList
+	}{
+		{
+			desc:              "Prefer resource requests from initContainer status",
+			initContainerName: "init-container",
+			pod: test.Pod().AddInitContainer(
+				test.Container().WithName("init-container").
+					WithCPURequest(resource.MustParse("1")).
+					WithMemRequest(resource.MustParse("10Mi")).
+					WithCPULimit(resource.MustParse("2")).
+					WithMemLimit(resource.MustParse("20Mi")).Get()).
+				AddInitContainerStatus(
+					test.ContainerStatus().WithName("init-container").
+						WithCPURequest(resource.MustParse("3")).
+						WithMemRequest(resource.MustParse("30Mi")).
+						WithCPULimit(resource.MustParse("4")).
+						WithMemLimit(resource.MustParse("40Mi")).Get()).Get(),
+			wantRequests: apiv1.ResourceList{
+				apiv1.ResourceCPU:    resource.MustParse("3"),
+				apiv1.ResourceMemory: resource.MustParse("30Mi"),
+			},
+			wantLimits: apiv1.ResourceList{
+				apiv1.ResourceCPU:    resource.MustParse("4"),
+				apiv1.ResourceMemory: resource.MustParse("40Mi"),
+			},
+		},
+		{
+			desc:              "No initContainer status, get resources from pod spec",
+			initContainerName: "init-container",
+			pod: test.Pod().AddInitContainer(
+				test.Container().WithName("init-container").
+					WithCPURequest(resource.MustParse("1")).
+					WithMemRequest(resource.MustParse("10Mi")).
+					WithCPULimit(resource.MustParse("2")).
+					WithMemLimit(resource.MustParse("20Mi")).Get()).Get(),
+			wantRequests: apiv1.ResourceList{
+				apiv1.ResourceCPU:    resource.MustParse("1"),
+				apiv1.ResourceMemory: resource.MustParse("10Mi"),
+			},
+			wantLimits: apiv1.ResourceList{
+				apiv1.ResourceCPU:    resource.MustParse("2"),
+				apiv1.ResourceMemory: resource.MustParse("20Mi"),
+			},
+		},
+		{
+			desc:              "Only initContainerStatus, get resources from initContainerStatus",
+			initContainerName: "init-container",
+			pod: test.Pod().AddInitContainerStatus(
+				test.ContainerStatus().WithName("init-container").
+					WithCPURequest(resource.MustParse("0")).
+					WithMemRequest(resource.MustParse("30Mi")).
+					WithCPULimit(resource.MustParse("4")).
+					WithMemLimit(resource.MustParse("40Mi")).Get()).Get(),
+			wantRequests: apiv1.ResourceList{
+				apiv1.ResourceCPU:    resource.MustParse("0"),
+				apiv1.ResourceMemory: resource.MustParse("30Mi"),
+			},
+			wantLimits: apiv1.ResourceList{
+				apiv1.ResourceCPU:    resource.MustParse("4"),
+				apiv1.ResourceMemory: resource.MustParse("40Mi"),
+			},
+		},
+		{
+			desc:              "Inexistent initContainer",
+			initContainerName: "inexistent-init-container",
+			pod: test.Pod().AddInitContainer(
+				test.Container().WithName("init-container").
+					WithCPURequest(resource.MustParse("1")).
+					WithMemRequest(resource.MustParse("10Mi")).
+					WithCPULimit(resource.MustParse("2")).
+					WithMemLimit(resource.MustParse("20Mi")).Get()).Get(),
+			wantRequests: nil,
+			wantLimits:   nil,
+		},
+		{
+			desc:              "Container with the same name as the initContainer is ignored",
+			initContainerName: "container-1",
+			pod: test.Pod().AddInitContainer(
+				test.Container().WithName("container-1").
+					WithCPURequest(resource.MustParse("1")).
+					WithMemRequest(resource.MustParse("10Mi")).
+					WithCPULimit(resource.MustParse("2")).
+					WithMemLimit(resource.MustParse("20Mi")).Get()).
+				AddContainer(
+					test.Container().WithName("container-1").
+						WithCPURequest(resource.MustParse("4")).
+						WithMemRequest(resource.MustParse("40Mi")).
+						WithCPULimit(resource.MustParse("5")).
+						WithMemLimit(resource.MustParse("50Mi")).Get()).
+				Get(),
+			wantRequests: apiv1.ResourceList{
+				apiv1.ResourceCPU:    resource.MustParse("1"),
+				apiv1.ResourceMemory: resource.MustParse("10Mi"),
+			},
+			wantLimits: apiv1.ResourceList{
+				apiv1.ResourceCPU:    resource.MustParse("2"),
+				apiv1.ResourceMemory: resource.MustParse("20Mi"),
+			},
+		},
+		{
+			desc:              "InitContainer with no requests or limits returns non-nil resources",
+			initContainerName: "init-container",
+			pod:               test.Pod().AddInitContainer(test.Container().WithName("init-container").Get()).Get(),
+			wantRequests:      apiv1.ResourceList{},
+			wantLimits:        apiv1.ResourceList{},
+		},
+		{
+			desc:              "2 init containers",
+			initContainerName: "init-container-1",
+			pod: test.Pod().AddInitContainer(
+				test.Container().WithName("init-container-1").
+					WithCPURequest(resource.MustParse("1")).
+					WithMemRequest(resource.MustParse("10Mi")).
+					WithCPULimit(resource.MustParse("2")).
+					WithMemLimit(resource.MustParse("20Mi")).Get()).
+				AddInitContainerStatus(
+					test.ContainerStatus().WithName("init-container-1").
+						WithCPURequest(resource.MustParse("3")).
+						WithMemRequest(resource.MustParse("30Mi")).
+						WithCPULimit(resource.MustParse("4")).
+						WithMemLimit(resource.MustParse("40Mi")).Get()).
+				AddInitContainer(
+					test.Container().WithName("init-container-2").
+						WithCPURequest(resource.MustParse("5")).
+						WithMemRequest(resource.MustParse("5Mi")).
+						WithCPULimit(resource.MustParse("5")).
+						WithMemLimit(resource.MustParse("5Mi")).Get()).
+				AddInitContainerStatus(
+					test.ContainerStatus().WithName("init-container-2").
+						WithCPURequest(resource.MustParse("5")).
+						WithMemRequest(resource.MustParse("5Mi")).
+						WithCPULimit(resource.MustParse("5")).
+						WithMemLimit(resource.MustParse("5Mi")).Get()).
+				Get(),
+			wantRequests: apiv1.ResourceList{
+				apiv1.ResourceCPU:    resource.MustParse("3"),
+				apiv1.ResourceMemory: resource.MustParse("30Mi"),
+			},
+			wantLimits: apiv1.ResourceList{
+				apiv1.ResourceCPU:    resource.MustParse("4"),
+				apiv1.ResourceMemory: resource.MustParse("40Mi"),
+			},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			gotRequests, gotLimits := InitContainerRequestsAndLimits(tc.initContainerName, tc.pod)
+			assert.Equal(t, tc.wantRequests, gotRequests, "requests don't match")
+			assert.Equal(t, tc.wantLimits, gotLimits, "limits don't match")
+		})
+	}
+}

vertical-pod-autoscaler/pkg/utils/test/test_pod.go

@@ -25,7 +25,9 @@ import (
 type PodBuilder interface {
 	WithName(name string) PodBuilder
 	AddContainer(container apiv1.Container) PodBuilder
+	AddInitContainer(initContainer apiv1.Container) PodBuilder
 	AddContainerStatus(containerStatus apiv1.ContainerStatus) PodBuilder
+	AddInitContainerStatus(initContainerStatus apiv1.ContainerStatus) PodBuilder
 	WithCreator(creatorObjectMeta *metav1.ObjectMeta, creatorTypeMeta *metav1.TypeMeta) PodBuilder
 	WithLabels(labels map[string]string) PodBuilder
 	WithAnnotations(annotations map[string]string) PodBuilder
@@ -42,14 +44,16 @@ func Pod() PodBuilder {
 }
 
 type podBuilderImpl struct {
-	name              string
+	name                  string
-	containers        []apiv1.Container
+	containers            []apiv1.Container
-	creatorObjectMeta *metav1.ObjectMeta
+	initContainers        []apiv1.Container
-	creatorTypeMeta   *metav1.TypeMeta
+	creatorObjectMeta     *metav1.ObjectMeta
-	labels            map[string]string
+	creatorTypeMeta       *metav1.TypeMeta
-	annotations       map[string]string
+	labels                map[string]string
-	phase             apiv1.PodPhase
+	annotations           map[string]string
-	containerStatuses []apiv1.ContainerStatus
+	phase                 apiv1.PodPhase
+	containerStatuses     []apiv1.ContainerStatus
+	initContainerStatuses []apiv1.ContainerStatus
 }
 
 func (pb *podBuilderImpl) WithLabels(labels map[string]string) PodBuilder {
@@ -76,6 +80,12 @@ func (pb *podBuilderImpl) AddContainer(container apiv1.Container) PodBuilder {
 	return &r
 }
 
+func (pb *podBuilderImpl) AddInitContainer(initContainer apiv1.Container) PodBuilder {
+	r := *pb
+	r.initContainers = append(r.initContainers, initContainer)
+	return &r
+}
+
 func (pb *podBuilderImpl) WithCreator(creatorObjectMeta *metav1.ObjectMeta, creatorTypeMeta *metav1.TypeMeta) PodBuilder {
 	r := *pb
 	r.creatorObjectMeta = creatorObjectMeta
@@ -95,6 +105,12 @@ func (pb *podBuilderImpl) AddContainerStatus(containerStatus apiv1.ContainerStat
 	return &r
 }
 
+func (pb *podBuilderImpl) AddInitContainerStatus(initContainerStatus apiv1.ContainerStatus) PodBuilder {
+	r := *pb
+	r.initContainerStatuses = append(r.initContainerStatuses, initContainerStatus)
+	return &r
+}
+
 func (pb *podBuilderImpl) Get() *apiv1.Pod {
 	startTime := metav1.Time{
 		Time: testTimestamp,
@@ -105,7 +121,8 @@ func (pb *podBuilderImpl) Get() *apiv1.Pod {
 			Name:      pb.name,
 		},
 		Spec: apiv1.PodSpec{
-			Containers: pb.containers,
+			Containers:     pb.containers,
+			InitContainers: pb.initContainers,
 		},
 		Status: apiv1.PodStatus{
 			StartTime: &startTime,
@@ -139,6 +156,9 @@ func (pb *podBuilderImpl) Get() *apiv1.Pod {
 	if pb.containerStatuses != nil {
 		pod.Status.ContainerStatuses = pb.containerStatuses
 	}
+	if pb.initContainerStatuses != nil {
+		pod.Status.InitContainerStatuses = pb.initContainerStatuses
+	}
 
 	return pod
 }