Skip to content

[FG:InPlacePodVerticalScaling] Enable removing resource limits from containers #127143

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
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[FG:InPlacePodVerticalScaling] Enable removing resource limits from containers #127143

wants to merge 3 commits into from

Conversation

hshiina
Copy link
Contributor

@hshiina hshiina commented Sep 5, 2024 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR fixes handling cases where resources limits are max at InPlacePodVerticalScaling.

When one of containers in a pod has no limits for a resource, no limits will be configured in the cgroup of the pod. This fix allows resizing the resource in a container that has limits when another container in the pod does not have limits for the resource. In doPodResizeAction(), this fix allows a container to be resized even if the pod resources are unlimited (the values in the pod resource is nil).

This PR also enables removing resource limits from containers by following changes:

  • Update pod cgroup resources to max
  • Update container resources to max with CRI API UpdateContainerResource().
  • Display resource limits in container status while limits are being removed.

Which issue(s) this PR fixes:

Fixes #128675

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Enabled removing resource limits and requests at InPlacePodVerticalScaling.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 5, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @hshiina. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Sep 5, 2024
@k8s-ci-robot k8s-ci-robot added area/kubelet area/test sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 5, 2024
@hshiina hshiina changed the title [InPlacePodVerticalScaling [WIP][InPlacePodVerticalScaling] Enable removing resource limits from containers Sep 5, 2024
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 5, 2024
@hshiina
Copy link
Contributor Author

hshiina commented Sep 5, 2024

/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 5, 2024
@hshiina hshiina changed the title [WIP][InPlacePodVerticalScaling] Enable removing resource limits from containers [WIP][FG:InPlacePodVerticalScaling] Enable removing resource limits from containers Sep 5, 2024
@kannon92
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 11, 2024
@hshiina
Copy link
Contributor Author

hshiina commented Sep 27, 2024

/test pull-kubernetes-node-kubelet-serial-podresize

@tallclair
Copy link
Member

/assign

@k8s-ci-robot k8s-ci-robot requested a review from tallclair May 25, 2025 17:29
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2025
@tico88612
Copy link
Member

Hello @hshiina @tallclair @natasha41575
This PR has not been updated for 1 month, so I'd like to check the status. If there's anything we can do, please let us know.
The code freeze is starting 02:00 UTC Friday 25th July 2025 (about 3 weeks from now). Please make sure the PR has both lgtm and approved labels before the code freeze. Thanks!

@tico88612 tico88612 moved this from Pending inclusion to Tracked in [sig-release] Bug Triage Jun 30, 2025
@hashim21223445
Copy link

#129160
#131799

@natasha41575 natasha41575 mentioned this pull request Jul 7, 2025
Open
90 tasks
@natasha41575
Copy link
Contributor

/lgtm

@hshiina are the test failures unrelated?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 5911b5d899102964e5ff59df7661a7c5fb2598a7

@hshiina
Copy link
Contributor Author

hshiina commented Jul 8, 2025

/retest

3 similar comments
@hshiina
Copy link
Contributor Author

hshiina commented Jul 8, 2025

/retest

@hshiina
Copy link
Contributor Author

hshiina commented Jul 9, 2025

/retest

@hshiina
Copy link
Contributor Author

hshiina commented Jul 9, 2025

/retest

@haircommander haircommander moved this from Work in progress to Needs Approver in SIG Node: code and documentation PRs Jul 9, 2025
@haircommander haircommander moved this from PRs Waiting on Author to PRs - Needs Approver in SIG Node CI/Test Board Jul 9, 2025
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2025
@tico88612
Copy link
Member

Hi @haircommander @hshiina @natasha41575
Appreciate all of your efforts with this PR! Is the plan still to resolve it for v1.34 release?

If so, a gentle reminder that the code freeze has started 02:00 UTC Friday 25th July 2025. Please make sure any PRs have both lgtm and approved labels ASAP, and file an Exception if you haven't done it yet.

Thanks!

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 23, 2025
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

hshiina added 3 commits July 23, 2025 11:02
@hshiina
api: accept removing requsets & limits for Burstable pods
d8a863f
@hshiina
kubelet: Enable removing resource limits and requests
7162b0e 
This fix enables removing resource limits and requests from containers
by following changes:
- Update pod cgroup resources to max
- Update container resources to max with CRI API
  UpdateContainerResource().
- Display resource limits in container status while limits are being
  removed.
@hshiina
Address feedback
e7aa4b3
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 23, 2025 •
edited
Loading

@hshiina: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-unit-windows-master e7aa4b3 link false /test pull-kubernetes-unit-windows-master
pull-kubernetes-e2e-gce e7aa4b3 link true /test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@natasha41575
Copy link
Contributor

@tallclair is out this week, and I'm not sure if there is another approver with sufficient context who will be able to take a look, so this may slip to 1.35

@Rajalakshmi-Girish
Copy link
Contributor

/milestone v1.35

@k8s-ci-robot k8s-ci-robot modified the milestones: v1.34, v1.35 Jul 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AnishShah AnishShah AnishShah left review comments

@aojea aojea aojea left review comments

@hashim21223445 hashim21223445 hashim21223445 approved these changes

@bart0sh bart0sh Awaiting requested review from bart0sh

@bobbypage bobbypage Awaiting requested review from bobbypage

@natasha41575 natasha41575 Awaiting requested review from natasha41575

@tallclair tallclair Awaiting requested review from tallclair

Assignees

@natasha41575 natasha41575

@tallclair tallclair

Labels
area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Apps
Status: Needs Triage
SIG Node CI/Test Board
Status: PRs - Needs Approver
SIG Node: In Place Pod Vertical Scaling
Status: In Progress
SIG Node: code and documentation PRs
Status: Needs Approver
[sig-release] Bug Triage
Status: Tracked
Milestone
v1.35
Development

Successfully merging this pull request may close these issues.

[FG:InPlacePodVerticalScaling] Support removing requests and limits (from Burstable pods)