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

Promote SidecarContainers feature to GA #129731

Merged
merged 2 commits into from
Feb 5, 2025
Merged

Promote SidecarContainers feature to GA #129731

merged 2 commits into from
Feb 5, 2025

Conversation

gjkim42
Copy link
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
Contributor

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.

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
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
pkg/kubelet/kuberuntime/kuberuntime_manager.go
@@ -934,6 +935,8 @@ func (m *kubeGenericRuntimeManager) computePodActions(ctx context.Context, pod *
// is done and there is no container to start.
if len(containersToStart) == 0 {
hasInitialized := false
// TODO: Remove this code path as logically it is the subset of the next
// code path.
Copy link
Member Author

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
Contributor

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
Member

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
Member

Any reason to keep the WIP label?

@SergeyKanzhelev SergeyKanzhelev mentioned this pull request Jan 23, 2025
Open
15 tasks
@gjkim42
Copy link
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?

@gjkim42
Copy link
Member Author

gjkim42 commented Jan 31, 2025

cleaned up the commits and removed the WIP label from the PR.

/assign @SergeyKanzhelev
Could you PTAL?

@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
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
Contributor

LGTM label has been added.

Git tree hash: c89c14a805a62503e6d8b62286ac1db496583965

@SergeyKanzhelev
Copy link
Member

/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
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
Member

janetkuo commented Feb 4, 2025

/approve

@msau42
Copy link
Member

msau42 commented Feb 4, 2025

/approve

for API

@k8s-ci-robot
Copy link
Contributor

[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

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
15 checks passed
@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
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toVersus toVersus toVersus left review comments

@janetkuo janetkuo janetkuo approved these changes

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

@denkensk denkensk Awaiting requested review from denkensk

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@matthyx matthyx Awaiting requested review from matthyx

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
Status: Done
SIG Node CI/Test Board
Status: Done
Milestone
v1.33
Development

Successfully merging this pull request may close these issues.
6 participants
@gjkim42 @k8s-ci-robot @SergeyKanzhelev @janetkuo @msau42 @toVersus