Merge pull request #93692 from brianpursley/kubectl-901

Sort kubectl top pod output when --sort-by and --containers are used together

staging/src/k8s.io/kubectl/pkg/cmd/top/BUILD

@@ -47,7 +47,6 @@ go_test(
         "//staging/src/k8s.io/client-go/rest/fake:go_default_library",
         "//staging/src/k8s.io/client-go/testing:go_default_library",
         "//staging/src/k8s.io/kubectl/pkg/cmd/testing:go_default_library",
-        "//staging/src/k8s.io/kubectl/pkg/cmd/util:go_default_library",
         "//staging/src/k8s.io/kubectl/pkg/scheme:go_default_library",
         "//staging/src/k8s.io/metrics/pkg/apis/metrics/v1alpha1:go_default_library",
         "//staging/src/k8s.io/metrics/pkg/apis/metrics/v1beta1:go_default_library",

staging/src/k8s.io/kubectl/pkg/cmd/top/top_pod_test.go

@@ -34,7 +34,6 @@ import (
 	"k8s.io/client-go/rest/fake"
 	core "k8s.io/client-go/testing"
 	cmdtesting "k8s.io/kubectl/pkg/cmd/testing"
-	cmdutil "k8s.io/kubectl/pkg/cmd/util"
 	"k8s.io/kubectl/pkg/scheme"
 	metricsv1alpha1api "k8s.io/metrics/pkg/apis/metrics/v1alpha1"
 	metricsv1beta1api "k8s.io/metrics/pkg/apis/metrics/v1beta1"
@@ -86,6 +85,7 @@ func TestTopPod(t *testing.T) {
 		args               []string
 		expectedQuery      string
 		expectedPods       []string
+		expectedContainers []string
 		namespaces         []string
 		containers         bool
 		listsNamespaces    bool
@@ -115,21 +115,53 @@ func TestTopPod(t *testing.T) {
 			name:    "pod with container metrics",
 			options: &TopPodOptions{PrintContainers: true},
 			args:    []string{"pod1"},
+			expectedContainers: []string{
+				"container1-1",
+				"container1-2",
+			},
 			namespaces: []string{testNS},
 			containers: true,
 		},
 		{
-			name:         "pod with label sort by cpu",
+			name:         "pod sort by cpu",
 			options:      &TopPodOptions{SortBy: "cpu"},
 			expectedPods: []string{"pod2", "pod3", "pod1"},
 			namespaces:   []string{testNS, testNS, testNS},
 		},
 		{
-			name:         "pod with label sort by memory",
+			name:         "pod sort by memory",
 			options:      &TopPodOptions{SortBy: "memory"},
 			expectedPods: []string{"pod2", "pod3", "pod1"},
 			namespaces:   []string{testNS, testNS, testNS},
 		},
+		{
+			name:    "container sort by cpu",
+			options: &TopPodOptions{PrintContainers: true, SortBy: "cpu"},
+			expectedContainers: []string{
+				"container2-3",
+				"container2-2",
+				"container2-1",
+				"container3-1",
+				"container1-2",
+				"container1-1",
+			},
+			namespaces: []string{testNS, testNS, testNS},
+			containers: true,
+		},
+		{
+			name:    "container sort by memory",
+			options: &TopPodOptions{PrintContainers: true, SortBy: "memory"},
+			expectedContainers: []string{
+				"container2-3",
+				"container2-2",
+				"container2-1",
+				"container3-1",
+				"container1-2",
+				"container1-1",
+			},
+			namespaces: []string{testNS, testNS, testNS},
+			containers: true,
+		},
 	}
 	cmdtesting.InitTestErrorHandler(t)
 	for _, testCase := range testCases {
@@ -234,23 +266,34 @@ func TestTopPod(t *testing.T) {
 					t.Errorf("unexpected metrics for %s: \n%s", name, result)
 				}
 			}
-			if cmdutil.GetFlagString(cmd, "sort-by") == "cpu" || cmdutil.GetFlagString(cmd, "sort-by") == "memory" {
+			if testCase.expectedPods != nil {
-				resultLines := strings.Split(result, "\n")
+				resultPods := getResultColumnValues(result, 0)
-				resultPods := make([]string, len(resultLines)-2) // don't process first (header) and last (empty) line
-
-				for i, line := range resultLines[1 : len(resultLines)-1] { // don't process first (header) and last (empty) line
-					lineFirstColumn := strings.Split(line, " ")[0]
-					resultPods[i] = lineFirstColumn
-				}
-
 				if !reflect.DeepEqual(testCase.expectedPods, resultPods) {
-					t.Errorf("kinds not matching:\n\texpectedKinds: %v\n\tgotKinds: %v\n", testCase.expectedPods, resultPods)
+					t.Errorf("pods not matching:\n\texpectedPods: %v\n\tresultPods: %v\n", testCase.expectedPods, resultPods)
+				}
+			}
+			if testCase.expectedContainers != nil {
+				resultContainers := getResultColumnValues(result, 1)
+				if !reflect.DeepEqual(testCase.expectedContainers, resultContainers) {
+					t.Errorf("containers not matching:\n\texpectedContainers: %v\n\tresultContainers: %v\n", testCase.expectedContainers, resultContainers)
 				}
 			}
 		})
 	}
 }
 
+func getResultColumnValues(result string, columnIndex int) []string {
+	resultLines := strings.Split(result, "\n")
+	values := make([]string, len(resultLines)-2) // don't process first (header) and last (empty) line
+
+	for i, line := range resultLines[1 : len(resultLines)-1] { // don't process first (header) and last (empty) line
+		value := strings.Fields(line)[columnIndex]
+		values[i] = value
+	}
+
+	return values
+}
+
 func TestTopPodNoResourcesFound(t *testing.T) {
 	testNS := "testns"
 	testCases := []struct {

staging/src/k8s.io/kubectl/pkg/metricsutil/metrics_printer.go

@@ -55,7 +55,6 @@ func NewTopCmdPrinter(out io.Writer) *TopCmdPrinter {
 type NodeMetricsSorter struct {
 	metrics []metricsapi.NodeMetrics
 	sortBy  string
-	usages  []v1.ResourceList
 }
 
 func (n *NodeMetricsSorter) Len() int {
@@ -64,37 +63,24 @@ func (n *NodeMetricsSorter) Len() int {
 
 func (n *NodeMetricsSorter) Swap(i, j int) {
 	n.metrics[i], n.metrics[j] = n.metrics[j], n.metrics[i]
-	n.usages[i], n.usages[j] = n.usages[j], n.usages[i]
 }
 
 func (n *NodeMetricsSorter) Less(i, j int) bool {
 	switch n.sortBy {
 	case "cpu":
-		qi := n.usages[i][v1.ResourceCPU]
+		return n.metrics[i].Usage.Cpu().MilliValue() > n.metrics[j].Usage.Cpu().MilliValue()
-		qj := n.usages[j][v1.ResourceCPU]
-		return qi.MilliValue() > qj.MilliValue()
 	case "memory":
-		qi := n.usages[i][v1.ResourceMemory]
+		return n.metrics[i].Usage.Memory().Value() > n.metrics[j].Usage.Memory().Value()
-		qj := n.usages[j][v1.ResourceMemory]
-		return qi.Value() > qj.Value()
 	default:
 		return n.metrics[i].Name < n.metrics[j].Name
 	}
 }
 
-func NewNodeMetricsSorter(metrics []metricsapi.NodeMetrics, sortBy string) (*NodeMetricsSorter, error) {
+func NewNodeMetricsSorter(metrics []metricsapi.NodeMetrics, sortBy string) *NodeMetricsSorter {
-	var usages = make([]v1.ResourceList, len(metrics))
-	if len(sortBy) > 0 {
-		for i, v := range metrics {
-			v.Usage.DeepCopyInto(&usages[i])
-		}
-	}
-
 	return &NodeMetricsSorter{
 		metrics: metrics,
 		sortBy:  sortBy,
-		usages:  usages,
+	}
-	}, nil
 }
 
 type PodMetricsSorter struct {
@@ -116,13 +102,9 @@ func (p *PodMetricsSorter) Swap(i, j int) {
 func (p *PodMetricsSorter) Less(i, j int) bool {
 	switch p.sortBy {
 	case "cpu":
-		qi := p.podMetrics[i][v1.ResourceCPU]
+		return p.podMetrics[i].Cpu().MilliValue() > p.podMetrics[j].Cpu().MilliValue()
-		qj := p.podMetrics[j][v1.ResourceCPU]
-		return qi.MilliValue() > qj.MilliValue()
 	case "memory":
-		qi := p.podMetrics[i][v1.ResourceMemory]
+		return p.podMetrics[i].Memory().Value() > p.podMetrics[j].Memory().Value()
-		qj := p.podMetrics[j][v1.ResourceMemory]
-		return qi.Value() > qj.Value()
 	default:
 		if p.withNamespace && p.metrics[i].Namespace != p.metrics[j].Namespace {
 			return p.metrics[i].Namespace < p.metrics[j].Namespace
@@ -131,11 +113,11 @@ func (p *PodMetricsSorter) Less(i, j int) bool {
 	}
 }
 
-func NewPodMetricsSorter(metrics []metricsapi.PodMetrics, printContainers bool, withNamespace bool, sortBy string) (*PodMetricsSorter, error) {
+func NewPodMetricsSorter(metrics []metricsapi.PodMetrics, withNamespace bool, sortBy string) *PodMetricsSorter {
 	var podMetrics = make([]v1.ResourceList, len(metrics))
 	if len(sortBy) > 0 {
 		for i, v := range metrics {
-			podMetrics[i], _, _ = getPodMetrics(&v, printContainers)
+			podMetrics[i] = getPodMetrics(&v)
 		}
 	}
 
@@ -144,7 +126,38 @@ func NewPodMetricsSorter(metrics []metricsapi.PodMetrics, printContainers bool,
 		sortBy:        sortBy,
 		withNamespace: withNamespace,
 		podMetrics:    podMetrics,
-	}, nil
+	}
+}
+
+type ContainerMetricsSorter struct {
+	metrics []metricsapi.ContainerMetrics
+	sortBy  string
+}
+
+func (s *ContainerMetricsSorter) Len() int {
+	return len(s.metrics)
+}
+
+func (s *ContainerMetricsSorter) Swap(i, j int) {
+	s.metrics[i], s.metrics[j] = s.metrics[j], s.metrics[i]
+}
+
+func (s *ContainerMetricsSorter) Less(i, j int) bool {
+	switch s.sortBy {
+	case "cpu":
+		return s.metrics[i].Usage.Cpu().MilliValue() > s.metrics[j].Usage.Cpu().MilliValue()
+	case "memory":
+		return s.metrics[i].Usage.Memory().Value() > s.metrics[j].Usage.Memory().Value()
+	default:
+		return s.metrics[i].Name < s.metrics[j].Name
+	}
+}
+
+func NewContainerMetricsSorter(metrics []metricsapi.ContainerMetrics, sortBy string) *ContainerMetricsSorter {
+	return &ContainerMetricsSorter{
+		metrics: metrics,
+		sortBy:  sortBy,
+	}
 }
 
 func (printer *TopCmdPrinter) PrintNodeMetrics(metrics []metricsapi.NodeMetrics, availableResources map[string]v1.ResourceList, noHeaders bool, sortBy string) error {
@@ -154,11 +167,7 @@ func (printer *TopCmdPrinter) PrintNodeMetrics(metrics []metricsapi.NodeMetrics,
 	w := printers.GetNewTabWriter(printer.out)
 	defer w.Flush()
 
-	n, err := NewNodeMetricsSorter(metrics, sortBy)
+	sort.Sort(NewNodeMetricsSorter(metrics, sortBy))
-	if err != nil {
-		return err
-	}
-	sort.Sort(n)
 
 	if !noHeaders {
 		printColumnNames(w, NodeColumns)
@@ -197,16 +206,14 @@ func (printer *TopCmdPrinter) PrintPodMetrics(metrics []metricsapi.PodMetrics, p
 		printColumnNames(w, PodColumns)
 	}
 
-	p, err := NewPodMetricsSorter(metrics, printContainers, withNamespace, sortBy)
+	sort.Sort(NewPodMetricsSorter(metrics, withNamespace, sortBy))
-	if err != nil {
-		return err
-	}
-	sort.Sort(p)
 
 	for _, m := range metrics {
-		err := printSinglePodMetrics(w, &m, printContainers, withNamespace)
+		if printContainers {
-		if err != nil {
+			sort.Sort(NewContainerMetricsSorter(m.Containers, sortBy))
-			return err
+			printSinglePodContainerMetrics(w, &m, withNamespace)
+		} else {
+			printSinglePodMetrics(w, &m, withNamespace)
 		}
 	}
 	return nil
@@ -219,24 +226,8 @@ func printColumnNames(out io.Writer, names []string) {
 	fmt.Fprint(out, "\n")
 }
 
-func printSinglePodMetrics(out io.Writer, m *metricsapi.PodMetrics, printContainersOnly bool, withNamespace bool) error {
+func printSinglePodMetrics(out io.Writer, m *metricsapi.PodMetrics, withNamespace bool) {
-	podMetrics, containers, err := getPodMetrics(m, printContainersOnly)
+	podMetrics := getPodMetrics(m)
-	if err != nil {
-		return err
-	}
-	if printContainersOnly {
-		for contName := range containers {
-			if withNamespace {
-				printValue(out, m.Namespace)
-			}
-			printValue(out, m.Name)
-			printMetricsLine(out, &ResourceMetricsInfo{
-				Name:      contName,
-				Metrics:   containers[contName],
-				Available: v1.ResourceList{},
-			})
-		}
-	} else {
 	if withNamespace {
 		printValue(out, m.Namespace)
 	}
@@ -246,29 +237,35 @@ func printSinglePodMetrics(out io.Writer, m *metricsapi.PodMetrics, printContain
 		Available: v1.ResourceList{},
 	})
 }
-	return nil
+
+func printSinglePodContainerMetrics(out io.Writer, m *metricsapi.PodMetrics, withNamespace bool) {
+	for _, c := range m.Containers {
+		if withNamespace {
+			printValue(out, m.Namespace)
+		}
+		printValue(out, m.Name)
+		printMetricsLine(out, &ResourceMetricsInfo{
+			Name:      c.Name,
+			Metrics:   c.Usage,
+			Available: v1.ResourceList{},
+		})
+	}
 }
 
-func getPodMetrics(m *metricsapi.PodMetrics, printContainersOnly bool) (v1.ResourceList, map[string]v1.ResourceList, error) {
+func getPodMetrics(m *metricsapi.PodMetrics) v1.ResourceList {
-	containers := make(map[string]v1.ResourceList)
 	podMetrics := make(v1.ResourceList)
 	for _, res := range MeasuredResources {
 		podMetrics[res], _ = resource.ParseQuantity("0")
 	}
 
-	var usage v1.ResourceList
 	for _, c := range m.Containers {
-		c.Usage.DeepCopyInto(&usage)
-		containers[c.Name] = usage
-		if !printContainersOnly {
 		for _, res := range MeasuredResources {
 			quantity := podMetrics[res]
-				quantity.Add(usage[res])
+			quantity.Add(c.Usage[res])
 			podMetrics[res] = quantity
 		}
 	}
-	}
+	return podMetrics
-	return podMetrics, containers, nil
 }
 
 func printMetricsLine(out io.Writer, metrics *ResourceMetricsInfo) {