From c32d10f3cf52d82b606d9cfed432aed1870b2cee Mon Sep 17 00:00:00 2001 From: juanvallejo Date: Wed, 30 May 2018 15:24:55 -0400 Subject: [PATCH] add prototype sorting for table rows --- hack/testdata/sorted-pods/sorted-pod1.yaml | 3 +- hack/testdata/sorted-pods/sorted-pod2.yaml | 3 +- hack/testdata/sorted-pods/sorted-pod3.yaml | 3 +- pkg/kubectl/BUILD | 1 + pkg/kubectl/cmd/get/BUILD | 1 + pkg/kubectl/cmd/get/get.go | 134 +++++++++++++++++-- pkg/kubectl/cmd/get/get_test.go | 146 +++++++++++++++++++++ pkg/kubectl/sorter.go | 94 ++++++++++--- test/cmd/get.sh | 18 +++ 9 files changed, 370 insertions(+), 33 deletions(-) diff --git a/hack/testdata/sorted-pods/sorted-pod1.yaml b/hack/testdata/sorted-pods/sorted-pod1.yaml index aa767a2ea2..cb8862678b 100644 --- a/hack/testdata/sorted-pods/sorted-pod1.yaml +++ b/hack/testdata/sorted-pods/sorted-pod1.yaml @@ -2,9 +2,10 @@ apiVersion: v1 kind: Pod metadata: name: sorted-pod1 + creationTimestamp: 2018-08-30T14:10:58Z labels: name: sorted-pod3-label spec: containers: - - name: kubernetes-pause + - name: kubernetes-pause2 image: k8s.gcr.io/pause:2.0 diff --git a/hack/testdata/sorted-pods/sorted-pod2.yaml b/hack/testdata/sorted-pods/sorted-pod2.yaml index f05040569b..1837dcdf7a 100644 --- a/hack/testdata/sorted-pods/sorted-pod2.yaml +++ b/hack/testdata/sorted-pods/sorted-pod2.yaml @@ -2,9 +2,10 @@ apiVersion: v1 kind: Pod metadata: name: sorted-pod2 + creationTimestamp: 2018-08-30T14:10:55Z labels: name: sorted-pod2-label spec: containers: - - name: kubernetes-pause + - name: kubernetes-pause1 image: k8s.gcr.io/pause:2.0 diff --git a/hack/testdata/sorted-pods/sorted-pod3.yaml b/hack/testdata/sorted-pods/sorted-pod3.yaml index e02c501fa1..b77416fd22 100644 --- a/hack/testdata/sorted-pods/sorted-pod3.yaml +++ b/hack/testdata/sorted-pods/sorted-pod3.yaml @@ -2,9 +2,10 @@ apiVersion: v1 kind: Pod metadata: name: sorted-pod3 + creationTimestamp: 2018-08-30T14:10:53Z labels: name: sorted-pod1-label spec: containers: - - name: kubernetes-pause + - name: kubernetes-pause3 image: k8s.gcr.io/pause:2.0 diff --git a/pkg/kubectl/BUILD b/pkg/kubectl/BUILD index 8c5d44409e..d87986be3a 100644 --- a/pkg/kubectl/BUILD +++ b/pkg/kubectl/BUILD @@ -144,6 +144,7 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1beta1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", diff --git a/pkg/kubectl/cmd/get/BUILD b/pkg/kubectl/cmd/get/BUILD index 73dc67a7c5..4768afcf98 100644 --- a/pkg/kubectl/cmd/get/BUILD +++ b/pkg/kubectl/cmd/get/BUILD @@ -81,6 +81,7 @@ go_test( "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/api/extensions/v1beta1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1beta1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json:go_default_library", diff --git a/pkg/kubectl/cmd/get/get.go b/pkg/kubectl/cmd/get/get.go index 510ab89fc6..185dac3438 100644 --- a/pkg/kubectl/cmd/get/get.go +++ b/pkg/kubectl/cmd/get/get.go @@ -207,10 +207,10 @@ func (o *GetOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []stri o.NoHeaders = cmdutil.GetFlagBool(cmd, "no-headers") - // TODO (soltysh): currently we don't support sorting and custom columns + // TODO (soltysh): currently we don't support custom columns // with server side print. So in these cases force the old behavior. outputOption := cmd.Flags().Lookup("output").Value.String() - if o.Sort && outputOption == "custom-columns" { + if outputOption == "custom-columns" { o.ServerPrint = false } @@ -296,6 +296,96 @@ func (o *GetOptions) Validate(cmd *cobra.Command) error { return nil } +type OriginalPositioner interface { + OriginalPosition(int) int +} + +type NopPositioner struct{} + +func (t *NopPositioner) OriginalPosition(ix int) int { + return ix +} + +type RuntimeSorter struct { + field string + decoder runtime.Decoder + objects []runtime.Object + positioner OriginalPositioner +} + +func (r *RuntimeSorter) Sort() error { + if len(r.objects) <= 1 { + // a list is only considered "sorted" if there are 0 or 1 items in it + // AND (if 1 item) the item is not a Table object + _, isTable := r.objects[0].(*metav1beta1.Table) + if len(r.objects) == 0 || !isTable { + return nil + } + } + + includesTable := false + includesRuntimeObjs := false + + for _, obj := range r.objects { + switch t := obj.(type) { + case *metav1beta1.Table: + includesTable = true + + if err := kubectl.NewTableSorter(t, r.field).Sort(); err != nil { + continue + } + default: + includesRuntimeObjs = true + } + } + + // we use a NopPositioner when dealing with Table objects + // because the objects themselves are not swapped, but rather + // the rows in each object are swapped / sorted. + r.positioner = &NopPositioner{} + + if includesRuntimeObjs && includesTable { + return fmt.Errorf("sorting is not supported on mixed Table and non-Table object lists") + } + if includesTable { + return nil + } + + // if not dealing with a Table response from the server, assume + // all objects are runtime.Object as usual, and sort using old method. + var err error + if r.positioner, err = kubectl.SortObjects(r.decoder, r.objects, r.field); err != nil { + return err + } + return nil +} + +func (r *RuntimeSorter) OriginalPosition(ix int) int { + if r.positioner == nil { + return 0 + } + return r.positioner.OriginalPosition(ix) +} + +// allows custom decoder to be set for testing +func (r *RuntimeSorter) WithDecoder(decoder runtime.Decoder) *RuntimeSorter { + r.decoder = decoder + return r +} + +func NewRuntimeSorter(objects []runtime.Object, sortBy string) *RuntimeSorter { + parsedField, err := printers.RelaxedJSONPathExpression(sortBy) + if err != nil { + parsedField = sortBy + } + + return &RuntimeSorter{ + field: parsedField, + decoder: cmdutil.InternalVersionDecoder(), + objects: objects, + } +} + // Run performs the get operation. // TODO: remove the need to pass these arguments, like other commands. func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) error { @@ -311,6 +401,13 @@ func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e fmt.Fprintf(o.IOStreams.ErrOut, "warning: --%s requested, --%s will be ignored\n", useOpenAPIPrintColumnFlagLabel, useServerPrintColumns) } + chunkSize := o.ChunkSize + if o.Sort { + // TODO(juanvallejo): in the future, we could have the client use chunking + // to gather all results, then sort them all at the end to reduce server load. + chunkSize = 0 + } + r := f.NewBuilder(). Unstructured(). NamespaceParam(o.Namespace).DefaultNamespace().AllNamespaces(o.AllNamespaces). @@ -318,7 +415,7 @@ func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e LabelSelectorParam(o.LabelSelector). FieldSelectorParam(o.FieldSelector). ExportParam(o.Export). - RequestChunksOf(o.ChunkSize). + RequestChunksOf(chunkSize). IncludeUninitialized(o.IncludeUninitialized). ResourceTypeOrNameArgs(true, args...). ContinueOnError(). @@ -329,12 +426,19 @@ func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e if o.PrintWithOpenAPICols { return } - if o.ServerPrint && o.IsHumanReadablePrinter && !o.Sort { - group := metav1beta1.GroupName - version := metav1beta1.SchemeGroupVersion.Version + if !o.ServerPrint || !o.IsHumanReadablePrinter { + return + } - tableParam := fmt.Sprintf("application/json;as=Table;v=%s;g=%s, application/json", version, group) - req.SetHeader("Accept", tableParam) + group := metav1beta1.GroupName + version := metav1beta1.SchemeGroupVersion.Version + + tableParam := fmt.Sprintf("application/json;as=Table;v=%s;g=%s, application/json", version, group) + req.SetHeader("Accept", tableParam) + + // if sorting, ensure we receive the full object in order to introspect its fields via jsonpath + if o.Sort { + req.Param("includeObject", "Object") } }). Do() @@ -378,12 +482,14 @@ func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e if err != nil { return err } - var sorter *kubectl.RuntimeSort - if o.Sort && len(objs) > 1 { - // TODO: questionable - if sorter, err = kubectl.SortObjects(cmdutil.InternalVersionDecoder(), objs, sorting); err != nil { + + var positioner OriginalPositioner + if o.Sort { + sorter := NewRuntimeSorter(objs, sorting) + if err := sorter.Sort(); err != nil { return err } + positioner = sorter } var printer printers.ResourcePrinter @@ -393,8 +499,8 @@ func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e for ix := range objs { var mapping *meta.RESTMapping var info *resource.Info - if sorter != nil { - info = infos[sorter.OriginalPosition(ix)] + if positioner != nil { + info = infos[positioner.OriginalPosition(ix)] mapping = info.Mapping } else { info = infos[ix] diff --git a/pkg/kubectl/cmd/get/get_test.go b/pkg/kubectl/cmd/get/get_test.go index c84a3e168b..757e9e8dc4 100644 --- a/pkg/kubectl/cmd/get/get_test.go +++ b/pkg/kubectl/cmd/get/get_test.go @@ -19,6 +19,7 @@ package get import ( "bytes" encjson "encoding/json" + "fmt" "io" "io/ioutil" "net/http" @@ -34,6 +35,7 @@ import ( api "k8s.io/api/core/v1" apiextensionsv1beta1 "k8s.io/api/extensions/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + metav1beta1 "k8s.io/apimachinery/pkg/apis/meta/v1beta1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer/json" @@ -526,6 +528,150 @@ c 0/0 0 } } +func sortTestData() []runtime.Object { + return []runtime.Object{ + &api.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "test", ResourceVersion: "10"}, + Spec: apitesting.V1DeepEqualSafePodSpec(), + }, + &api.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "b", Namespace: "test", ResourceVersion: "11"}, + Spec: apitesting.V1DeepEqualSafePodSpec(), + }, + &api.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "test", ResourceVersion: "9"}, + Spec: apitesting.V1DeepEqualSafePodSpec(), + }, + } +} + +func sortTestTableData() []runtime.Object { + return []runtime.Object{ + &metav1beta1.Table{ + TypeMeta: metav1.TypeMeta{Kind: "Table"}, + Rows: []metav1beta1.TableRow{ + { + Object: runtime.RawExtension{ + Object: &api.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "test", ResourceVersion: "10"}, + Spec: apitesting.V1DeepEqualSafePodSpec(), + }, + }, + }, + { + Object: runtime.RawExtension{ + Object: &api.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "b", Namespace: "test", ResourceVersion: "11"}, + Spec: apitesting.V1DeepEqualSafePodSpec(), + }, + }, + }, + { + Object: runtime.RawExtension{ + Object: &api.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "test", ResourceVersion: "9"}, + Spec: apitesting.V1DeepEqualSafePodSpec(), + }, + }, + }, + }, + }, + } +} + +func TestRuntimeSorter(t *testing.T) { + tests := []struct { + name string + field string + objs []runtime.Object + op func(sorter *RuntimeSorter, objs []runtime.Object, out io.Writer) error + expect string + expectError string + }{ + { + name: "ensure sorter returns original position", + field: "metadata.name", + objs: sortTestData(), + op: func(sorter *RuntimeSorter, objs []runtime.Object, out io.Writer) error { + for idx := range objs { + p := sorter.OriginalPosition(idx) + fmt.Fprintf(out, "%v,", p) + } + return nil + }, + expect: "2,1,0,", + }, + { + name: "ensure sorter handles table object position", + field: "metadata.name", + objs: sortTestTableData(), + op: func(sorter *RuntimeSorter, objs []runtime.Object, out io.Writer) error { + for idx := range objs { + p := sorter.OriginalPosition(idx) + fmt.Fprintf(out, "%v,", p) + } + return nil + }, + expect: "0,", + }, + { + name: "ensure sorter sorts table objects", + field: "metadata.name", + objs: sortTestData(), + op: func(sorter *RuntimeSorter, objs []runtime.Object, out io.Writer) error { + for _, o := range objs { + fmt.Fprintf(out, "%s,", o.(*api.Pod).Name) + } + return nil + }, + expect: "a,b,c,", + }, + { + name: "ensure sorter rejects mixed Table + non-Table object lists", + field: "metadata.name", + objs: append(sortTestData(), sortTestTableData()...), + op: func(sorter *RuntimeSorter, objs []runtime.Object, out io.Writer) error { return nil }, + expectError: "sorting is not supported on mixed Table", + }, + { + name: "ensure sorter errors out on invalid jsonpath", + field: "metadata.unknown", + objs: sortTestData(), + op: func(sorter *RuntimeSorter, objs []runtime.Object, out io.Writer) error { return nil }, + expectError: "couldn't find any field with path", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + sorter := NewRuntimeSorter(tc.objs, tc.field) + if err := sorter.Sort(); err != nil { + if len(tc.expectError) > 0 && strings.Contains(err.Error(), tc.expectError) { + return + } + + if len(tc.expectError) > 0 { + t.Fatalf("unexpected error: expecting %s, but got %s", tc.expectError, err) + } + + t.Fatalf("unexpected error: %v", err) + } + + out := bytes.NewBuffer([]byte{}) + err := tc.op(sorter, tc.objs, out) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if tc.expect != out.String() { + t.Fatalf("unexpected output: expecting %s, but got %s", tc.expect, out.String()) + } + + }) + } + +} + func TestGetObjectsIdentifiedByFile(t *testing.T) { pods, _, _ := testData() diff --git a/pkg/kubectl/sorter.go b/pkg/kubectl/sorter.go index 8c7566a563..b7c339dfb2 100644 --- a/pkg/kubectl/sorter.go +++ b/pkg/kubectl/sorter.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + metav1beta1 "k8s.io/apimachinery/pkg/apis/meta/v1beta1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/util/integer" "k8s.io/client-go/util/jsonpath" @@ -111,12 +112,7 @@ func SortObjects(decoder runtime.Decoder, objs []runtime.Object, fieldInput stri // Note that this requires empty fields to be considered later, when sorting. var fieldFoundOnce bool for _, obj := range objs { - var values [][]reflect.Value - if unstructured, ok := obj.(*unstructured.Unstructured); ok { - values, err = parser.FindResults(unstructured.Object) - } else { - values, err = parser.FindResults(reflect.ValueOf(obj).Elem().Interface()) - } + values, err := findJSONPathResults(parser, obj) if err != nil { return nil, err } @@ -274,20 +270,12 @@ func (r *RuntimeSort) Less(i, j int) bool { panic(err) } - if unstructured, ok := iObj.(*unstructured.Unstructured); ok { - iValues, err = parser.FindResults(unstructured.Object) - } else { - iValues, err = parser.FindResults(reflect.ValueOf(iObj).Elem().Interface()) - } + iValues, err = findJSONPathResults(parser, iObj) if err != nil { glog.Fatalf("Failed to get i values for %#v using %s (%#v)", iObj, r.field, err) } - if unstructured, ok := jObj.(*unstructured.Unstructured); ok { - jValues, err = parser.FindResults(unstructured.Object) - } else { - jValues, err = parser.FindResults(reflect.ValueOf(jObj).Elem().Interface()) - } + jValues, err = findJSONPathResults(parser, jObj) if err != nil { glog.Fatalf("Failed to get j values for %#v using %s (%v)", jObj, r.field, err) } @@ -316,3 +304,77 @@ func (r *RuntimeSort) OriginalPosition(ix int) int { } return r.origPosition[ix] } + +type TableSorter struct { + field string + obj *metav1beta1.Table +} + +func (t *TableSorter) Len() int { + return len(t.obj.Rows) +} + +func (t *TableSorter) Swap(i, j int) { + t.obj.Rows[i], t.obj.Rows[j] = t.obj.Rows[j], t.obj.Rows[i] +} + +func (t *TableSorter) Less(i, j int) bool { + iObj := t.obj.Rows[i].Object.Object + jObj := t.obj.Rows[j].Object.Object + + var iValues [][]reflect.Value + var jValues [][]reflect.Value + var err error + + parser := jsonpath.New("sorting").AllowMissingKeys(true) + err = parser.Parse(t.field) + if err != nil { + glog.Fatalf("sorting error: %v\n", err) + } + + // TODO(juanvallejo): this is expensive for very large sets. + // To improve runtime complexity, build an array which contains all + // resolved fields, and sort that instead. + iValues, err = findJSONPathResults(parser, iObj) + if err != nil { + glog.Fatalf("Failed to get i values for %#v using %s (%#v)", iObj, t.field, err) + } + + jValues, err = findJSONPathResults(parser, jObj) + if err != nil { + glog.Fatalf("Failed to get j values for %#v using %s (%v)", jObj, t.field, err) + } + + if len(iValues) == 0 || len(iValues[0]) == 0 || len(jValues) == 0 || len(jValues[0]) == 0 { + glog.Fatalf("couldn't find any field with path %q in the list of objects", t.field) + } + + iField := iValues[0][0] + jField := jValues[0][0] + + less, err := isLess(iField, jField) + if err != nil { + glog.Fatalf("Field %s in %T is an unsortable type: %s, err: %v", t.field, iObj, iField.Kind().String(), err) + } + return less +} + +func (t *TableSorter) Sort() error { + sort.Sort(t) + return nil +} + +func NewTableSorter(table *metav1beta1.Table, field string) *TableSorter { + return &TableSorter{ + obj: table, + field: field, + } +} + +func findJSONPathResults(parser *jsonpath.JSONPath, from runtime.Object) ([][]reflect.Value, error) { + if unstructuredObj, ok := from.(*unstructured.Unstructured); ok { + return parser.FindResults(unstructuredObj.Object) + } + + return parser.FindResults(reflect.ValueOf(from).Elem().Interface()) +} diff --git a/test/cmd/get.sh b/test/cmd/get.sh index e42b0ea27f..089e5aa4e5 100755 --- a/test/cmd/get.sh +++ b/test/cmd/get.sh @@ -240,6 +240,11 @@ run_kubectl_sort_by_tests() { # Check output of sort-by output_message=$(kubectl get pods --sort-by="{metadata.name}") kube::test::if_has_string "${output_message}" "valid-pod" + # ensure sort-by receivers objects as Table + output_message=$(kubectl get pods --v=8 --sort-by="{metadata.name}" 2>&1) + kube::test::if_has_string "${output_message}" "as=Table" + # ensure sort-by requests the full object + kube::test::if_has_string "${output_message}" "includeObject=Object" ### Clean up # Pre-condition: valid-pod exists kube::test::get_object_assert pods "{{range.items}}{{$id_field}}:{{end}}" 'valid-pod:' @@ -273,6 +278,19 @@ run_kubectl_sort_by_tests() { output_message=$(kubectl get pods --sort-by="{metadata.labels.name}") kube::test::if_sort_by_has_correct_order "${output_message}" "sorted-pod3:sorted-pod2:sorted-pod1:" + # if sorting, we should be able to use any field in our objects + output_message=$(kubectl get pods --sort-by="{spec.containers[0].name}") + kube::test::if_sort_by_has_correct_order "${output_message}" "sorted-pod2:sorted-pod1:sorted-pod3:" + + # ensure sorting by creation timestamps works + 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 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:" + kube::test::if_has_not_string "${output_message}" "Table" + ### Clean up # Pre-condition: valid-pod exists kube::test::get_object_assert pods "{{range.items}}{{$id_field}}:{{end}}" 'sorted-pod1:sorted-pod2:sorted-pod3:'