Stabilize map order in kubectl describe #26046

timoreimann · 2016-05-22T21:34:44Z

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:

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.
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().
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.

k8s-bot · 2016-05-22T21:35:11Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-05-22T21:36:11Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-05-22T21:36:27Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

derekwaynecarr · 2016-05-23T02:45:44Z

pkg/api/types.go

@@ -2033,9 +2035,39 @@ const (
 	// Number of Pods that may be running on this Node: see ResourcePods
 )

+// ResourceNameList is a collection of ResourceName values.
+type ResourceNameList []ResourceName


The sort functions should go in a separate package. Probably somewhere under a util. That said, since a resource name is just a string can we just cast to a string slice and use sort strings?

To elaborate, we tend to keep types.go as just holding the structs and all the utility functions should go elsewhere. It makes it easier to work with the versioned types files if we don't tend to have the utility functions mixed in.

bgrant0607 · 2016-05-23T21:13:13Z

cc @kubernetes/kubectl

bgrant0607 · 2016-05-23T21:13:31Z

ok to test

derekwaynecarr · 2016-05-23T21:21:27Z

See: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/describe.go#L304

And: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/sorted_resource_name_list.go

Can we localize sort logic to that second file?

And have a utility to does what's required there?

timoreimann · 2016-05-23T22:50:35Z

@derekwaynecarr Thanks for the pointer to those files. Very helpful, I missed to see sorted_resource_name_list.go when I put together the PR. My latest commit moved the sorting logic to that place.

Let me know what you think.

timoreimann · 2016-05-25T08:56:21Z

@derekwaynecarr did you have a chance to look at the PR again? Thanks!

derekwaynecarr · 2016-05-25T21:42:51Z

pkg/kubectl/describe.go

@@ -842,21 +842,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) {


I think we should stop reporting qos per resource in 1.3

@bgrant0607 @vishh - now that we have merged #14943 should we update the CLI to just output the QoS of the pod?

Yes. We should do that. I can post a patch for it later today, unless we
want to make that change in this PR itself.

On Wed, May 25, 2016 at 2:43 PM, Derek Carr notifications@github.com
wrote:

In pkg/kubectl/describe.go
#26046 (comment)
:

@@ -842,21 +842,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) {

I think we should stop reporting qos per resource in 1.3

@bgrant0607 https://github.com/bgrant0607 @vishh
https://github.com/vishh - now that we have merged #14943
#14943 should we update
the CLI to just output the QoS of the pod?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/26046/files/c8a14e376e3df237d6d0d6a2d5266056cbfeab87#r64658693

I think we should stop reporting qos per resource in 1.3

Does this mean that if I have a deployment and describe it I won't be able to see its QoS anymore?

I could be wrong here, but my understanding is that QoS just moves one layer up from containers to pods.

Qos has changed this release to be at the pod level and not for individual
compute resources. A follow on PR was made that now reports the effective
QoS for a pod in describe.

On Thursday, May 26, 2016, Michail Kargakis notifications@github.com
wrote:

In pkg/kubectl/describe.go
#26046 (comment)
:

@@ -842,21 +842,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) {

I think we should stop reporting qos per resource in 1.3

Does this mean that if I have a deployment and describe it I won't be able
to see its QoS anymore?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/26046/files/c8a14e376e3df237d6d0d6a2d5266056cbfeab87#r64710897

derekwaynecarr · 2016-05-25T21:45:10Z

@timoreimann - just the one open question that this PR triggered where I think we should stop showing per resource qos information, will let some others weigh in before merging this.

derekwaynecarr · 2016-05-25T21:46:26Z

can you squash your commits in the interim?

Includes container and QoS resources.

timoreimann · 2016-05-25T21:55:37Z

@derekwaynecarr done.

timoreimann · 2016-05-25T22:07:58Z

@derekwaynecarr @vishh I'm open to providing another / extending this PR to move QoS information to the pod level but might lag a bit due to a long weekend ahead. If you're in a hurry to get things in for the upcoming 1.3 release, I'd happily leave it to another PR of yours. Just let me know.

vishh · 2016-05-25T22:17:39Z

@timoreimann I will most likely tackle pod level QoS reporting this week. I can open a tracker issue in case you are interested.
The functionality this PR attempts to introduce is orthogonal to moving to pod level QoS. So this PR as-is LGTM!

timoreimann · 2016-05-25T22:22:39Z

@vishh A tracker issue would be great.

Thanks for the feedback!

timoreimann · 2016-05-25T22:47:59Z

First time contributor here. @@derekwaynecarr, do I understand correctly that I still need LGTM and ok-to-merge labels?

derekwaynecarr · 2016-05-26T00:25:14Z

This PR looks good. Applying all the required labels. Thanks for the contribution!

alex-mohr · 2016-05-26T19:41:00Z

Because submit queue is blocked, I'm going to kick off re-tests of the top 5 PRs in the queue, then merge them if they pass.
@k8s-bot test this please: issue #IGNORE

alex-mohr · 2016-05-26T20:24:08Z

Apologies for not looking into this particular test failure, using IGNORE bypass as buildcop.
@k8s-bot node test this please: issue #IGNORE

timoreimann · 2016-05-27T19:40:46Z

@alex-mohr my PR still seems to be stuck. is the submit queue still having issues?

k8s-github-robot · 2016-05-28T17:00:45Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-05-28T17:42:22Z

GCE e2e build/test passed for commit 32aa740.

k8s-github-robot · 2016-05-28T17:49:57Z

Automatic merge from submit-queue

googlebot added the cla: yes label May 22, 2016

k8s-github-robot assigned bgrant0607 May 22, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 22, 2016

derekwaynecarr reviewed May 23, 2016
View reviewed changes

bgrant0607 assigned derekwaynecarr and unassigned bgrant0607 May 23, 2016

k8s-github-robot added the needs-ok-to-merge label May 24, 2016

derekwaynecarr reviewed May 25, 2016
View reviewed changes

derekwaynecarr added this to the v1.3 milestone May 25, 2016

derekwaynecarr added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels May 25, 2016

Show resources in pod descriptions by sorted names.

32aa740

Includes container and QoS resources.

timoreimann force-pushed the stabilize-map-order-in-kubectl-describe branch from c8a14e3 to 32aa740 Compare May 25, 2016 21:52

derekwaynecarr added lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-merge and removed needs-ok-to-merge labels May 26, 2016

k8s-github-robot merged commit 03fc51f into kubernetes:master May 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stabilize map order in kubectl describe #26046

Stabilize map order in kubectl describe #26046

timoreimann commented May 22, 2016 •

edited

Loading

k8s-bot commented May 22, 2016

k8s-bot commented May 22, 2016

k8s-bot commented May 22, 2016

derekwaynecarr May 23, 2016

derekwaynecarr May 23, 2016

bgrant0607 commented May 23, 2016

bgrant0607 commented May 23, 2016

derekwaynecarr commented May 23, 2016

timoreimann commented May 23, 2016

timoreimann commented May 25, 2016

derekwaynecarr May 25, 2016

vishh May 25, 2016

0xmichalis May 26, 2016

timoreimann May 26, 2016

derekwaynecarr Jun 9, 2016

derekwaynecarr commented May 25, 2016

derekwaynecarr commented May 25, 2016

timoreimann commented May 25, 2016

timoreimann commented May 25, 2016

vishh commented May 25, 2016

timoreimann commented May 25, 2016

timoreimann commented May 25, 2016

derekwaynecarr commented May 26, 2016

alex-mohr commented May 26, 2016

alex-mohr commented May 26, 2016

timoreimann commented May 27, 2016

k8s-github-robot commented May 28, 2016

k8s-bot commented May 28, 2016

k8s-github-robot commented May 28, 2016

Stabilize map order in kubectl describe #26046

Stabilize map order in kubectl describe #26046

Conversation

timoreimann commented May 22, 2016 • edited Loading

k8s-bot commented May 22, 2016

k8s-bot commented May 22, 2016

k8s-bot commented May 22, 2016

derekwaynecarr May 23, 2016

Choose a reason for hiding this comment

derekwaynecarr May 23, 2016

Choose a reason for hiding this comment

bgrant0607 commented May 23, 2016

bgrant0607 commented May 23, 2016

derekwaynecarr commented May 23, 2016

timoreimann commented May 23, 2016

timoreimann commented May 25, 2016

derekwaynecarr May 25, 2016

Choose a reason for hiding this comment

vishh May 25, 2016

Choose a reason for hiding this comment

0xmichalis May 26, 2016

Choose a reason for hiding this comment

timoreimann May 26, 2016

Choose a reason for hiding this comment

derekwaynecarr Jun 9, 2016

Choose a reason for hiding this comment

derekwaynecarr commented May 25, 2016

derekwaynecarr commented May 25, 2016

timoreimann commented May 25, 2016

timoreimann commented May 25, 2016

vishh commented May 25, 2016

timoreimann commented May 25, 2016

timoreimann commented May 25, 2016

derekwaynecarr commented May 26, 2016

alex-mohr commented May 26, 2016

alex-mohr commented May 26, 2016

timoreimann commented May 27, 2016

k8s-github-robot commented May 28, 2016

k8s-bot commented May 28, 2016

k8s-github-robot commented May 28, 2016

timoreimann commented May 22, 2016 •

edited

Loading