Merge pull request #26046 from timoreimann/stabilize-map-order-in-kubectl-describe

Automatic merge from submit-queue

Stabilize map order in kubectl describe

Refs #25251.

Add `SortedResourceNames()` methods to map type aliases in order to achieve stable output order for `kubectl` descriptors.

This affects QoS classes, resource limits, and resource requests.

A few remarks:

1. I couldn't find map usages for described fields other than the ones mentioned above. Then again, I failed to identify those programmatically/systematically. Pointers given, I'd be happy to cover any gaps within this PR or along additional ones.
1. It's somewhat difficult to deterministically test a function that brings reliable ordering to Go maps due to its randomizing nature. None of the possibilities I came up with (rely a "probabilistic testing" against repeatedly created maps, add complexity through additional interfaces) seemed very appealing to me, so I went with testing my `sort.Interface` implementation and the changed logic in `kubectl.describeContainers()`.
1. It's apparently not possible to implement a single function that sorts any map's keys generically in Go without producing lots of boilerplate: a `map[<key type>]interface{}` is different from any other map type and thus requires explicit iteration on the caller site to convert back and forth. Unfortunately, this makes it hard to completely avoid code/test duplication.

Please let me know what you think.

pkg/kubectl/describe.go

@@ -847,21 +847,25 @@ func describeContainers(label string, containers []api.Container, containerStatu
 		if len(resourceToQoS) > 0 {
 			fmt.Fprintf(out, "    QoS Tier:\n")
 		}
-		for resource, qos := range resourceToQoS {
+		for _, resource := range SortedQoSResourceNames(resourceToQoS) {
+			qos := resourceToQoS[resource]
 			fmt.Fprintf(out, "      %s:\t%s\n", resource, qos)
 		}
 
-		if len(container.Resources.Limits) > 0 {
+		resources := container.Resources
+		if len(resources.Limits) > 0 {
 			fmt.Fprintf(out, "    Limits:\n")
 		}
-		for name, quantity := range container.Resources.Limits {
+		for _, name := range SortedResourceNames(resources.Limits) {
+			quantity := resources.Limits[name]
 			fmt.Fprintf(out, "      %s:\t%s\n", name, quantity.String())
 		}
 
-		if len(container.Resources.Requests) > 0 {
+		if len(resources.Requests) > 0 {
 			fmt.Fprintf(out, "    Requests:\n")
 		}
-		for name, quantity := range container.Resources.Requests {
+		for _, name := range SortedResourceNames(resources.Requests) {
+			quantity := resources.Requests[name]
 			fmt.Fprintf(out, "      %s:\t%s\n", name, quantity.String())
 		}
 

pkg/kubectl/describe_test.go

@@ -238,6 +238,19 @@ func TestDescribeContainers(t *testing.T) {
 			},
 			expectedElements: []string{"test", "State", "Waiting", "Ready", "True", "Restart Count", "7", "Image", "image", "time", "1000"},
 		},
+		// QoS classes
+		{
+			container: api.Container{
+				Name:  "test",
+				Image: "image",
+			},
+			status: api.ContainerStatus{
+				Name:         "test",
+				Ready:        true,
+				RestartCount: 7,
+			},
+			expectedElements: []string{"cpu", "BestEffort", "memory", "BestEffort"},
+		},
 		// Using limits.
 		{
 			container: api.Container{
@@ -258,6 +271,21 @@ func TestDescribeContainers(t *testing.T) {
 			},
 			expectedElements: []string{"cpu", "1k", "memory", "4G", "storage", "20G"},
 		},
+		// Using requests.
+		{
+			container: api.Container{
+				Name:  "test",
+				Image: "image",
+				Resources: api.ResourceRequirements{
+					Requests: api.ResourceList{
+						api.ResourceName(api.ResourceCPU):     resource.MustParse("1000"),
+						api.ResourceName(api.ResourceMemory):  resource.MustParse("4G"),
+						api.ResourceName(api.ResourceStorage): resource.MustParse("20G"),
+					},
+				},
+			},
+			expectedElements: []string{"cpu", "1k", "memory", "4G", "storage", "20G"},
+		},
 	}
 
 	for i, testCase := range testCases {

pkg/kubectl/sorted_resource_name_list.go

@@ -17,7 +17,10 @@ limitations under the License.
 package kubectl
 
 import (
+	"sort"
+
 	"k8s.io/kubernetes/pkg/api"
+	qosutil "k8s.io/kubernetes/pkg/kubelet/qos/util"
 )
 
 type SortableResourceNames []api.ResourceName
@@ -34,6 +37,16 @@ func (list SortableResourceNames) Less(i, j int) bool {
 	return list[i] < list[j]
 }
 
+// SortedResourceNames returns the sorted resource names of a resource list.
+func SortedResourceNames(list api.ResourceList) []api.ResourceName {
+	resources := make([]api.ResourceName, 0, len(list))
+	for res := range list {
+		resources = append(resources, res)
+	}
+	sort.Sort(SortableResourceNames(resources))
+	return resources
+}
+
 type SortableResourceQuotas []api.ResourceQuota
 
 func (list SortableResourceQuotas) Len() int {
@@ -47,3 +60,13 @@ func (list SortableResourceQuotas) Swap(i, j int) {
 func (list SortableResourceQuotas) Less(i, j int) bool {
 	return list[i].Name < list[j].Name
 }
+
+// SortedQoSResourceNames returns the sorted resource names of a QoS list.
+func SortedQoSResourceNames(list qosutil.QoSList) []api.ResourceName {
+	resources := make([]api.ResourceName, 0, len(list))
+	for res := range list {
+		resources = append(resources, res)
+	}
+	sort.Sort(SortableResourceNames(resources))
+	return resources
+}

pkg/kubectl/sorted_resource_name_list_test.go

@@ -0,0 +1,50 @@
+/*
+Copyright 2016 The Kubernetes Authors All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package kubectl
+
+import (
+	"reflect"
+	"sort"
+	"testing"
+
+	"k8s.io/kubernetes/pkg/api"
+)
+
+func TestSortableResourceNamesSorting(t *testing.T) {
+	want := SortableResourceNames{
+		api.ResourceName(""),
+		api.ResourceName("42"),
+		api.ResourceName("bar"),
+		api.ResourceName("foo"),
+		api.ResourceName("foo"),
+		api.ResourceName("foobar"),
+	}
+
+	in := SortableResourceNames{
+		api.ResourceName("foo"),
+		api.ResourceName("42"),
+		api.ResourceName("foobar"),
+		api.ResourceName("foo"),
+		api.ResourceName("bar"),
+		api.ResourceName(""),
+	}
+
+	sort.Sort(in)
+	if !reflect.DeepEqual(in, want) {
+		t.Errorf("got %v, want %v", in, want)
+	}
+}

pkg/kubelet/qos/util/qos.go

@@ -105,9 +105,12 @@ func GetPodQos(pod *api.Pod) string {
 	return Burstable
 }
 
-// GetQos returns a mapping of resource name to QoS class of a container
+// QoSList is a set of (resource name, QoS class) pairs.
-func GetQoS(container *api.Container) map[api.ResourceName]string {
+type QoSList map[api.ResourceName]string
-	resourceToQoS := map[api.ResourceName]string{}
+
+// GetQoS returns a mapping of resource name to QoS class of a container
+func GetQoS(container *api.Container) QoSList {
+	resourceToQoS := QoSList{}
 	for resource := range allResources(container) {
 		switch {
 		case isResourceGuaranteed(container, resource):