Show pods number when describing services #8778
Conversation
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain @ixdy. |
|
LGTM, but I am not core UX team. |
|
|
||
| return describeService(service, endpoints, events) | ||
| return describeService(service, endpoints, events, pods.Items) |
There was a problem hiding this comment.
We're already fetching endpoints. Why not just count that instead?
There was a problem hiding this comment.
Endpoints has the additional advantages that dead and not-ready pods are filtered out, and that it would work for manually populated Endpoints (for services w/o selectors).
|
@Kargakis Maybe some example output will make this slightly easier to review? I'm thinking the output of |
| @@ -477,8 +477,9 @@ func (d *ServiceDescriber) Describe(namespace, name string) (string, error) { | |||
|
|
|||
| endpoints, _ := d.Endpoints(namespace).Get(name) | |||
| events, _ := d.Events(namespace).Search(service) | |||
| pods, _ := d.Pods(namespace).List(labels.Set(service.Spec.Selector).AsSelector(), fields.Everything()) | |||
There was a problem hiding this comment.
This isn't going to get the set you want, I don't think. It will include terminated and not-ready pods.
|
@bgrant0607 I've updated the PR as per your comments, PTAL. @kelseyhightower is there any example that spawns a service with multiple pods that I can test this against? A simple describe follows but I am not really happy with it. |
| } | ||
| fmt.Fprintf(out, "Pods:\t%d\n", pods) |
|
@bgrant0607 I've made the changes you suggested but I am not sure we should let the user guess that the number of their pods is the number of their endpoints... |
|
Thanks. LGTM. This is a service. It doesn't necessarily target any pods, though it typically does. |
bgrant0607
added
the
lgtm
Show pods number when describing services
No description provided.