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

feat: introducing a karmadactl top command #3593

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

chaunceyjiang
Copy link
Member

@chaunceyjiang chaunceyjiang commented May 28, 2023

What type of PR is this?
/kind feature

What this PR does / why we need it:

image

The difference between kubectl and karmadactl lies in the fact that karmadactl has introduced a new column called "Cluster", which allows you to view the resource usage of pods in different clusters.

Please see more details.

#3578 (comment)

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmadactl`: Introduced `top` command.

@karmada-bot karmada-bot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 28, 2023
@chaunceyjiang chaunceyjiang changed the title feat: introducing a karmadactl top command [WIP] feat: introducing a karmadactl top command May 28, 2023
@karmada-bot karmada-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 28, 2023
@karmada-bot karmada-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 31, 2023
@chaunceyjiang
Copy link
Member Author

image

@codecov-commenter
Copy link

codecov-commenter commented May 31, 2023

Codecov Report

Merging #3593 (62482f4) into master (25db1e0) will decrease coverage by 0.02%.
Report is 2 commits behind head on master.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #3593      +/-   ##
==========================================
- Coverage   54.71%   54.69%   -0.02%     
==========================================
  Files         229      229              
  Lines       22074    22074              
==========================================
- Hits        12077    12074       -3     
- Misses       9351     9354       +3     
  Partials      646      646              
Flag Coverage Δ
unittests 54.69% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

pkg/karmadactl/top/top.go Outdated Show resolved Hide resolved
pkg/karmadactl/top/top.go Outdated Show resolved Hide resolved
@chaunceyjiang
Copy link
Member Author

image

@chaunceyjiang chaunceyjiang marked this pull request as ready for review June 29, 2023 02:59
@chaunceyjiang chaunceyjiang changed the title [WIP] feat: introducing a karmadactl top command feat: introducing a karmadactl top command Jun 29, 2023
@karmada-bot karmada-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 29, 2023
@chaunceyjiang
Copy link
Member Author

/cc @carlory @lonelyCZ Please take a look.

pkg/karmadactl/top/top.go Outdated Show resolved Hide resolved
@jwcesign
Copy link
Member

jwcesign commented Aug 8, 2023

Can we push it forward?

@chaunceyjiang
Copy link
Member Author

/cc @jwcesign PTAL.

Copy link
Member

@jwcesign jwcesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

pkg/karmadactl/top/top_pods.go Show resolved Hide resolved
@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2023
@chaunceyjiang
Copy link
Member Author

/cc @RainbowMango @carlory @lonelyCZ

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/assign

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just some nits.

karmadactl has introduced a new column called "Cluster",

Two questions on that:

  1. I see the Cluster is present as the first column, have you ever considered putting it right after the pod name? Just like karmadactl get:
# karmadactl get pods -A 
NAMESPACE            NAME                                            CLUSTER   READY   STATUS    RESTARTS       AGE
kube-system          coredns-5d78c9869d-f98rf                        member1   1/1     Running   0              5d4h
kube-system          coredns-5d78c9869d-j9znc                        member1   1/1     Running   0              5d4h
  1. Is the output sorted by default? I think it would be better to sort it first by cluster and then by resource name.

pkg/karmadactl/top/top_pods.go Outdated Show resolved Hide resolved
pkg/karmadactl/top/top_pods.go Outdated Show resolved Hide resolved
@chaunceyjiang
Copy link
Member Author

image

image

image

image

Is the output sorted by default?

Default Sorting

  1. Sort by cluster name
  2. Sort by resource namespace
  3. Sort by resource name

Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
@chaunceyjiang
Copy link
Member Author

image

@chaunceyjiang
Copy link
Member Author

/cc @RainbowMango Updated.

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve
Thanks.

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2023
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 15, 2023
@karmada-bot karmada-bot merged commit f243a18 into karmada-io:master Aug 15, 2023
12 checks passed
@chaunceyjiang chaunceyjiang deleted the top branch August 15, 2023 13:06
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. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants