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 --sort-by option to kubectl top command #75920

Merged
merged 1 commit into from May 31, 2019

Conversation

@artmello
Copy link
Contributor

commented Mar 30, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
This adds the ability to sort the output from kubectl top node/pod by Memory or CPU.

Which issue(s) this PR fixes:

Fixes #47163

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add --sort-by option to kubectl top command
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

Hi @artmello. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@neolit123
Copy link
Member

left a comment

overall the implementation LGTM
added a couple of minor comments.

/kind feature
/priority backlog
/ok-to-test


for _, c := range m.Containers {
var usage v1.ResourceList
err := scheme.Scheme.Convert(&c.Usage, &usage, nil)

This comment has been minimized.

Copy link
@neolit123

neolit123 Mar 30, 2019

Member

can be:
if err := ......; err != nil {

This comment has been minimized.

Copy link
@soltysh

soltysh Apr 1, 2019

Contributor

Ditto wrt conversions being expensive.

qi := mi[v1.ResourceCPU]
qj := mj[v1.ResourceCPU]
return qi.Value() > qj.Value()
}

This comment has been minimized.

Copy link
@neolit123

neolit123 Mar 30, 2019

Member

} else if {
or use swich/case
[1]

qi := ui[v1.ResourceCPU]
qj := uj[v1.ResourceCPU]
return qi.Value() > qj.Value()
}

This comment has been minimized.

Copy link
@neolit123
@neolit123

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

@artmello
please add Add --sort-by option to kubectl top command instead of NONE for the release note.

@caiobegotti

This comment has been minimized.

Copy link

commented Mar 30, 2019

I just mentioned this the other day and now it gets implemented, that's a flag I could actually use a lot 😄

@artmello

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

@neolit123 Thanks a lot for reviewing this. Hopefully, all your comments are addressed with latest commit

@@ -150,6 +152,11 @@ func (o *TopNodeOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []
}

func (o *TopNodeOptions) Validate() error {
if len(o.SortBy) > 0 {
if o.SortBy != "cpu" && o.SortBy != "memory" {
return errors.New("provided field to sort-by can be only 'cpu' or 'memory'")

This comment has been minimized.

Copy link
@soltysh

soltysh Apr 1, 2019

Contributor

--sort-by accepts only cpu or memory

@@ -144,6 +146,11 @@ func (o *TopPodOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []s
}

func (o *TopPodOptions) Validate() error {
if len(o.SortBy) > 0 {
if o.SortBy != "cpu" && o.SortBy != "memory" {
return errors.New("provided field to sort-by can be only 'cpu' or 'memory'")

This comment has been minimized.

Copy link
@soltysh

soltysh Apr 1, 2019

Contributor

Ditto.

if len(metrics) == 0 {
return nil
}
w := printers.GetNewTabWriter(printer.out)
defer w.Flush()

sort.Slice(metrics, func(i, j int) bool {
if len(sortBy) > 0 {

This comment has been minimized.

Copy link
@soltysh

soltysh Apr 1, 2019

Contributor

Make it a separate function.

if len(metrics) == 0 {
return nil
}
w := printers.GetNewTabWriter(printer.out)
defer w.Flush()

sort.Slice(metrics, func(i, j int) bool {
if len(sortBy) > 0 {
var ui, uj v1.ResourceList
err := scheme.Scheme.Convert(&metrics[i].Usage, &ui, nil)

This comment has been minimized.

Copy link
@soltysh

soltysh Apr 1, 2019

Contributor

This will be expensive, since you'll always convert the field upon every invocation. It'll be reasonable to convert those once and sort the converted list.


for _, c := range m.Containers {
var usage v1.ResourceList
err := scheme.Scheme.Convert(&c.Usage, &usage, nil)

This comment has been minimized.

Copy link
@soltysh

soltysh Apr 1, 2019

Contributor

Ditto wrt conversions being expensive.

@@ -104,6 +126,26 @@ func (printer *TopCmdPrinter) PrintPodMetrics(metrics []metricsapi.PodMetrics, p
}

sort.Slice(metrics, func(i, j int) bool {
if len(sortBy) > 0 {

This comment has been minimized.

Copy link
@soltysh

soltysh Apr 1, 2019

Contributor

Ditto wrt separate function.

@artmello

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

@soltysh thanks for reviewing and suggestions. Hopefully, latest commit addressed your comments.

@artmello

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Ok, unfortunately, I am not sure why the "pull-kubernetes-verify" job is failing. I executed ./hack/verify-shellcheck at my local machine and it seems ok. I don't think I have rights to ask bot for another run. Any idea what can be wrong?

var usages = make([]v1.ResourceList, len(metrics))
for i, v := range metrics {
if err := scheme.Scheme.Convert(&v.Usage, &usages[i], nil); err != nil {
usages[i] = nil

This comment has been minimized.

Copy link
@soltysh

soltysh Apr 9, 2019

Contributor

We can't silently pass errors, if there's a problem we should error out.

sort.Slice(metrics, func(i, j int) bool {
return metrics[i].Name < metrics[j].Name
})
}

This comment has been minimized.

Copy link
@soltysh

soltysh Apr 9, 2019

Contributor

Why not just implementing sort.Interface (https://golang.org/pkg/sort/#Interface) and pass parameters, in it you'd act accordingly whether or not sortBy was passed. Have a look at

func NewTableSorter(table *metav1beta1.Table, field string) (*TableSorter, error) {

@@ -118,6 +152,31 @@ func (printer *TopCmdPrinter) PrintPodMetrics(metrics []metricsapi.PodMetrics, p
return nil
}

func sortPodsByMetrics(metrics []metricsapi.PodMetrics, printContainers bool, sortBy string) {
var podMetrics = make([]v1.ResourceList, len(metrics))

This comment has been minimized.

Copy link
@soltysh

soltysh Apr 9, 2019

Contributor

Ditto here wrt implementing sort.Interface.

@artmello

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@soltysh Thanks for the last review, hopefully last commit address your comments.

@soltysh
Copy link
Contributor

left a comment

Two final nits, also please squash your changes into a single commit.

@@ -144,6 +146,11 @@ func (o *TopPodOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []s
}

func (o *TopPodOptions) Validate() error {
if len(o.SortBy) > 0 {
if o.SortBy != "cpu" && o.SortBy != "memory" {

This comment has been minimized.

Copy link
@soltysh

soltysh Apr 23, 2019

Contributor

I'd consider making these constants.

var podMetrics = make([]v1.ResourceList, len(metrics))
if len(sortBy) > 0 {
for i, v := range metrics {
podMetrics[i], _, _ = getPodMetrics(&v, printContainers)

This comment has been minimized.

Copy link
@soltysh

soltysh Apr 23, 2019

Contributor

Any particular reason to swallow error?

@artmello artmello force-pushed the artmello:kubectl_top_sortby branch from 4d15074 to 625d60e Apr 23, 2019

@artmello

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@soltysh Thanks for taking a look once again. Your latest comments should be addressed now

Add --sort-by option to kubectl top command
- Add ability to sort kubectl top nodes/pod by either CPU or memory usage

@artmello artmello force-pushed the artmello:kubectl_top_sortby branch from 625d60e to b61e46f Apr 24, 2019

@soltysh
Copy link
Contributor

left a comment

/lgtm
/approve
/retest

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: artmello, soltysh

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

@fejta-bot

This comment has been minimized.

Copy link

commented May 31, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 3d871df into kubernetes:master May 31, 2019

21 checks passed

cla/linuxfoundation artmello authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.