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

Added utilization plugin #49

Merged
merged 8 commits into from Jan 16, 2019

Conversation

Projects
None yet
3 participants
@etopeter
Copy link
Contributor

etopeter commented Jan 13, 2019

No description provided.

etopeter added some commits Jan 13, 2019

@googlebot

This comment has been minimized.

Copy link

googlebot commented Jan 13, 2019

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers
1 similar comment
@googlebot

This comment has been minimized.

Copy link

googlebot commented Jan 13, 2019

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers
@etopeter

This comment has been minimized.

Copy link
Contributor Author

etopeter commented Jan 13, 2019

I signed it!

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers
Corporate signers

I signed it!

@googlebot

This comment has been minimized.

Copy link

googlebot commented Jan 13, 2019

CLAs look good, thanks!

1 similar comment
@googlebot

This comment has been minimized.

Copy link

googlebot commented Jan 13, 2019

CLAs look good, thanks!

@ahmetb

This comment has been minimized.

Copy link
Contributor

ahmetb commented Jan 13, 2019

Thanks for your pull request.

A name like print-utilization or show-utilization would be more appropriate. I need to think how many plugins named "utilization" will show up.

By the way, when I ran this on a GKE cluster I got:

➜ kubectl utilization
Runtime error (func=(main), adr=13): Divide by zero
cores 4.2/0 (%)
memory 5GiB/13GiB (38%)

etopeter added some commits Jan 13, 2019

@etopeter

This comment has been minimized.

Copy link
Contributor Author

etopeter commented Jan 13, 2019

Added fix for what I think is the issue you seeing @ahmetb . I think your cluster nodes are reporting CPU in 1000m the easiest way would be to run to verify. The first column should either show full integers or 1000m per cpu.

kubectl get nodes -o=jsonpath="{range .items[*]} {.status.allocatable.cpu}{'\t'}{.status.allocatable.memory}{'\n'}{end}"

For name of plugin I would insist on staying as utilization because I'm hoping that in the future it will be implemented natively in kubectl command. This plugin references this issue #17512. Alternatively if this doesn't get implemented, this plugin could be written in go and be native to each platform instead of BASH.

If you insist on having it prefixed I would use same thing other plugins are currently using in this index view.

@ahmetb

This comment has been minimized.

Copy link
Contributor

ahmetb commented Jan 14, 2019

Yeah the name might be okay.

When I run that command I get an output starting with empty line (maybe that's the problem)?


 940m	2702252Ki
 940m	2702204Ki
 940m	2702204Ki
 940m	2702244Ki
 940m	2702212Ki
@etopeter

This comment has been minimized.

Copy link
Contributor Author

etopeter commented Jan 14, 2019

Just to clarify you think utilization is ok or its better renamed to view-utilization?
I added some tests and I think error you saw should be resolved now if you don't mind checking latest release.

@ahmetb

This comment has been minimized.

Copy link
Contributor

ahmetb commented Jan 14, 2019

I definitely prefer view-utilization but utilization might do it too.

The whole reason we try to avoid generic naming is:

  1. prevent first-comer advantage (name grabs)
  2. express users the intent of the plugin clearly.

For example, if you think anyone who sees kubectl utilization will definitely think of "printing utilization stats" and not "increase utilization" through kubectl utilization reschedule-pods --strategy=binpack or something like that, feel free to stick to this name.

@ahmetb

This comment has been minimized.

Copy link
Contributor

ahmetb commented Jan 15, 2019

On another note, I don't recommend calling this plugin 1.0 since it's very new and you're patching things every day. :)

@etopeter

This comment has been minimized.

Copy link
Contributor Author

etopeter commented Jan 15, 2019

Great catch. I renamed plugin and changed versioning.

@ahmetb

This comment has been minimized.

Copy link
Contributor

ahmetb commented Jan 15, 2019

Looks great. I recommend reading semver.org sometime. Most working things start at v0.1.0 :)

@etopeter

This comment has been minimized.

Copy link
Contributor Author

etopeter commented Jan 16, 2019

Thanks for catching that. v0.1.0 is now where plugin is at.

@ahmetb

ahmetb approved these changes Jan 16, 2019

@ahmetb ahmetb merged commit 403cdaf into kubernetes-sigs:master Jan 16, 2019

1 check passed

cla/google All necessary CLAs are signed
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.