Fixed sort-by not sorting Resources as expected #100435

lauchokyip · 2021-03-21T21:51:28Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

When using sort-by to sort Resources, output is sorted unexpectedly

Which issue(s) this PR fixes:

Fixes kubernetes/kubectl#1019

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Support correct sorting for cpu, memory, storage, ephemeral-storage, hugepages, and attachable-volumes

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-03-21T21:51:36Z

Hi @lauchokyip. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lauchokyip · 2021-03-21T21:53:27Z

/cc @eddiezane

rikatz · 2021-03-21T22:03:20Z

/ok-to-test

rikatz

@lauchokyip thanks for your PR.

I've left some comments, PTAL :)

rikatz · 2021-03-22T14:46:26Z

staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go

+
+// IsQuantity returns true if the reflect.Value is type Quantity
+func IsQuantity(obj reflect.Value) bool {
+	objInterface := obj.Interface()
+	switch objInterface.(type) {
+	case apiresource.Quantity:
+		return true
+	default:
+		return false
+	}
+}
+
+// QuantityToString returns the string value of Quantity.
+// IsQuantity needs to be called in advance to check if it's Quantity
+func QuantityToString(obj reflect.Value) string {
+	objInterface := obj.Interface()
+	quantity := objInterface.(apiresource.Quantity)
+	return quantity.String()
+}


I'm thinking if those two functions couldn't be simplified into one, as:

func GetQuantity(obj reflect.Value) reflect.Value { switch objInterface := obj.Interface(); objInterface.(type) { case apiresource.Quantity: objStr := objInterface.(apiresource.Quantity) return reflect.ValueOf(objStr.String()) default: return obj } }

And then, when it's called, just to a later iField = cmdutil.GetQuantity(iField) :)

rikatz · 2021-03-22T14:47:58Z

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

@@ -95,6 +97,8 @@ func (s *SortingPrinter) sortObj(obj runtime.Object) error {
 	return meta.SetList(obj, objs)
 }

+// SortObjects sorts the runtime.Obejct based on filedInput and returns RuntimeSort that implements


typo here - Obejct/Object

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

lauchokyip · 2021-03-22T18:36:37Z

Thank You @rikatz, your feedbacks are very helpful!

lauchokyip · 2021-03-22T22:45:15Z

/retest

rikatz · 2021-04-05T14:41:43Z

/lgtm

Thank you!

eddiezane · 2021-05-19T19:37:49Z

/hold cancel

eddiezane · 2021-05-19T20:00:52Z

Great job on this one! I think we all learned a ton 😅 .

/approve

For lgtm
/cc @KnVerey @rikatz

k8s-ci-robot · 2021-05-19T20:01:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eddiezane, lauchokyip, rikatz

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:

~~hack/testdata/OWNERS~~ [eddiezane]
~~staging/src/k8s.io/kubectl/OWNERS~~ [eddiezane]
~~test/cmd/OWNERS~~ [eddiezane]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pacoxu · 2021-05-25T03:37:21Z

Will this support sort of ephemeral-storage?

lauchokyip · 2021-05-25T03:47:22Z

Hi @pacoxu I believe it will because essentially the field pods.spec.containers.resources.limits.ephemeral-storage is the same as pods.spec.containers.resources.limits.cpu / pods.spec.containers.resources.limits.memory, and they are all Resources type

@eddiezane @KnVerey Do you think I should add more test cases for all of them

pacoxu · 2021-05-25T04:52:54Z

This change LGTM.
I think you can fix others(storage and ephemeral-storage) in a new PR(if needed).

KnVerey · 2021-05-25T16:13:54Z

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

@@ -95,6 +96,8 @@ func (s *SortingPrinter) sortObj(obj runtime.Object) error {
 	return meta.SetList(obj, objs)
 }

+// SortObjects sorts the runtime.Object based on filedInput and returns RuntimeSort that implements


Suggested change

// SortObjects sorts the runtime.Object based on filedInput and returns RuntimeSort that implements

// SortObjects sorts the runtime.Object based on fieldInput and returns RuntimeSort that implements

KnVerey · 2021-05-25T16:21:52Z

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

+				// check if it's a Quantity
+				itypeQuantity, err := resource.ParseQuantity(itype)
+				if err != nil {
+					return sortorder.NaturalLess(itype, jtype), nil


instead of ignoring the error parsing jtypeQuanity below, I think we should return this default behavior if either one cannot be parsed to a quantity.

KnVerey · 2021-05-25T16:31:47Z

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

+	q, _ := resource.ParseQuantity(str)
+	return q
+}
+func createPodSpecResource(memReq, memLimit, cpuReq, cpuLimit string) corev1.PodSpec {


Let's pass t *testing.T as the first argument and use it to make the test fail if the setup is invalid. In other words, if the helper is given an invalid quantity, the test should fail, not ignore the parsing error and potentially not test what it is expected to. You can call t.Helper() to make it a proper test helper with better line information on failures.

Make sense!

KnVerey · 2021-05-25T18:50:39Z

Do you think I should add more test cases for all of them

No, I don't think it's necessary to add coverage for additional container.resources keys. I agree with you that the implementation is not sensitive to the quantity being at one key in resources vs another, and you already have coverage for two different ones.

lauchokyip · 2021-05-25T20:50:36Z

@KnVerey , fixed, PTAL 👍🏼

KnVerey

Just a couple more little test quibbles, but after that I think this looks ready to go!

KnVerey · 2021-05-26T00:00:13Z

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

@@ -47,6 +48,61 @@ func encodeOrDie(obj runtime.Object) []byte {
 	}
 	return data
 }
+func createResource(str string) (resource.Quantity, error) {
+	q, err := resource.ParseQuantity(str)


This could just return resource.ParseQuantity(str)... so let's get rid of this function and inline the resource.ParseQuantity(str) calls.

KnVerey · 2021-05-26T00:09:04Z

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

+
+	return podSpec
+}
+func createUnstructuredPodResource(t *testing.T, memReq, memLimit, cpuReq, cpuLimit string) unstructured.Unstructured {


Any reason not to add t.Helper() here too?

I just thought because that function doesnn't have t.Errorf() so it might not be needed 😕 will add that in

What t.Helper() does is let Go know that this is a test helper, so that it can skip it when printing line and file information, which makes error reporting easier to understand when a test fails. A pretty minor thing, but since the Testing instance already has to be passed in, might as well get this benefit. 🙂

lauchokyip · 2021-05-26T01:39:30Z

@KnVerey fixed! ready for final look 😃

KnVerey · 2021-05-26T15:27:22Z

/lgtm

k8s-ci-robot requested review from dougsland and rikatz March 21, 2021 21:52

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 21, 2021

k8s-ci-robot requested a review from eddiezane March 21, 2021 21:53

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 21, 2021

rikatz suggested changes Mar 22, 2021

View reviewed changes

lauchokyip force-pushed the fixSort branch from aa54793 to 4be9c18 Compare March 22, 2021 20:04

lauchokyip requested a review from rikatz March 22, 2021 20:05

k8s-ci-robot assigned rikatz Apr 5, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2021

rikatz approved these changes Apr 5, 2021

View reviewed changes

k8s-ci-robot requested a review from rikatz May 19, 2021 20:00

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 19, 2021

lauchokyip mentioned this pull request May 19, 2021

Fix spelling errors in staging/.../get_test.go #101996

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2021

KnVerey reviewed May 25, 2021

View reviewed changes

lauchokyip force-pushed the fixSort branch from 034f28d to 8580c39 Compare May 25, 2021 20:47

lauchokyip force-pushed the fixSort branch from 8580c39 to 48fba97 Compare May 25, 2021 21:34

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2021

KnVerey reviewed May 26, 2021

View reviewed changes

Fixed sort-by not sorting Resources as expected

e43e969

lauchokyip force-pushed the fixSort branch from 48fba97 to e43e969 Compare May 26, 2021 01:38

k8s-ci-robot assigned KnVerey May 26, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2021

k8s-ci-robot merged commit 02d0878 into kubernetes:master May 26, 2021

k8s-ci-robot added this to the v1.22 milestone May 26, 2021

github-actions bot mentioned this pull request Jun 2, 2021

Week Ending May 30, 2021 dev-obs/actus#387

Open

lauchokyip deleted the fixSort branch June 2, 2021 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed sort-by not sorting Resources as expected #100435

Fixed sort-by not sorting Resources as expected #100435

lauchokyip commented Mar 21, 2021 •

edited

k8s-ci-robot commented Mar 21, 2021

lauchokyip commented Mar 21, 2021

rikatz commented Mar 21, 2021

rikatz left a comment

rikatz Mar 22, 2021

rikatz Mar 22, 2021

lauchokyip commented Mar 22, 2021

lauchokyip commented Mar 22, 2021

rikatz commented Apr 5, 2021

eddiezane commented May 19, 2021

eddiezane commented May 19, 2021

k8s-ci-robot commented May 19, 2021

pacoxu commented May 25, 2021

lauchokyip commented May 25, 2021 •

edited

pacoxu commented May 25, 2021 •

edited

KnVerey May 25, 2021

KnVerey May 25, 2021

KnVerey May 25, 2021

lauchokyip May 25, 2021

KnVerey commented May 25, 2021

lauchokyip commented May 25, 2021

KnVerey left a comment

KnVerey May 26, 2021

KnVerey May 26, 2021

lauchokyip May 26, 2021 •

edited

KnVerey May 26, 2021

lauchokyip commented May 26, 2021 •

edited

KnVerey commented May 26, 2021

	// SortObjects sorts the runtime.Object based on filedInput and returns RuntimeSort that implements
	// SortObjects sorts the runtime.Object based on fieldInput and returns RuntimeSort that implements

Fixed sort-by not sorting Resources as expected #100435

Fixed sort-by not sorting Resources as expected #100435

Conversation

lauchokyip commented Mar 21, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Mar 21, 2021

lauchokyip commented Mar 21, 2021

rikatz commented Mar 21, 2021

rikatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauchokyip commented Mar 22, 2021

lauchokyip commented Mar 22, 2021

rikatz commented Apr 5, 2021

eddiezane commented May 19, 2021

eddiezane commented May 19, 2021

k8s-ci-robot commented May 19, 2021

pacoxu commented May 25, 2021

lauchokyip commented May 25, 2021 • edited

pacoxu commented May 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KnVerey commented May 25, 2021

lauchokyip commented May 25, 2021

KnVerey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauchokyip May 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauchokyip commented May 26, 2021 • edited

KnVerey commented May 26, 2021

lauchokyip commented Mar 21, 2021 •

edited

lauchokyip commented May 25, 2021 •

edited

pacoxu commented May 25, 2021 •

edited

lauchokyip May 26, 2021 •

edited

lauchokyip commented May 26, 2021 •

edited