diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 5255f97019f..1d1966d9b9f 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5649,22 +5649,29 @@ func ValidatePodResize(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel originalCPUMemPodSpec.Containers = newContainers // Ensure that only CPU and memory resources are mutable for restartable init containers. + // Also ensure that resources are immutable for non-restartable init containers. var newInitContainers []core.Container if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) { for ix, container := range originalCPUMemPodSpec.InitContainers { if isRestartableInitContainer(&container) { // restartable init container dropCPUMemoryResourcesFromContainer(&container, &oldPod.Spec.InitContainers[ix]) + if !apiequality.Semantic.DeepEqual(container, oldPod.Spec.InitContainers[ix]) { + // This likely means that the user has made changes to resources other than CPU and memory for sidecar container. + specDiff := cmp.Diff(oldPod.Spec.InitContainers[ix], container) + errs := field.Forbidden(specPath, fmt.Sprintf("only cpu and memory resources for sidecar containers are mutable\n%v", specDiff)) + allErrs = append(allErrs, errs) + return allErrs + } + } else if !apiequality.Semantic.DeepEqual(container, oldPod.Spec.InitContainers[ix]) { // non-restartable init container + // This likely means that the user has modified resources of non-sidecar init container. + specDiff := cmp.Diff(oldPod.Spec.InitContainers[ix], container) + errs := field.Forbidden(specPath, fmt.Sprintf("resources for non-sidecar init containers are immutable\n%v", specDiff)) + allErrs = append(allErrs, errs) + return allErrs } newInitContainers = append(newInitContainers, container) } originalCPUMemPodSpec.InitContainers = newInitContainers - if !apiequality.Semantic.DeepEqual(originalCPUMemPodSpec.InitContainers, oldPod.Spec.InitContainers) { - // This likely means that the user has modified non-sidecar container resources. - specDiff := cmp.Diff(oldPod.Spec.InitContainers, originalCPUMemPodSpec.InitContainers) - errs := field.Forbidden(specPath, fmt.Sprintf("cpu and memory resources for only sidecar containers are mutable\n%v", specDiff)) - allErrs = append(allErrs, errs) - return allErrs - } } if !apiequality.Semantic.DeepEqual(originalCPUMemPodSpec, oldPod.Spec) { diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 81a74a69672..c5f00033d0b 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -25911,7 +25911,7 @@ func TestValidatePodResize(t *testing.T) { test: "Pod QoS unchanged, burstable -> burstable, remove cpu and memory requests", old: mkPodWithInitContainers(getResources("100m", "100Mi", "", ""), getResources("100m", "", "", ""), ""), new: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "", "", ""), ""), - err: "spec: Forbidden: cpu and memory resources for only sidecar containers are mutable", + err: "spec: Forbidden: resources for non-sidecar init containers are immutable", }, { test: "Pod with nil Resource field in Status", old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", ""), podtest.SetStatus(core.PodStatus{ @@ -25993,7 +25993,7 @@ func TestValidatePodResize(t *testing.T) { test: "storage limit change for sidecar containers", old: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "100Mi", "2Gi", ""), core.ContainerRestartPolicyAlways), new: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "100Mi", "1Gi", ""), core.ContainerRestartPolicyAlways), - err: "spec: Forbidden: cpu and memory resources for only sidecar containers are mutable", + err: "spec: Forbidden: only cpu and memory resources for sidecar containers are mutable", }, { test: "cpu request change for sidecar containers", old: mkPodWithInitContainers(getResources("200m", "0", "", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways), @@ -26008,7 +26008,7 @@ func TestValidatePodResize(t *testing.T) { test: "storage request change for sidecar containers", old: mkPodWithInitContainers(getResources("100m", "0", "1Gi", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways), new: mkPodWithInitContainers(getResources("100m", "0", "2Gi", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways), - err: "spec: Forbidden: cpu and memory resources for only sidecar containers are mutable", + err: "spec: Forbidden: only cpu and memory resources for sidecar containers are mutable", }, } diff --git a/pkg/kubelet/kubelet_pods_test.go b/pkg/kubelet/kubelet_pods_test.go index b423b2c2b8d..1f0679445bd 100644 --- a/pkg/kubelet/kubelet_pods_test.go +++ b/pkg/kubelet/kubelet_pods_test.go @@ -6880,44 +6880,59 @@ func TestAllocatedResourcesMatchStatus(t *testing.T) { }} for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - allocatedPod := v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - }, - Spec: v1.PodSpec{ - Containers: []v1.Container{{ - Name: "c", - Resources: test.allocatedResources, - }}, - InitContainers: []v1.Container{{ - Name: "c1-init", - Resources: test.allocatedResources, - RestartPolicy: &containerRestartPolicyAlways, - }}, - }, - } - state := kubecontainer.ContainerStateRunning - if test.statusTerminated { - state = kubecontainer.ContainerStateExited - } - podStatus := &kubecontainer.PodStatus{ - Name: "test", - ContainerStatuses: []*kubecontainer.Status{ - { - Name: "c", - State: state, - Resources: test.statusResources, + for _, isSidecarContainer := range []bool{false, true} { + t.Run(test.name, func(t *testing.T) { + var podStatus *kubecontainer.PodStatus + state := kubecontainer.ContainerStateRunning + if test.statusTerminated { + state = kubecontainer.ContainerStateExited + } + + allocatedPod := v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", }, - { - Name: "c1-init", - State: state, - Resources: test.statusResources, - }, - }, - } - match := allocatedResourcesMatchStatus(&allocatedPod, podStatus) - assert.Equal(t, test.expectMatch, match) - }) + } + + if isSidecarContainer { + allocatedPod.Spec = v1.PodSpec{ + InitContainers: []v1.Container{{ + Name: "c1-init", + Resources: test.allocatedResources, + RestartPolicy: &containerRestartPolicyAlways, + }}, + } + podStatus = &kubecontainer.PodStatus{ + Name: "test", + ContainerStatuses: []*kubecontainer.Status{ + { + Name: "c1-init", + State: state, + Resources: test.statusResources, + }, + }, + } + } else { + allocatedPod.Spec = v1.PodSpec{ + Containers: []v1.Container{{ + Name: "c", + Resources: test.allocatedResources, + }}, + } + podStatus = &kubecontainer.PodStatus{ + Name: "test", + ContainerStatuses: []*kubecontainer.Status{ + { + Name: "c", + State: state, + Resources: test.statusResources, + }, + }, + } + } + match := allocatedResourcesMatchStatus(&allocatedPod, podStatus) + assert.Equal(t, test.expectMatch, match) + }) + } } } diff --git a/staging/src/k8s.io/kubectl/pkg/util/resource/resource.go b/staging/src/k8s.io/kubectl/pkg/util/resource/resource.go index 736a2600710..369277ba6dd 100644 --- a/staging/src/k8s.io/kubectl/pkg/util/resource/resource.go +++ b/staging/src/k8s.io/kubectl/pkg/util/resource/resource.go @@ -45,16 +45,15 @@ func podRequests(pod *corev1.Pod) corev1.ResourceList { for i := range pod.Status.ContainerStatuses { containerStatuses[pod.Status.ContainerStatuses[i].Name] = &pod.Status.ContainerStatuses[i] } + for i := range pod.Status.InitContainerStatuses { + containerStatuses[pod.Status.InitContainerStatuses[i].Name] = &pod.Status.InitContainerStatuses[i] + } for _, container := range pod.Spec.Containers { containerReqs := container.Resources.Requests cs, found := containerStatuses[container.Name] if found && cs.Resources != nil { - if pod.Status.Resize == corev1.PodResizeStatusInfeasible { - containerReqs = cs.Resources.Requests.DeepCopy() - } else { - containerReqs = max(container.Resources.Requests, cs.Resources.Requests) - } + containerReqs = determineContainerReqs(pod, &container, cs) } addResourceList(reqs, containerReqs) } @@ -66,6 +65,10 @@ func podRequests(pod *corev1.Pod) corev1.ResourceList { containerReqs := container.Resources.Requests if container.RestartPolicy != nil && *container.RestartPolicy == corev1.ContainerRestartPolicyAlways { + cs, found := containerStatuses[container.Name] + if found && cs.Resources != nil { + containerReqs = determineContainerReqs(pod, &container, cs) + } // and add them to the resulting cumulative container requests addResourceList(reqs, containerReqs) @@ -137,6 +140,14 @@ func podLimits(pod *corev1.Pod) corev1.ResourceList { return limits } +// determineContainerReqs will return a copy of the container requests based on if resizing is feasible or not. +func determineContainerReqs(pod *corev1.Pod, container *corev1.Container, cs *corev1.ContainerStatus) corev1.ResourceList { + if pod.Status.Resize == corev1.PodResizeStatusInfeasible { + return cs.Resources.Requests.DeepCopy() + } + return max(container.Resources.Requests, cs.Resources.Requests) +} + // max returns the result of max(a, b) for each named resource and is only used if we can't // accumulate into an existing resource list func max(a corev1.ResourceList, b corev1.ResourceList) corev1.ResourceList {