Skip to content

Promote SidecarContainers feature to GA#129731

Merged
k8s-ci-robot merged 2 commits intokubernetes:masterfrom
gjkim42:promote-sidecar-containers-to-ga
Feb 5, 2025
Merged

Promote SidecarContainers feature to GA#129731
k8s-ci-robot merged 2 commits intokubernetes:masterfrom
gjkim42:promote-sidecar-containers-to-ga

Conversation

@gjkim42
Copy link
Copy Markdown
Member

@gjkim42 gjkim42 commented Jan 21, 2025 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR promotes the SidecarContainers feature to the GA.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

The `SidecarContainers` feature has graduated to GA. 'SidecarContainers' feature gate was locked to default value and will be removed in v1.36. If you were setting this feature gate explicitly, please remove it now.

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

- [KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/753-sidecar-containers/README.md

@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. kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 21, 2025
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jan 21, 2025

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet area/test 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/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. 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 Jan 21, 2025
@gjkim42
Copy link
Copy Markdown
Member Author

gjkim42 commented Jan 21, 2025

/cc @matthyx @SergeyKanzhelev
/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 21, 2025
@gjkim42 gjkim42 force-pushed the promote-sidecar-containers-to-ga branch from 8e388e4 to 2cb6899 Compare January 21, 2025 14:35
gjkim42
gjkim42 commented Jan 21, 2025
View reviewed changes
Comment thread pkg/kubelet/kuberuntime/kuberuntime_manager.go
if len(containersToStart) == 0 {
hasInitialized := false
// TODO: Remove this code path as logically it is the subset of the next
// code path.
Copy link
Copy Markdown
Member Author

@gjkim42 gjkim42 Jan 21, 2025

Choose a reason for hiding this comment

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

xref: #127162 (comment)

Copy link
Copy Markdown
Contributor

@toVersus toVersus Feb 2, 2025

Choose a reason for hiding this comment

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

I'm not entirely sure about the intent behind keeping this code path. When will it be possible to remove it completely?

Copy link
Copy Markdown
Member

@SergeyKanzhelev SergeyKanzhelev Feb 3, 2025

Choose a reason for hiding this comment

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

we will need to introduce a separate feature gate so we switch logic safely. Keeping two code path doesn't really makes sense long term, but short term we want to be extra cautious

@gjkim42 gjkim42 force-pushed the promote-sidecar-containers-to-ga branch 2 times, most recently from 021e88d to 9e23fa9 Compare January 22, 2025 00:51
@SergeyKanzhelev
Copy link
Copy Markdown
Member

SergeyKanzhelev commented Jan 23, 2025

Any reason to keep the WIP label?

@SergeyKanzhelev SergeyKanzhelev mentioned this pull request Jan 23, 2025
Closed
15 tasks
@gjkim42
Copy link
Copy Markdown
Member Author

gjkim42 commented Jan 24, 2025 •
edited
Loading

Any reason to keep the WIP label?

@SergeyKanzhelev

#129731 (comment)

only to address this comment.

We'll leave the TODO comment here and add a new PR to create a temporary feature gate to remove the redundant code in the next release(1.34), right?

SergeyKanzhelev
SergeyKanzhelev reviewed Jan 31, 2025
View reviewed changes
Comment thread pkg/apis/core/validation/validation.go
SergeyKanzhelev
SergeyKanzhelev reviewed Jan 31, 2025
View reviewed changes
Comment thread test/integration/scheduler/scoring/priorities_test.go
@toVersus toVersus mentioned this pull request Feb 2, 2025
Merged
@gjkim42
Promote SidecarContainers feature to GA
0bee0bc
@gjkim42
Leave TODOs in pkg/kubelet/kuberuntime for later removal
8d27bf2 
This leaves TODOs in pkg/kubelet/kuberuntime to remove these redundant
code paths later, since they are supposed to be a subset of the new code
paths.
@gjkim42 gjkim42 force-pushed the promote-sidecar-containers-to-ga branch from 8008162 to 8d27bf2 Compare February 2, 2025 08:46
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 2, 2025
SergeyKanzhelev
SergeyKanzhelev approved these changes Feb 3, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

hold for KEP GA approval: kubernetes/enhancements#5081

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 3, 2025
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Feb 3, 2025

LGTM label has been added.

DetailsGit tree hash: c89c14a805a62503e6d8b62286ac1db496583965

@SergeyKanzhelev
Copy link
Copy Markdown
Member

SergeyKanzhelev commented Feb 4, 2025

/unhold

/assign @janetkuo @msau42

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 4, 2025
@janetkuo
Copy link
Copy Markdown
Member

janetkuo commented Feb 4, 2025

@gjkim42 in the release note 'SidecarContainers' feature was locked to default value and will be removed in v1.36. Would you change it from "feature" to "feature gate" to make it more clear that the feature isn't going to be removed, and just the feature gate will be?

@janetkuo
Copy link
Copy Markdown
Member

janetkuo commented Feb 4, 2025

/approve

@msau42
Copy link
Copy Markdown
Member

msau42 commented Feb 4, 2025

/approve

for API

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Feb 4, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gjkim42, janetkuo, msau42, SergeyKanzhelev

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 4, 2025
@k8s-ci-robot k8s-ci-robot merged commit fab0d76 into kubernetes:master Feb 5, 2025
@k8s-ci-robot k8s-ci-robot added this to the v1.33 milestone Feb 5, 2025
@gjkim42 gjkim42 deleted the promote-sidecar-containers-to-ga branch February 5, 2025 02:05
@gjkim42 gjkim42 mentioned this pull request Feb 5, 2025
Merged
@pacoxu pacoxu mentioned this pull request Feb 8, 2025
Merged
@github-actions github-actions Bot mentioned this pull request Oct 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@denkensk denkensk Awaiting requested review from denkensk

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@matthyx matthyx Awaiting requested review from matthyx

3 more reviewers

@toVersus toVersus toVersus left review comments

@janetkuo janetkuo janetkuo approved these changes

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@janetkuo janetkuo

@SergeyKanzhelev SergeyKanzhelev

@msau42 msau42

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. 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/apps Categorizes an issue or PR as relevant to SIG Apps. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

SIG Apps
Archived in project
SIG Node CI/Test Board
Archived in project

Milestone

v1.33

Development

Successfully merging this pull request may close these issues.

6 participants

@gjkim42 @k8s-ci-robot @SergeyKanzhelev @janetkuo @msau42 @toVersus