Merge pull request #100435 from lauchokyip/fixSort

Fixed sort-by not sorting Resources as expected

hack/testdata/sorted-pods/sorted-pod1.yaml

@@ -9,3 +9,10 @@ spec:
   containers:
   - name: kubernetes-pause2
     image: k8s.gcr.io/pause:3.5
+    resources: 
+      requests:
+        memory: "64Mi"
+        cpu: "250m"
+      limits:
+        memory: "128Mi"
+        cpu: "500m"

hack/testdata/sorted-pods/sorted-pod2.yaml

@@ -9,3 +9,11 @@ spec:
   containers:
   - name: kubernetes-pause1
     image: k8s.gcr.io/pause:3.5
+    resources: 
+      requests:
+        memory: "1G"
+        cpu: "0.5"
+      limits:
+        memory: "2G"
+        cpu: "1"
+

staging/src/k8s.io/kubectl/pkg/cmd/get/get_test.go

@@ -860,7 +860,7 @@ func TestGetSortedObjects(t *testing.T) {
 	cmd := NewCmdGet("kubectl", tf, streams)
 	cmd.SetOutput(buf)
 
-	// sorting with metedata.name
+	// sorting with metadata.name
 	cmd.Flags().Set("sort-by", ".metadata.name")
 	cmd.Run(cmd, []string{"pods"})
 
@@ -899,7 +899,7 @@ func TestGetSortedObjectsUnstructuredTable(t *testing.T) {
 	cmd := NewCmdGet("kubectl", tf, streams)
 	cmd.SetOutput(buf)
 
-	// sorting with metedata.name
+	// sorting with metadata.name
 	cmd.Flags().Set("sort-by", ".metadata.name")
 	cmd.Run(cmd, []string{"pods"})
 

staging/src/k8s.io/kubectl/pkg/cmd/get/sorter.go

@@ -26,6 +26,7 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -95,6 +96,8 @@ func (s *SortingPrinter) sortObj(obj runtime.Object) error {
 	return meta.SetList(obj, objs)
 }
 
+// SortObjects sorts the runtime.Object based on fieldInput and returns RuntimeSort that implements
+// the golang sort interface
 func SortObjects(decoder runtime.Decoder, objs []runtime.Object, fieldInput string) (*RuntimeSort, error) {
 	for ix := range objs {
 		item := objs[ix]
@@ -151,6 +154,7 @@ type RuntimeSort struct {
 	origPosition []int
 }
 
+// NewRuntimeSort creates a new RuntimeSort struct that implements golang sort interface
 func NewRuntimeSort(field string, objs []runtime.Object) *RuntimeSort {
 	sorter := &RuntimeSort{field: field, objs: objs, origPosition: make([]int, len(objs))}
 	for ix := range objs {
@@ -187,6 +191,11 @@ func isLess(i, j reflect.Value) (bool, error) {
 			time := j.Interface().(metav1.Time)
 			return t.Before(&time), nil
 		}
+		// sort resource.Quantity
+		if iQuantity, ok := in.(resource.Quantity); ok {
+			jQuantity := j.Interface().(resource.Quantity)
+			return iQuantity.Cmp(jQuantity) < 0, nil
+		}
 		// fallback to the fields comparison
 		for idx := 0; idx < i.NumField(); idx++ {
 			less, err := isLess(i.Field(idx), j.Field(idx))
@@ -204,7 +213,6 @@ func isLess(i, j reflect.Value) (bool, error) {
 			}
 		}
 		return true, nil
-
 	case reflect.Interface:
 		if i.IsNil() && j.IsNil() {
 			return false, nil
@@ -264,8 +272,18 @@ func isLess(i, j reflect.Value) (bool, error) {
 			}
 		case string:
 			if jtype, ok := j.Interface().(string); ok {
+				// check if it's a Quantity
+				itypeQuantity, err := resource.ParseQuantity(itype)
+				if err != nil {
 					return sortorder.NaturalLess(itype, jtype), nil
 				}
+				jtypeQuantity, _ := resource.ParseQuantity(jtype)
+				if err != nil {
+					return sortorder.NaturalLess(itype, jtype), nil
+				}
+				// Both strings are quantity
+				return itypeQuantity.Cmp(jtypeQuantity) < 0, nil
+			}
 		default:
 			return false, fmt.Errorf("unsortable type: %T", itype)
 		}

staging/src/k8s.io/kubectl/pkg/cmd/get/sorter_test.go

@@ -24,6 +24,7 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	metav1beta1 "k8s.io/apimachinery/pkg/apis/meta/v1beta1"
@@ -47,6 +48,59 @@ func encodeOrDie(obj runtime.Object) []byte {
 	}
 	return data
 }
+func createPodSpecResource(t *testing.T, memReq, memLimit, cpuReq, cpuLimit string) corev1.PodSpec {
+	t.Helper()
+	podSpec := corev1.PodSpec{
+		Containers: []corev1.Container{
+			{
+				Resources: corev1.ResourceRequirements{
+					Requests: corev1.ResourceList{},
+					Limits:   corev1.ResourceList{},
+				},
+			},
+		},
+	}
+
+	req := podSpec.Containers[0].Resources.Requests
+	if memReq != "" {
+		memReq, err := resource.ParseQuantity(memReq)
+		if err != nil {
+			t.Errorf("memory request string is not a valid quantity")
+		}
+		req["memory"] = memReq
+	}
+	if cpuReq != "" {
+		cpuReq, err := resource.ParseQuantity(cpuReq)
+		if err != nil {
+			t.Errorf("cpu request string is not a valid quantity")
+		}
+		req["cpu"] = cpuReq
+	}
+	limit := podSpec.Containers[0].Resources.Limits
+	if memLimit != "" {
+		memLimit, err := resource.ParseQuantity(memLimit)
+		if err != nil {
+			t.Errorf("memory limit string is not a valid quantity")
+		}
+		limit["memory"] = memLimit
+	}
+	if cpuLimit != "" {
+		cpuLimit, err := resource.ParseQuantity(cpuLimit)
+		if err != nil {
+			t.Errorf("cpu limit string is not a valid quantity")
+		}
+		limit["cpu"] = cpuLimit
+	}
+
+	return podSpec
+}
+func createUnstructuredPodResource(t *testing.T, memReq, memLimit, cpuReq, cpuLimit string) unstructured.Unstructured {
+	t.Helper()
+	pod := &corev1.Pod{
+		Spec: createPodSpecResource(t, memReq, memLimit, cpuReq, cpuLimit),
+	}
+	return *toUnstructuredOrDie(encodeOrDie(pod))
+}
 
 func TestSortingPrinter(t *testing.T) {
 	intPtr := func(val int32) *int32 { return &val }
@@ -435,6 +489,158 @@ func TestSortingPrinter(t *testing.T) {
 			},
 			field: "{.lastTimestamp}",
 		},
+		{
+			name: "pod-resources-cpu-random-order-with-missing-fields",
+			obj: &corev1.PodList{
+				Items: []corev1.Pod{
+					{
+						Spec: createPodSpecResource(t, "", "", "0.5", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "", "", "10", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "", "", "100m", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "", "", "", ""),
+					},
+				},
+			},
+			sort: &corev1.PodList{
+				Items: []corev1.Pod{
+					{
+						Spec: createPodSpecResource(t, "", "", "", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "", "", "100m", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "", "", "0.5", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "", "", "10", ""),
+					},
+				},
+			},
+			field: "{.spec.containers[].resources.requests.cpu}",
+		},
+		{
+			name: "pod-resources-memory-random-order-with-missing-fields",
+			obj: &corev1.PodList{
+				Items: []corev1.Pod{
+					{
+						Spec: createPodSpecResource(t, "128Mi", "", "", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "10Ei", "", "", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "8Ti", "", "", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "64Gi", "", "", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "55Pi", "", "", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "2Ki", "", "", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "", "", "", ""),
+					},
+				},
+			},
+			sort: &corev1.PodList{
+				Items: []corev1.Pod{
+					{
+						Spec: createPodSpecResource(t, "", "", "", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "2Ki", "", "", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "128Mi", "", "", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "64Gi", "", "", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "8Ti", "", "", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "55Pi", "", "", ""),
+					},
+					{
+						Spec: createPodSpecResource(t, "10Ei", "", "", ""),
+					},
+				},
+			},
+			field: "{.spec.containers[].resources.requests.memory}",
+		},
+		{
+			name: "pod-unstructured-resources-cpu-random-order-with-missing-fields",
+			obj: &unstructured.UnstructuredList{
+				Object: map[string]interface{}{
+					"kind":       "List",
+					"apiVersion": "v1",
+				},
+				Items: []unstructured.Unstructured{
+					createUnstructuredPodResource(t, "", "", "0.5", ""),
+					createUnstructuredPodResource(t, "", "", "10", ""),
+					createUnstructuredPodResource(t, "", "", "100m", ""),
+					createUnstructuredPodResource(t, "", "", "", ""),
+				},
+			},
+			sort: &unstructured.UnstructuredList{
+				Object: map[string]interface{}{
+					"kind":       "List",
+					"apiVersion": "v1",
+				},
+				Items: []unstructured.Unstructured{
+					createUnstructuredPodResource(t, "", "", "", ""),
+					createUnstructuredPodResource(t, "", "", "100m", ""),
+					createUnstructuredPodResource(t, "", "", "0.5", ""),
+					createUnstructuredPodResource(t, "", "", "10", ""),
+				},
+			},
+			field: "{.spec.containers[].resources.requests.cpu}",
+		},
+		{
+			name: "pod-unstructured-resources-memory-random-order-with-missing-fields",
+			obj: &unstructured.UnstructuredList{
+				Object: map[string]interface{}{
+					"kind":       "List",
+					"apiVersion": "v1",
+				},
+				Items: []unstructured.Unstructured{
+					createUnstructuredPodResource(t, "128Mi", "", "", ""),
+					createUnstructuredPodResource(t, "10Ei", "", "", ""),
+					createUnstructuredPodResource(t, "8Ti", "", "", ""),
+					createUnstructuredPodResource(t, "64Gi", "", "", ""),
+					createUnstructuredPodResource(t, "55Pi", "", "", ""),
+					createUnstructuredPodResource(t, "2Ki", "", "", ""),
+					createUnstructuredPodResource(t, "", "", "", ""),
+				},
+			},
+			sort: &unstructured.UnstructuredList{
+				Object: map[string]interface{}{
+					"kind":       "List",
+					"apiVersion": "v1",
+				},
+				Items: []unstructured.Unstructured{
+					createUnstructuredPodResource(t, "", "", "", ""),
+					createUnstructuredPodResource(t, "2Ki", "", "", ""),
+					createUnstructuredPodResource(t, "128Mi", "", "", ""),
+					createUnstructuredPodResource(t, "64Gi", "", "", ""),
+					createUnstructuredPodResource(t, "8Ti", "", "", ""),
+					createUnstructuredPodResource(t, "55Pi", "", "", ""),
+					createUnstructuredPodResource(t, "10Ei", "", "", ""),
+				},
+			},
+			field: "{.spec.containers[].resources.requests.memory}",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name+" table", func(t *testing.T) {
@@ -505,16 +711,19 @@ func TestRuntimeSortLess(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "b",
 				},
+				Spec: createPodSpecResource(t, "0.5", "", "1Gi", ""),
 			},
 			{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "c",
 				},
+				Spec: createPodSpecResource(t, "2", "", "1Ti", ""),
 			},
 			{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "a",
 				},
+				Spec: createPodSpecResource(t, "10m", "", "1Ki", ""),
 			},
 		},
 	}
@@ -524,8 +733,15 @@ func TestRuntimeSortLess(t *testing.T) {
 		t.Fatalf("ExtractList testobj got unexpected error: %v", err)
 	}
 
-	testfield := "{.metadata.name}"
+	testfieldName := "{.metadata.name}"
-	testruntimeSort := NewRuntimeSort(testfield, testobjs)
+	testruntimeSortName := NewRuntimeSort(testfieldName, testobjs)
+
+	testfieldCPU := "{.spec.containers[].resources.requests.cpu}"
+	testruntimeSortCPU := NewRuntimeSort(testfieldCPU, testobjs)
+
+	testfieldMemory := "{.spec.containers[].resources.requests.memory}"
+	testruntimeSortMemory := NewRuntimeSort(testfieldMemory, testobjs)
+
 	tests := []struct {
 		name         string
 		runtimeSort  *RuntimeSort
@@ -535,19 +751,68 @@ func TestRuntimeSortLess(t *testing.T) {
 		expectErr    bool
 	}{
 		{
-			name:         "test less true",
+			name:         "test name b c less true",
-			runtimeSort:  testruntimeSort,
+			runtimeSort:  testruntimeSortName,
 			i:            0,
 			j:            1,
 			expectResult: true,
 		},
 		{
-			name:         "test less false",
+			name:         "test name c a less false",
-			runtimeSort:  testruntimeSort,
+			runtimeSort:  testruntimeSortName,
 			i:            1,
 			j:            2,
 			expectResult: false,
 		},
+		{
+			name:         "test name b a less false",
+			runtimeSort:  testruntimeSortName,
+			i:            0,
+			j:            2,
+			expectResult: false,
+		},
+		{
+			name:         "test cpu 0.5 2 less true",
+			runtimeSort:  testruntimeSortCPU,
+			i:            0,
+			j:            1,
+			expectResult: true,
+		},
+		{
+			name:         "test cpu 2 10mi less false",
+			runtimeSort:  testruntimeSortCPU,
+			i:            1,
+			j:            2,
+			expectResult: false,
+		},
+		{
+			name:         "test cpu 0.5 10mi less false",
+			runtimeSort:  testruntimeSortCPU,
+			i:            0,
+			j:            2,
+			expectResult: false,
+		},
+		{
+			name:         "test memory 1Gi 1Ti less true",
+			runtimeSort:  testruntimeSortMemory,
+			i:            0,
+			j:            1,
+			expectResult: true,
+		},
+		{
+			name:         "test memory 1Ti 1Ki less false",
+			runtimeSort:  testruntimeSortMemory,
+			i:            1,
+			j:            2,
+			expectResult: false,
+		},
+		{
+			name:         "test memory 1Gi 1Ki less false",
+			runtimeSort:  testruntimeSortMemory,
+			i:            0,
+			j:            2,
+			expectResult: false,
+		},
 	}
 
 	for i, test := range tests {

test/cmd/get.sh

@@ -315,6 +315,22 @@ run_kubectl_sort_by_tests() {
   output_message=$(kubectl get pods --sort-by="{metadata.creationTimestamp}")
   kube::test::if_sort_by_has_correct_order "${output_message}" "sorted-pod1:sorted-pod2:sorted-pod3:"
 
+  # ensure sorting by resource memory request works
+  output_message=$(kubectl get pods --sort-by="{.spec.containers[0].resources.requests.memory}")
+  kube::test::if_sort_by_has_correct_order "${output_message}" "sorted-pod3:sorted-pod1:sorted-pod2:"
+
+  # ensure sorting by resource cpu request works
+  output_message=$(kubectl get pods --sort-by="{.spec.containers[0].resources.requests.cpu}")
+  kube::test::if_sort_by_has_correct_order "${output_message}" "sorted-pod3:sorted-pod1:sorted-pod2:"
+
+  # ensure sorting by resource memory limit works
+  output_message=$(kubectl get pods --sort-by="{.spec.containers[0].resources.limits.memory}")
+  kube::test::if_sort_by_has_correct_order "${output_message}" "sorted-pod3:sorted-pod1:sorted-pod2:"
+
+  # ensure sorting by resource cpu limit works
+   output_message=$(kubectl get pods --sort-by="{.spec.containers[0].resources.limits.cpu}")
+  kube::test::if_sort_by_has_correct_order "${output_message}" "sorted-pod3:sorted-pod1:sorted-pod2:"
+
   # ensure sorting using fallback codepath still works
   output_message=$(kubectl get pods --sort-by="{spec.containers[0].name}" --server-print=false --v=8 2>&1)
   kube::test::if_sort_by_has_correct_order "${output_message}" "sorted-pod2:sorted-pod1:sorted-pod3:"