From 9a805db010fa1fcaed053f6074c3d4067c33f2e1 Mon Sep 17 00:00:00 2001 From: vinay kulkarni Date: Fri, 10 Mar 2023 01:57:14 +0000 Subject: [PATCH] Set default resize policy only for specified resource types, rename RestartNotRequired -> NotRequired --- pkg/api/pod/util_test.go | 2 +- pkg/apis/core/types.go | 6 ++-- pkg/apis/core/v1/defaults.go | 30 ++++++++++--------- pkg/apis/core/v1/defaults_test.go | 3 -- pkg/apis/core/validation/validation.go | 4 +-- pkg/apis/core/validation/validation_test.go | 20 ++++++------- pkg/kubelet/container/helpers_test.go | 12 ++++---- .../kuberuntime/kuberuntime_manager_test.go | 4 +-- staging/src/k8s.io/api/core/v1/types.go | 6 ++-- test/e2e/node/pod_resize.go | 8 ++--- 10 files changed, 47 insertions(+), 48 deletions(-) diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 69da98eb5da..e9d0f4a1e39 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -2290,7 +2290,7 @@ func TestDropInPlacePodVerticalScaling(t *testing.T) { Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("200m")}, }, ResizePolicy: []api.ContainerResizePolicy{ - {ResourceName: api.ResourceCPU, RestartPolicy: api.RestartNotRequired}, + {ResourceName: api.ResourceCPU, RestartPolicy: api.NotRequired}, {ResourceName: api.ResourceMemory, RestartPolicy: api.RestartContainer}, }, }, diff --git a/pkg/apis/core/types.go b/pkg/apis/core/types.go index 7d31e34502c..77ebf53ac13 100644 --- a/pkg/apis/core/types.go +++ b/pkg/apis/core/types.go @@ -2143,11 +2143,11 @@ type ResourceResizeRestartPolicy string // These are the valid resource resize restart policy values: const ( - // 'RestartNotRequired' means Kubernetes will try to resize the container + // 'NotRequired' means Kubernetes will try to resize the container // without restarting it, if possible. Kubernetes may however choose to // restart the container if it is unable to actuate resize without a // restart. For e.g. the runtime doesn't support restart-free resizing. - RestartNotRequired ResourceResizeRestartPolicy = "RestartNotRequired" + NotRequired ResourceResizeRestartPolicy = "NotRequired" // 'RestartContainer' means Kubernetes will resize the container in-place // by stopping and starting the container when new resources are applied. // This is needed for legacy applications. For e.g. java apps using the @@ -2161,7 +2161,7 @@ type ContainerResizePolicy struct { // Supported values: cpu, memory. ResourceName ResourceName // Restart policy to apply when specified resource is resized. - // If not specified, it defaults to RestartNotRequired. + // If not specified, it defaults to NotRequired. RestartPolicy ResourceResizeRestartPolicy } diff --git a/pkg/apis/core/v1/defaults.go b/pkg/apis/core/v1/defaults.go index 4a2b8b399ac..433ae39b51f 100644 --- a/pkg/apis/core/v1/defaults.go +++ b/pkg/apis/core/v1/defaults.go @@ -159,25 +159,27 @@ func SetDefaults_Pod(obj *v1.Pod) { } } } - if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { - // For normal containers, set resize restart policy to default value (RestartNotRequired), if not specified. + if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) && + obj.Spec.Containers[i].Resources.Requests != nil { + // For normal containers, set resize restart policy to default value (NotRequired), if not specified. resizePolicySpecified := make(map[v1.ResourceName]bool) for _, p := range obj.Spec.Containers[i].ResizePolicy { resizePolicySpecified[p.ResourceName] = true } - if _, found := resizePolicySpecified[v1.ResourceCPU]; !found { - obj.Spec.Containers[i].ResizePolicy = append(obj.Spec.Containers[i].ResizePolicy, - v1.ContainerResizePolicy{ - ResourceName: v1.ResourceCPU, - RestartPolicy: v1.RestartNotRequired, - }) + setDefaultResizePolicy := func(resourceName v1.ResourceName) { + if _, found := resizePolicySpecified[resourceName]; !found { + obj.Spec.Containers[i].ResizePolicy = append(obj.Spec.Containers[i].ResizePolicy, + v1.ContainerResizePolicy{ + ResourceName: resourceName, + RestartPolicy: v1.NotRequired, + }) + } } - if _, found := resizePolicySpecified[v1.ResourceMemory]; !found { - obj.Spec.Containers[i].ResizePolicy = append(obj.Spec.Containers[i].ResizePolicy, - v1.ContainerResizePolicy{ - ResourceName: v1.ResourceMemory, - RestartPolicy: v1.RestartNotRequired, - }) + if _, exists := obj.Spec.Containers[i].Resources.Requests[v1.ResourceCPU]; exists { + setDefaultResizePolicy(v1.ResourceCPU) + } + if _, exists := obj.Spec.Containers[i].Resources.Requests[v1.ResourceMemory]; exists { + setDefaultResizePolicy(v1.ResourceMemory) } } } diff --git a/pkg/apis/core/v1/defaults_test.go b/pkg/apis/core/v1/defaults_test.go index 256e29bf158..5c9688471e3 100644 --- a/pkg/apis/core/v1/defaults_test.go +++ b/pkg/apis/core/v1/defaults_test.go @@ -322,9 +322,6 @@ func testPodDefaults(t *testing.T, featuresEnabled bool) { ".Spec.Volumes[0].VolumeSource.ScaleIO.StorageMode": `"ThinProvisioned"`, ".Spec.Volumes[0].VolumeSource.Secret.DefaultMode": `420`, } - if featuresEnabled { - expectedDefaults[".Spec.Containers[0].ResizePolicy"] = `[{"resourceName":"cpu","restartPolicy":"RestartNotRequired"},{"resourceName":"memory","restartPolicy":"RestartNotRequired"}]` - } defaults := detectDefaults(t, pod, reflect.ValueOf(pod)) if !reflect.DeepEqual(expectedDefaults, defaults) { t.Errorf("Defaults for PodSpec changed. This can cause spurious restarts of containers on API server upgrade.") diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index ed8c4e3801b..fc5033028cf 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3016,7 +3016,7 @@ func validatePullPolicy(policy core.PullPolicy, fldPath *field.Path) field.Error } var supportedResizeResources = sets.NewString(string(core.ResourceCPU), string(core.ResourceMemory)) -var supportedResizePolicies = sets.NewString(string(core.RestartNotRequired), string(core.RestartContainer)) +var supportedResizePolicies = sets.NewString(string(core.NotRequired), string(core.RestartContainer)) func validateResizePolicy(policyList []core.ContainerResizePolicy, fldPath *field.Path) field.ErrorList { allErrors := field.ErrorList{} @@ -3036,7 +3036,7 @@ func validateResizePolicy(policyList []core.ContainerResizePolicy, fldPath *fiel allErrors = append(allErrors, field.NotSupported(fldPath, p.ResourceName, supportedResizeResources.List())) } switch p.RestartPolicy { - case core.RestartNotRequired, core.RestartContainer: + case core.NotRequired, core.RestartContainer: case "": allErrors = append(allErrors, field.Required(fldPath, "")) default: diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 23f1d6ee56c..2e73b1ee37d 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -7009,7 +7009,7 @@ func TestValidatePullPolicy(t *testing.T) { func TestValidateResizePolicy(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.InPlacePodVerticalScaling, true)() tSupportedResizeResources := sets.NewString(string(core.ResourceCPU), string(core.ResourceMemory)) - tSupportedResizePolicies := sets.NewString(string(core.RestartNotRequired), string(core.RestartContainer)) + tSupportedResizePolicies := sets.NewString(string(core.NotRequired), string(core.RestartContainer)) type T struct { PolicyList []core.ContainerResizePolicy ExpectError bool @@ -7018,7 +7018,7 @@ func TestValidateResizePolicy(t *testing.T) { testCases := map[string]T{ "ValidCPUandMemoryPolicies": { []core.ContainerResizePolicy{ - {ResourceName: "cpu", RestartPolicy: "RestartNotRequired"}, + {ResourceName: "cpu", RestartPolicy: "NotRequired"}, {ResourceName: "memory", RestartPolicy: "RestartContainer"}, }, false, @@ -7033,7 +7033,7 @@ func TestValidateResizePolicy(t *testing.T) { }, "ValidMemoryPolicy": { []core.ContainerResizePolicy{ - {ResourceName: "memory", RestartPolicy: "RestartNotRequired"}, + {ResourceName: "memory", RestartPolicy: "NotRequired"}, }, false, nil, @@ -7045,7 +7045,7 @@ func TestValidateResizePolicy(t *testing.T) { }, "ValidCPUandInvalidMemoryPolicy": { []core.ContainerResizePolicy{ - {ResourceName: "cpu", RestartPolicy: "RestartNotRequired"}, + {ResourceName: "cpu", RestartPolicy: "NotRequired"}, {ResourceName: "memory", RestartPolicy: "Restarrrt"}, }, true, @@ -7061,7 +7061,7 @@ func TestValidateResizePolicy(t *testing.T) { }, "InvalidResourceNameValidPolicy": { []core.ContainerResizePolicy{ - {ResourceName: "cpuuu", RestartPolicy: "RestartNotRequired"}, + {ResourceName: "cpuuu", RestartPolicy: "NotRequired"}, }, true, field.ErrorList{field.NotSupported(field.NewPath("field"), core.ResourceName("cpuuu"), tSupportedResizeResources.List())}, @@ -7075,7 +7075,7 @@ func TestValidateResizePolicy(t *testing.T) { }, "RepeatedPolicies": { []core.ContainerResizePolicy{ - {ResourceName: "cpu", RestartPolicy: "RestartNotRequired"}, + {ResourceName: "cpu", RestartPolicy: "NotRequired"}, {ResourceName: "memory", RestartPolicy: "RestartContainer"}, {ResourceName: "cpu", RestartPolicy: "RestartContainer"}, }, @@ -7475,7 +7475,7 @@ func TestValidateEphemeralContainers(t *testing.T) { ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", ResizePolicy: []core.ContainerResizePolicy{ - {ResourceName: "cpu", RestartPolicy: "RestartNotRequired"}, + {ResourceName: "cpu", RestartPolicy: "NotRequired"}, }, }, }, @@ -7702,7 +7702,7 @@ func TestValidateContainers(t *testing.T) { Name: "resources-resize-policy", Image: "image", ResizePolicy: []core.ContainerResizePolicy{ - {ResourceName: "cpu", RestartPolicy: "RestartNotRequired"}, + {ResourceName: "cpu", RestartPolicy: "NotRequired"}, {ResourceName: "memory", RestartPolicy: "RestartContainer"}, }, ImagePullPolicy: "IfNotPresent", @@ -9477,7 +9477,7 @@ func TestValidatePodSpec(t *testing.T) { Name: "initctr", Image: "initimage", ResizePolicy: []core.ContainerResizePolicy{ - {ResourceName: "cpu", RestartPolicy: "RestartNotRequired"}, + {ResourceName: "cpu", RestartPolicy: "NotRequired"}, }, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", @@ -9488,7 +9488,7 @@ func TestValidatePodSpec(t *testing.T) { Name: "ctr", Image: "image", ResizePolicy: []core.ContainerResizePolicy{ - {ResourceName: "cpu", RestartPolicy: "RestartNotRequired"}, + {ResourceName: "cpu", RestartPolicy: "NotRequired"}, }, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", diff --git a/pkg/kubelet/container/helpers_test.go b/pkg/kubelet/container/helpers_test.go index 113d6270956..4964510dd2a 100644 --- a/pkg/kubelet/container/helpers_test.go +++ b/pkg/kubelet/container/helpers_test.go @@ -915,8 +915,8 @@ func TestHashContainerWithoutResources(t *testing.T) { cpu200m := resource.MustParse("200m") mem100M := resource.MustParse("100Mi") mem200M := resource.MustParse("200Mi") - cpuPolicyRestartNotRequired := v1.ContainerResizePolicy{ResourceName: v1.ResourceCPU, RestartPolicy: v1.RestartNotRequired} - memPolicyRestartNotRequired := v1.ContainerResizePolicy{ResourceName: v1.ResourceMemory, RestartPolicy: v1.RestartNotRequired} + cpuPolicyRestartNotRequired := v1.ContainerResizePolicy{ResourceName: v1.ResourceCPU, RestartPolicy: v1.NotRequired} + memPolicyRestartNotRequired := v1.ContainerResizePolicy{ResourceName: v1.ResourceMemory, RestartPolicy: v1.NotRequired} cpuPolicyRestartRequired := v1.ContainerResizePolicy{ResourceName: v1.ResourceCPU, RestartPolicy: v1.RestartContainer} memPolicyRestartRequired := v1.ContainerResizePolicy{ResourceName: v1.ResourceMemory, RestartPolicy: v1.RestartContainer} @@ -938,7 +938,7 @@ func TestHashContainerWithoutResources(t *testing.T) { }, ResizePolicy: []v1.ContainerResizePolicy{cpuPolicyRestartRequired, memPolicyRestartNotRequired}, }, - 0xbf0fc03d, + 0x5f62cb4c, }, { "Burstable pod with memory policy restart required", @@ -951,7 +951,7 @@ func TestHashContainerWithoutResources(t *testing.T) { }, ResizePolicy: []v1.ContainerResizePolicy{cpuPolicyRestartNotRequired, memPolicyRestartRequired}, }, - 0x97dd7301, + 0xcdab9e00, }, { "Guaranteed pod with CPU policy restart required", @@ -964,7 +964,7 @@ func TestHashContainerWithoutResources(t *testing.T) { }, ResizePolicy: []v1.ContainerResizePolicy{cpuPolicyRestartRequired, memPolicyRestartNotRequired}, }, - 0xbf0fc03d, + 0x5f62cb4c, }, { "Guaranteed pod with memory policy restart required", @@ -977,7 +977,7 @@ func TestHashContainerWithoutResources(t *testing.T) { }, ResizePolicy: []v1.ContainerResizePolicy{cpuPolicyRestartNotRequired, memPolicyRestartRequired}, }, - 0x97dd7301, + 0xcdab9e00, }, } for _, tc := range tests { diff --git a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go index 333ba4ca7eb..0318f394855 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go @@ -1654,8 +1654,8 @@ func TestComputePodActionsForPodResize(t *testing.T) { cpu200m := resource.MustParse("200m") mem100M := resource.MustParse("100Mi") mem200M := resource.MustParse("200Mi") - cpuPolicyRestartNotRequired := v1.ContainerResizePolicy{ResourceName: v1.ResourceCPU, RestartPolicy: v1.RestartNotRequired} - memPolicyRestartNotRequired := v1.ContainerResizePolicy{ResourceName: v1.ResourceMemory, RestartPolicy: v1.RestartNotRequired} + cpuPolicyRestartNotRequired := v1.ContainerResizePolicy{ResourceName: v1.ResourceCPU, RestartPolicy: v1.NotRequired} + memPolicyRestartNotRequired := v1.ContainerResizePolicy{ResourceName: v1.ResourceMemory, RestartPolicy: v1.NotRequired} cpuPolicyRestartRequired := v1.ContainerResizePolicy{ResourceName: v1.ResourceCPU, RestartPolicy: v1.RestartContainer} memPolicyRestartRequired := v1.ContainerResizePolicy{ResourceName: v1.ResourceMemory, RestartPolicy: v1.RestartContainer} diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index 3456e08ad70..1ef7971078a 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -2268,11 +2268,11 @@ type ResourceResizeRestartPolicy string // These are the valid resource resize restart policy values: const ( - // 'RestartNotRequired' means Kubernetes will try to resize the container + // 'NotRequired' means Kubernetes will try to resize the container // without restarting it, if possible. Kubernetes may however choose to // restart the container if it is unable to actuate resize without a // restart. For e.g. the runtime doesn't support restart-free resizing. - RestartNotRequired ResourceResizeRestartPolicy = "RestartNotRequired" + NotRequired ResourceResizeRestartPolicy = "NotRequired" // 'RestartContainer' means Kubernetes will resize the container in-place // by stopping and starting the container when new resources are applied. // This is needed for legacy applications. For e.g. java apps using the @@ -2286,7 +2286,7 @@ type ContainerResizePolicy struct { // Supported values: cpu, memory. ResourceName ResourceName `json:"resourceName" protobuf:"bytes,1,opt,name=resourceName,casttype=ResourceName"` // Restart policy to apply when specified resource is resized. - // If not specified, it defaults to RestartNotRequired. + // If not specified, it defaults to NotRequired. RestartPolicy ResourceResizeRestartPolicy `json:"restartPolicy" protobuf:"bytes,2,opt,name=restartPolicy,casttype=ResourceResizeRestartPolicy"` } diff --git a/test/e2e/node/pod_resize.go b/test/e2e/node/pod_resize.go index 003d65a6390..5a37f4075dd 100644 --- a/test/e2e/node/pod_resize.go +++ b/test/e2e/node/pod_resize.go @@ -157,7 +157,7 @@ func getTestResourceInfo(tcInfo TestContainerInfo) (v1.ResourceRequirements, v1. } func initDefaultResizePolicy(containers []TestContainerInfo) { - noRestart := v1.RestartNotRequired + noRestart := v1.NotRequired setDefaultPolicy := func(ci *TestContainerInfo) { if ci.CPUPolicy == nil { ci.CPUPolicy = &noRestart @@ -500,7 +500,7 @@ func doPodResizeTests() { expected []TestContainerInfo } - noRestart := v1.RestartNotRequired + noRestart := v1.NotRequired doRestart := v1.RestartContainer tests := []testCase{ { @@ -1010,7 +1010,7 @@ func doPodResizeTests() { }, }, { - name: "Guaranteed QoS pod, one container - increase CPU (RestartNotRequired) & memory (RestartContainer)", + name: "Guaranteed QoS pod, one container - increase CPU (NotRequired) & memory (RestartContainer)", containers: []TestContainerInfo{ { Name: "c1", @@ -1033,7 +1033,7 @@ func doPodResizeTests() { }, }, { - name: "Burstable QoS pod, one container - decrease CPU (RestartContainer) & memory (RestartNotRequired)", + name: "Burstable QoS pod, one container - decrease CPU (RestartContainer) & memory (NotRequired)", containers: []TestContainerInfo{ { Name: "c1",