Promote EphemeralContainers feature to GA #111402

verb · 2022-07-25T15:43:54Z

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:

Ephemeral Containers have been in beta for at least 2 releases.
Ephemeral Containers see use in 3 projects or articles.
Ephemeral Container creation is covered by conformance tests.
- Node conformance added in Add Ephemeral Containers to node conformance tests #111404
- project-wide conformance follow-up in Promote Ephemeral Containers e2e test to Conformance #111440
Fix: kubectl incorrectly suggests a debug container can be reattached after exit
- Addressed in Fix incorrect message when attaching to ephemeral containers #111185
Fix: validateEphemeralContainers adds a superfluous index to ephemeral container spec paths
- Addressed in Improve tests and fix bugs in container validation #111401

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 · 2022-07-25T15:43:55Z

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

verb · 2022-07-25T15:44:29Z

/triage accepted
/test all

verb · 2022-07-25T19:07:47Z

/retest

rata

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 · 2022-07-26T18:16:09Z

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.

rata

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).

rata · 2022-07-27T14:49:03Z

pkg/api/v1/pod/util.go

 func AllFeatureEnabledContainers() ContainerType {
-	containerType := AllContainers
-	if !utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers) {
-		containerType &= ^EphemeralContainers
-	}
-	return containerType
+	return AllContainers


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

pkg/features/kube_features.go

test/integration/apiserver/admissionwebhook/admission_test.go

rata · 2022-07-27T14:55:35Z

pkg/api/pod/util.go

-	containerType := AllContainers
-	if !utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers) {
-		containerType &= ^EphemeralContainers
-	}
-	return containerType
+	return AllContainers


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?

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.

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

mrunalp · 2022-07-28T18:29:17Z

/lgtm
/approve

mrunalp · 2022-07-28T18:29:46Z

@dchen1107 ptal

dchen1107 · 2022-07-28T23:44:41Z

/lgtm
/approve

verb · 2022-07-29T05:57:40Z

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

/assign @liggitt

liggitt

/approve

for API changes

liggitt · 2022-07-29T19:00:57Z

pkg/api/pod/util.go

-	containerType := AllContainers
-	if !utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers) {
-		containerType &= ^EphemeralContainers
-	}
-	return containerType
+	return AllContainers


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 · 2022-07-29T19:08:46Z

[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:

~~api/OWNERS~~ [liggitt]
~~pkg/api/OWNERS~~ [liggitt]
~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/features/OWNERS~~ [dchen1107,liggitt]
~~pkg/generated/openapi/OWNERS~~ [dchen1107,liggitt]
~~pkg/kubelet/OWNERS~~ [dchen1107,liggitt,mrunalp]
~~pkg/registry/OWNERS~~ [dchen1107,liggitt]
~~pkg/volume/OWNERS~~ [dchen1107,liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~test/e2e/common/OWNERS~~ [dchen1107,liggitt]
~~test/e2e/framework/OWNERS~~ [dchen1107,liggitt]
~~test/integration/apiserver/OWNERS~~ [dchen1107,liggitt]
~~test/integration/pods/OWNERS~~ [dchen1107,liggitt]

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

sftim · 2022-08-01T18:06:32Z

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

sftim · 2022-08-01T18:07:30Z

See #111194 for an example of a detailed release note

verb · 2022-08-03T17:12:24Z

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.

- 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 <tdowney@vmware.com>

k8s-ci-robot requested review from caesarxuchao and dchen1107 July 25, 2022 15:45

verb mentioned this pull request Jul 25, 2022

Ephemeral Containers kubernetes/enhancements#277

Closed

23 tasks

verb force-pushed the 111030-ec-ga branch 4 times, most recently from a7fbf51 to e7c8961 Compare July 25, 2022 19:07

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2022

rata reviewed Jul 26, 2022

View reviewed changes

verb mentioned this pull request Jul 27, 2022

Promote Ephemeral Containers e2e test to Conformance #111440

Merged

rata reviewed Jul 27, 2022

View reviewed changes

k8s-ci-robot assigned mrunalp Jul 28, 2022

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

k8s-ci-robot assigned liggitt Jul 29, 2022

verb mentioned this pull request Jul 29, 2022

Mark EphemeralContainers as GA in 1.25 kubernetes/website#35545

Merged

2 tasks

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

liggitt added this to Assigned in API Reviews Jul 29, 2022

liggitt reviewed Jul 29, 2022

View reviewed changes

liggitt moved this from Assigned to API review completed, 1.25 in API Reviews Jul 29, 2022

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 added this to the v1.25 milestone Jul 29, 2022

k8s-ci-robot merged commit cf2800b into kubernetes:master Jul 30, 2022

SIG Node CI/Test Board automation moved this from Archive-it to Done Jul 30, 2022

SIG Node PR Triage automation moved this from Triage to Done Jul 30, 2022

verb deleted the 111030-ec-ga branch August 3, 2022 15:22

SataQiu mentioned this pull request Dec 11, 2022

Remove GA feature gates: CSIInlineVolume, CSIMigration, DaemonSetUpdateSurge, EphemeralContainers, IdentifyPodOS, LocalStorageCapacityIsolation, NetworkPolicyEndPort, StatefulSetMinReadySeconds #114410

Merged

tcdowney mentioned this pull request Jun 1, 2023

Remove EphemeralContainers feature gate from Kind config cloudfoundry/korifi#2564

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promote EphemeralContainers feature to GA #111402

Promote EphemeralContainers feature to GA #111402

verb commented Jul 25, 2022 •

edited

k8s-ci-robot commented Jul 25, 2022

verb commented Jul 25, 2022

verb commented Jul 25, 2022

rata left a comment

verb commented Jul 26, 2022

rata left a comment

rata Jul 27, 2022

rata Jul 27, 2022

verb Jul 28, 2022

rata Jul 28, 2022

liggitt Jul 29, 2022

mrunalp commented Jul 28, 2022

mrunalp commented Jul 28, 2022

dchen1107 commented Jul 28, 2022

verb commented Jul 29, 2022

liggitt left a comment

liggitt Jul 29, 2022

k8s-ci-robot commented Jul 29, 2022

sftim commented Aug 1, 2022

sftim commented Aug 1, 2022

verb commented Aug 3, 2022

Promote EphemeralContainers feature to GA #111402

Promote EphemeralContainers feature to GA #111402

Conversation

verb commented Jul 25, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Jul 25, 2022

verb commented Jul 25, 2022

verb commented Jul 25, 2022

rata left a comment

Choose a reason for hiding this comment

verb commented Jul 26, 2022

rata left a comment

Choose a reason for hiding this comment

rata Jul 27, 2022

Choose a reason for hiding this comment

rata Jul 27, 2022

Choose a reason for hiding this comment

verb Jul 28, 2022

Choose a reason for hiding this comment

rata Jul 28, 2022

Choose a reason for hiding this comment

liggitt Jul 29, 2022

Choose a reason for hiding this comment

mrunalp commented Jul 28, 2022

mrunalp commented Jul 28, 2022

dchen1107 commented Jul 28, 2022

verb commented Jul 29, 2022

liggitt left a comment

Choose a reason for hiding this comment

liggitt Jul 29, 2022

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 29, 2022

sftim commented Aug 1, 2022

sftim commented Aug 1, 2022

verb commented Aug 3, 2022

verb commented Jul 25, 2022 •

edited