Skip to content

[FG:InPlacePodVerticalScaling] Implement resize for sidecar containers#128367

Merged
k8s-ci-robot merged 22 commits intokubernetes:masterfrom
vivzbansal:sidecar-2
Feb 5, 2025
Merged

[FG:InPlacePodVerticalScaling] Implement resize for sidecar containers#128367
k8s-ci-robot merged 22 commits intokubernetes:masterfrom
vivzbansal:sidecar-2

Conversation

@vivzbansal
Copy link
Copy Markdown
Contributor

@vivzbansal vivzbansal commented Oct 27, 2024 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

Resize of sidecar containers should work the same as resize of regular containers. This PR adds the capability to resize the sidecar containers (restartable init containers) when in-place pod resizing is enabled. Resize of non-restartable init containers is still not allowed.

Which issue(s) this PR fixes:

Fixes #128070

Special notes for your reviewer:

There was an earlier PR #128272 created which got closed due to some significant changes made in PR #128269

Does this PR introduce a user-facing change?

Added support for in-place vertical scaling of Pods with sidecars (containers defined within `initContainers` where the `restartPolicy` is Always).

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

[KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/1287-in-place-update-pod-resources

/sig node
/priority important-soon
/milestone v1.33

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ 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. do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. labels Oct 27, 2024
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Oct 27, 2024

Hi @vivzbansal. 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.

Details

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 needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 27, 2024
@vivzbansal vivzbansal mentioned this pull request Oct 27, 2024
Closed
@vivzbansal
Copy link
Copy Markdown
Contributor Author

vivzbansal commented Oct 27, 2024

/assign @tallclair

@vivzbansal
Copy link
Copy Markdown
Contributor Author

vivzbansal commented Oct 28, 2024

/ok-to-test

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Oct 28, 2024

@vivzbansal: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/ok-to-test

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.

@tallclair
Copy link
Copy Markdown
Member

tallclair commented Oct 28, 2024

/ok-to-test
/priority important-soon
/triage accepted
/milestone v1.32

@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Oct 28, 2024
@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. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 28, 2024
tallclair
tallclair reviewed Oct 28, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@tallclair tallclair left a comment

Choose a reason for hiding this comment

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

Review is still WIP, but publishing my comments so far

Comment thread pkg/api/pod/util.go Outdated
Comment thread pkg/api/pod/util.go Outdated
Comment thread pkg/apis/core/validation/validation.go Outdated
Comment thread pkg/kubelet/cm/memorymanager/policy_static.go Outdated
Comment thread pkg/kubelet/cm/cpumanager/policy_static.go Outdated
Comment thread pkg/kubelet/kuberuntime/kuberuntime_container.go
Comment thread pkg/kubelet/kuberuntime/kuberuntime_container.go Outdated
Comment thread pkg/kubelet/kuberuntime/kuberuntime_manager.go
Comment thread pkg/kubelet/kuberuntime/kuberuntime_manager.go
Comment thread pkg/kubelet/kuberuntime/kuberuntime_manager.go Outdated
@vivzbansal
Copy link
Copy Markdown
Contributor Author

vivzbansal commented Jan 27, 2025

/retest

tallclair
tallclair reviewed Jan 27, 2025
View reviewed changes
Comment thread pkg/apis/core/validation/validation.go Outdated
Comment thread pkg/apis/core/validation/validation.go Outdated
Comment thread pkg/apis/core/validation/validation.go Outdated
Comment thread pkg/apis/core/validation/validation.go Outdated
tallclair
tallclair reviewed Jan 28, 2025
View reviewed changes
Comment thread pkg/apis/core/validation/validation.go Outdated
tallclair
tallclair reviewed Jan 28, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@tallclair tallclair left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jan 28, 2025

LGTM label has been added.

DetailsGit tree hash: be8e5aba891b5d1bd1875f965c75bc335c0d2520

thockin
thockin approved these changes Feb 5, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

validation and strategy are OK.
/approve

gjkim42
gjkim42 reviewed Feb 5, 2025
View reviewed changes
Comment thread pkg/kubelet/cm/cpumanager/policy_static.go
if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) {
if cs, ok := podutil.GetContainerStatus(pod.Status.ContainerStatuses, container.Name); ok {
containerStatuses := pod.Status.ContainerStatuses
if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) && podutil.IsRestartableInitContainer(container) {
Copy link
Copy Markdown
Member

@gjkim42 gjkim42 Feb 5, 2025

Choose a reason for hiding this comment

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

#129731

SidecarContainers promotes to GA in this release.
Can you remove the feature gate check from the kubelet code?

Copy link
Copy Markdown
Contributor Author

@vivzbansal vivzbansal Feb 5, 2025

Choose a reason for hiding this comment

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

Sure, we can remove the feature gate check in a separate PR.

@tallclair
Copy link
Copy Markdown
Member

tallclair commented Feb 5, 2025

Hmm, not sure what owners are missing.
/approve

@tallclair
Copy link
Copy Markdown
Member

tallclair commented Feb 5, 2025

Weird. Looks like @thockin's approval didn't get registered.

@vivzbansal
Copy link
Copy Markdown
Contributor Author

vivzbansal commented Feb 5, 2025

Weird. Looks like @thockin's approval didn't get registered.

Yes I think so.

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Feb 5, 2025

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: tallclair, vivzbansal

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Feb 5, 2025 •
edited
Loading

@vivzbansal: The following test 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-node-kubelet-serial-podresize 149921a link false /test pull-kubernetes-node-kubelet-serial-podresize

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.

Details

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.

@vivzbansal
Copy link
Copy Markdown
Contributor Author

vivzbansal commented Feb 5, 2025

/retest

@hshiina hshiina mentioned this pull request Mar 10, 2025
Closed
11 tasks
@es-ma-n es-ma-n mentioned this pull request Mar 13, 2025
Merged
@github-actions github-actions Bot mentioned this pull request Oct 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@soltysh soltysh soltysh left review comments

@thockin thockin thockin approved these changes

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@haircommander haircommander Awaiting requested review from haircommander

+7 more reviewers

@haosdent haosdent haosdent left review comments

@AnishShah AnishShah AnishShah left review comments

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev left review comments

@gjkim42 gjkim42 gjkim42 left review comments

@tallclair tallclair tallclair left review comments

@hshiina hshiina hshiina left review comments

@HirazawaUi HirazawaUi HirazawaUi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@thockin thockin

@tallclair tallclair

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/kubectl area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Projects

API Reviews
Status: API review completed, 1.33
SIG Apps
Archived in project
SIG CLI
Archived in project
SIG Node CI/Test Board
Archived in project
SIG Node: In Place Pod Vertical Scaling
Status: Done
SIG Node: code and documentation PRs
Archived in project
[sig-release] Bug Triage
Archived in project

Milestone

v1.33

Development

Successfully merging this pull request may close these issues.

[FG:InPlacePodVerticalScaling] Implement resize for sidecar containers