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

Kubectl exec support resource/name format #73664

Merged
merged 1 commit into from Apr 27, 2019

Conversation

@prksu
Copy link
Member

commented Feb 2, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:

Add support kubectl to execute container from pod selected by resource/name.

Which issue(s) this PR fixes:

Fixes #72104

Special notes for your reviewer:

cc @soltysh

like what port-forward does, i'm using AttachablePodForObject to select a pod using resource name. but I resolve the pod from the resource name in the Run() method. because if it's on the Complete() method this makes panic when retrieving the object from builder.Do().Object() when do a test. this can be compiled correctly and running properly. it's just that when testing it causes panic.

Does this PR introduce a user-facing change?:

kubectl exec now allows using resource name (e.g., deployment/mydeployment) to select a matching pod.
kubectl exec now allows using --pod-running-timeout flag to wait till at least one pod is running.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2019

Hi @prksu. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@neolit123
Copy link
Member

left a comment

/priority important-longterm

@soltysh
Copy link
Contributor

left a comment

I left you some comments, additionally I'd like to see a test (can be test-cmd) verifying that both the old way of specifying just pod name and the new with resource/name work equally well.

pkg/kubectl/cmd/exec/exec.go Outdated Show resolved Hide resolved
pkg/kubectl/cmd/exec/exec.go Outdated Show resolved Hide resolved
pkg/kubectl/cmd/exec/exec.go Outdated Show resolved Hide resolved
pkg/kubectl/cmd/exec/exec.go Outdated Show resolved Hide resolved
pkg/kubectl/cmd/exec/exec.go Outdated Show resolved Hide resolved
pkg/kubectl/cmd/exec/exec.go Outdated Show resolved Hide resolved

@soltysh soltysh self-assigned this Feb 4, 2019

@prksu prksu force-pushed the prksu:kubectl-exec-resource-name branch from b353e0a to 0384e01 Feb 5, 2019

@prksu prksu force-pushed the prksu:kubectl-exec-resource-name branch from 0384e01 to 42b5587 Feb 5, 2019

@prksu

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

@soltysh updated, and i make a test-cmd script to verifying both ways work as well. PTAL

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

@prksu: Those labels are not set on the issue: sig/apps

In response to this:

/remove-sig apps

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

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

@prksu: Those labels are not set on the issue: sig/testing

In response to this:

/remove-sig testing

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

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

@prksu: Those labels are not set on the issue: sig/cloud-provider

In response to this:

/remove-sig cloud-provider

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.

@prksu

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

/retest

@dvdbot

This comment has been minimized.

Copy link

commented Apr 18, 2019

Sorry to mix in but when can this be expected to be released? This would make it so easy to get into pods :D
Currently just getting the pod's name and using that but it's so cumbersome

pkg/kubectl/cmd/exec/exec.go Outdated Show resolved Hide resolved
pkg/kubectl/cmd/exec/exec.go Outdated Show resolved Hide resolved
pkg/kubectl/cmd/exec/exec.go Outdated Show resolved Hide resolved
pkg/kubectl/cmd/exec/exec.go Show resolved Hide resolved
pkg/kubectl/cmd/exec/exec.go Outdated Show resolved Hide resolved
pkg/kubectl/cmd/exec/exec.go Outdated Show resolved Hide resolved
pkg/kubectl/cmd/exec/exec.go Outdated Show resolved Hide resolved

@prksu prksu force-pushed the prksu:kubectl-exec-resource-name branch 2 times, most recently from e6a3394 to 3b406c8 Apr 20, 2019

test/cmd/exec.sh Show resolved Hide resolved
pkg/kubectl/cmd/exec/exec.go Outdated Show resolved Hide resolved

@kargakis kargakis requested a review from soltysh Apr 25, 2019

@kargakis

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

/lgtm
/hold cancel

@kubernetes/sig-cli-pr-reviews needs an approval

@k8s-ci-robot k8s-ci-robot added lgtm and removed do-not-merge/hold labels Apr 25, 2019

@prksu prksu force-pushed the prksu:kubectl-exec-resource-name branch from 3b406c8 to 0c39d7d Apr 25, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Apr 25, 2019

@kargakis

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 25, 2019

@janetkuo

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo, kargakis, prksu

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 e5dd452 into kubernetes:master Apr 27, 2019

20 checks passed

cla/linuxfoundation prksu authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@prksu prksu deleted the prksu:kubectl-exec-resource-name branch Apr 27, 2019

@kfox1111

This comment has been minimized.

Copy link

commented Apr 30, 2019

This is great. Thanks. :)

Any chance that the non interactive mode can run the command on all matching pods rather then just the first one?

@prksu

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

This is great. Thanks. :)

Any chance that the non interactive mode can run the command on all matching pods rather then just the first one?

you mean bulk exec? unfortunately i think kubectl exec doesn't support that. you can open a new issue with that topic with the feature request format.

or you can implement this as kubectl plugin. like https://github.com/txn2/kubefwd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.