Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Handle nil elements when sorting, instead of panicking #94666

Merged
merged 1 commit into from Sep 21, 2020
Merged

Handle nil elements when sorting, instead of panicking #94666

merged 1 commit into from Sep 21, 2020

Conversation

soltysh
Copy link
Contributor

@soltysh soltysh commented Sep 9, 2020

What type of PR is this?
/kind bug
/sig cli

What this PR does / why we need it:
Currently if you try to sort using an optional field in the API, which can be empty you'll get a nasty error like this:

$ kubectl get events --sort-by='.lastTimestamp'
F0909 22:00:10.892563   61466 sorter.go:353] Field {.lastTimestamp} in [][][]reflect.Value is an unsortable type: interface, err: unsortable type: <nil>
goroutine 1 [running]:
k8s.io/kubernetes/vendor/k8s.io/klog/v2.stacks(0xc0000ca001, 0xc000554000, 0x99, 0xeb)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/v2/klog.go:996 +0xb8
k8s.io/kubernetes/vendor/k8s.io/klog/v2.(*loggingT).output(0x2f69600, 0xc000000003, 0x0, 0x0, 0xc0001560e0, 0x2d3c647, 0x9, 0x161, 0x0)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/v2/klog.go:945 +0x19d
k8s.io/kubernetes/vendor/k8s.io/klog/v2.(*loggingT).printf(0x2f69600, 0x3, 0x0, 0x0, 0x1bd95b8, 0x31, 0xc00067d6d0, 0x4, 0x4)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/v2/klog.go:733 +0x17b
k8s.io/kubernetes/vendor/k8s.io/klog/v2.Fatalf(...)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/v2/klog.go:1456
k8s.io/kubernetes/vendor/k8s.io/kubectl/pkg/cmd/get.(*TableSorter).Less(0xc0009618f0, 0x0, 0x2a, 0xc00067d868)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/kubectl/pkg/cmd/get/sorter.go:353 +0x2d7
sort.medianOfThree(0x1e8a1e0, 0xc0009618f0, 0x0, 0x2a, 0x54)
	/.gvm/gos/go1.14.6/src/sort/sort.go:76 +0x49
sort.doPivot(0x1e8a1e0, 0xc0009618f0, 0x0, 0x150, 0xc000945e40, 0xc0003b6820)
	/.gvm/gos/go1.14.6/src/sort/sort.go:101 +0x5c5
sort.quickSort(0x1e8a1e0, 0xc0009618f0, 0x0, 0x150, 0x12)
	/.gvm/gos/go1.14.6/src/sort/sort.go:190 +0x9a
sort.Sort(0x1e8a1e0, 0xc0009618f0)
	/.gvm/gos/go1.14.6/src/sort/sort.go:218 +0x79
k8s.io/kubernetes/vendor/k8s.io/kubectl/pkg/cmd/get.(*TableSorter).Sort(...)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/kubectl/pkg/cmd/get/sorter.go:359
k8s.io/kubernetes/vendor/k8s.io/kubectl/pkg/cmd/get.(*SortingPrinter).PrintObj(0xc0008d5800, 0x1e5aee0, 0xc00065e120, 0x1e4e200, 0xc0005c0540, 0x0, 0xc0005c0540)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/kubectl/pkg/cmd/get/sorter.go:56 +0x208
k8s.io/kubernetes/vendor/k8s.io/kubectl/pkg/cmd/get.(*TablePrinter).PrintObj(0xc0008cd7f0, 0x1e5b020, 0xc000286018, 0x1e4e200, 0xc0005c0540, 0xc0003bdf00, 0xc00044357d)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/kubectl/pkg/cmd/get/table_printer.go:41 +0x1ba
k8s.io/kubernetes/vendor/k8s.io/cli-runtime/pkg/printers.ResourcePrinterFunc.PrintObj(0xc000a05bc0, 0x1e5b020, 0xc000286018, 0x1e4e200, 0xc0005c0540, 0x0, 0x20)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/cli-runtime/pkg/printers/interface.go:31 +0x4e
k8s.io/kubernetes/vendor/k8s.io/kubectl/pkg/cmd/get.(*GetOptions).Run(0xc0003f61e0, 0x1eaf580, 0xc000177bc0, 0xc0004018c0, 0xc0000cfa80, 0x1, 0x4, 0x0, 0x0)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/kubectl/pkg/cmd/get/get.go:579 +0x83a
k8s.io/kubernetes/vendor/k8s.io/kubectl/pkg/cmd/get.NewCmdGet.func1(0xc0004018c0, 0xc0000cfa80, 0x1, 0x4)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/kubectl/pkg/cmd/get/get.go:167 +0x12d
k8s.io/kubernetes/vendor/github.com/spf13/cobra.(*Command).execute(0xc0004018c0, 0xc0000cfa40, 0x4, 0x4, 0xc0004018c0, 0xc0000cfa40)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/github.com/spf13/cobra/command.go:846 +0x29d
k8s.io/kubernetes/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0xc0000f6000, 0xc0000cc180, 0xc0000cc120, 0x6)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/github.com/spf13/cobra/command.go:950 +0x349
k8s.io/kubernetes/vendor/github.com/spf13/cobra.(*Command).Execute(...)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/github.com/spf13/cobra/command.go:887
main.main()
	_output/local/go/src/k8s.io/kubernetes/cmd/kubectl/kubectl.go:49 +0x21d

goroutine 18 [chan receive]:
k8s.io/kubernetes/vendor/k8s.io/klog/v2.(*loggingT).flushDaemon(0x2f69600)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/v2/klog.go:1131 +0x8b
created by k8s.io/kubernetes/vendor/k8s.io/klog/v2.init.0
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/v2/klog.go:416 +0xd6

goroutine 5 [select]:
k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.BackoffUntil(0x1c7fed0, 0x1e4e800, 0xc00026c000, 0x1, 0xc0000a40c0)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:167 +0x13f
k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.JitterUntil(0x1c7fed0, 0x12a05f200, 0x0, 0x1, 0xc0000a40c0)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:133 +0x98
k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.Until(0x1c7fed0, 0x12a05f200, 0xc0000a40c0)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:90 +0x4d
created by k8s.io/kubernetes/vendor/k8s.io/kubectl/pkg/util/logs.InitLogs
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/kubectl/pkg/util/logs/logs.go:51 +0x96

goroutine 25 [IO wait]:
internal/poll.runtime_pollWait(0x7f027e91bfe0, 0x72, 0xffffffffffffffff)
	/.gvm/gos/go1.14.6/src/runtime/netpoll.go:203 +0x55
internal/poll.(*pollDesc).wait(0xc0008f9098, 0x72, 0x3000, 0x309a, 0xffffffffffffffff)
	/.gvm/gos/go1.14.6/src/internal/poll/fd_poll_runtime.go:87 +0x45
internal/poll.(*pollDesc).waitRead(...)
	/.gvm/gos/go1.14.6/src/internal/poll/fd_poll_runtime.go:92
internal/poll.(*FD).Read(0xc0008f9080, 0xc0006b8000, 0x309a, 0x309a, 0x0, 0x0, 0x0)
	/.gvm/gos/go1.14.6/src/internal/poll/fd_unix.go:169 +0x19b
net.(*netFD).Read(0xc0008f9080, 0xc0006b8000, 0x309a, 0x309a, 0x203000, 0x63ac10, 0xc0003ba4b8)
	/.gvm/gos/go1.14.6/src/net/fd_unix.go:202 +0x4f
net.(*conn).Read(0xc00000e020, 0xc0006b8000, 0x309a, 0x309a, 0x0, 0x0, 0x0)
	/.gvm/gos/go1.14.6/src/net/net.go:184 +0x8e
crypto/tls.(*atLeastReader).Read(0xc0005ce020, 0xc0006b8000, 0x309a, 0x309a, 0x0, 0x2076, 0xc00009f9c8)
	/.gvm/gos/go1.14.6/src/crypto/tls/conn.go:760 +0x60
bytes.(*Buffer).ReadFrom(0xc0003ba5d8, 0x1e4d200, 0xc0005ce020, 0x40a545, 0x198a7a0, 0x1b0d120)
	/.gvm/gos/go1.14.6/src/bytes/buffer.go:204 +0xb1
crypto/tls.(*Conn).readFromUntil(0xc0003ba380, 0x1e4fd40, 0xc00000e020, 0x5, 0xc00000e020, 0x8)
	/.gvm/gos/go1.14.6/src/crypto/tls/conn.go:782 +0xec
crypto/tls.(*Conn).readRecordOrCCS(0xc0003ba380, 0x0, 0x0, 0xc00009fd38)
	/.gvm/gos/go1.14.6/src/crypto/tls/conn.go:589 +0x115
crypto/tls.(*Conn).readRecord(...)
	/.gvm/gos/go1.14.6/src/crypto/tls/conn.go:557
crypto/tls.(*Conn).Read(0xc0003ba380, 0xc000334000, 0x1000, 0x1000, 0x0, 0x0, 0x0)
	/.gvm/gos/go1.14.6/src/crypto/tls/conn.go:1233 +0x15b
bufio.(*Reader).Read(0xc000355ce0, 0xc00067e1f8, 0x9, 0x9, 0xc00009fd38, 0x1c80c00, 0x94ce55)
	/.gvm/gos/go1.14.6/src/bufio/bufio.go:226 +0x24f
io.ReadAtLeast(0x1e4d020, 0xc000355ce0, 0xc00067e1f8, 0x9, 0x9, 0x9, 0xc0000a6050, 0x0, 0x1e4d400)
	/.gvm/gos/go1.14.6/src/io/io.go:310 +0x87
io.ReadFull(...)
	/.gvm/gos/go1.14.6/src/io/io.go:329
k8s.io/kubernetes/vendor/golang.org/x/net/http2.readFrameHeader(0xc00067e1f8, 0x9, 0x9, 0x1e4d020, 0xc000355ce0, 0x0, 0x0, 0xc000a90030, 0x0)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/golang.org/x/net/http2/frame.go:237 +0x87
k8s.io/kubernetes/vendor/golang.org/x/net/http2.(*Framer).ReadFrame(0xc00067e1c0, 0xc000a90030, 0x0, 0x0, 0x0)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/golang.org/x/net/http2/frame.go:492 +0xa1
k8s.io/kubernetes/vendor/golang.org/x/net/http2.(*clientConnReadLoop).run(0xc00009ffa8, 0xc000066fb0, 0x738752)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/golang.org/x/net/http2/transport.go:1746 +0x8d
k8s.io/kubernetes/vendor/golang.org/x/net/http2.(*ClientConn).readLoop(0xc000093500)
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/golang.org/x/net/http2/transport.go:1674 +0x6f
created by k8s.io/kubernetes/vendor/golang.org/x/net/http2.(*Transport).newClientConn
	/workspace/k8s/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/golang.org/x/net/http2/transport.go:674 +0x64a

This PR treats those nil elements as 0/empty and sorts them this way, it might not be consistent with the output from kubectl get because that fetches that data from other fields. With Event's lastTimestamp it sometimes is replaced with its firstTimestamp, see here:

kubernetes/pkg/printers/internalversion/printers.go

Lines 1724 to 1732 in b56d0ac

firstTimestamp := translateTimestampSince(obj.FirstTimestamp)
if obj.FirstTimestamp.IsZero() {
firstTimestamp = translateMicroTimestampSince(obj.EventTime)
}
lastTimestamp := translateTimestampSince(obj.LastTimestamp)
if obj.LastTimestamp.IsZero() {
lastTimestamp = firstTimestamp
}

Special notes for your reviewer:
/assign @sallyom

Does this PR introduce a user-facing change?:

Do not fail sorting empty elements.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/cli Categorizes an issue or PR as relevant to SIG CLI. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 9, 2020
@soltysh
Copy link
Contributor Author

soltysh commented Sep 9, 2020

/priority important-longterm

@k8s-ci-robot k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. area/kubectl and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 9, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2020
hasheddan
hasheddan approved these changes Sep 12, 2020
View reviewed changes
Copy link
Contributor

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2020
@soltysh
Copy link
Contributor Author

soltysh commented Sep 21, 2020

/milestone v1.20

@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Sep 21, 2020
@k8s-ci-robot k8s-ci-robot merged commit 2d8fbd6 into kubernetes:master Sep 21, 2020
This was referenced Sep 21, 2020
Merged
Merged
Merged
@github-actions github-actions bot mentioned this pull request Sep 22, 2020
Open
@soltysh soltysh deleted the sort_nil branch September 22, 2020 10:27
@ingvagabund ingvagabund mentioned this pull request Sep 24, 2020
Merged
k8s-ci-robot added a commit that referenced this pull request Sep 29, 2020
@k8s-ci-robot
Merge pull request #94945 from ialidzhikov/automated-cherry-pick-of-#…
7fc4cb9 
…94666-upstream-release-1.18

Automated cherry pick of #94666: Handle nil elements when sorting, instead of panicking
k8s-ci-robot added a commit that referenced this pull request Sep 29, 2020
@k8s-ci-robot
Merge pull request #94946 from ialidzhikov/automated-cherry-pick-of-#…
09e5ba1 
…94666-upstream-release-1.17

Automated cherry pick of #94666: Handle nil elements when sorting, instead of panicking
k8s-ci-robot added a commit that referenced this pull request Sep 29, 2020
@k8s-ci-robot
Merge pull request #94944 from ialidzhikov/automated-cherry-pick-of-#…
e0aeb1b 
…94666-upstream-release-1.19

Automated cherry pick of #94666: Handle nil elements when sorting, instead of panicking
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hasheddan hasheddan hasheddan approved these changes

@zhouya0 zhouya0 zhouya0 approved these changes

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@sallyom sallyom

@hasheddan hasheddan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.20
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@soltysh @k8s-ci-robot @hasheddan @zhouya0 @sallyom