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

Add plugin: debug #248

Merged
merged 1 commit into from
Oct 21, 2019
Merged

Add plugin: debug #248

merged 1 commit into from
Oct 21, 2019

Conversation

verb
Copy link
Contributor

@verb verb commented Sep 23, 2019

This adds a new plugin for kubectl alpha-debug-pod, which attaches an ephemeral container to a running pod to be used for debugging.

This plugin relies on an alpha feature to be enabled in the cluster, and users should expect it to be unstable as the feature evolves. I wanted to emphasize this, so I chose a name with "alpha" in it. (Also I didn't want to steal @aylei's plugin name from #135.)

@k8s-ci-robot
Copy link
Contributor

Welcome @verb!

It looks like this is your first PR to kubernetes-sigs/krew-index 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/krew-index has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 23, 2019
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 23, 2019
@verb
Copy link
Contributor Author

verb commented Sep 23, 2019

/assign @ahmetb

Ahmet has a bit of a context here and I'd appreciate feedback on the naming.

The build that failed is using an old release of krew with an unsupported version of Go. I'm guessing that something changed in krew selector, though. Should I try to get this working with the old version?

@ahmetb
Copy link
Member

ahmetb commented Sep 23, 2019

/cc @corneliusweig I guess we need to update the CI bits to use newer krew.

@corneliusweig
Copy link
Contributor

/retest

Copy link
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

Hey, thanks for submitting this to krew-index, this looks pretty cool!

I think given that it requires to enable a cluster-side feature gate, it is a good idea to prefix it with alpha. I saw that the GitHub site for this plugin says

The functionality of this plugin should eventually be included in kubectl proper.

I'm not aware of the chances of this landing in kubectl, but if so, I'm more in favor of calling this alpha-debug instead of alpha-debug-pod. I would like to avoid a situation where we have a debug plugin in the index which is technically very different from an official kubectl debug command. However, I'm also no expert here and am not aware of the differences between the approach taken by @aylei and this plugin.

One more point to note: currently this project is quite lightweight regarding documentation. Would you care to add a little more?

Finally, let's wait for @ahmetb's feedback. As he's OOO, this might take a little.

plugins/alpha-debug-pod.yaml Outdated Show resolved Hide resolved
plugins/alpha-debug-pod.yaml Outdated Show resolved Hide resolved
@corneliusweig
Copy link
Contributor

Btw, if you push changes, the CI should pass now :)

@ahmetb
Copy link
Member

ahmetb commented Sep 26, 2019

I think given that it requires to enable a cluster-side feature gate, it is a good idea to prefix it with alpha.

If we call it kubectl debug, and it gets merged into kubectl, it will peacefully transition into using the builtin command due to shadowing of builtin subcommands.

I also assume the feature gate can be detected by the plugin? If so, the alpha- prefix isn't really necessary? We can also make a note of it.

On the other hand, releasing it alpha-debug and renaming it to debug later (because, say, KEP did not succeed) will NOT leave its users on a graceful transition path.

@verb
Copy link
Contributor Author

verb commented Sep 26, 2019

I think given that it requires to enable a cluster-side feature gate, it is a good idea to prefix it with alpha.

If we call it kubectl debug, and it gets merged into kubectl, it will peacefully transition into using the builtin command due to shadowing of builtin subcommands.

I really don't know whether to expect whether kubectl debug will be met with resistance or whether it will undergo a name change.

I also assume the feature gate can be detected by the plugin? If so, the alpha- prefix isn't really necessary? We can also make a note of it.

It can't, really. We can't query the feature gates of a cluster, so the plugin just tries to add a container and it may fail.

On the other hand, releasing it alpha-debug and renaming it to debug later (because, say, KEP did not succeed) will NOT leave its users on a graceful transition path.

naming is hard. :(

If there isn't good support around plugin renaming then maybe we should choose a long-term name. The plugin will continue to work even if the functionality becomes native to kubectl

Copy link
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

To me the manifest looks good already.


The naming is hard here!

So if I understood @ahmetb correctly you are saying:

  • debug + KEP -> users will transition seamlessly from plugin to kubectl native
  • debug + KEP -> users will continue using the plugin
  • alpha-debug + KEP -> users will have to switch to debug
  • alpha-debug + KEP -> users will be stuck with the alpha- command forever, because we cannot rename

This looks like the name debug is the better bet for now. However, what about the situation where this KEP for this debug implementation gets rejected, but another one gets through? Then our users would switch to a different command with possibly quite different behavior.

I'm completely unsure how likely such a scenario is. But the shorter command name has quite an allure to it. So how about this: We take kubectl-debug (without alpha) for now. If the KEP regarding this implementation gets rejected, we start a migration of the plugin away from the canonical name to something else and add a deprecation notice to the debug plugin (like "this plugin has been renamed to >ephemeral-debug<")

plugins/alpha-debug-pod.yaml Outdated Show resolved Hide resolved
@corneliusweig
Copy link
Contributor

Hey @ahmetb. Can you take another look? I think @verb is waiting for us here.

@ahmetb
Copy link
Member

ahmetb commented Oct 7, 2019

@corneliusweig I'm mostly waiting for it to be renamed. Feel free to ping me again. Per my/your comment above, alpha- prefix isn't really a good idea.

@ahmetb
Copy link
Member

ahmetb commented Oct 16, 2019

Ping @verb you can rename the name field and the file to either debug or debug-pod –you're free to use either.

@verb verb changed the title Add krew plugin for alpha-debug-pod Add krew plugin for kubectl debug Oct 21, 2019
@verb
Copy link
Contributor Author

verb commented Oct 21, 2019

@ahmetb @corneliusweig renamed!

@ahmetb
Copy link
Member

ahmetb commented Oct 21, 2019

/retitle Add plugin: debug

Note: We're making an exception for a generic name because this command has been in the KEP process for a while and has a potential path to graduation, which in case, the builtin would shadow the plugin name gracefully.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot changed the title Add krew plugin for kubectl debug Add plugin: debug Oct 21, 2019
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, 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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2019
@k8s-ci-robot k8s-ci-robot merged commit d9b5180 into kubernetes-sigs:master Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants