fix: include pod count in Node.Get output #3523
Conversation
|
I can’t reproduce this. For me, it shows |
|
Heya @uozalp ! I added a couple screenshots to make it clearer. Please see the value in the Currently: |
|
Thanks for posting the screenshot, I see now what you meant. |
internal/dao/node.go
Outdated
| @@ -152,7 +152,21 @@ func (n *Node) Get(ctx context.Context, path string) (runtime.Object, error) { | |||
| nmx, _ = client.DialMetrics(n.Client()).FetchNodeMetrics(ctx, path) | |||
| } | |||
|
|
|||
| return &render.NodeWithMetrics{Raw: raw, MX: nmx}, nil | |||
| podCount := -1 | |||
There was a problem hiding this comment.
I think we should DRY this up and convert this into a helper so that we can reuse for both list/get aka something like getPodCount
There was a problem hiding this comment.
Sure thing, will refactor it this weekend.
There was a problem hiding this comment.
I got a little carried away refactoring, need a couple more days to tidy things up!
| @@ -132,96 +132,118 @@ func (n *Node) Drain(path string, opts DrainOptions, w io.Writer) error { | |||
|
|
|||
| // Get returns a node resource. | |||
| func (n *Node) Get(ctx context.Context, path string) (runtime.Object, error) { | |||
| oo, err := n.Resource.List(ctx, "") | |||
| o, err := n.Resource.Get(ctx, client.FQN(client.ClusterScope, path)) | |||
There was a problem hiding this comment.
Switched to Get here instead of List, means we don't have to filter below.
| } | ||
|
|
||
| var nmx client.NodesMetricsMap | ||
| if withMx, ok := ctx.Value(internal.KeyWithMetrics).(bool); withMx || !ok { | ||
| if withMx, ok := ctx.Value(internal.KeyWithMetrics).(bool); ok && withMx { |
There was a problem hiding this comment.
Switched this to match how checks are being made currently elsewhere.
| res := make([]runtime.Object, 0, len(oo)) | ||
| for _, o := range oo { | ||
| u, ok := o.(*unstructured.Unstructured) | ||
| if !ok { | ||
| return res, fmt.Errorf("expecting *unstructured.Unstructured but got `%T", o) | ||
| return nil, fmt.Errorf("expecting *unstructured.Unstructured but got `%T", o) |
There was a problem hiding this comment.
Returning nil on err instead of res.
| } | ||
|
|
||
| // List returns a collection of node resources. | ||
| func (n *Node) List(ctx context.Context, ns string) ([]runtime.Object, error) { | ||
| oo, err := n.Resource.List(ctx, ns) | ||
| if err != nil { | ||
| return oo, err | ||
| return nil, err |
There was a problem hiding this comment.
Returning nil on err instead of oo.
|
|
||
| var podCounts map[string]int | ||
| if shouldCountPods { | ||
| podCounts, err = n.CountPodsByNode(pods) |
There was a problem hiding this comment.
Moved pod counting outside the loop and made CountPodsByNode return a map[nodeName]podCount. Only requires looping through the pods once. This is more performant that calling CountPods for each node.
| return r, nil | ||
| } | ||
|
|
||
| pods, err := n.getFactory().List(client.PodGVR, client.BlankNamespace, false, labels.Everything()) |
There was a problem hiding this comment.
Would be nice to filter the pods we're interested in here using a field selector on spec.nodeName but it doens't look like it's currently possible?
|
Hey @derailed, please let me know if you'd like any changes here. My local tests against a kind cluster have gone well, will run it against some bigger aws clusters next week too. |
Currently if you go to a pod and hit o to go to its parent node, the pod count is always 0.
Tracked the cause to Node.Get where PodCount wasn't being set.
Defaulted podCount to -1 so it appears as "n/a" instead of 0.