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

Switch alpha Pod ephemeralcontainers API to use Pod kind #101034

Merged
merged 3 commits into from Apr 22, 2021

Conversation

@verb
Copy link
Contributor

@verb verb commented Apr 12, 2021

What type of PR is this?

/kind feature
/kind api-change
/sig auth
/priority important-longterm

What this PR does / why we need it:

This changes the /ephemeralcontainers subresource of /pods to use the Pod kind rather than EphemeralContainers.

When designing this API initially it seemed preferable to create a new kind containing only the pod's ephemeral containers, similar to how binding and scaling work.

It later became clear that this made admission control more difficult because the controller wouldn't be presented with the entire Pod, so we updated this to operate on the entire Pod, similar to how /status works.

This change is described in more detail in KEP-277 update kubernetes/enhancements#2618.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

The `pods/ephemeralcontainers` API now returns and expects a `Pod` object instead of `EphemeralContainers`. This is incompatible with the previous alpha-level API.

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

- [KEP]: https://git.k8s.io/enhancements/keps/sig-node/277-ephemeral-containers
@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Apr 12, 2021

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

@k8s-ci-robot k8s-ci-robot commented Apr 12, 2021

@verb: 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/test-infra repository.

@verb
Copy link
Contributor Author

@verb verb commented Apr 12, 2021

/test all

verb added 2 commits Apr 13, 2021
This changes the `/ephemeralcontainers` subresource of `/pods` to use
the `Pod` kind rather than `EphemeralContainers`.

When designing this API initially it seemed preferable to create a new
kind containing only the pod's ephemeral containers, similar to how
binding and scaling work.

It later became clear that this made admission control more difficult
because the controller wouldn't be presented with the entire Pod, so we
updated this to operate on the entire Pod, similar to how `/status`
works.
@fedebongio
Copy link
Contributor

@fedebongio fedebongio commented Apr 13, 2021

/remove-sig api-machinery
/cc @liggitt

pkg/registry/core/pod/strategy.go Outdated Show resolved Hide resolved
pkg/registry/core/pod/strategy.go Outdated Show resolved Hide resolved
@liggitt liggitt self-assigned this Apr 15, 2021
@liggitt liggitt added this to Assigned in API Reviews Apr 15, 2021
pkg/registry/core/pod/strategy.go Outdated Show resolved Hide resolved
@liggitt
Copy link
Member

@liggitt liggitt commented Apr 15, 2021

one comment, lgtm otherwise. should wait for kubernetes/enhancements#2618 before merging

@liggitt liggitt moved this from Assigned to API review completed, 1.22 in API Reviews Apr 15, 2021
@liggitt liggitt added this to the v1.22 milestone Apr 15, 2021
@fedebongio
Copy link
Contributor

@fedebongio fedebongio commented Apr 15, 2021

/remove-sig api-machinery

* Use deep copies in `PrepareForUpdate()`
* Preserve select metadata from new pod
* Use patch to add ephemeral container `kubectl debug`
* Distinguish between pod vs /ephemeralcontainers NotFound
@verb
Copy link
Contributor Author

@verb verb commented Apr 16, 2021

/hold
until kubernetes/enhancements#2618 merges

@verb
Copy link
Contributor Author

@verb verb commented Apr 16, 2021

/retest

@fedebongio
Copy link
Contributor

@fedebongio fedebongio commented Apr 20, 2021

/remove-sig api-machinery

@verb
Copy link
Contributor Author

@verb verb commented Apr 21, 2021

kubernetes/enhancements#2618 has merged
/remove-hold

@liggitt
Copy link
Member

@liggitt liggitt commented Apr 22, 2021

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Apr 22, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, 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 merged commit 972ee2d into kubernetes:master Apr 22, 2021
15 checks passed
@verb verb deleted the 1.22-ec-api branch Apr 22, 2021
BinacsLee added a commit to BinacsLee/kubernetes that referenced this issue Apr 22, 2021
Switch alpha Pod ephemeralcontainers API to use Pod kind
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment