WIP: Update Kubernetes API to support Debug Containers #46243
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: verb Assign the PR to them by writing
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
needs-rebase
pkg/registry/core/pod/strategy.go
Outdated
| loc := &url.URL{ | ||
| Scheme: nodeInfo.Scheme, | ||
| Host: net.JoinHostPort(nodeInfo.Hostname, nodeInfo.Port), | ||
| Path: fmt.Sprintf("/podDebug/%s/%s/%s", pod.Namespace, pod.Name, opts.Container), |
There was a problem hiding this comment.
most of the other two word kubelet paths use mixed case (e.g. portForward, containerLogs), but then there's "runningpods" so perhaps that's an old convention.
There was a problem hiding this comment.
Yeah, I think we want to stop using camel cased paths
There was a problem hiding this comment.
ok, thanks. I've made the change and it will appear when I push a rebase
|
One thing I wanted to ask about: the kubelet needs a v1.Container to create the debug container. I think it would be ideal if the client could post a v1.Container to the /debug subresource rather than passing individual fields via http parameters. That's possible to do prior to upgrading the connection, but it doesn't seem well supported by the api machinery and I'm not sure how well that fits in kubernetes API conventions. Advice would be much appreciated. |
verb
changed the title
|
Update: I'm still working on this. I've reworked the API and will update this PR in the next week or so. |
k8s-github-robot
added
size/M
verb
force-pushed
the
pod-tshoot-api-1
branch
from
54dfc6d to
8a12cdf
Compare
k8s-ci-robot
added
size/L
k8s-github-robot
removed
the
needs-rebase
|
@verb: Your pull request title starts with "WIP", so the do-not-merge/work-in-progress label will be added. This label will ensure that your pull request will not be merged. Remove the prefix from your pull request title to trigger the removal of the label and allow for your pull request to be merged. 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. |
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
/sig node |
k8s-ci-robot
added
the
sig/node
verb
force-pushed
the
pod-tshoot-api-1
branch
from
8a12cdf to
27cdca3
Compare
verb
force-pushed
the
pod-tshoot-api-1
branch
from
27cdca3 to
d38b622
Compare
|
@verb: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
@verb PR needs rebase |
k8s-github-robot
added
the
needs-rebase
|
this work will require us to implement this first, correct? will we implement the above before this pr lands? |
|
@derekwaynecarr It's not necessary because the features are only loosely coupled. The code I already have uses the existing, kubelet-flagged shared PID support. We can order them if you'd like, though. I consider Shared PID more urgent, but I'm not sure how long the review of kubernetes/community#1048 will take. |
|
We're not going to get consensus here any time soon. Will reopen in the future if we figure things out. |
What this PR does / why we need it: Debug Containers were proposed in kubernetes/community#649 and are to be implemented mostly in the kubelet. This change proposes changes to the Kubernetes API to allow creating a Debug Container via the existing exec subresource.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
Release note:
Dependencies: