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

plugins: add tail (github.com/boz/kail) #150

Merged
merged 1 commit into from May 29, 2019
Merged

plugins: add tail (github.com/boz/kail) #150

merged 1 commit into from May 29, 2019

Conversation

boz
Copy link
Contributor

@boz boz commented May 16, 2019

see boz/kail#34

Checklist for plugin developers:

  • Read the Plugin Naming Guide (for new plugins)
  • Verify the installation from URL or a local archive works (kubectl krew install --manifest=[...] --archive=[...])

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 16, 2019
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 16, 2019
@boz
Copy link
Contributor Author

boz commented May 16, 2019

/assign @juanvallejo

@ahmetb
Copy link
Member

ahmetb commented May 16, 2019

/assign

looks like -h is failing for me on macOS

kubectl tail -h
usage: kubectl-tail [<flags>] <command> [<args> ...]

Tail for kubernetes pods

Flags:
  -h, --help                  Show context-sensitive help (also try --help-long and --help-man).
      --ignore=SELECTOR ...   ignore selector
  -l, --label=SELECTOR ...    label
  -p, --pod=NAME ...          pod
  -n, --ns=NAME ...           namespace
      --ignore-ns=NAME ...    ignore namespace
      --svc=NAME ...          service
      --rc=NAME ...           replication controller
      --rs=NAME ...           replica set
      --ds=NAME ...           daemonset
  -d, --deploy=NAME ...       deployment
      --node=NAME ...         node
      --ing=NAME ...          ingress
      --context=CONTEXT-NAME  kubernetes context
      --current-ns            use namespace from current context
  -c, --containers=NAME ...   containers
      --dry-run               print matching pods and exit
      --log-file=/dev/stderr  log file output
      --log-level=error       log level
      --since=DURATION        Display logs generated since given duration, like 5s, 2m, 1.5h or 2h45m. Defaults to 1s.panic: template: usage:30:53: executing "usage" at <
>: write /dev/stderr: resource temporarily unavailable

goroutine 1 [running]:
github.com/boz/kail/vendor/gopkg.in/alecthomas/kingpin%2ev2.(*Application).writeUsage(0xc420276000, 0xc42010b680, 0x0, 0x0)
	/home/travis/gopath/src/github.com/boz/kail/vendor/gopkg.in/alecthomas/kingpin.v2/app.go:235 +0x11b
github.com/boz/kail/vendor/gopkg.in/alecthomas/kingpin%2ev2.(*Application).maybeHelp(0xc420276000, 0xc42010b680)
	/home/travis/gopath/src/github.com/boz/kail/vendor/gopkg.in/alecthomas/kingpin.v2/app.go:249 +0xd0
github.com/boz/kail/vendor/gopkg.in/alecthomas/kingpin%2ev2.(*Application).Parse(0xc420276000, 0xc4200be010, 0x1, 0x1, 0xc420276000, 0x1d2e41b, 0x7, 0x1d3a53b)
	/home/travis/gopath/src/github.com/boz/kail/vendor/gopkg.in/alecthomas/kingpin.v2/app.go:213 +0x18a
github.com/boz/kail/vendor/gopkg.in/alecthomas/kingpin%2ev2.Parse(0x1d2e41b, 0x7)
	/home/travis/gopath/src/github.com/boz/kail/vendor/gopkg.in/alecthomas/kingpin.v2/global.go:37 +0x77
main.main()
	/home/travis/gopath/src/github.com/boz/kail/cmd/kail/main.go:97 +0x121

@boz
Copy link
Contributor Author

boz commented May 16, 2019

Thanks @ahmetb. Fixed in v0.9.1.

/assign @ahmetb

@ahmetb
Copy link
Member

ahmetb commented May 16, 2019

/cc @corneliusweig

@k8s-ci-robot
Copy link
Contributor

@ahmetb: GitHub didn't allow me to request PR reviews from the following users: corneliusweig.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @corneliusweig

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.

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.

Thanks for your contribution!

Please also add a caveats field. It is shown post-install which gives you the opportunity to show

  • how your tool is invoked
  • where to find usage instructions

When I run tail (linux,amd64) and end it with Ctrl+C, it gives the following error:

E0517 11:42:07.325821   20111 streamwatcher.go:109] Unable to decode an event from the watch stream: context canceled

Is that expected behavior?

plugins/tail.yml Outdated Show resolved Hide resolved
plugins/tail.yml Outdated Show resolved Hide resolved
plugins/tail.yml Outdated Show resolved Hide resolved
plugins/tail.yml Outdated Show resolved Hide resolved
@boz
Copy link
Contributor Author

boz commented May 17, 2019

Thanks for the feedback @corneliusweig. I'll address these things when I have a chance.

E0517 11:42:07.325821 20111 streamwatcher.go:109] Unable to decode an event from the watch stream: context canceled
Is that expected behavior?

It's from glog somewhere inside k8s.io/{client-go,apimachinery,etc...}. I tried to suppress these but apparently they're still showing up.

I don't see them when I run kail by itself. Is kubectl adding glog configuration? Does your environment have glog settings?

Anyways... it's somewhat expected because a context is indeed cancelled to shut things down, but of course ideally the message wouldn't be shown 🤷‍♂️

@corneliusweig
Copy link
Contributor

@boz I haven't checked where exactly the error is logged. But if you have it under control, do you already do a check like

if err ! = nil && ctx.Err() == context.Cancelled { return nil } 

(beware, typed on phone)

@ahmetb
Copy link
Member

ahmetb commented May 17, 2019

If you’re seeing some weird behavior when process ran as a plugin vs standalone, this is totally worth filing a bug to kubectl.
Plugins are executed with execve(2) syscall, which would replace the kubectl process with your executable, so it shouldn't behave differently than running your executable standalone.

@boz
Copy link
Contributor Author

boz commented May 17, 2019

@boz I haven't checked where exactly the error is logged. But if you have it under control, do you already do a check like
[snip]

I don't use glog. I'm... not a fan ;)

[snip]
Plugins are executed with execve(2) syscall, which would replace the kubectl process with your executable, so it shouldn't behave differently than running your executable standalone.

I don't know of any other way of executing new programs. It could still be that kubectl is adding environment variables that govern glog behavior.

It could be a lot of things, tho - including recent changes to kail. I'll look into it when I have a chance. I'm also in the middle of bumping the k8s lib dependencies, which might resolve the issue.

@corneliusweig, @ahmetb: thanks for the help!

@ahmetb
Copy link
Member

ahmetb commented May 17, 2019

OK no rush. Let’s hold until the issue is fixed.
/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 17, 2019
@boz
Copy link
Contributor Author

boz commented May 18, 2019

Looks like glog has been removed from k8s.io libs 🎊 🎊 🎊

I've updated the various manifest fields and bumped to a version of kail that uses up-to-date k8s.io libs. Let me know what you guys think.

@corneliusweig
Copy link
Contributor

Looks like some examples in the caveats section are duplicated. I think three are fine anyway.

@boz
Copy link
Contributor Author

boz commented May 28, 2019

bump @ahmetb

@ahmetb
Copy link
Member

ahmetb commented May 28, 2019

Sorry just got back from kubecon!

The only problem I'm seeing so far is a discrepancy with the potential style guide we'll be adding (#152): kail uses --ns flag whereas the recommended Kubernetes guideline is to use -n/--namespace (via git.k8s.io/cli-runtime).

But I think we can make an exception since the tool was existing before and has a broad user base that is not easy to break. @corneliusweig any input?

@boz
Copy link
Contributor Author

boz commented May 28, 2019

Right on.

FWIW -n NAMESPACE works and --namespace NAMESPACE can be added. Note that the default is all namespaces sans kube-system; this can be overridden with --current-ns or KAIL_CURRENT_NS=true.

If it's a huge problem, adding cmd/kubectl-tail next to cmd/kail that conforms to some standard set of flags/behaviors is doable but something I'd like to avoid if at all possible.

@ahmetb
Copy link
Member

ahmetb commented May 29, 2019

SG. I guess you can add —namespace later.
/lgtm
/approve

We don’t strictly enforce the flags etc for now. So I think we can have this.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, boz

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 May 29, 2019
@ahmetb
Copy link
Member

ahmetb commented May 29, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 29, 2019
@k8s-ci-robot k8s-ci-robot merged commit 2ea62f0 into kubernetes-sigs:master May 29, 2019
@corneliusweig
Copy link
Contributor

Sorry, for being so slow to respond.

I think it would be a great idea to also add --namespace. Having --ns in addition won't hurt anyone, but conforming to the k8s standard -n/--namespace is in general a good idea. I opened boz/kail#38 so it is tracked.

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.

None yet

5 participants