diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index f947c021352..d53d7fc010b 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5596,6 +5596,29 @@ func ValidatePodResize(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel allErrs = append(allErrs, field.Forbidden(specPath, "Pod running on node without support for resize")) } + // Do not allow removing resource requests/limits on resize. + if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) { + for ix, ctr := range oldPod.Spec.InitContainers { + if ctr.RestartPolicy != nil && *ctr.RestartPolicy != core.ContainerRestartPolicyAlways { + continue + } + if resourcesRemoved(newPod.Spec.InitContainers[ix].Resources.Requests, ctr.Resources.Requests) { + allErrs = append(allErrs, field.Forbidden(specPath.Child("initContainers").Index(ix).Child("resources").Child("requests"), "resource requests cannot be removed")) + } + if resourcesRemoved(newPod.Spec.InitContainers[ix].Resources.Limits, ctr.Resources.Limits) { + allErrs = append(allErrs, field.Forbidden(specPath.Child("initContainers").Index(ix).Child("resources").Child("limits"), "resource limits cannot be removed")) + } + } + } + for ix, ctr := range oldPod.Spec.Containers { + if resourcesRemoved(newPod.Spec.Containers[ix].Resources.Requests, ctr.Resources.Requests) { + allErrs = append(allErrs, field.Forbidden(specPath.Child("containers").Index(ix).Child("resources").Child("requests"), "resource requests cannot be removed")) + } + if resourcesRemoved(newPod.Spec.Containers[ix].Resources.Limits, ctr.Resources.Limits) { + allErrs = append(allErrs, field.Forbidden(specPath.Child("containers").Index(ix).Child("resources").Child("limits"), "resource limits cannot be removed")) + } + } + // Ensure that only CPU and memory resources are mutable. originalCPUMemPodSpec := *newPod.Spec.DeepCopy() var newContainers []core.Container @@ -5653,6 +5676,19 @@ func isPodResizeRequestSupported(pod core.Pod) bool { return true } +func resourcesRemoved(resourceList, oldResourceList core.ResourceList) bool { + if len(oldResourceList) > len(resourceList) { + return true + } + for name := range oldResourceList { + if _, ok := resourceList[name]; !ok { + return true + } + } + + return false +} + // ValidatePodBinding tests if required fields in the pod binding are legal. func ValidatePodBinding(binding *core.Binding) field.ErrorList { allErrs := field.ErrorList{} diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index b43842d3bfa..7c70af236e8 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -25463,6 +25463,8 @@ func TestValidateSELinuxChangePolicy(t *testing.T) { } func TestValidatePodResize(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SidecarContainers, true) + mkPod := func(req, lim core.ResourceList, tweaks ...podtest.Tweak) *core.Pod { return podtest.MakePod("pod", append(tweaks, podtest.SetContainers( @@ -25479,6 +25481,23 @@ func TestValidatePodResize(t *testing.T) { )...) } + mkPodWithInitContainers := func(req, lim core.ResourceList, restartPolicy core.ContainerRestartPolicy, tweaks ...podtest.Tweak) *core.Pod { + return podtest.MakePod("pod", append(tweaks, + podtest.SetInitContainers( + podtest.MakeContainer( + "container", + podtest.SetContainerResources( + core.ResourceRequirements{ + Requests: req, + Limits: lim, + }, + ), + podtest.SetContainerRestartPolicy(restartPolicy), + ), + ), + )...) + } + tests := []struct { test string old *core.Pod @@ -25672,20 +25691,10 @@ func TestValidatePodResize(t *testing.T) { old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}), new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), err: "", - }, { - test: "Pod QoS unchanged, burstable -> burstable, remove limits", - old: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), - new: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}), - err: "", }, { test: "Pod QoS unchanged, burstable -> burstable, add requests", old: mkPod(core.ResourceList{}, getResources("200m", "500Mi", "1Gi", "")), - new: mkPod(getResources("300m", "", "", ""), getResources("400m", "", "1Gi", "")), - err: "", - }, { - test: "Pod QoS unchanged, burstable -> burstable, remove requests", - old: mkPod(getResources("100m", "200Mi", "", ""), getResources("200m", "300Mi", "2Gi", "")), - new: mkPod(core.ResourceList{}, getResources("400m", "500Mi", "2Gi", "")), + new: mkPod(getResources("300m", "", "", ""), getResources("400m", "500Mi", "1Gi", "")), err: "", }, { test: "Pod QoS change, guaranteed -> burstable", @@ -25717,6 +25726,71 @@ func TestValidatePodResize(t *testing.T) { old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", ""), podtest.SetOS(core.Windows)), new: mkPod(core.ResourceList{}, getResources("200m", "0", "1Gi", ""), podtest.SetOS(core.Windows)), err: "Forbidden: windows pods cannot be resized", + }, { + test: "Pod QoS unchanged, burstable -> burstable, remove cpu limit", + old: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "", "")), + new: mkPod(core.ResourceList{}, getResources("", "100Mi", "", "")), + err: "spec.containers[0].resources.limits: Forbidden: resource limits cannot be removed", + }, { + test: "Pod QoS unchanged, burstable -> burstable, remove memory limit", + old: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "", "")), + new: mkPod(core.ResourceList{}, getResources("100m", "", "", "")), + err: "spec.containers[0].resources.limits: Forbidden: resource limits cannot be removed", + }, { + test: "Pod QoS unchanged, burstable -> burstable, remove cpu request", + old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}), + new: mkPod(getResources("", "100Mi", "", ""), core.ResourceList{}), + err: "spec.containers[0].resources.requests: Forbidden: resource requests cannot be removed", + }, { + test: "Pod QoS unchanged, burstable -> burstable, remove memory request", + old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}), + new: mkPod(getResources("100m", "", "", ""), core.ResourceList{}), + err: "spec.containers[0].resources.requests: Forbidden: resource requests cannot be removed", + }, { + test: "Pod QoS unchanged, burstable -> burstable, remove cpu and memory limits", + old: mkPod(getResources("100m", "", "", ""), getResources("100m", "100Mi", "", "")), + new: mkPod(getResources("100m", "", "", ""), core.ResourceList{}), + err: "spec.containers[0].resources.limits: Forbidden: resource limits cannot be removed", + }, { + test: "Pod QoS unchanged, burstable -> burstable, remove cpu and memory requests", + old: mkPod(getResources("100m", "100Mi", "", ""), getResources("100m", "", "", "")), + new: mkPod(core.ResourceList{}, getResources("100m", "", "", "")), + err: "spec.containers[0].resources.requests: Forbidden: resource requests cannot be removed", + }, { + test: "Pod QoS unchanged, burstable -> burstable, remove cpu limit", + old: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "100Mi", "", ""), core.ContainerRestartPolicyAlways), + new: mkPodWithInitContainers(core.ResourceList{}, getResources("", "100Mi", "", ""), core.ContainerRestartPolicyAlways), + err: "spec.initContainers[0].resources.limits: Forbidden: resource limits cannot be removed", + }, { + test: "Pod QoS unchanged, burstable -> burstable, remove memory limit", + old: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "100Mi", "", ""), core.ContainerRestartPolicyAlways), + new: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "", "", ""), core.ContainerRestartPolicyAlways), + err: "spec.initContainers[0].resources.limits: Forbidden: resource limits cannot be removed", + }, { + test: "Pod QoS unchanged, burstable -> burstable, remove cpu request", + old: mkPodWithInitContainers(getResources("100m", "100Mi", "", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways), + new: mkPodWithInitContainers(getResources("", "100Mi", "", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways), + err: "spec.initContainers[0].resources.requests: Forbidden: resource requests cannot be removed", + }, { + test: "Pod QoS unchanged, burstable -> burstable, remove memory request", + old: mkPodWithInitContainers(getResources("100m", "100Mi", "", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways), + new: mkPodWithInitContainers(getResources("100m", "", "", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways), + err: "spec.initContainers[0].resources.requests: Forbidden: resource requests cannot be removed", + }, { + test: "Pod QoS unchanged, burstable -> burstable, remove cpu and memory limits", + old: mkPodWithInitContainers(getResources("100m", "", "", ""), getResources("100m", "100Mi", "", ""), core.ContainerRestartPolicyAlways), + new: mkPodWithInitContainers(getResources("100m", "", "", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways), + err: "spec.initContainers[0].resources.limits: Forbidden: resource limits cannot be removed", + }, { + test: "Pod QoS unchanged, burstable -> burstable, remove cpu and memory requests", + old: mkPodWithInitContainers(getResources("100m", "100Mi", "", ""), getResources("100m", "", "", ""), core.ContainerRestartPolicyAlways), + new: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "", "", ""), core.ContainerRestartPolicyAlways), + err: "spec.initContainers[0].resources.requests: Forbidden: resource requests cannot be removed", + }, { + 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: only cpu and memory resources are mutable", }, { test: "Pod with nil Resource field in Status", diff --git a/test/e2e/common/node/pod_resize.go b/test/e2e/common/node/pod_resize.go index 21a756cf0b9..347def6f339 100644 --- a/test/e2e/common/node/pod_resize.go +++ b/test/e2e/common/node/pod_resize.go @@ -919,13 +919,89 @@ func doPodResizeErrorTests(f *framework.Framework) { patchString: `{"spec":{"containers":[ {"name":"c1", "resources":{"requests":{"memory":"400Mi"}}} ]}}`, - patchError: "Pod QoS is immutable", + patchError: "Pod QOS Class may not change as a result of resizing", expected: []e2epod.ResizableContainerInfo{ { Name: "c1", }, }, }, + { + name: "Burstable QoS pod, one container with cpu & memory requests + limits - remove memory limits", + containers: []e2epod.ResizableContainerInfo{ + { + Name: "c1", + Resources: &e2epod.ContainerResources{CPUReq: "200m", CPULim: "400m", MemReq: "250Mi", MemLim: "500Mi"}, + }, + }, + patchString: `{"spec":{"containers":[ + {"name":"c1", "resources":{"limits":{"memory": null}}} + ]}}`, + patchError: "resource limits cannot be removed", + expected: []e2epod.ResizableContainerInfo{ + { + Name: "c1", + Resources: &e2epod.ContainerResources{CPUReq: "200m", CPULim: "400m", MemReq: "250Mi", MemLim: "500Mi"}, + }, + }, + }, + { + name: "Burstable QoS pod, one container with cpu & memory requests + limits - remove CPU limits", + containers: []e2epod.ResizableContainerInfo{ + { + Name: "c1", + Resources: &e2epod.ContainerResources{CPUReq: "200m", CPULim: "400m", MemReq: "250Mi", MemLim: "500Mi"}, + }, + }, + patchString: `{"spec":{"containers":[ + {"name":"c1", "resources":{"limits":{"cpu": null}}} + ]}}`, + patchError: "resource limits cannot be removed", + expected: []e2epod.ResizableContainerInfo{ + { + Name: "c1", + Resources: &e2epod.ContainerResources{CPUReq: "200m", CPULim: "400m", MemReq: "250Mi", MemLim: "500Mi"}, + }, + }, + }, + { + name: "Burstable QoS pod, one container with memory requests + limits, cpu requests - remove CPU requests", + containers: []e2epod.ResizableContainerInfo{ + { + Name: "c1", + Resources: &e2epod.ContainerResources{CPUReq: "200m", MemReq: "250Mi", MemLim: "500Mi"}, + }, + }, + patchString: `{"spec":{"containers":[ + {"name":"c1", "resources":{"requests":{"cpu": null}}} + ]}}`, + patchError: "resource requests cannot be removed", + expected: []e2epod.ResizableContainerInfo{ + { + Name: "c1", + Resources: &e2epod.ContainerResources{CPUReq: "200m", MemReq: "250Mi", MemLim: "500Mi"}, + }, + }, + }, + { + name: "Burstable QoS pod, one container with CPU requests + limits, cpu requests - remove memory requests", + containers: []e2epod.ResizableContainerInfo{ + { + Name: "c1", + Resources: &e2epod.ContainerResources{CPUReq: "200m", CPULim: "400m", MemReq: "250Mi"}, + }, + }, + patchString: `{"spec":{"containers":[ + {"name":"c1", "resources":{"requests":{"memory": null}}} + ]}}`, + patchError: "resource requests cannot be removed", + expected: []e2epod.ResizableContainerInfo{ + { + Name: "c1", + Resources: &e2epod.ContainerResources{CPUReq: "200m", CPULim: "400m", MemReq: "250Mi"}, + }, + }, + }, } timeouts := f.Timeouts @@ -959,7 +1035,8 @@ func doPodResizeErrorTests(f *framework.Framework) { if tc.patchError == "" { framework.ExpectNoError(pErr, "failed to patch pod for resize") } else { - gomega.Expect(pErr).To(gomega.HaveOccurred(), tc.patchError) + gomega.Expect(pErr).To(gomega.HaveOccurred()) + gomega.Expect(pErr.Error()).To(gomega.ContainSubstring(tc.patchError)) patchedPod = newPod }