Merge pull request #128683 from AnishShah/validation

[FG:InPlacePodVerticalScaling] Disallow removing requests & limits for Burstable pods.

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{}

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", "")),
+			new:  mkPod(getResources("300m", "", "", ""), getResources("400m", "500Mi", "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", "")),
 			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",

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
 			}