Skip to content

Commit

Permalink
Merge pull request kubernetes#128683 from AnishShah/validation
Browse files Browse the repository at this point in the history 
[FG:InPlacePodVerticalScaling] Disallow removing requests & limits for Burstable pods.
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Nov 8, 2024
2 parents c25f5ee + 7680f0f commit 2691a29
Show file tree
Hide file tree
Showing 3 changed files with 200 additions and 13 deletions.
36 changes: 36 additions & 0 deletions pkg/apis/core/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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{}
Expand Down
96 changes: 85 additions & 11 deletions pkg/apis/core/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
81 changes: 79 additions & 2 deletions test/e2e/common/node/pod_resize.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down

0 comments on commit 2691a29

Please sign in to comment.