-
Notifications
You must be signed in to change notification settings - Fork 185
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
cmd: Introduce runtime and k8s columns #2056
Conversation
fc522e2
to
74e8146
Compare
74e8146
to
2f8b97f
Compare
ebdb5df
to
0d5ed51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great. I don't know if having the K8s.
prefix in kubectl-gadget has been discussed, IMO that's good:
$ ./kubectl-gadget trace open
INFO[0000] Experimental features enabled
K8S.NODE K8S.NAMESPACE K8S.POD K8S.CONTAINER PID COMM FD ERR P
I didn't find anything to be fixed in the code, only some things that are missing:
- It's missing to update all gadgets guides (sorry, I now it's to annoying to do it, perhaps a good regexp to replace can help)
- Prometheus documentation should be updated as well
inspektor-gadget/docs/gadgets/prometheus.md
Lines 95 to 98 in 146bc5a
labels: - namespace - pod - container
(other places in same file)
btw, this will also fix #2066.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it and it works well, so thank you for this contribution:
root@minikube-containerd:/# ig list-containers -o custom-columns=runtime.containerid,runtime.containername,k8s.pod,k8s.namespace
RUNTIME.CONTAINE… RUNTIME.CONTAINERNAME K8S.POD K8S.NAMESPACE
0dbc2b236492024a… coredns coredns-6d4b75cb6d-6fz8w kube-system
d316f9ad44642ad2… etcd etcd-minikube-containerd kube-system
ceec8a2150ffb369… kindnet-cni kindnet-t7z5q kube-system
c90e7729f491ff46… kube-apiserver kube-apiserver-minikube-containerd kube-system
b9270e09b88d70ad… kube-controller-manager kube-controller-man…minikube-containerd kube-system
22f4ffe5fda3dfb7… kube-proxy kube-proxy-l8lvs kube-system
77ad2368722885b8… kube-scheduler kube-scheduler-minikube-containerd kube-system
fa55550caeb0b391… storage-provisioner storage-provisioner kube-system
root@minikube-containerd:/# ig trace open -o columns=runtime.containername,k8s.pod,k8s.namespace,pid,comm | head
RUNTIME.CONTAINERNAME K8S.POD K8S.NAMESPACE PID COMM
coredns coredns-6d4b75cb6d-6fz8w kube-system 22878 coredns
storage-provisioner storage-provisioner kube-system 23048 storage-provisi
coredns coredns-6d4b75cb6d-6fz8w kube-system 22878 coredns
coredns coredns-6d4b75cb6d-6fz8w kube-system 22878 coredns
coredns coredns-6d4b75cb6d-6fz8w kube-system 22878 coredns
kube-proxy kube-proxy-l8lvs kube-system 22562 kube-proxy
kube-proxy kube-proxy-l8lvs kube-system 22562 kube-proxy
kube-proxy kube-proxy-l8lvs kube-system 23370 iptables
kube-proxy kube-proxy-l8lvs kube-system 23370 iptables
I do not have a lot of comments, just address Mauricio comment and you can merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the approach of hiding the columns instead of filtering them. It should also make the implementation of the --k8s
and --runtime
flags easier. BTW, given that you will add that in a second PR, this one is not fully fixing #737.
LGTM after managing the comments.
I'm wondering about what's the idea there. Provide independent flags to enable the runtime and k8s columns? IMO we should avoid adding so many flags, what about having something like |
0d5ed51
to
fcfa5dd
Compare
Done. Agreed, it wasn't fun to do but I realized we were missing |
My initial idea was to add |
I changed the linked issue from "Fixes" to "Related" to make sure we do follow up PR.
Good idea. Let me start a discussion in the slack around this! |
Currently we are filtering 'k8s' columns for ig and 'runtime' columns for kubectl-gadget. This change makes the columns hidden rather then filtered to allow user to be able to apply filters and use them in custom column format with '-o columns=' Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>
Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>
Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>
fcfa5dd
to
769d176
Compare
Thanks for the reviews! |
#1702 added support for k8s metadata in JSON/YAML but the support for columns was still missing. This PR allows the same information to be available as columns as well.
Related: #737
Fixes: #2066
Idea
ig
andkubectl-gadget
would filterk8s
andruntime
columns respectively rather then hiding. So we will switch to marking them hidden as needed by each client.-o columns
as needed by the user.list-containers
needs an extra work butk8s
metadata can also be displayed using-o custom-columns
Tesing done