[FG:InPlacePodVerticalScaling] Forbid memory limit decrease #130183
Conversation
|
/assign @thockin |
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
| } | ||
| } | ||
|
|
||
| // TweakContainers applies the container tweaks to all containers in the pod with a masked type. |
There was a problem hiding this comment.
What does "a masked type" mean here?
There was a problem hiding this comment.
Oops, this was originally using podutil.VisitContainers, which take a container type mask, but I had to remove that due to a circular dependency, and forgot to update the comment.
|
LGTM label has been added. Git tree hash: 85fc1ab139746c143447771de3d0f950297d3126
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tallclair, thockin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
https://testgrid.k8s.io/sig-release-master-blocking#gce-cos-master-alpha-features failed after this? E2E test may need some updates. |
| // Special case: memory limits may not be decreased if resize policy is NotRequired. | ||
| var memRestartPolicy core.ResourceResizeRestartPolicy | ||
| for _, policy := range resizePolicies { | ||
| if policy.ResourceName == core.ResourceMemory { | ||
| memRestartPolicy = policy.RestartPolicy | ||
| break | ||
| } | ||
| } | ||
| if memRestartPolicy == core.NotRequired || memRestartPolicy == "" { | ||
| newLimit, hasNewLimit := newRequirements.Limits[core.ResourceMemory] | ||
| oldLimit, hasOldLimit := oldRequirements.Limits[core.ResourceMemory] | ||
| if hasNewLimit && hasOldLimit { | ||
| if newLimit.Cmp(oldLimit) < 0 { | ||
| allErrs = append(allErrs, field.Forbidden( | ||
| fldPath.Child("limits").Key(core.ResourceMemory.String()), | ||
| fmt.Sprintf("memory limits cannot be decreased unless resizePolicy is %s", core.RestartContainer))) | ||
| } | ||
| } else if hasNewLimit && !hasOldLimit { | ||
| // Adding a memory limit is implicitly decreasing the memory limit (from 'max') | ||
| allErrs = append(allErrs, field.Forbidden( | ||
| fldPath.Child("limits").Key(core.ResourceMemory.String()), | ||
| fmt.Sprintf("memory limits cannot be added unless resizePolicy is %s", core.RestartContainer))) | ||
| } | ||
| } | ||
|
|
||
| // TODO(tallclair): Move resizable resource checks here. |
There was a problem hiding this comment.
This is breaking existing e2e tests
failed [FAILED] failed to patch pod for resize: Pod "resize-test-262pc" is invalid: spec.initContainers[0].resources.limits[memory]: Forbidden: memory limits cannot be decreased unless resizePolicy is RestartContainer
In [It] at: k8s.io/kubernetes/test/e2e/common/node/pod_resize.go:1334 @ 02/21/25 00:49:20.398
|
Thanks for flagging the test failures. I'm looking into it. |
|
Tests fixed in #130357 |
Thanks for the fast response |
|
@tallclair It seems I have to mutate restartPolicy for increase/decrease memory limit scenario. |
Unfortunately that's not possible today. The good news is that we're planning to lift this restriction in v1.34, and allow memory limit decreases. |
OK, thanks for reply ~ |
What type of PR is this?
/kind feature
/kind api-change
What this PR does / why we need it:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/sig node
/milestone v1.33
/priority important-soon