diff --git a/vertical-pod-autoscaler/pkg/recommender/input/spec/spec_client.go b/vertical-pod-autoscaler/pkg/recommender/input/spec/spec_client.go index 81de730735..70e0ed4175 100644 --- a/vertical-pod-autoscaler/pkg/recommender/input/spec/spec_client.go +++ b/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{ - PodName: pod.Name, - Namespace: pod.Namespace, - } - containerSpecs := newContainerSpecs(podId, pod.Spec.Containers) - initContainerSpecs := newContainerSpecs(podId, pod.Spec.InitContainers) + containerSpecs := newContainerSpecs(pod, pod.Spec.Containers, false /* isInitContainer */) + initContainerSpecs := newContainerSpecs(pod, pod.Spec.InitContainers, true /* isInitContainer */) 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 { - cpuQuantity := container.Resources.Requests[v1.ResourceCPU] +func calculateRequestedResources(pod *v1.Pod, container v1.Container, isInitContainer bool) model.Resources { + 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, + } +} diff --git a/vertical-pod-autoscaler/pkg/recommender/input/spec/spec_client_test.go b/vertical-pod-autoscaler/pkg/recommender/input/spec/spec_client_test.go index cdbda3ba92..3c9f6db292 100644 --- a/vertical-pod-autoscaler/pkg/recommender/input/spec/spec_client_test.go +++ b/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") } } diff --git a/vertical-pod-autoscaler/pkg/recommender/input/spec/spec_client_test_util.go b/vertical-pod-autoscaler/pkg/recommender/input/spec/spec_client_test_util.go index e6cfc606ab..38106e135c 100644 --- a/vertical-pod-autoscaler/pkg/recommender/input/spec/spec_client_test_util.go +++ b/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}, diff --git a/vertical-pod-autoscaler/pkg/utils/resources/resourcehelpers.go b/vertical-pod-autoscaler/pkg/utils/resources/resourcehelpers.go index 420f2da25e..3e49ffdb49 100644 --- a/vertical-pod-autoscaler/pkg/utils/resources/resourcehelpers.go +++ b/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 +} diff --git a/vertical-pod-autoscaler/pkg/utils/resources/resourcehelpers_test.go b/vertical-pod-autoscaler/pkg/utils/resources/resourcehelpers_test.go index 70cc8bc6f1..857785f90b 100644 --- a/vertical-pod-autoscaler/pkg/utils/resources/resourcehelpers_test.go +++ b/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", - containerName: "inexistent-container", + desc: "Init container with the same name as the container is ignored", + 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, - wantLimits: nil, + wantRequests: apiv1.ResourceList{}, + 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") + }) + } +} diff --git a/vertical-pod-autoscaler/pkg/utils/test/test_pod.go b/vertical-pod-autoscaler/pkg/utils/test/test_pod.go index 3e152c79bf..1f3b5dcc18 100644 --- a/vertical-pod-autoscaler/pkg/utils/test/test_pod.go +++ b/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 - containers []apiv1.Container - creatorObjectMeta *metav1.ObjectMeta - creatorTypeMeta *metav1.TypeMeta - labels map[string]string - annotations map[string]string - phase apiv1.PodPhase - containerStatuses []apiv1.ContainerStatus + name string + containers []apiv1.Container + initContainers []apiv1.Container + creatorObjectMeta *metav1.ObjectMeta + creatorTypeMeta *metav1.TypeMeta + labels map[string]string + annotations map[string]string + 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 }