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.

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

Promote EphemeralContainers feature to GA #111402

Merged
merged 3 commits into from
Jul 30, 2022

Conversation

verb
Copy link
Contributor

@verb verb commented Jul 25, 2022

What type of PR is this?

/kind feature
/sig node
/priority important-longterm

What this PR does / why we need it:

This promotes the EphemeralContainers feature to GA, updates documentation and removes feature gate checks.

Which issue(s) this PR fixes:

Fixes #111030

Special notes for your reviewer:

The graduation criteria from KEP #277 are:

Notes:

Does this PR introduce a user-facing change?

[Ephemeral Containers](https://kubernetes.io/docs/concepts/workloads/pods/ephemeral-containers/) are now generally available. The `EphemeralContainers` feature gate is always enabled and should be removed from `--feature-gates` flag on the kube-apiserver and the kubelet command lines. The `EphemeralContainers` feature gate is [deprecated and scheduled for removal](https://kubernetes.io/docs/reference/using-api/deprecation-policy/#deprecation) in a future release.

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

- [KEP]: https://features.k8s.io/277

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. sig/node Categorizes an issue or PR as relevant to SIG Node. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 25, 2022
@verb
Copy link
Contributor Author

verb commented Jul 25, 2022

/triage accepted
/test all

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. area/code-generation area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/kubelet area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 25, 2022
@verb verb force-pushed the 111030-ec-ga branch 4 times, most recently from a7fbf51 to e7c8961 Compare July 25, 2022 19:07
@verb
Copy link
Contributor Author

verb commented Jul 25, 2022

/retest

@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 26, 2022
Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

On a cursory look, this LGTM. I'll have a closer look tomorrow, when it is not so late here :)

I'm assuming the e2e and integration tests are fine as they are today. Is that right?

@verb
Copy link
Contributor Author

verb commented Jul 26, 2022

On a cursory look, this LGTM. I'll have a closer look tomorrow, when it is not so late here :)

👍 Thanks!

I'm assuming the e2e and integration tests are fine as they are today. Is that right?

I think so. There's integration coverage for the API operations and e2e for ephemeral container creation that's part of NodeConformance.

I have some additional test work lined up:

If you have more suggestions I'm happy to add follow up items. I would try to avoid blocking this PR since test freeze is later than code freeze.

Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

This mostly LGTM. Did a more detailed pass now. I assume the questions I have asked will have the expected answer so this is probably is fine as it is :)

We should keep in mind that this should go in tandem with the validation fixes IIUC (no strong opinion if keeping them in separate PRs vs having it all in one).

Comment on lines 68 to +69
func AllFeatureEnabledContainers() ContainerType {
containerType := AllContainers
if !utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers) {
containerType &= ^EphemeralContainers
}
return containerType
return AllContainers
Copy link
Member

Choose a reason for hiding this comment

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

idem for the comment in the other definition of AllFeatureEnabledContainers()

pkg/features/kube_features.go Show resolved Hide resolved
containerType := AllContainers
if !utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers) {
containerType &= ^EphemeralContainers
}
return containerType
return AllContainers
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to keep this function now?

Is this a function that was added back then by ephemeralContainers or is it one of these abstractions that we always had "just in case" and we want to keep it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We added the container visitor during ephemeral containers because it tipped complexity of visiting all containers over a threshold. We can retire AllFeatureEnabledContainers() if we don't think we're ever going to add another type of container.

If we do it, I recommend decoupling from this feature graduation and do it early in the next dev cycle so we don't cause a bunch of conflicts for people racing the the merge window.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM :)

Copy link
Member

Choose a reason for hiding this comment

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

I'd probably keep the method, just in case, but agree that either way, it shouldn't be changed in this PR

@mrunalp
Copy link
Contributor

mrunalp commented Jul 28, 2022

/lgtm
/approve

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

mrunalp commented Jul 28, 2022

@dchen1107 ptal

@dchen1107
Copy link
Member

/lgtm
/approve

@verb
Copy link
Contributor Author

verb commented Jul 29, 2022

Hi Jordan, this should be ready for you now. Thanks!

/assign @liggitt

@liggitt liggitt added the api-review Categorizes an issue or PR as actively needing an API review. label Jul 29, 2022
Copy link
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

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

/approve

for API changes

containerType := AllContainers
if !utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers) {
containerType &= ^EphemeralContainers
}
return containerType
return AllContainers
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably keep the method, just in case, but agree that either way, it shouldn't be changed in this PR

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, liggitt, mrunalp, verb

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 Jul 29, 2022
@liggitt liggitt added this to the v1.25 milestone Jul 29, 2022
@k8s-ci-robot k8s-ci-robot merged commit cf2800b into kubernetes:master Jul 30, 2022
@sftim
Copy link
Contributor

sftim commented Aug 1, 2022

@verb:
For the extended release note, could we also link to https://kubernetes.io/docs/concepts/workloads/pods/ephemeral-containers/ ?

@sftim
Copy link
Contributor

sftim commented Aug 1, 2022

See #111194 for an example of a detailed release note

@verb verb deleted the 111030-ec-ga branch August 3, 2022 15:22
@verb
Copy link
Contributor Author

verb commented Aug 3, 2022

For the extended release note, could we also link to https://kubernetes.io/docs/concepts/workloads/pods/ephemeral-containers/ ?

@sftim 👍 Thanks for the example. I've updated the release note in the details of this PR.

tcdowney added a commit to cloudfoundry/korifi that referenced this pull request Jun 1, 2023
- Ephemeral Containers have been promoted to GA as of Kubernetes 1.27
  and this feature gate has been removed
  See: kubernetes/kubernetes#111402
- Having it set prevents Kind clusters from starting successfully with
versions >= 1.27

Authored-by: Tim Downey <[email protected]>
davewalter pushed a commit to cloudfoundry/korifi that referenced this pull request Jun 2, 2023
- Ephemeral Containers have been promoted to GA as of Kubernetes 1.27
  and this feature gate has been removed
  See: kubernetes/kubernetes#111402
- Having it set prevents Kind clusters from starting successfully with
versions >= 1.27

Authored-by: Tim Downey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/code-generation 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/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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. 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/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/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. 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. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Status: API review completed, 1.25
Archived in project
Development

Successfully merging this pull request may close these issues.

Promote Ephemeral Containers to GA
9 participants