From 4dae42a7961d3c640940b60aebdaccf607f091c2 Mon Sep 17 00:00:00 2001 From: Sreeram Venkitesh Date: Wed, 6 Nov 2024 19:13:28 +0530 Subject: [PATCH 1/6] Updated version skew strategy for InPlacePodVerticalScaling --- pkg/apis/core/validation/validation.go | 25 +++ pkg/apis/core/validation/validation_test.go | 167 ++++++++++++-------- 2 files changed, 124 insertions(+), 68 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 5aa310e223d..c0fe9393069 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5515,6 +5515,12 @@ func ValidatePodResize(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel allErrs = append(allErrs, field.Invalid(specPath, newPod.Status.QOSClass, "Pod QOS Class may not change as a result of resizing")) } + isPodResizeRequestValid := isPodResizeRequestValid(*oldPod) + + if !isPodResizeRequestValid { + allErrs = append(allErrs, field.Forbidden(specPath, "Pod running on node without InPlacePodVerticalScaling feature gate enabled may not be updated")) + } + // Ensure that only CPU and memory resources are mutable. originalCPUMemPodSpec := *newPod.Spec.DeepCopy() var newContainers []core.Container @@ -5555,6 +5561,25 @@ func ValidatePodResize(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel return allErrs } +func isPodResizeRequestValid(pod core.Pod) bool { + // TODO: Remove this after GA+3 releases of InPlacePodVerticalScaling + // This code handles the version skew as described in the KEP. + // For handling version skew we're only allowing to update the Pod's Resources + // if the Pod already has Pod.Status.ContainerStatuses[i].Resources. This means + // that the apiserver would only allow updates to Pods running on Nodes with + // the InPlacePodVerticalScaling feature gate enabled. + if !utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { + return false + } + for _, c := range pod.Status.ContainerStatuses { + if c.State.Running != nil { + return c.Resources != nil + } + } + // No running containers + return true +} + // 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 bd457b40adf..bdc31eebff5 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -25092,91 +25092,120 @@ func TestValidatePodResize(t *testing.T) { } tests := []struct { - test string - old *core.Pod - new *core.Pod - err string + test string + old *core.Pod + new *core.Pod + enableInPlacePodVerticalScaling bool + err string }{ { - test: "cpu limit change", - old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", "")), - new: mkPod(core.ResourceList{}, getResources("200m", "0", "1Gi", "")), - err: "", + test: "cpu limit change", + old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", "")), + new: mkPod(core.ResourceList{}, getResources("200m", "0", "1Gi", "")), + enableInPlacePodVerticalScaling: true, + err: "", }, { - test: "memory limit change", - old: mkPod(core.ResourceList{}, getResources("100m", "200Mi", "", "")), - new: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "", "")), - err: "", + test: "memory limit change", + old: mkPod(core.ResourceList{}, getResources("100m", "200Mi", "", "")), + new: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "", "")), + enableInPlacePodVerticalScaling: true, + err: "", }, { - test: "storage limit change", - old: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "2Gi", "")), - new: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "1Gi", "")), - err: "spec: Forbidden: only cpu and memory resources are mutable", + test: "memory limit change without feature gate enabled", + old: mkPod(core.ResourceList{}, getResources("100m", "200Mi", "", "")), + new: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "", "")), + enableInPlacePodVerticalScaling: false, + err: "spec: Forbidden: Pod running on node without InPlacePodVerticalScaling feature gate enabled may not be updated", }, { - test: "cpu request change", - old: mkPod(getResources("200m", "0", "", ""), core.ResourceList{}), - new: mkPod(getResources("100m", "0", "", ""), core.ResourceList{}), - err: "", + test: "CPU limit change without feature gate enabled", + old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", "")), + new: mkPod(core.ResourceList{}, getResources("200m", "0", "1Gi", "")), + enableInPlacePodVerticalScaling: false, + err: "spec: Forbidden: Pod running on node without InPlacePodVerticalScaling feature gate enabled may not be updated", }, { - test: "memory request change", - old: mkPod(getResources("0", "100Mi", "", ""), core.ResourceList{}), - new: mkPod(getResources("0", "200Mi", "", ""), core.ResourceList{}), - err: "", + test: "storage limit change", + old: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "2Gi", "")), + new: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "1Gi", "")), + enableInPlacePodVerticalScaling: true, + err: "spec: Forbidden: only cpu and memory resources are mutable", }, { - test: "storage request change", - old: mkPod(getResources("100m", "0", "1Gi", ""), core.ResourceList{}), - new: mkPod(getResources("100m", "0", "2Gi", ""), core.ResourceList{}), - err: "spec: Forbidden: only cpu and memory resources are mutable", + test: "cpu request change", + old: mkPod(getResources("200m", "0", "", ""), core.ResourceList{}), + new: mkPod(getResources("100m", "0", "", ""), core.ResourceList{}), + enableInPlacePodVerticalScaling: true, + err: "", }, { - test: "Pod QoS unchanged, guaranteed -> guaranteed", - old: mkPod(getResources("100m", "100Mi", "1Gi", ""), getResources("100m", "100Mi", "1Gi", "")), - new: mkPod(getResources("200m", "400Mi", "1Gi", ""), getResources("200m", "400Mi", "1Gi", "")), - err: "", + test: "memory request change", + old: mkPod(getResources("0", "100Mi", "", ""), core.ResourceList{}), + new: mkPod(getResources("0", "200Mi", "", ""), core.ResourceList{}), + enableInPlacePodVerticalScaling: true, + err: "", }, { - test: "Pod QoS unchanged, burstable -> burstable", - old: mkPod(getResources("200m", "200Mi", "1Gi", ""), getResources("400m", "400Mi", "2Gi", "")), - new: mkPod(getResources("100m", "100Mi", "1Gi", ""), getResources("200m", "200Mi", "2Gi", "")), - err: "", + test: "storage request change", + old: mkPod(getResources("100m", "0", "1Gi", ""), core.ResourceList{}), + new: mkPod(getResources("100m", "0", "2Gi", ""), core.ResourceList{}), + enableInPlacePodVerticalScaling: true, + err: "spec: Forbidden: only cpu and memory resources are mutable", }, { - test: "Pod QoS unchanged, burstable -> burstable, add limits", - old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}), - new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), - err: "", + test: "Pod QoS unchanged, guaranteed -> guaranteed", + old: mkPod(getResources("100m", "100Mi", "1Gi", ""), getResources("100m", "100Mi", "1Gi", "")), + new: mkPod(getResources("200m", "400Mi", "1Gi", ""), getResources("200m", "400Mi", "1Gi", "")), + enableInPlacePodVerticalScaling: true, + 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", + old: mkPod(getResources("200m", "200Mi", "1Gi", ""), getResources("400m", "400Mi", "2Gi", "")), + new: mkPod(getResources("100m", "100Mi", "1Gi", ""), getResources("200m", "200Mi", "2Gi", "")), + enableInPlacePodVerticalScaling: true, + 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, add limits", + old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}), + new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), + enableInPlacePodVerticalScaling: true, + 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 unchanged, burstable -> burstable, remove limits", + old: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), + new: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}), + enableInPlacePodVerticalScaling: true, + err: "", }, { - test: "Pod QoS change, guaranteed -> burstable", - old: mkPod(getResources("100m", "100Mi", "", ""), getResources("100m", "100Mi", "", "")), - new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), - err: "Pod QOS Class may not change as a result of resizing", + test: "Pod QoS unchanged, burstable -> burstable, add requests", + old: mkPod(core.ResourceList{}, getResources("200m", "500Mi", "1Gi", "")), + new: mkPod(getResources("300m", "", "", ""), getResources("400m", "", "1Gi", "")), + enableInPlacePodVerticalScaling: true, + err: "", }, { - test: "Pod QoS change, burstable -> guaranteed", - old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}), - new: mkPod(getResources("100m", "100Mi", "", ""), getResources("100m", "100Mi", "", "")), - err: "Pod QOS Class may not change as a result of resizing", + 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", "")), + enableInPlacePodVerticalScaling: true, + err: "", }, { - test: "Pod QoS change, besteffort -> burstable", - old: mkPod(core.ResourceList{}, core.ResourceList{}), - new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), - err: "Pod QOS Class may not change as a result of resizing", + test: "Pod QoS change, guaranteed -> burstable", + old: mkPod(getResources("100m", "100Mi", "", ""), getResources("100m", "100Mi", "", "")), + new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), + enableInPlacePodVerticalScaling: true, + err: "Pod QOS Class may not change as a result of resizing", }, { - test: "Pod QoS change, burstable -> besteffort", - old: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), - new: mkPod(core.ResourceList{}, core.ResourceList{}), - err: "Pod QOS Class may not change as a result of resizing", + test: "Pod QoS change, burstable -> guaranteed", + old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}), + new: mkPod(getResources("100m", "100Mi", "", ""), getResources("100m", "100Mi", "", "")), + enableInPlacePodVerticalScaling: true, + err: "Pod QOS Class may not change as a result of resizing", + }, { + test: "Pod QoS change, besteffort -> burstable", + old: mkPod(core.ResourceList{}, core.ResourceList{}), + new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), + enableInPlacePodVerticalScaling: true, + err: "Pod QOS Class may not change as a result of resizing", + }, { + test: "Pod QoS change, burstable -> besteffort", + old: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), + new: mkPod(core.ResourceList{}, core.ResourceList{}), + enableInPlacePodVerticalScaling: true, + err: "Pod QOS Class may not change as a result of resizing", }, { test: "windows pod, no resource change", old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", ""), podtest.SetOS(core.Windows)), @@ -25191,6 +25220,8 @@ func TestValidatePodResize(t *testing.T) { } for _, test := range tests { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.InPlacePodVerticalScaling, test.enableInPlacePodVerticalScaling) + test.new.ObjectMeta.ResourceVersion = "1" test.old.ObjectMeta.ResourceVersion = "1" From 7d1d7182f3ed185b9e976b3891dcf290d6c36ef3 Mon Sep 17 00:00:00 2001 From: Sreeram Venkitesh Date: Thu, 7 Nov 2024 01:28:48 +0530 Subject: [PATCH 2/6] Update function name and remove feature gate check --- pkg/apis/core/validation/validation.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index c0fe9393069..e04ec43dc86 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5515,7 +5515,7 @@ func ValidatePodResize(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel allErrs = append(allErrs, field.Invalid(specPath, newPod.Status.QOSClass, "Pod QOS Class may not change as a result of resizing")) } - isPodResizeRequestValid := isPodResizeRequestValid(*oldPod) + isPodResizeRequestValid := isPodResizeRequestSupported(*oldPod) if !isPodResizeRequestValid { allErrs = append(allErrs, field.Forbidden(specPath, "Pod running on node without InPlacePodVerticalScaling feature gate enabled may not be updated")) @@ -5561,22 +5561,20 @@ func ValidatePodResize(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel return allErrs } -func isPodResizeRequestValid(pod core.Pod) bool { +// isPodResizeRequestSupported checks whether the pod is running on a node with InPlacePodVerticalScaling enabled. +func isPodResizeRequestSupported(pod core.Pod) bool { // TODO: Remove this after GA+3 releases of InPlacePodVerticalScaling // This code handles the version skew as described in the KEP. // For handling version skew we're only allowing to update the Pod's Resources // if the Pod already has Pod.Status.ContainerStatuses[i].Resources. This means // that the apiserver would only allow updates to Pods running on Nodes with // the InPlacePodVerticalScaling feature gate enabled. - if !utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { - return false - } for _, c := range pod.Status.ContainerStatuses { if c.State.Running != nil { return c.Resources != nil } } - // No running containers + // No running containers. We cannot tell whether the node supports resize at this point, so we assume it does. return true } From 385d2b198c9cfa968aa0cf8ebe90623d5545470c Mon Sep 17 00:00:00 2001 From: Sreeram Venkitesh Date: Thu, 7 Nov 2024 11:02:11 +0530 Subject: [PATCH 3/6] Fixes from review, updated tests cases --- pkg/apis/core/validation/validation.go | 6 +- pkg/apis/core/validation/validation_test.go | 179 +++++++++----------- 2 files changed, 84 insertions(+), 101 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index e04ec43dc86..527a0c4bef6 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5515,10 +5515,8 @@ func ValidatePodResize(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel allErrs = append(allErrs, field.Invalid(specPath, newPod.Status.QOSClass, "Pod QOS Class may not change as a result of resizing")) } - isPodResizeRequestValid := isPodResizeRequestSupported(*oldPod) - - if !isPodResizeRequestValid { - allErrs = append(allErrs, field.Forbidden(specPath, "Pod running on node without InPlacePodVerticalScaling feature gate enabled may not be updated")) + if !isPodResizeRequestSupported(*oldPod) { + allErrs = append(allErrs, field.Forbidden(specPath, "Pod running on node without support for resize")) } // Ensure that only CPU and memory resources are mutable. diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index bdc31eebff5..677378f49a7 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -25092,121 +25092,108 @@ func TestValidatePodResize(t *testing.T) { } tests := []struct { - test string - old *core.Pod - new *core.Pod - enableInPlacePodVerticalScaling bool - err string + test string + old *core.Pod + new *core.Pod + err string }{ { - test: "cpu limit change", - old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", "")), - new: mkPod(core.ResourceList{}, getResources("200m", "0", "1Gi", "")), - enableInPlacePodVerticalScaling: true, - err: "", + test: "cpu limit change", + old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", "")), + new: mkPod(core.ResourceList{}, getResources("200m", "0", "1Gi", "")), + err: "", }, { - test: "memory limit change", - old: mkPod(core.ResourceList{}, getResources("100m", "200Mi", "", "")), - new: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "", "")), - enableInPlacePodVerticalScaling: true, - err: "", + test: "memory limit change", + old: mkPod(core.ResourceList{}, getResources("100m", "200Mi", "", "")), + new: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "", "")), + err: "", }, { - test: "memory limit change without feature gate enabled", - old: mkPod(core.ResourceList{}, getResources("100m", "200Mi", "", "")), - new: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "", "")), - enableInPlacePodVerticalScaling: false, - err: "spec: Forbidden: Pod running on node without InPlacePodVerticalScaling feature gate enabled may not be updated", + test: "storage limit change", + old: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "2Gi", "")), + new: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "1Gi", "")), + err: "spec: Forbidden: only cpu and memory resources are mutable", }, { - test: "CPU limit change without feature gate enabled", - old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", "")), - new: mkPod(core.ResourceList{}, getResources("200m", "0", "1Gi", "")), - enableInPlacePodVerticalScaling: false, - err: "spec: Forbidden: Pod running on node without InPlacePodVerticalScaling feature gate enabled may not be updated", + test: "cpu request change", + old: mkPod(getResources("200m", "0", "", ""), core.ResourceList{}), + new: mkPod(getResources("100m", "0", "", ""), core.ResourceList{}), + err: "", }, { - test: "storage limit change", - old: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "2Gi", "")), - new: mkPod(core.ResourceList{}, getResources("100m", "100Mi", "1Gi", "")), - enableInPlacePodVerticalScaling: true, - err: "spec: Forbidden: only cpu and memory resources are mutable", + test: "memory request change", + old: mkPod(getResources("0", "100Mi", "", ""), core.ResourceList{}), + new: mkPod(getResources("0", "200Mi", "", ""), core.ResourceList{}), + err: "", }, { - test: "cpu request change", - old: mkPod(getResources("200m", "0", "", ""), core.ResourceList{}), - new: mkPod(getResources("100m", "0", "", ""), core.ResourceList{}), - enableInPlacePodVerticalScaling: true, - err: "", + test: "storage request change", + old: mkPod(getResources("100m", "0", "1Gi", ""), core.ResourceList{}), + new: mkPod(getResources("100m", "0", "2Gi", ""), core.ResourceList{}), + err: "spec: Forbidden: only cpu and memory resources are mutable", }, { - test: "memory request change", - old: mkPod(getResources("0", "100Mi", "", ""), core.ResourceList{}), - new: mkPod(getResources("0", "200Mi", "", ""), core.ResourceList{}), - enableInPlacePodVerticalScaling: true, - err: "", + test: "Pod QoS unchanged, guaranteed -> guaranteed", + old: mkPod(getResources("100m", "100Mi", "1Gi", ""), getResources("100m", "100Mi", "1Gi", "")), + new: mkPod(getResources("200m", "400Mi", "1Gi", ""), getResources("200m", "400Mi", "1Gi", "")), + err: "", }, { - test: "storage request change", - old: mkPod(getResources("100m", "0", "1Gi", ""), core.ResourceList{}), - new: mkPod(getResources("100m", "0", "2Gi", ""), core.ResourceList{}), - enableInPlacePodVerticalScaling: true, - err: "spec: Forbidden: only cpu and memory resources are mutable", + test: "Pod QoS unchanged, burstable -> burstable", + old: mkPod(getResources("200m", "200Mi", "1Gi", ""), getResources("400m", "400Mi", "2Gi", "")), + new: mkPod(getResources("100m", "100Mi", "1Gi", ""), getResources("200m", "200Mi", "2Gi", "")), + err: "", }, { - test: "Pod QoS unchanged, guaranteed -> guaranteed", - old: mkPod(getResources("100m", "100Mi", "1Gi", ""), getResources("100m", "100Mi", "1Gi", "")), - new: mkPod(getResources("200m", "400Mi", "1Gi", ""), getResources("200m", "400Mi", "1Gi", "")), - enableInPlacePodVerticalScaling: true, - err: "", + test: "Pod QoS unchanged, burstable -> burstable, add limits", + old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}), + new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), + err: "", }, { - test: "Pod QoS unchanged, burstable -> burstable", - old: mkPod(getResources("200m", "200Mi", "1Gi", ""), getResources("400m", "400Mi", "2Gi", "")), - new: mkPod(getResources("100m", "100Mi", "1Gi", ""), getResources("200m", "200Mi", "2Gi", "")), - enableInPlacePodVerticalScaling: true, - 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 limits", - old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}), - new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), - enableInPlacePodVerticalScaling: true, - 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 limits", - old: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), - new: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}), - enableInPlacePodVerticalScaling: true, - 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 unchanged, burstable -> burstable, add requests", - old: mkPod(core.ResourceList{}, getResources("200m", "500Mi", "1Gi", "")), - new: mkPod(getResources("300m", "", "", ""), getResources("400m", "", "1Gi", "")), - enableInPlacePodVerticalScaling: true, - err: "", + test: "Pod QoS change, guaranteed -> burstable", + old: mkPod(getResources("100m", "100Mi", "", ""), getResources("100m", "100Mi", "", "")), + new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), + err: "Pod QOS Class may not change as a result of resizing", }, { - 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", "")), - enableInPlacePodVerticalScaling: true, - err: "", + test: "Pod QoS change, burstable -> guaranteed", + old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}), + new: mkPod(getResources("100m", "100Mi", "", ""), getResources("100m", "100Mi", "", "")), + err: "Pod QOS Class may not change as a result of resizing", }, { - test: "Pod QoS change, guaranteed -> burstable", - old: mkPod(getResources("100m", "100Mi", "", ""), getResources("100m", "100Mi", "", "")), - new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), - enableInPlacePodVerticalScaling: true, - err: "Pod QOS Class may not change as a result of resizing", + test: "Pod QoS change, besteffort -> burstable", + old: mkPod(core.ResourceList{}, core.ResourceList{}), + new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), + err: "Pod QOS Class may not change as a result of resizing", }, { - test: "Pod QoS change, burstable -> guaranteed", - old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}), - new: mkPod(getResources("100m", "100Mi", "", ""), getResources("100m", "100Mi", "", "")), - enableInPlacePodVerticalScaling: true, - err: "Pod QOS Class may not change as a result of resizing", + test: "Pod QoS change, burstable -> guaranteed", + old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}), + new: mkPod(getResources("100m", "100Mi", "", ""), getResources("100m", "100Mi", "", "")), + err: "Pod QOS Class may not change as a result of resizing", }, { - test: "Pod QoS change, besteffort -> burstable", - old: mkPod(core.ResourceList{}, core.ResourceList{}), - new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), - enableInPlacePodVerticalScaling: true, - err: "Pod QOS Class may not change as a result of resizing", + test: "Pod QoS change, besteffort -> burstable", + old: mkPod(core.ResourceList{}, core.ResourceList{}), + new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), + err: "Pod QOS Class may not change as a result of resizing", }, { - test: "Pod QoS change, burstable -> besteffort", - old: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), - new: mkPod(core.ResourceList{}, core.ResourceList{}), - enableInPlacePodVerticalScaling: true, - err: "Pod QOS Class may not change as a result of resizing", + test: "Pod QoS change, burstable -> besteffort", + old: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), + new: mkPod(core.ResourceList{}, core.ResourceList{}), + err: "Pod QOS Class may not change as a result of resizing", }, { + test: "Pod QoS change, burstable -> besteffort", + old: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), + new: mkPod(core.ResourceList{}, core.ResourceList{}), + err: "Pod QOS Class may not change as a result of resizing", + }, + { test: "windows pod, no resource change", old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", ""), podtest.SetOS(core.Windows)), new: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", ""), podtest.SetOS(core.Windows)), @@ -25220,8 +25207,6 @@ func TestValidatePodResize(t *testing.T) { } for _, test := range tests { - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.InPlacePodVerticalScaling, test.enableInPlacePodVerticalScaling) - test.new.ObjectMeta.ResourceVersion = "1" test.old.ObjectMeta.ResourceVersion = "1" From 1739ee2ba9f66b77b2ed2928cc8c021c0c867f43 Mon Sep 17 00:00:00 2001 From: Sreeram Venkitesh Date: Thu, 7 Nov 2024 11:38:54 +0530 Subject: [PATCH 4/6] Removed duplicated tests after rebase --- pkg/apis/core/validation/validation_test.go | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 677378f49a7..bd457b40adf 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -25172,28 +25172,12 @@ func TestValidatePodResize(t *testing.T) { old: mkPod(core.ResourceList{}, core.ResourceList{}), new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), err: "Pod QOS Class may not change as a result of resizing", - }, { - test: "Pod QoS change, burstable -> guaranteed", - old: mkPod(getResources("100m", "100Mi", "", ""), core.ResourceList{}), - new: mkPod(getResources("100m", "100Mi", "", ""), getResources("100m", "100Mi", "", "")), - err: "Pod QOS Class may not change as a result of resizing", - }, { - test: "Pod QoS change, besteffort -> burstable", - old: mkPod(core.ResourceList{}, core.ResourceList{}), - new: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), - err: "Pod QOS Class may not change as a result of resizing", }, { test: "Pod QoS change, burstable -> besteffort", old: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), new: mkPod(core.ResourceList{}, core.ResourceList{}), err: "Pod QOS Class may not change as a result of resizing", }, { - test: "Pod QoS change, burstable -> besteffort", - old: mkPod(getResources("100m", "100Mi", "", ""), getResources("200m", "200Mi", "", "")), - new: mkPod(core.ResourceList{}, core.ResourceList{}), - err: "Pod QOS Class may not change as a result of resizing", - }, - { test: "windows pod, no resource change", old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", ""), podtest.SetOS(core.Windows)), new: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", ""), podtest.SetOS(core.Windows)), From 8f1e69bbb0d7668940e6ef7b87a49683f79dd477 Mon Sep 17 00:00:00 2001 From: Sreeram Venkitesh Date: Thu, 7 Nov 2024 13:28:40 +0530 Subject: [PATCH 5/6] Fix verify-gofmt.sh --- pkg/apis/core/validation/validation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 527a0c4bef6..35514722b65 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5559,7 +5559,7 @@ func ValidatePodResize(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel return allErrs } -// isPodResizeRequestSupported checks whether the pod is running on a node with InPlacePodVerticalScaling enabled. +// isPodResizeRequestSupported checks whether the pod is running on a node with InPlacePodVerticalScaling enabled. func isPodResizeRequestSupported(pod core.Pod) bool { // TODO: Remove this after GA+3 releases of InPlacePodVerticalScaling // This code handles the version skew as described in the KEP. From 851dbf25e5984f9409ac9a73d3996b960065d759 Mon Sep 17 00:00:00 2001 From: Sreeram Venkitesh Date: Fri, 8 Nov 2024 01:17:05 +0530 Subject: [PATCH 6/6] Added unit tests --- pkg/apis/core/validation/validation_test.go | 68 +++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index bd457b40adf..412b4116958 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -25188,6 +25188,74 @@ func TestValidatePodResize(t *testing.T) { new: mkPod(core.ResourceList{}, getResources("200m", "0", "1Gi", ""), podtest.SetOS(core.Windows)), err: "Forbidden: windows pods cannot be resized", }, + { + test: "Pod with nil Resource field in Status", + old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", ""), podtest.SetStatus(core.PodStatus{ + ContainerStatuses: []core.ContainerStatus{{ + ContainerID: "docker://numbers", + Image: "nginx:alpine", + Name: "main", + Ready: true, + Started: proto.Bool(true), + Resources: nil, + State: core.ContainerState{ + Running: &core.ContainerStateRunning{ + StartedAt: metav1.NewTime(time.Now()), + }, + }, + }}, + })), + new: mkPod(core.ResourceList{}, getResources("200m", "0", "1Gi", "")), + err: "Forbidden: Pod running on node without support for resize", + }, + { + test: "Pod with non-nil Resources field in Status", + old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", ""), podtest.SetStatus(core.PodStatus{ + ContainerStatuses: []core.ContainerStatus{{ + ContainerID: "docker://numbers", + Image: "nginx:alpine", + Name: "main", + Ready: true, + Started: proto.Bool(true), + Resources: &core.ResourceRequirements{}, + State: core.ContainerState{ + Running: &core.ContainerStateRunning{ + StartedAt: metav1.NewTime(time.Now()), + }, + }, + }}, + })), + new: mkPod(core.ResourceList{}, getResources("200m", "0", "1Gi", "")), + err: "", + }, + { + test: "Pod without running containers", + old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", ""), podtest.SetStatus(core.PodStatus{ + ContainerStatuses: []core.ContainerStatus{}, + })), + new: mkPod(core.ResourceList{}, getResources("200m", "0", "1Gi", "")), + err: "", + }, + { + test: "Pod with containers which are not running yet", + old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", ""), podtest.SetStatus(core.PodStatus{ + ContainerStatuses: []core.ContainerStatus{{ + ContainerID: "docker://numbers", + Image: "nginx:alpine", + Name: "main", + Ready: true, + Started: proto.Bool(true), + Resources: &core.ResourceRequirements{}, + State: core.ContainerState{ + Waiting: &core.ContainerStateWaiting{ + Reason: "PodInitializing", + }, + }, + }}, + })), + new: mkPod(core.ResourceList{}, getResources("200m", "0", "1Gi", "")), + err: "", + }, } for _, test := range tests {