-
Notifications
You must be signed in to change notification settings - Fork 40.9k
[KEP-2400] kubectl top: add a --show-swap option #129458
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
Conversation
Skipping CI for Draft Pull Request. |
Changelog suggestion -Add a "--show-swap" option to kubectl top subcommands
+Added a `--show-swap` option to `kubectl top` subcommands |
c3b0c09
to
4d75293
Compare
LGTM |
183b011
to
63c1cd8
Compare
2aa7eff
to
3dc7e71
Compare
@kannon92 I tend to disagree. I don't see a reason to disallow using the explicit |
@soltysh Done. It will now look like the following:
Withous swap:
I prefer to leave the kubernetes/pkg/kubelet/nodestatus/setters.go Lines 359 to 363 in beef784
In addition, currently the pod printer relies solely on metrics, which only say that the pod is not using any swap. To gather information about nodes we'd need to add an addition list node API call. |
51d6881
to
88b66ab
Compare
@soltysh WDYT? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two final comments, and lets land this. @iholder101 ping me on slack when you get that updated and I'll tag.
@@ -198,6 +200,13 @@ func (o TopNodeOptions) RunTopNode() error { | |||
} else { | |||
availableResources[n.Name] = n.Status.Capacity | |||
} | |||
|
|||
swapCapacity := int64(0) | |||
if n.Status.NodeInfo.Swap != nil && n.Status.NodeInfo.Swap.Capacity != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Itamar is correct here.
fmt.Fprintf(out, "%vMi", quantity.Value()/(1024*1024)) | ||
default: | ||
fmt.Fprintf(out, "%v", quantity.Value()) | ||
} | ||
} | ||
|
||
func printPodResourcesSum(out io.Writer, total v1.ResourceList, columnWidth int) { | ||
func printSingleMissingResource(out io.Writer) { | ||
const unavailableStr = "unavailable" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const unavailableStr = "unavailable" | |
const unavailableStr = "<unkown>" |
is what we're mostly using, in kubectl decribe
output, for example. So let's be consistent with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! 👍
88b66ab
to
c9efa28
Compare
Signed-off-by: Itamar Holder <[email protected]>
Example output: > kubectl top node --show-swap NAME CPU(cores) CPU(%) MEMORY(bytes) MEMORY(%) SWAP(bytes) SWAP(%) node01 500m 8% 2836Mi 60% 0Mi 0% node02 260m 5% 2206Mi 47% 512Mi 50% Signed-off-by: Itamar Holder <[email protected]>
Example output: > kubectl top pod -n kube-system --show-swap NAME CPU(cores) MEMORY(bytes) SWAP(bytes) coredns-58d5bc5cdb-5nbk4 2m 19Mi 0Mi coredns-58d5bc5cdb-jsh26 3m 37Mi 0Mi etcd-node01 51m 143Mi 0Mi kube-apiserver-node01 98m 824Mi 0Mi kube-controller-manager-node01 20m 135Mi 0Mi kube-proxy-ffgs2 1m 24Mi 0Mi kube-proxy-fhvwx 1m 39Mi 0Mi kube-scheduler-node01 13m 69Mi 0Mi metrics-server-8598789fdb-d2kcj 5m 26Mi 0Mi Signed-off-by: Itamar Holder <[email protected]>
Signed-off-by: Itamar Holder <[email protected]>
c9efa28
to
ed9c816
Compare
Thanks @soltysh! |
Signed-off-by: Itamar Holder <[email protected]>
ed9c816
to
a6c8736
Compare
Signed-off-by: Itamar Holder <[email protected]>
a6c8736
to
710b162
Compare
/test pull-kubernetes-unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
LGTM label has been added. Git tree hash: 3b79e3a41287c21dd673ee47fc3ffd7d72a2f40a
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: iholder101, 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 |
* top, refactor: turn package-exposed variables to unexpose struct fields Signed-off-by: Itamar Holder <[email protected]> * kubectl top node: add the --show-swap option Example output: > kubectl top node --show-swap NAME CPU(cores) CPU(%) MEMORY(bytes) MEMORY(%) SWAP(bytes) SWAP(%) node01 500m 8% 2836Mi 60% 0Mi 0% node02 260m 5% 2206Mi 47% 512Mi 50% Signed-off-by: Itamar Holder <[email protected]> * kubectl top pod: add the --show-swap option Example output: > kubectl top pod -n kube-system --show-swap NAME CPU(cores) MEMORY(bytes) SWAP(bytes) coredns-58d5bc5cdb-5nbk4 2m 19Mi 0Mi coredns-58d5bc5cdb-jsh26 3m 37Mi 0Mi etcd-node01 51m 143Mi 0Mi kube-apiserver-node01 98m 824Mi 0Mi kube-controller-manager-node01 20m 135Mi 0Mi kube-proxy-ffgs2 1m 24Mi 0Mi kube-proxy-fhvwx 1m 39Mi 0Mi kube-scheduler-node01 13m 69Mi 0Mi metrics-server-8598789fdb-d2kcj 5m 26Mi 0Mi Signed-off-by: Itamar Holder <[email protected]> * kubectl top node --show-swap: add unit tests Signed-off-by: Itamar Holder <[email protected]> * kubectl top pod --show-swap: Add unit tests Signed-off-by: Itamar Holder <[email protected]> * Explicitly mark swap as unavailable when necessary Signed-off-by: Itamar Holder <[email protected]> --------- Signed-off-by: Itamar Holder <[email protected]>
* top, refactor: turn package-exposed variables to unexpose struct fields Signed-off-by: Itamar Holder <[email protected]> * kubectl top node: add the --show-swap option Example output: > kubectl top node --show-swap NAME CPU(cores) CPU(%) MEMORY(bytes) MEMORY(%) SWAP(bytes) SWAP(%) node01 500m 8% 2836Mi 60% 0Mi 0% node02 260m 5% 2206Mi 47% 512Mi 50% Signed-off-by: Itamar Holder <[email protected]> * kubectl top pod: add the --show-swap option Example output: > kubectl top pod -n kube-system --show-swap NAME CPU(cores) MEMORY(bytes) SWAP(bytes) coredns-58d5bc5cdb-5nbk4 2m 19Mi 0Mi coredns-58d5bc5cdb-jsh26 3m 37Mi 0Mi etcd-node01 51m 143Mi 0Mi kube-apiserver-node01 98m 824Mi 0Mi kube-controller-manager-node01 20m 135Mi 0Mi kube-proxy-ffgs2 1m 24Mi 0Mi kube-proxy-fhvwx 1m 39Mi 0Mi kube-scheduler-node01 13m 69Mi 0Mi metrics-server-8598789fdb-d2kcj 5m 26Mi 0Mi Signed-off-by: Itamar Holder <[email protected]> * kubectl top node --show-swap: add unit tests Signed-off-by: Itamar Holder <[email protected]> * kubectl top pod --show-swap: Add unit tests Signed-off-by: Itamar Holder <[email protected]> * Explicitly mark swap as unavailable when necessary Signed-off-by: Itamar Holder <[email protected]> --------- Signed-off-by: Itamar Holder <[email protected]>
What type of PR is this?
/kind feature
Background
A while ago, swap stats were added to summary and metric APIs as part of #118865. Although these stats are extremely valuable, they're mostly designed to be consumed by programs (like HPAs or metrics solutions), not by humans. This raised the request for a better way of understanding which nodes have swap, the swap capacity and usage, which pods consume swap space and how much, etc.
I've tried to address this need in two past attempts.
In the first one, #123963, I've tried to add swap as another
ReasourceName
, hence to display it as part ofkubectl describe node
's status. This attempt was eventually denied with the main concern being that adding swap as aResourceName
is confusing since it cannot be directly/explicitly consumed (since the swap limit is auto-calculated).In the second one, #125278, I've tried to add a swap condition to nodes. This PR was also denied with the main concern being that a condition is not suited to report such data, and that a field is recommended in this case, although it wasn't easy thinking of a good field that will benefit the API.
Eventually we agreed to defer this to the future. However, while attempting to GA the swap feature there was a hard push-back against GA without addressing these needs.
Therefore, this is my third attempt to address it. My hope is that it will suffice the need of debugability and will pave the way for the upcoming GA.
What this PR does / why we need it:
This PR adds adds the
--show-swap
option to thekubectl top
subcommands.Behind the scenes, this purely relies on consuming the stats added in #118865, making them human-readable.
Example output:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
I recommend to review per-commit.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: