Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Restructure resize policy naming and set default resize policy values #116119

Merged
merged 5 commits into from Mar 15, 2023
Merged

Restructure resize policy naming and set default resize policy values #116119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8b23497
Restructure naming of resource resize restart policy
vinaykul Feb 28, 2023
c5130fb
Restructure naming of resource resize restart policy - generated files
vinaykul Feb 28, 2023
9411050
Set default resource resize restart policy to RestartNotRequired
vinaykul Mar 1, 2023
9a805db
Set default resize policy only for specified resource types, rename R…
vinaykul Mar 10, 2023
06e1692
Update generated files
vinaykul Mar 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 6 additions & 6 deletions api/openapi-spec/swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 6 additions & 6 deletions api/openapi-spec/v3/api__v1_openapi.json
View file
Open in desktop
Expand Up @@ -1306,22 +1306,22 @@
"type": "object"
},
"io.k8s.api.core.v1.ContainerResizePolicy": {
"description": "ContainerResizePolicy represents resource resize policy for a single container.",
"description": "ContainerResizePolicy represents resource resize policy for the container.",
"properties": {
"policy": {
"resourceName": {
"default": "",
"description": "Resource resize policy applicable to the specified resource name. If not specified, it defaults to RestartNotRequired.",
"description": "Name of the resource to which this resource resize policy applies. Supported values: cpu, memory.",
"type": "string"
},
"resourceName": {
"restartPolicy": {
"default": "",
"description": "Name of the resource type to which this resource resize policy applies. Supported values: cpu, memory.",
"description": "Restart policy to apply when specified resource is resized. If not specified, it defaults to NotRequired.",
"type": "string"
}
},
"required": [
"resourceName",
"policy"
"restartPolicy"
],
"type": "object"
},
Expand Down
12 changes: 6 additions & 6 deletions api/openapi-spec/v3/apis__apps__v1_openapi.json
View file
Open in desktop
Expand Up @@ -1910,22 +1910,22 @@
"type": "object"
},
"io.k8s.api.core.v1.ContainerResizePolicy": {
"description": "ContainerResizePolicy represents resource resize policy for a single container.",
"description": "ContainerResizePolicy represents resource resize policy for the container.",
"properties": {
"policy": {
"resourceName": {
"default": "",
"description": "Resource resize policy applicable to the specified resource name. If not specified, it defaults to RestartNotRequired.",
"description": "Name of the resource to which this resource resize policy applies. Supported values: cpu, memory.",
"type": "string"
},
"resourceName": {
"restartPolicy": {
"default": "",
"description": "Name of the resource type to which this resource resize policy applies. Supported values: cpu, memory.",
"description": "Restart policy to apply when specified resource is resized. If not specified, it defaults to NotRequired.",
"type": "string"
}
},
"required": [
"resourceName",
"policy"
"restartPolicy"
],
"type": "object"
},
Expand Down
12 changes: 6 additions & 6 deletions api/openapi-spec/v3/apis__batch__v1_openapi.json
View file
Open in desktop
Expand Up @@ -1201,22 +1201,22 @@
"type": "object"
},
"io.k8s.api.core.v1.ContainerResizePolicy": {
"description": "ContainerResizePolicy represents resource resize policy for a single container.",
"description": "ContainerResizePolicy represents resource resize policy for the container.",
"properties": {
"policy": {
"resourceName": {
"default": "",
"description": "Resource resize policy applicable to the specified resource name. If not specified, it defaults to RestartNotRequired.",
"description": "Name of the resource to which this resource resize policy applies. Supported values: cpu, memory.",
"type": "string"
},
"resourceName": {
"restartPolicy": {
"default": "",
"description": "Name of the resource type to which this resource resize policy applies. Supported values: cpu, memory.",
"description": "Restart policy to apply when specified resource is resized. If not specified, it defaults to NotRequired.",
"type": "string"
}
},
"required": [
"resourceName",
"policy"
"restartPolicy"
],
"type": "object"
},
Expand Down
4 changes: 2 additions & 2 deletions pkg/api/pod/util_test.go
View file
Open in desktop
Expand Up @@ -2290,8 +2290,8 @@ func TestDropInPlacePodVerticalScaling(t *testing.T) {
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("200m")},
},
ResizePolicy: []api.ContainerResizePolicy{
{ResourceName: api.ResourceCPU, Policy: api.RestartNotRequired},
{ResourceName: api.ResourceMemory, Policy: api.RestartRequired},
{ResourceName: api.ResourceCPU, RestartPolicy: api.NotRequired},
{ResourceName: api.ResourceMemory, RestartPolicy: api.RestartContainer},
},
},
},
Expand Down
24 changes: 12 additions & 12 deletions pkg/apis/core/types.go
View file
Open in desktop
Expand Up @@ -2138,31 +2138,31 @@ const (
PullIfNotPresent PullPolicy = "IfNotPresent"
)

// ResourceResizePolicy specifies how Kubernetes should handle resource resize.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC: @marquiz, was it you we discussed it with when talking about QoS-class resources KEP: kubernetes/enhancements#3008. Cannot recall now. Maybe you have some naming input here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I don't remember discussing this aspect of in-place updates in context of the QoS resources KEP. I don't think we've got this deep in the details there, yet.

type ResourceResizePolicy string
// ResourceResizeRestartPolicy specifies how to handle container resource resize.
type ResourceResizeRestartPolicy string
thockin marked this conversation as resolved.
Show resolved Hide resolved

// These are the valid resource resize policy values:
// These are the valid resource resize restart policy values:
const (
// RestartNotRequired tells Kubernetes to resize the container in-place
// '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 ResourceResizePolicy = "RestartNotRequired"
// 'RestartRequired' tells Kubernetes to resize the container in-place
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
// -xmxN flag which are unable to use resized memory without restarting.
RestartRequired ResourceResizePolicy = "RestartRequired"
RestartContainer ResourceResizeRestartPolicy = "RestartContainer"
)

// ContainerResizePolicy represents resource resize policy for a single container.
// ContainerResizePolicy represents resource resize policy for the container.
type ContainerResizePolicy struct {
// Name of the resource type to which this resource resize policy applies.
// Name of the resource to which this resource resize policy applies.
// Supported values: cpu, memory.
ResourceName ResourceName
// Resource resize policy applicable to the specified resource name.
// If not specified, it defaults to RestartNotRequired.
Policy ResourceResizePolicy
// Restart policy to apply when specified resource is resized.
// If not specified, it defaults to NotRequired.
RestartPolicy ResourceResizeRestartPolicy
}

// PreemptionPolicy describes a policy for if/when to preempt a pod.
Expand Down
25 changes: 25 additions & 0 deletions pkg/apis/core/v1/defaults.go
View file
Open in desktop
Expand Up @@ -22,6 +22,8 @@ import (
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/intstr"
utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/kubernetes/pkg/features"
"k8s.io/kubernetes/pkg/util/parsers"
"k8s.io/utils/pointer"
)
Expand Down Expand Up @@ -157,6 +159,29 @@ func SetDefaults_Pod(obj *v1.Pod) {
}
}
}
if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up with a test? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a more comprehensive test is warranted, sorry for the miss. I'll bring it with GetPodQoS -> PodQOSClass change.
Having a stiff drink rn :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thockin Please review PR #116684 that adds missing unit test for defaulting

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
}
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 _, 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)
}
}
}
for i := range obj.Spec.InitContainers {
if obj.Spec.InitContainers[i].Resources.Limits != nil {
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/core/v1/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions pkg/apis/core/validation/validation.go
View file
Open in desktop
Expand Up @@ -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.RestartRequired))
var supportedResizePolicies = sets.NewString(string(core.NotRequired), string(core.RestartContainer))

func validateResizePolicy(policyList []core.ContainerResizePolicy, fldPath *field.Path) field.ErrorList {
allErrors := field.ErrorList{}
Expand All @@ -3035,12 +3035,12 @@ func validateResizePolicy(policyList []core.ContainerResizePolicy, fldPath *fiel
default:
allErrors = append(allErrors, field.NotSupported(fldPath, p.ResourceName, supportedResizeResources.List()))
}
switch p.Policy {
case core.RestartNotRequired, core.RestartRequired:
switch p.RestartPolicy {
case core.NotRequired, core.RestartContainer:
case "":
allErrors = append(allErrors, field.Required(fldPath, ""))
default:
allErrors = append(allErrors, field.NotSupported(fldPath, p.Policy, supportedResizePolicies.List()))
allErrors = append(allErrors, field.NotSupported(fldPath, p.RestartPolicy, supportedResizePolicies.List()))
}
}
return allErrors
Expand Down