WIP: Create a new kubectl command to attach debug container to a running pod #46256

verb · 2017-05-22T23:54:46Z

What this PR does / why we need it: This is the client command to attach a debug container in a running pod, as proposed in kubernetes/community#649

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #45922

Special notes for your reviewer: This PR depends on kubectl feature gates #46151

Release note:

NONE

Dependencies:

Add alpha command to kubectl #46151 Alpha feature gating for kubectl

WIP:

Need decision from api-machinery on API endpoint

k8s-github-robot · 2017-05-22T23:59:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: verb
We suggest the following additional approver: brendandburns

Assign the PR to them by writing /assign @brendandburns in a comment when ready.

Needs approval from an approver in each of these OWNERS Files:

pkg/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

verb · 2017-05-23T00:22:02Z

/cc @pwittrock

alexandercampbell · 2017-05-25T22:38:27Z

pkg/kubectl/cmd/cmd.go

@@ -252,6 +255,12 @@ var (

 // NewKubectlCommand creates the `kubectl` command and its nested children.
 func NewKubectlCommand(f cmdutil.Factory, in io.Reader, out, err io.Writer) *cobra.Command {
+	if f := os.Getenv("KUBECTL_FEATURE_GATES"); f != "" {
+		if e := utilfeature.DefaultFeatureGate.Set(f); e != nil {
+			fmt.Fprintf(err, "Invalid KUBECTL_FEATURE_GATES %q. Available features:\n%s\n", f, strings.Join(utilfeature.DefaultFeatureGate.KnownFeatures(), "\n"))


Maybe do strings.Join with "\n\t" instead of just "\n"?

alexandercampbell · 2017-05-25T22:41:35Z

pkg/kubectl/cmd/templates/command_groups.go

+		}
+		g[i].Commands = gc
+	}
+}


This means that kubectl help debug will fail to print the documentation if the feature is disabled by the environment.
I assume this is what you want, but what if it instead printed information about how to enable the command?

Yup, that's what I intended here, but what you describe could definitely be a better user experience. I'll try to figure out something for this in #46151

alexandercampbell · 2017-05-25T22:42:15Z

pkg/kubectl/cmd/debug.go

+	cmd := &cobra.Command{
+		Use:     "debug POD [-c CONTAINER] -- COMMAND [args...]",
+		Short:   i18n.T("Run a debug container in a pod"),
+		Long:    "Run a new container inside an existing pod.",


I feel like the Long help message should also include the word "debug".

alexandercampbell · 2017-05-25T22:43:34Z

pkg/kubectl/cmd/debug.go

+		return fmt.Errorf("both output and error output must be provided")
+	}
+	if p.Debugger == nil || p.PodClient == nil || p.Config == nil {
+		return fmt.Errorf("client, client config, and debugutor must be provided")


What is a debugutor?

a find/replace fail...

Fixed, thanks.

alexandercampbell · 2017-05-25T22:44:07Z

pkg/kubectl/cmd/debug.go

+		return err
+	}
+
+	return nil


Just do

return t.Safe(fn)

Fixed, thanks.

pwittrock · 2017-05-26T16:55:57Z

pkg/kubectl/cmd/debug.go

+)
+
+func NewCmdDebug(f cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer) *cobra.Command {
+	if !utilfeature.DefaultFeatureGate.Enabled(features.DebugContainers) {


Having kubectl depend on Kubernetes repo is an anti pattern. Don't do this. It either needs to go in one of the staging repos (client-go, etc) or be copied into the kubectl packages

@sttts mentioned wanting to use a separate feature gate in #46151 (comment), I imagine this must be why. Can kubectl depend on apiserver?

I have little preference on how to hide alpha features, so I need guidance from those who do. I mention some trade-offs here: #45922 (comment)

pwittrock · 2017-05-26T16:56:12Z

pkg/kubectl/cmd/debug.go

@@ -0,0 +1,231 @@
+/*
+Copyright 2014 The Kubernetes Authors.


pwittrock · 2017-05-26T16:58:26Z

pkg/kubectl/cmd/debug.go

+	utilfeature "k8s.io/apiserver/pkg/util/feature"
+	restclient "k8s.io/client-go/rest"
+	"k8s.io/client-go/tools/remotecommand"
+	"k8s.io/kubernetes/pkg/api"


New imports from k8s.io/kubernetes are forbidden

pwittrock · 2017-05-26T16:59:14Z

pkg/kubectl/cmd/debug.go

+	"k8s.io/kubernetes/pkg/features"
+	"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
+	cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
+	"k8s.io/kubernetes/pkg/util/i18n"


this one too

pwittrock · 2017-05-26T17:54:40Z

Hm. I don't want to block this functionality on kubectl deps issues which are hard to address, but don't want to create a bunch of additional work to pull them out.

If we accept the deps for this PR, will you remove the deps after code freeze?

verb · 2017-05-26T19:47:58Z

@pwittrock yesterday there arose some disagreement in api-machinery about this API change, so I'm happy to take a little more time to get deps/alpha gate right and miss the 1.7 deadline. I'm writing up the suggested API change and would value your opinion on it.

pwittrock · 2017-05-26T20:38:31Z

@verb great. that takes the pressure off

alexandercampbell · 2017-06-13T18:26:17Z

@verb is this ready for re-review?

verb · 2017-06-14T10:30:10Z

@alexandercampbell Not quite yet, I need resolution from api-machinery about naming the api endpoint.

pwittrock · 2017-07-11T02:03:42Z

/unassign

kubectl debug runs a debug container in a currently running pod.

k8s-ci-robot · 2017-07-14T14:28:43Z

@verb: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`a0e6333`	link	`/test pull-kubernetes-e2e-kops-aws`
pull-kubernetes-kubemark-e2e-gce	`a0e6333`	link	`/test pull-kubernetes-kubemark-e2e-gce`
pull-kubernetes-federation-e2e-gce	`a0e6333`	link	`/test pull-kubernetes-federation-e2e-gce`
pull-kubernetes-e2e-gce-etcd3	`a0e6333`	link	`/test pull-kubernetes-e2e-gce-etcd3`
pull-kubernetes-unit	`a0e6333`	link	`/test pull-kubernetes-unit`
pull-kubernetes-bazel	`a0e6333`	link	`/test pull-kubernetes-bazel`
pull-kubernetes-verify	`a0e6333`	link	`/test pull-kubernetes-verify`

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.

pwittrock · 2017-07-18T01:22:36Z

/unassign

alexandercampbell · 2017-08-17T16:06:13Z

@verb close for now and re-open when the apimachinery question is resolved?

verb · 2017-08-18T11:14:53Z

@alexandercampbell SGTM. Thanks for your patience.

verb · 2017-10-02T18:43:19Z

@r2d4 FYI, this is the example kubectl debug PR. It doesn't have the alpha integration, but you can see where it would have gone here: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/alpha.go#L39

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 22, 2017

k8s-github-robot assigned rootfs and ghodss May 22, 2017

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels May 22, 2017

k8s-ci-robot requested a review from pwittrock May 23, 2017 00:22

verb force-pushed the pod-tshoot-kubectl-1 branch from 7756e9a to 0f820d4 Compare May 23, 2017 23:51

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2017

pwittrock assigned droot and alexandercampbell May 24, 2017

alexandercampbell reviewed May 25, 2017

View reviewed changes

verb changed the title ~~Create a new kubectl command to attach debug container to a running pod~~ WIP: Create a new kubectl command to attach debug container to a running pod May 25, 2017

pwittrock reviewed May 26, 2017

View reviewed changes

pkg/kubectl/cmd/debug.go Outdated

@@ -0,0 +1,231 @@

/*

Copyright 2014 The Kubernetes Authors.

Copy link

Member

pwittrock May 26, 2017

Choose a reason for hiding this comment

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

copyright

pwittrock reviewed May 26, 2017

View reviewed changes

xiangpengzhao mentioned this pull request May 30, 2017

sidecar exec #27464

Closed

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 2, 2017

verb force-pushed the pod-tshoot-kubectl-1 branch from 0f820d4 to 26e1125 Compare June 2, 2017 02:45

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 2, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2017

Add new command kubectl debug

a0e6333

kubectl debug runs a debug container in a currently running pod.

verb force-pushed the pod-tshoot-kubectl-1 branch from 26e1125 to a0e6333 Compare July 14, 2017 13:49

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2017

verb closed this Aug 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Create a new kubectl command to attach debug container to a running pod #46256

WIP: Create a new kubectl command to attach debug container to a running pod #46256

verb commented May 22, 2017 •

edited

Loading

k8s-github-robot commented May 22, 2017

verb commented May 23, 2017

alexandercampbell May 25, 2017

alexandercampbell May 25, 2017

verb May 31, 2017

alexandercampbell May 25, 2017

alexandercampbell May 25, 2017

verb May 31, 2017

alexandercampbell May 25, 2017

verb May 31, 2017

pwittrock May 26, 2017

verb May 26, 2017

pwittrock May 26, 2017

pwittrock May 26, 2017

pwittrock May 26, 2017

pwittrock commented May 26, 2017

verb commented May 26, 2017 •

edited

Loading

pwittrock commented May 26, 2017

alexandercampbell commented Jun 13, 2017

verb commented Jun 14, 2017

pwittrock commented Jul 11, 2017

k8s-ci-robot commented Jul 14, 2017

pwittrock commented Jul 18, 2017

alexandercampbell commented Aug 17, 2017

verb commented Aug 18, 2017

verb commented Oct 2, 2017

WIP: Create a new kubectl command to attach debug container to a running pod #46256

WIP: Create a new kubectl command to attach debug container to a running pod #46256

Conversation

verb commented May 22, 2017 • edited Loading

k8s-github-robot commented May 22, 2017

verb commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented May 26, 2017

verb commented May 26, 2017 • edited Loading

pwittrock commented May 26, 2017

alexandercampbell commented Jun 13, 2017

verb commented Jun 14, 2017

pwittrock commented Jul 11, 2017

k8s-ci-robot commented Jul 14, 2017

pwittrock commented Jul 18, 2017

alexandercampbell commented Aug 17, 2017

verb commented Aug 18, 2017

verb commented Oct 2, 2017

verb commented May 22, 2017 •

edited

Loading

verb commented May 26, 2017 •

edited

Loading