Skip to content

Commit 7680f0f

Browse files
AnishShahAnishShah
committed
api: reject removing requsets & limits for Burstable pods.
1 parent 210f129 commit 7680f0f

File tree

3 files changed

+200
-13
lines changed

3 files changed

+200
-13
lines changed

‎pkg/apis/core/validation/validation.go

+36
Original file line numberDiff line numberDiff line change
@@ -5507,6 +5507,29 @@ func ValidatePodResize(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel
55075507
allErrs = append(allErrs, field.Forbidden(specPath, "Pod running on node without support for resize"))
55085508
}
55095509

5510+
// Do not allow removing resource requests/limits on resize.
5511+
if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) {
5512+
for ix, ctr := range oldPod.Spec.InitContainers {
5513+
if ctr.RestartPolicy != nil && *ctr.RestartPolicy != core.ContainerRestartPolicyAlways {
5514+
continue
5515+
}
5516+
if resourcesRemoved(newPod.Spec.InitContainers[ix].Resources.Requests, ctr.Resources.Requests) {
5517+
allErrs = append(allErrs, field.Forbidden(specPath.Child("initContainers").Index(ix).Child("resources").Child("requests"), "resource requests cannot be removed"))
5518+
}
5519+
if resourcesRemoved(newPod.Spec.InitContainers[ix].Resources.Limits, ctr.Resources.Limits) {
5520+
allErrs = append(allErrs, field.Forbidden(specPath.Child("initContainers").Index(ix).Child("resources").Child("limits"), "resource limits cannot be removed"))
5521+
}
5522+
}
5523+
}
5524+
for ix, ctr := range oldPod.Spec.Containers {
5525+
if resourcesRemoved(newPod.Spec.Containers[ix].Resources.Requests, ctr.Resources.Requests) {
5526+
allErrs = append(allErrs, field.Forbidden(specPath.Child("containers").Index(ix).Child("resources").Child("requests"), "resource requests cannot be removed"))
5527+
}
5528+
if resourcesRemoved(newPod.Spec.Containers[ix].Resources.Limits, ctr.Resources.Limits) {
5529+
allErrs = append(allErrs, field.Forbidden(specPath.Child("containers").Index(ix).Child("resources").Child("limits"), "resource limits cannot be removed"))
5530+
}
5531+
}
5532+
55105533
// Ensure that only CPU and memory resources are mutable.
55115534
originalCPUMemPodSpec := *newPod.Spec.DeepCopy()
55125535
var newContainers []core.Container
@@ -5564,6 +5587,19 @@ func isPodResizeRequestSupported(pod core.Pod) bool {
55645587
return true
55655588
}
55665589

5590+
func resourcesRemoved(resourceList, oldResourceList core.ResourceList) bool {
5591+
if len(oldResourceList) > len(resourceList) {
5592+
return true
5593+
}
5594+
for name := range oldResourceList {
5595+
if _, ok := resourceList[name]; !ok {
5596+
return true
5597+
}
5598+
}
5599+
5600+
return false
5601+
}
5602+
55675603
// ValidatePodBinding tests if required fields in the pod binding are legal.
55685604
func ValidatePodBinding(binding *core.Binding) field.ErrorList {
55695605
allErrs := field.ErrorList{}

‎pkg/apis/core/validation/validation_test.go

+85-11
Original file line numberDiff line numberDiff line change
@@ -25090,6 +25090,8 @@ func TestValidateSELinuxChangePolicy(t *testing.T) {
2509025090
}
2509125091

2509225092
func TestValidatePodResize(t *testing.T) {
25093+
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SidecarContainers, true)
25094+
2509325095
mkPod := func(req, lim core.ResourceList, tweaks ...podtest.Tweak) *core.Pod {
2509425096
return podtest.MakePod("pod", append(tweaks,
2509525097
podtest.SetContainers(
@@ -25106,6 +25108,23 @@ func TestValidatePodResize(t *testing.T) {
2510625108
)...)
2510725109
}
2510825110

25111+
mkPodWithInitContainers := func(req, lim core.ResourceList, restartPolicy core.ContainerRestartPolicy, tweaks ...podtest.Tweak) *core.Pod {
25112+
return podtest.MakePod("pod", append(tweaks,
25113+
podtest.SetInitContainers(
25114+
podtest.MakeContainer(
25115+
"container",
25116+
podtest.SetContainerResources(
25117+
core.ResourceRequirements{
25118+
Requests: req,
25119+
Limits: lim,
25120+
},
25121+
),
25122+
podtest.SetContainerRestartPolicy(restartPolicy),
25123+
),
25124+
),
25125+
)...)
25126+
}
25127+
2510925128
tests := []struct {
2511025129
test string
2511125130
old *core.Pod
@@ -25157,20 +25176,10 @@ func TestValidatePodResize(t *testing.T) {
2515725176
old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}),
2515825177
new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")),
2515925178
err: "",
25160-
}, {
25161-
test: "Pod QoS unchanged, burstable -> burstable, remove limits",
25162-
old: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")),
25163-
new: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}),
25164-
err: "",
2516525179
}, {
2516625180
test: "Pod QoS unchanged, burstable -> burstable, add requests",
2516725181
old: mkPod(core.ResourceList{}, getResources("200m", "500Mi", "1Gi", "")),
25168-
new: mkPod(getResources("300m", "", "", ""), getResources("400m", "", "1Gi", "")),
25169-
err: "",
25170-
}, {
25171-
test: "Pod QoS unchanged, burstable -> burstable, remove requests",
25172-
old: mkPod(getResources("100m", "200Mi", "", ""), getResources("200m", "300Mi", "2Gi", "")),
25173-
new: mkPod(core.ResourceList{}, getResources("400m", "500Mi", "2Gi", "")),
25182+
new: mkPod(getResources("300m", "", "", ""), getResources("400m", "500Mi", "1Gi", "")),
2517425183
err: "",
2517525184
}, {
2517625185
test: "Pod QoS change, guaranteed -> burstable",
@@ -25202,6 +25211,71 @@ func TestValidatePodResize(t *testing.T) {
2520225211
old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", ""), podtest.SetOS(core.Windows)),
2520325212
new: mkPod(core.ResourceList{}, getResources("200m", "0", "1Gi", ""), podtest.SetOS(core.Windows)),
2520425213
err: "Forbidden: windows pods cannot be resized",
25214+
}, {
25215+
test: "Pod QoS unchanged, burstable -> burstable, remove cpu limit",
25216+
old: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "", "")),
25217+
new: mkPod(core.ResourceList{}, getResources("", "100Mi", "", "")),
25218+
err: "spec.containers[0].resources.limits: Forbidden: resource limits cannot be removed",
25219+
}, {
25220+
test: "Pod QoS unchanged, burstable -> burstable, remove memory limit",
25221+
old: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "", "")),
25222+
new: mkPod(core.ResourceList{}, getResources("100m", "", "", "")),
25223+
err: "spec.containers[0].resources.limits: Forbidden: resource limits cannot be removed",
25224+
}, {
25225+
test: "Pod QoS unchanged, burstable -> burstable, remove cpu request",
25226+
old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}),
25227+
new: mkPod(getResources("", "100Mi", "", ""), core.ResourceList{}),
25228+
err: "spec.containers[0].resources.requests: Forbidden: resource requests cannot be removed",
25229+
}, {
25230+
test: "Pod QoS unchanged, burstable -> burstable, remove memory request",
25231+
old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}),
25232+
new: mkPod(getResources("100m", "", "", ""), core.ResourceList{}),
25233+
err: "spec.containers[0].resources.requests: Forbidden: resource requests cannot be removed",
25234+
}, {
25235+
test: "Pod QoS unchanged, burstable -> burstable, remove cpu and memory limits",
25236+
old: mkPod(getResources("100m", "", "", ""), getResources("100m", "100Mi", "", "")),
25237+
new: mkPod(getResources("100m", "", "", ""), core.ResourceList{}),
25238+
err: "spec.containers[0].resources.limits: Forbidden: resource limits cannot be removed",
25239+
}, {
25240+
test: "Pod QoS unchanged, burstable -> burstable, remove cpu and memory requests",
25241+
old: mkPod(getResources("100m", "100Mi", "", ""), getResources("100m", "", "", "")),
25242+
new: mkPod(core.ResourceList{}, getResources("100m", "", "", "")),
25243+
err: "spec.containers[0].resources.requests: Forbidden: resource requests cannot be removed",
25244+
}, {
25245+
test: "Pod QoS unchanged, burstable -> burstable, remove cpu limit",
25246+
old: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "100Mi", "", ""), core.ContainerRestartPolicyAlways),
25247+
new: mkPodWithInitContainers(core.ResourceList{}, getResources("", "100Mi", "", ""), core.ContainerRestartPolicyAlways),
25248+
err: "spec.initContainers[0].resources.limits: Forbidden: resource limits cannot be removed",
25249+
}, {
25250+
test: "Pod QoS unchanged, burstable -> burstable, remove memory limit",
25251+
old: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "100Mi", "", ""), core.ContainerRestartPolicyAlways),
25252+
new: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "", "", ""), core.ContainerRestartPolicyAlways),
25253+
err: "spec.initContainers[0].resources.limits: Forbidden: resource limits cannot be removed",
25254+
}, {
25255+
test: "Pod QoS unchanged, burstable -> burstable, remove cpu request",
25256+
old: mkPodWithInitContainers(getResources("100m", "100Mi", "", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways),
25257+
new: mkPodWithInitContainers(getResources("", "100Mi", "", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways),
25258+
err: "spec.initContainers[0].resources.requests: Forbidden: resource requests cannot be removed",
25259+
}, {
25260+
test: "Pod QoS unchanged, burstable -> burstable, remove memory request",
25261+
old: mkPodWithInitContainers(getResources("100m", "100Mi", "", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways),
25262+
new: mkPodWithInitContainers(getResources("100m", "", "", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways),
25263+
err: "spec.initContainers[0].resources.requests: Forbidden: resource requests cannot be removed",
25264+
}, {
25265+
test: "Pod QoS unchanged, burstable -> burstable, remove cpu and memory limits",
25266+
old: mkPodWithInitContainers(getResources("100m", "", "", ""), getResources("100m", "100Mi", "", ""), core.ContainerRestartPolicyAlways),
25267+
new: mkPodWithInitContainers(getResources("100m", "", "", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways),
25268+
err: "spec.initContainers[0].resources.limits: Forbidden: resource limits cannot be removed",
25269+
}, {
25270+
test: "Pod QoS unchanged, burstable -> burstable, remove cpu and memory requests",
25271+
old: mkPodWithInitContainers(getResources("100m", "100Mi", "", ""), getResources("100m", "", "", ""), core.ContainerRestartPolicyAlways),
25272+
new: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "", "", ""), core.ContainerRestartPolicyAlways),
25273+
err: "spec.initContainers[0].resources.requests: Forbidden: resource requests cannot be removed",
25274+
}, {
25275+
test: "Pod QoS unchanged, burstable -> burstable, remove cpu and memory requests",
25276+
old: mkPodWithInitContainers(getResources("100m", "100Mi", "", ""), getResources("100m", "", "", ""), ""),
25277+
new: mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "", "", ""), ""),
25278+
err: "spec: Forbidden: only cpu and memory resources are mutable",
2520525279
},
2520625280
{
2520725281
test: "Pod with nil Resource field in Status",

‎test/e2e/common/node/pod_resize.go

+79-2
Original file line numberDiff line numberDiff line change
@@ -885,13 +885,89 @@ func doPodResizeErrorTests(f *framework.Framework) {
885885
patchString: `{"spec":{"containers":[
886886
{"name":"c1", "resources":{"requests":{"memory":"400Mi"}}}
887887
]}}`,
888-
patchError: "Pod QoS is immutable",
888+
patchError: "Pod QOS Class may not change as a result of resizing",
889889
expected: []e2epod.ResizableContainerInfo{
890890
{
891891
Name: "c1",
892892
},
893893
},
894894
},
895+
{
896+
name: "Burstable QoS pod, one container with cpu & memory requests + limits - remove memory limits",
897+
containers: []e2epod.ResizableContainerInfo{
898+
{
899+
Name: "c1",
900+
Resources: &e2epod.ContainerResources{CPUReq: "200m", CPULim: "400m", MemReq: "250Mi", MemLim: "500Mi"},
901+
},
902+
},
903+
patchString: `{"spec":{"containers":[
904+
{"name":"c1", "resources":{"limits":{"memory": null}}}
905+
]}}`,
906+
patchError: "resource limits cannot be removed",
907+
expected: []e2epod.ResizableContainerInfo{
908+
{
909+
Name: "c1",
910+
Resources: &e2epod.ContainerResources{CPUReq: "200m", CPULim: "400m", MemReq: "250Mi", MemLim: "500Mi"},
911+
},
912+
},
913+
},
914+
{
915+
name: "Burstable QoS pod, one container with cpu & memory requests + limits - remove CPU limits",
916+
containers: []e2epod.ResizableContainerInfo{
917+
{
918+
Name: "c1",
919+
Resources: &e2epod.ContainerResources{CPUReq: "200m", CPULim: "400m", MemReq: "250Mi", MemLim: "500Mi"},
920+
},
921+
},
922+
patchString: `{"spec":{"containers":[
923+
{"name":"c1", "resources":{"limits":{"cpu": null}}}
924+
]}}`,
925+
patchError: "resource limits cannot be removed",
926+
expected: []e2epod.ResizableContainerInfo{
927+
{
928+
Name: "c1",
929+
Resources: &e2epod.ContainerResources{CPUReq: "200m", CPULim: "400m", MemReq: "250Mi", MemLim: "500Mi"},
930+
},
931+
},
932+
},
933+
{
934+
name: "Burstable QoS pod, one container with memory requests + limits, cpu requests - remove CPU requests",
935+
containers: []e2epod.ResizableContainerInfo{
936+
{
937+
Name: "c1",
938+
Resources: &e2epod.ContainerResources{CPUReq: "200m", MemReq: "250Mi", MemLim: "500Mi"},
939+
},
940+
},
941+
patchString: `{"spec":{"containers":[
942+
{"name":"c1", "resources":{"requests":{"cpu": null}}}
943+
]}}`,
944+
patchError: "resource requests cannot be removed",
945+
expected: []e2epod.ResizableContainerInfo{
946+
{
947+
Name: "c1",
948+
Resources: &e2epod.ContainerResources{CPUReq: "200m", MemReq: "250Mi", MemLim: "500Mi"},
949+
},
950+
},
951+
},
952+
{
953+
name: "Burstable QoS pod, one container with CPU requests + limits, cpu requests - remove memory requests",
954+
containers: []e2epod.ResizableContainerInfo{
955+
{
956+
Name: "c1",
957+
Resources: &e2epod.ContainerResources{CPUReq: "200m", CPULim: "400m", MemReq: "250Mi"},
958+
},
959+
},
960+
patchString: `{"spec":{"containers":[
961+
{"name":"c1", "resources":{"requests":{"memory": null}}}
962+
]}}`,
963+
patchError: "resource requests cannot be removed",
964+
expected: []e2epod.ResizableContainerInfo{
965+
{
966+
Name: "c1",
967+
Resources: &e2epod.ContainerResources{CPUReq: "200m", CPULim: "400m", MemReq: "250Mi"},
968+
},
969+
},
970+
},
895971
}
896972

897973
timeouts := f.Timeouts
@@ -925,7 +1001,8 @@ func doPodResizeErrorTests(f *framework.Framework) {
9251001
if tc.patchError == "" {
9261002
framework.ExpectNoError(pErr, "failed to patch pod for resize")
9271003
} else {
928-
gomega.Expect(pErr).To(gomega.HaveOccurred(), tc.patchError)
1004+
gomega.Expect(pErr).To(gomega.HaveOccurred())
1005+
gomega.Expect(pErr.Error()).To(gomega.ContainSubstring(tc.patchError))
9291006
patchedPod = newPod
9301007
}
9311008

0 commit comments

Comments
 (0)