-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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 --sum flag to kubectl top pod #105100
Conversation
@@ -52,114 +52,6 @@ func NewTopCmdPrinter(out io.Writer) *TopCmdPrinter { | |||
return &TopCmdPrinter{out: out} | |||
} | |||
|
|||
type NodeMetricsSorter struct { |
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.
Move sorter out of metrics_printer
to metrics_sorter
because it clutters the printer file
if len(metrics) == 0 { | ||
return nil | ||
} | ||
w := printers.GetNewTabWriter(printer.out) | ||
defer w.Flush() | ||
if !noHeaders { | ||
if withNamespace { | ||
printValue(w, NamespaceColumn) | ||
PodColumns = append([]string{NamespaceColumn}, PodColumns...) |
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.
Used append
instead so we can know how many columns to skip
// skipping len(PodCoumns)-2 columns
for i := 0; i < len(PodColumns)-2; i++ {
printValue(out, "")
}
/assign @eddiezane |
/triage accepted |
/retest |
if len(metrics) == 0 { | ||
return nil | ||
} | ||
w := printers.GetNewTabWriter(printer.out) | ||
defer w.Flush() | ||
if !noHeaders { | ||
if withNamespace { | ||
printValue(w, NamespaceColumn) | ||
PodColumns = append([]string{NamespaceColumn}, PodColumns...) |
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.
How can we do this without mutating the globally scoped variable here?
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.
we can keep track of how many PodColums
will be outputted using a local variable I think. Will try that approach
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.
Looking good!
I am assuming this a quirk of how we round somewhere?
NAME CPU(cores) MEMORY(bytes)
hass-597bc475f5-knvr2 50m 276Mi
mosquitto-564d776dcd-2vdbw 1m 2Mi
zwavejs2mqtt-76c5fbb78c-6mr26 2m 74Mi
________ ________
52m 353Mi
@@ -115,6 +116,7 @@ func NewCmdTopPod(f cmdutil.Factory, o *TopPodOptions, streams genericclioptions | |||
cmd.Flags().BoolVarP(&o.AllNamespaces, "all-namespaces", "A", o.AllNamespaces, "If present, list the requested object(s) across all namespaces. Namespace in current context is ignored even if specified with --namespace.") | |||
cmd.Flags().BoolVar(&o.NoHeaders, "no-headers", o.NoHeaders, "If present, print output without headers.") | |||
cmd.Flags().BoolVar(&o.UseProtocolBuffers, "use-protocol-buffers", o.UseProtocolBuffers, "Enables using protocol-buffers to access Metrics API.") | |||
cmd.Flags().BoolVar(&o.Sum, "sum", o.Sum, "If present, print the sum of the resource usages") |
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.
cmd.Flags().BoolVar(&o.Sum, "sum", o.Sum, "If present, print the sum of the resource usages") | |
cmd.Flags().BoolVar(&o.Sum, "sum", o.Sum, "Print the sum of the resource usages") |
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.
Fixed
@@ -21,7 +21,7 @@ import ( | |||
"io" | |||
"sort" | |||
|
|||
"k8s.io/api/core/v1" | |||
v1 "k8s.io/api/core/v1" |
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.
v1 "k8s.io/api/core/v1" | |
corev1 "k8s.io/api/core/v1" |
func (adder *ResourceAdder) AddPodMetrics(m *metricsapi.PodMetrics) { | ||
for _, c := range m.Containers { | ||
for _, res := range adder.resources { | ||
total_val := adder.total[res] |
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.
nit: camelCase
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.
Fixed
} | ||
} | ||
|
||
// AddSinglePodMetrics will add each pod metrics to the total |
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.
// AddSinglePodMetrics will add each pod metrics to the total | |
// AddPodMetrics will add each pod metric to the total |
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.
Fixed
@@ -310,3 +216,21 @@ func printSingleResourceUsage(out io.Writer, resourceType v1.ResourceName, quant | |||
fmt.Fprintf(out, "%v", quantity.Value()) | |||
} | |||
} | |||
|
|||
func printPodResourcesSum(out io.Writer, total v1.ResourceList, withNamespace, printContainers bool, columnWidth int) { |
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.
withNamespace
and printContainers
are unused.
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.
Fixed
func getResourceQuantity(t *testing.T, quantityStr string) resource.Quantity { | ||
t.Helper() | ||
var err error | ||
quantity, _ := resource.ParseQuantity("0") |
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.
For future sanity let's check this error.
@@ -0,0 +1,130 @@ | |||
/* |
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.
Did you make any changes aside from moving this file out?
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.
No I didn't
@eddiezane yup, I believe we are just getting the estimate value over here |
75df48d
to
71acf56
Compare
/assign @KnVerey |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eddiezane, lauchokyip 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 |
Flake is #109133 /retest |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #kubernetes/kubectl#1061
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: